Skip to content

Commit ca392a2

Browse files
CyrusNajmabadiCyrusNajmabadi
authored
Fixed crash in RemoveUnnecessarySuppressions caused by multi-variable declaration (#78787)
2 parents a528f84 + 4097076 commit ca392a2

File tree

2 files changed

+213
-47
lines changed

2 files changed

+213
-47
lines changed

‎src/Analyzers/Core/Analyzers/RemoveUnnecessarySuppressions/AbstractRemoveUnnecessaryPragmaSuppressionsDiagnosticAnalyzer.cs

Lines changed: 63 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -759,62 +759,78 @@ private async Task<bool> ProcessSuppressMessageAttributesAsync(
759759
continue;
760760
}
761761

762-
var symbols = SemanticFacts.GetDeclaredSymbols(semanticModel, node, cancellationToken);
763-
foreach (var symbol in symbols)
762+
// In the case of declaration nodes that can have more than one symbol e.g. fields and events,
763+
// the attributes are shared between then. Given this, we only need to inspect the first symbol
764+
// of the node.
765+
var symbol = SemanticFacts
766+
.GetDeclaredSymbols(semanticModel, node, cancellationToken)
767+
.FirstOrDefault();
768+
769+
// If we somehow do not have a symbol, we can't do anything. Otherwise, check if our symbol is
770+
// a partial definition. If it is, skip it in favor of checking the implementation.
771+
if (symbol is null or
772+
IMethodSymbol { IsPartialDefinition: true } or
773+
IPropertySymbol { IsPartialDefinition: true })
764774
{
765-
switch (symbol?.Kind)
766-
{
767-
// Local SuppressMessageAttributes are only applicable for types and members.
768-
case SymbolKind.NamedType:
769-
case SymbolKind.Method:
770-
case SymbolKind.Field:
771-
case SymbolKind.Property:
772-
case SymbolKind.Event:
773-
break;
774-
775-
default:
776-
continue;
777-
}
775+
continue;
776+
}
778777

779-
// Skip already processed symbols from partial declarations
780-
var isPartial = symbol.Locations.Length > 1;
781-
if (isPartial && !processedPartialSymbols.Add(symbol))
782-
{
778+
switch (symbol?.Kind)
779+
{
780+
// Local SuppressMessageAttributes are only applicable for types and members.
781+
case SymbolKind.NamedType:
782+
case SymbolKind.Method:
783+
case SymbolKind.Field:
784+
case SymbolKind.Property:
785+
case SymbolKind.Event:
786+
break;
787+
788+
default:
783789
continue;
784-
}
790+
}
791+
792+
// Skip already processed symbols from partial declarations
793+
var isPartial = symbol.Locations.Length > 1;
785794

786-
foreach (var attribute in symbol.GetAttributes())
795+
if (isPartial && !processedPartialSymbols.Add(symbol))
796+
{
797+
continue;
798+
}
799+
800+
foreach (var attribute in symbol.GetAttributes())
801+
{
802+
if (attribute.ApplicationSyntaxReference != null &&
803+
TryGetSuppressedDiagnosticId(attribute, suppressMessageAttributeType, out var id, out var category))
787804
{
788-
if (attribute.ApplicationSyntaxReference != null &&
789-
TryGetSuppressedDiagnosticId(attribute, suppressMessageAttributeType, out var id, out var category))
805+
// Ignore unsupported IDs and those excluded through user option.
806+
if (!IsSupportedAnalyzerDiagnosticId(id) ||
807+
userIdExclusions.Contains(id, StringComparer.OrdinalIgnoreCase) ||
808+
category?.Length > 0 && userCategoryExclusions.Contains(category, StringComparer.OrdinalIgnoreCase))
809+
{
810+
continue;
811+
}
812+
813+
if (!idToSuppressMessageAttributesMap.TryGetValue(id, out var nodesForId))
790814
{
791-
// Ignore unsupported IDs and those excluded through user option.
792-
if (!IsSupportedAnalyzerDiagnosticId(id) ||
793-
userIdExclusions.Contains(id, StringComparer.OrdinalIgnoreCase) ||
794-
category?.Length > 0 && userCategoryExclusions.Contains(category, StringComparer.OrdinalIgnoreCase))
795-
{
796-
continue;
797-
}
798-
799-
if (!idToSuppressMessageAttributesMap.TryGetValue(id, out var nodesForId))
800-
{
801-
nodesForId = [];
802-
idToSuppressMessageAttributesMap.Add(id, nodesForId);
803-
}
804-
805-
var attributeNode = await attribute.ApplicationSyntaxReference.GetSyntaxAsync(cancellationToken).ConfigureAwait(false);
806-
nodesForId.Add(attributeNode);
807-
808-
// Initialize the attribute node as unnecessary at the start of the algorithm.
809-
// Later processing will identify attributes which are indeed responsible for suppressing diagnostics
810-
// and mark them as used.
811-
// NOTE: For attributes on partial symbols with multiple declarations, we conservatively
812-
// consider them as used and avoid unnecessary attribute analysis because that would potentially
813-
// require analysis across multiple files, which can be expensive from a performance standpoint.
814-
suppressMessageAttributesToIsUsedMap.Add(attributeNode, isPartial);
815+
nodesForId = [];
816+
idToSuppressMessageAttributesMap.Add(id, nodesForId);
815817
}
818+
819+
var attributeNode = await attribute.ApplicationSyntaxReference.GetSyntaxAsync(cancellationToken).ConfigureAwait(false);
820+
nodesForId.Add(attributeNode);
821+
822+
// Initialize the attribute node as unnecessary at the start of the algorithm.
823+
// Later processing will identify attributes which are indeed responsible for suppressing diagnostics
824+
// and mark them as used.
825+
// NOTE: For attributes on partial symbols with multiple declarations, we conservatively
826+
// consider them as used and avoid unnecessary attribute analysis because that would potentially
827+
// require analysis across multiple files, which can be expensive from a performance standpoint.
828+
suppressMessageAttributesToIsUsedMap.Add(attributeNode, isPartial);
816829
}
817830
}
831+
832+
// Individual variables within a variable declaration cannot be decorated with distinct attributes, so we
833+
// should avoid looking at any of the subsequent symbols for this node as they will be the same.
818834
}
819835
}
820836

‎src/Features/CSharpTest/Diagnostics/Suppression/RemoveUnnecessaryPragmaSuppressionsTests.cs

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,156 @@ void M()
461461
|]", new TestParameters(options: options));
462462
}
463463

464+
[Theory]
465+
[InlineData("event", "EventHandler")]
466+
[InlineData("static", "int")]
467+
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
468+
public async Task TestRemoveDiagnosticSuppression_Attribute_MultiVariableDeclaration(string keyword, string type)
469+
{
470+
await TestInRegularAndScript1Async(
471+
$$"""
472+
public class C
473+
{
474+
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
475+
public {{keyword}} {{type}} A, B;
476+
}
477+
""",
478+
$$"""
479+
public class C
480+
{
481+
public {{keyword}} {{type}} A, B;
482+
}
483+
""");
484+
}
485+
486+
[Fact]
487+
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
488+
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialMethodDefinition()
489+
{
490+
await TestInRegularAndScript1Async(
491+
"""
492+
public partial class C
493+
{
494+
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
495+
partial void M();
496+
}
497+
498+
public partial class C
499+
{
500+
partial void M()
501+
{
502+
}
503+
}
504+
""",
505+
"""
506+
public partial class C
507+
{
508+
partial void M();
509+
}
510+
511+
public partial class C
512+
{
513+
partial void M()
514+
{
515+
}
516+
}
517+
""");
518+
}
519+
520+
[Fact]
521+
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
522+
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialMethodImplementation()
523+
{
524+
await TestInRegularAndScript1Async(
525+
"""
526+
public partial class C
527+
{
528+
partial void M();
529+
}
530+
531+
public partial class C
532+
{
533+
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
534+
partial void M()
535+
{
536+
}
537+
}
538+
""",
539+
"""
540+
public partial class C
541+
{
542+
partial void M();
543+
}
544+
545+
public partial class C
546+
{
547+
partial void M()
548+
{
549+
}
550+
}
551+
""");
552+
}
553+
554+
[Fact]
555+
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
556+
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialPropertyDefinition()
557+
{
558+
await TestInRegularAndScript1Async(
559+
"""
560+
public partial class C
561+
{
562+
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
563+
partial int P { get; }
564+
}
565+
566+
public partial class C
567+
{
568+
partial int P => 5230;
569+
}
570+
""",
571+
"""
572+
public partial class C
573+
{
574+
partial int P { get; }
575+
}
576+
577+
public partial class C
578+
{
579+
partial int P => 5230;
580+
}
581+
""");
582+
}
583+
584+
[Fact]
585+
[WorkItem("https://github.com/dotnet/roslyn/issues/78786")]
586+
public async Task TestRemoveDiagnosticSuppression_Attribute_PartialPropertyImplementation()
587+
{
588+
await TestInRegularAndScript1Async(
589+
"""
590+
public partial class C
591+
{
592+
partial int P { get; }
593+
}
594+
595+
public partial class C
596+
{
597+
[|[System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1720:Identifier contains type name", Justification = "<Pending>")]|]
598+
partial int P => 5230;
599+
}
600+
""",
601+
"""
602+
public partial class C
603+
{
604+
partial int P { get; }
605+
}
606+
607+
public partial class C
608+
{
609+
partial int P => 5230;
610+
}
611+
""");
612+
}
613+
464614
[Fact]
465615
public async Task TestDoNotRemoveDiagnosticSuppression_Attribute_OnPartialDeclarations()
466616
{

0 commit comments

Comments
 (0)