Skip to content

Fixed crash in RemoveUnnecessarySuppressions caused by multi-variable declaration #78787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 2, 2025
Merged

Fixed crash in RemoveUnnecessarySuppressions caused by multi-variable declaration #78787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0cb851f
Fixed multi-variable declaration support in RemoveUnnecessarySuppress…
John-Leitch Jun 1, 2025
d054c4a
Removed whitespace.
John-Leitch Jun 1, 2025
b147a16
Simplified TestRemoveDiagnosticSuppression_Attribute_MultiVariableDec…
John-Leitch Jun 1, 2025
a73dcb9
Merge branch 'remove-diagnostic-suppression-fix' of https://github.co…
John-Leitch Jun 1, 2025
4097076
Expanded remove unnecessary suppression tests, removed unnecessary lo…
John-Leitch Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 63 additions & 47 deletions ...eUnnecessarySuppressions/AbstractRemoveUnnecessaryPragmaSuppressionsDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,62 +759,78 @@ private async Task<bool> ProcessSuppressMessageAttributesAsync(
continue;
}

var symbols = SemanticFacts.GetDeclaredSymbols(semanticModel, node, cancellationToken);
foreach (var symbol in symbols)
// In the case of declaration nodes that can have more than one symbol e.g. fields and events,
// the attributes are shared between then. Given this, we only need to inspect the first symbol
// of the node.
var symbol = SemanticFacts
.GetDeclaredSymbols(semanticModel, node, cancellationToken)
.FirstOrDefault();

// If we somehow do not have a symbol, we can't do anything. Otherwise, check if our symbol is
// a partial definition. If it is, skip it in favor of checking the implementation.
if (symbol is null or
IMethodSymbol { IsPartialDefinition: true } or
IPropertySymbol { IsPartialDefinition: true })
{
switch (symbol?.Kind)
{
// Local SuppressMessageAttributes are only applicable for types and members.
case SymbolKind.NamedType:
case SymbolKind.Method:
case SymbolKind.Field:
case SymbolKind.Property:
case SymbolKind.Event:
break;

default:
continue;
}
continue;
}

// Skip already processed symbols from partial declarations
var isPartial = symbol.Locations.Length > 1;
if (isPartial && !processedPartialSymbols.Add(symbol))
{
switch (symbol?.Kind)
{
// Local SuppressMessageAttributes are only applicable for types and members.
case SymbolKind.NamedType:
case SymbolKind.Method:
case SymbolKind.Field:
case SymbolKind.Property:
case SymbolKind.Event:
break;

default:
continue;
}
}

// Skip already processed symbols from partial declarations
var isPartial = symbol.Locations.Length > 1;

foreach (var attribute in symbol.GetAttributes())
if (isPartial && !processedPartialSymbols.Add(symbol))
{
continue;
}

foreach (var attribute in symbol.GetAttributes())
{
if (attribute.ApplicationSyntaxReference != null &&
TryGetSuppressedDiagnosticId(attribute, suppressMessageAttributeType, out var id, out var category))
{
if (attribute.ApplicationSyntaxReference != null &&
TryGetSuppressedDiagnosticId(attribute, suppressMessageAttributeType, out var id, out var category))
// Ignore unsupported IDs and those excluded through user option.
if (!IsSupportedAnalyzerDiagnosticId(id) ||
userIdExclusions.Contains(id, StringComparer.OrdinalIgnoreCase) ||
category?.Length > 0 && userCategoryExclusions.Contains(category, StringComparer.OrdinalIgnoreCase))
{
continue;
}

if (!idToSuppressMessageAttributesMap.TryGetValue(id, out var nodesForId))
{
// Ignore unsupported IDs and those excluded through user option.
if (!IsSupportedAnalyzerDiagnosticId(id) ||
userIdExclusions.Contains(id, StringComparer.OrdinalIgnoreCase) ||
category?.Length > 0 && userCategoryExclusions.Contains(category, StringComparer.OrdinalIgnoreCase))
{
continue;
}

if (!idToSuppressMessageAttributesMap.TryGetValue(id, out var nodesForId))
{
nodesForId = [];
idToSuppressMessageAttributesMap.Add(id, nodesForId);
}

var attributeNode = await attribute.ApplicationSyntaxReference.GetSyntaxAsync(cancellationToken).ConfigureAwait(false);
nodesForId.Add(attributeNode);

// Initialize the attribute node as unnecessary at the start of the algorithm.
// Later processing will identify attributes which are indeed responsible for suppressing diagnostics
// and mark them as used.
// NOTE: For attributes on partial symbols with multiple declarations, we conservatively
// consider them as used and avoid unnecessary attribute analysis because that would potentially
// require analysis across multiple files, which can be expensive from a performance standpoint.
suppressMessageAttributesToIsUsedMap.Add(attributeNode, isPartial);
nodesForId = [];
idToSuppressMessageAttributesMap.Add(id, nodesForId);
}

var attributeNode = await attribute.ApplicationSyntaxReference.GetSyntaxAsync(cancellationToken).ConfigureAwait(false);
nodesForId.Add(attributeNode);

// Initialize the attribute node as unnecessary at the start of the algorithm.
// Later processing will identify attributes which are indeed responsible for suppressing diagnostics
// and mark them as used.
// NOTE: For attributes on partial symbols with multiple declarations, we conservatively
// consider them as used and avoid unnecessary attribute analysis because that would potentially
// require analysis across multiple files, which can be expensive from a performance standpoint.
suppressMessageAttributesToIsUsedMap.Add(attributeNode, isPartial);
}
}

// Individual variables within a variable declaration cannot be decorated with distinct attributes, so we
// should avoid looking at any of the subsequent symbols for this node as they will be the same.
}
}

Expand Down
150 changes: 150 additions & 0 deletions src/Features/CSharpTest/Diagnostics/Suppression/RemoveUnnecessaryPragmaSuppressionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,156 @@ void M()
|]", new TestParameters(options: options));
}

[Theory]
[InlineData("event", "EventHandler")]
[InlineData("static", "int")]
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
public async Task TestRemoveDiagnosticSuppression_Attribute_MultiVariableDeclaration(string keyword, string type)
{
await TestInRegularAndScript1Async(
$$"""
public class C
{
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
public {{keyword}} {{type}} A, B;
}
""",
$$"""
public class C
{
public {{keyword}} {{type}} A, B;
}
""");
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialMethodDefinition()
{
await TestInRegularAndScript1Async(
"""
public partial class C
{
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
partial void M();
}

public partial class C
{
partial void M()
{
}
}
""",
"""
public partial class C
{
partial void M();
}

public partial class C
{
partial void M()
{
}
}
""");
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialMethodImplementation()
{
await TestInRegularAndScript1Async(
"""
public partial class C
{
partial void M();
}

public partial class C
{
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
partial void M()
{
}
}
""",
"""
public partial class C
{
partial void M();
}

public partial class C
{
partial void M()
{
}
}
""");
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialPropertyDefinition()
{
await TestInRegularAndScript1Async(
"""
public partial class C
{
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
partial int P { get; }
}

public partial class C
{
partial int P => 5230;
}
""",
"""
public partial class C
{
partial int P { get; }
}

public partial class C
{
partial int P => 5230;
}
""");
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialPropertyImplementation()
{
await TestInRegularAndScript1Async(
"""
public partial class C
{
partial int P { get; }
}

public partial class C
{
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
partial int P => 5230;
}
""",
"""
public partial class C
{
partial int P { get; }
}

public partial class C
{
partial int P => 5230;
}
""");
}

[Fact]
public async Task TestDoNotRemoveDiagnosticSuppression_Attribute_OnPartialDeclarations()
{
Expand Down
Loading