Refactor attribute-based suppressors and support Odin serialization attribute (#399)

* Refactor attribute-based suppressors and support Odin serialization attribute

* Fix documentation

* Fix documentation again

* Update index

* Typo

Author: sailro

doc/USP0004.md

@@ -1,6 +1,6 @@
-# USP0004 Don't set fields with SerializeField or SerializeReference attributes to read-only
+# USP0004 Don't flag fields decorated with serialization attributes (like `SerializeField`, `SerializeReference` or `OdinSerialize`) as read-only
 
-Fields with the `SerializeField` or `SerializeReference` attributes should not be marked read-only.
+Fields with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes should not be marked read-only.
 
 ## Suppressed Diagnostic ID
 
@@ -23,4 +23,4 @@ The IDE does not detect that the field is ever assigned outside of the declarati
 
 ## Why do we suppress this diagnostic?
 
-A field with the `SerializeField` or `SerializeReference` attributes are exposed and can be assigned in the Unity Inspector. Making the field read-only would break this behavior.
+A field with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes are exposed and can be assigned in the Unity Inspector. Making the field read-only would break this behavior.

doc/USP0006.md

@@ -1,6 +1,6 @@
-# USP0006 Don't flag private fields with SerializeField or SerializeReference attributes as unused
+# USP0006 Don't flag private fields decorated with serialization attributes (like `SerializeField`, `SerializeReference` or `OdinSerialize`) as unused
 
-Private fields with the `SerializeField` or `SerializeReference` attributes should not be marked as unused.
+Private fields with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes should not be marked as unused.
 
 ## Suppressed Diagnostic ID
 
@@ -23,4 +23,4 @@ The IDE does not detect that the field is ever used within the project. Therefor
 
 ## Why do we suppress this diagnostic?
 
-A field with the `SerializeField` or `SerializeReference` attributes are exposed and can be used in the Unity Inspector.
+A field with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes are exposed and can be used in the Unity Inspector.

doc/USP0007.md

@@ -1,6 +1,6 @@
-# USP0007 Don't flag fields with SerializeField or SerializeReference attributes as never assigned
+# USP0007 Don't flag fields decorated with serialization attributes (like `SerializeField`, `SerializeReference` or `OdinSerialize`) as never assigned
 
-Fields with the `SerializeField` or `SerializeReference` attributes should not be marked as unassigned.
+Fields with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes should not be marked as unassigned.
 
 ## Suppressed Diagnostic ID
 
@@ -23,4 +23,4 @@ The compiler detected an uninitialized private field declaration that is never a
 
 ## Why do we suppress this diagnostic?
 
-A field with the `SerializeField` or `SerializeReference` attributes are exposed to Unity and can be assigned in the Unity Inspector.
+A field with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes are exposed to Unity and can be assigned in the Unity Inspector.

doc/USP0009.md

@@ -1,4 +1,4 @@
-# USP0009 Don't flag methods with the ContextMenu attribute or referenced by a field with the ContextMenuItem attribute as unused
+# USP0009 Don't flag methods decorated with the ContextMenu attribute or referenced by a field with the ContextMenuItem attribute as unused
 
 Methods decorated with `ContextMenu`/`MenuItem` attribute, or referenced by a field with `ContextMenuItem` are not unused.
 

doc/USP0010.md

@@ -1,4 +1,4 @@
-# USP0010 Don't flag fields with the ContextMenuItem attribute as unused
+# USP0010 Don't flag fields decorated with the ContextMenuItem attribute as unused
 
 Fields decorated with the `ContextMenuItem` attribute are not unused.
 

doc/USP0011.md

@@ -1,4 +1,4 @@
-# USP0011 Don't make fields with the ContextMenuItem attribute read-only
+# USP0011 Don't make fields decorated with the ContextMenuItem attribute read-only
 
 Fields decorated with the `ContextMenuItem` attribute should not be made read-only.
 

doc/USP0013.md

@@ -1,6 +1,6 @@
-# USP0013 Don't flag private fields with SerializeField or SerializeReference attributes as unused
+# USP0013 Don't flag fields decorated with serialization attributes (like `SerializeField`, `SerializeReference` or `OdinSerialize`) as unused
 
-Private fields with the `SerializeField` or `SerializeReference` attributes should not be marked as unused by FxCop.
+Private fields with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes should not be marked as unused by FxCop.
 
 ## Suppressed Diagnostic ID
 
@@ -23,4 +23,4 @@ FxCop does not detect that the field is ever used within the project. Therefore,
 
 ## Why do we suppress this diagnostic?
 
-A field with the `SerializeField` or `SerializeReference` attributes are exposed and can be used in the Unity Inspector.
+A field with the `SerializeField`, `SerializeReference` or `OdinSerialize` attributes are exposed and can be used in the Unity Inspector.

doc/USP0019.md

@@ -1,6 +1,6 @@
-# USP0019 Don't flag private methods decorated with PreserveAttribute or UsedImplicitlyAttribute as unused
+# USP0019 Don't flag private members decorated with implicit-usage attributes as unused
 
-Methods decorated with `PreserveAttribute` or `UsedImplicitlyAttribute` attributes are not unused.
+Members decorated with `PreserveAttribute`, `UsedImplicitlyAttribute` or `CreatePropertyAttribute` attributes are not unused.
 
 ## Suppressed Diagnostic ID
 

doc/index.md

@@ -48,22 +48,22 @@ ID | Suppressed ID | Justification
 [USP0001](USP0001.md) | IDE0029 | Unity objects should not use null coalescing
 [USP0002](USP0002.md) | IDE0031 | Unity objects should not use null propagation
 [USP0003](USP0003.md) | IDE0051 | The Unity runtime invokes Unity messages
-[USP0004](USP0004.md) | IDE0044 | Don't set fields with `SerializeField` or `SerializeReference` attributes to read-only
+[USP0004](USP0004.md) | IDE0044 | Don't flag fields decorated with serialization attributes as read-only
 [USP0005](USP0005.md) | IDE0060 | The Unity runtime invokes Unity messages
-[USP0006](USP0006.md) | IDE0051 | Don't flag private fields with `SerializeField` or `SerializeReference` attributes as unused
-[USP0007](USP0007.md) | CS0649 | Don't flag fields with `SerializeField` or `SerializeReference` attributes as never assigned
+[USP0006](USP0006.md) | IDE0051 | Don't flag private fields decorated with serialization attributes as unused
+[USP0007](USP0007.md) | CS0649 | Don't flag fields decorated with serialization attributes as never assigned
 [USP0008](USP0008.md) | IDE0051 | Don't flag private methods used with `Invoke`/`InvokeRepeating` or `StartCoroutine`/`StopCoroutine` as unused
-[USP0009](USP0009.md) | IDE0051 | Don't flag methods with `MenuItem`/`ContextMenu` attribute or referenced by a field with the `ContextMenuItem` attribute as unused
-[USP0010](USP0010.md) | IDE0051 | Don't flag fields with the `ContextMenuItem` attribute as unused
-[USP0011](USP0011.md) | IDE0044 | Don't make fields with the `ContextMenuItem` attribute read-only
+[USP0009](USP0009.md) | IDE0051 | Don't flag methods decorated with `MenuItem`/`ContextMenu` attribute or referenced by a field with the `ContextMenuItem` attribute as unused
+[USP0010](USP0010.md) | IDE0051 | Don't flag fields decorated with the `ContextMenuItem` attribute as unused
+[USP0011](USP0011.md) | IDE0044 | Don't make fields decorated with the `ContextMenuItem` attribute read-only
 [USP0012](USP0012.md) | IDE0051 | Don't flag private methods with `InitializeOnLoadMethod`, `RuntimeInitializeOnLoadMethod` or `DidReloadScripts` attribute as unused
-[USP0013](USP0013.md) | CA1823 | Don't flag private fields with `SerializeField` or `SerializeReference` attributes as unused
+[USP0013](USP0013.md) | CA1823 | Don't flag private fields decorated with serialization attributes as unused
 [USP0014](USP0014.md) | CA1822 | The Unity runtime invokes Unity messages
 [USP0015](USP0015.md) | CA1801 | The Unity runtime invokes Unity messages
 [USP0016](USP0016.md) | CS8618 | Initialization detection with nullable reference types
 [USP0017](USP0017.md) | IDE0074 | Unity objects should not use coalescing assignment
 [USP0018](USP0018.md) | IDE0016 | Unity objects should not be used with throw expressions
-[USP0019](USP0012.md) | IDE0051 | Don't flag private methods with `PreserveAttribute` or `UsedImplicitlyAttribute` as unused
+[USP0019](USP0019.md) | IDE0051 | Don't flag private members with implicit-usage attributes as unused
 [USP0020](USP0020.md) | IDE0052 | The Unity runtime invokes Unity messages
 [USP0021](USP0021.md) | IDE0041 | Prefer reference equality
 [USP0022](USP0022.md) | IDE0270 | Unity objects should not use if null coalescing

src/Microsoft.Unity.Analyzers.Tests/ImplicitUsageAttributeSuppressorTests.cs

@@ -10,6 +10,27 @@ namespace Microsoft.Unity.Analyzers.Tests;
 
 public class ImplicitUsageAttributeSuppressorTests : BaseSuppressorVerifierTest<ImplicitUsageAttributeSuppressor>
 {
+	[Fact]
+	public async Task PrivateFieldWithCreateAttributeUnusedSuppressed()
+	{
+		const string test = @"
+using UnityEngine;
+
+class Camera : MonoBehaviour
+{
+    [Unity.Properties.CreateProperty]
+    string someField = ""default"";
+}
+";
+		var context = AnalyzerVerificationContext.Default
+			.WithAnalyzerOption("dotnet_style_readonly_field", "false");
+
+		var suppressor = ExpectSuppressor(ImplicitUsageAttributeSuppressor.Rule)
+			.WithLocation(7, 12);
+
+		await VerifyCSharpDiagnosticAsync(context, test, suppressor);
+	}
+
 	[Fact]
 	public async Task UnityPreserveTest()
 	{

src/Microsoft.Unity.Analyzers.Tests/SerializeFieldCodeQualitySuppressorTests.cs renamed to src/Microsoft.Unity.Analyzers.Tests/SerializedFieldCodeQualitySuppressorTests.cs

@@ -8,7 +8,7 @@
 
 namespace Microsoft.Unity.Analyzers.Tests;
 
-public class SerializeFieldCodeQualitySuppressorTests : BaseSuppressorVerifierTest<SerializeFieldSuppressor>
+public class SerializedFieldCodeQualitySuppressorTests : BaseSuppressorVerifierTest<SerializedFieldSuppressor>
 {
 	// Only load CodeQuality analyzers for those tests
 	protected override SuppressorVerifierAnalyzers SuppressorVerifierAnalyzers => SuppressorVerifierAnalyzers.CodeQuality;
@@ -26,7 +26,7 @@ class Camera : MonoBehaviour
 }
 ";
 
-		var suppressor = ExpectSuppressor(SerializeFieldSuppressor.UnusedCodeQualityRule)
+		var suppressor = ExpectSuppressor(SerializedFieldSuppressor.UnusedCodeQualityRule)
 			.WithLocation(7, 12);
 
 		await VerifyCSharpDiagnosticAsync(test, suppressor);

src/Microsoft.Unity.Analyzers.Tests/SerializeFieldSuppressorTests.cs renamed to src/Microsoft.Unity.Analyzers.Tests/SerializedFieldSuppressorTests.cs

@@ -9,7 +9,7 @@
 
 namespace Microsoft.Unity.Analyzers.Tests;
 
-public class SerializeFieldSuppressorTests : BaseSuppressorVerifierTest<SerializeFieldSuppressor>
+public class SerializedFieldSuppressorTests : BaseSuppressorVerifierTest<SerializedFieldSuppressor>
 {
 	[Fact]
 	public async Task PrivateFieldWithAttributeNeverAssignedSuppressed()
@@ -31,7 +31,7 @@ private void ReadField() {
 			.WithAnalyzerOption("dotnet_style_readonly_field", "false")
 			.WithAnalyzerFilter("IDE0051");
 
-		var suppressor = ExpectSuppressor(SerializeFieldSuppressor.NeverAssignedRule)
+		var suppressor = ExpectSuppressor(SerializedFieldSuppressor.NeverAssignedRule)
 			.WithLocation(7, 12);
 
 		await VerifyCSharpDiagnosticAsync(context, test, suppressor);
@@ -56,7 +56,7 @@ private void ReadField() {
 		var context = AnalyzerVerificationContext.Default
 			.WithAnalyzerFilter("IDE0051");
 
-		var suppressor = ExpectSuppressor(SerializeFieldSuppressor.ReadonlyRule)
+		var suppressor = ExpectSuppressor(SerializedFieldSuppressor.ReadonlyRule)
 			.WithLocation(7, 20);
 
 		await VerifyCSharpDiagnosticAsync(context, test, suppressor);
@@ -74,7 +74,7 @@ class Camera : MonoBehaviour
 }
 ";
 
-		var suppressor = ExpectSuppressor(SerializeFieldSuppressor.NeverAssignedRule)
+		var suppressor = ExpectSuppressor(SerializedFieldSuppressor.NeverAssignedRule)
 			.WithLocation(6, 19);
 
 		await VerifyCSharpDiagnosticAsync(test, suppressor);
@@ -91,7 +91,7 @@ class Test : System.Object
 ";
 
 		// We don't want to suppress 'never assigned' for standard types
-		var diagnostic = new DiagnosticResult(SerializeFieldSuppressor.NeverAssignedRule.SuppressedDiagnosticId, DiagnosticSeverity.Warning)
+		var diagnostic = new DiagnosticResult(SerializedFieldSuppressor.NeverAssignedRule.SuppressedDiagnosticId, DiagnosticSeverity.Warning)
 			.WithLocation(4, 19)
 			.WithMessage("Field 'Test.someField' is never assigned to, and will always have its default value null");
 
@@ -114,28 +114,7 @@ class Camera : MonoBehaviour
 		var context = AnalyzerVerificationContext.Default
 			.WithAnalyzerOption("dotnet_style_readonly_field", "false");
 
-		var suppressor = ExpectSuppressor(SerializeFieldSuppressor.UnusedRule)
-			.WithLocation(7, 12);
-
-		await VerifyCSharpDiagnosticAsync(context, test, suppressor);
-	}
-
-	[Fact]
-	public async Task PrivateFieldWithCreateAttributeUnusedSuppressed()
-	{
-		const string test = @"
-using UnityEngine;
-
-class Camera : MonoBehaviour
-{
-    [Unity.Properties.CreateProperty]
-    string someField = ""default"";
-}
-";
-		var context = AnalyzerVerificationContext.Default
-			.WithAnalyzerOption("dotnet_style_readonly_field", "false");
-
-		var suppressor = ExpectSuppressor(SerializeFieldSuppressor.UnusedRule)
+		var suppressor = ExpectSuppressor(SerializedFieldSuppressor.UnusedRule)
 			.WithLocation(7, 12);
 
 		await VerifyCSharpDiagnosticAsync(context, test, suppressor);

src/Microsoft.Unity.Analyzers/BaseAttributeSuppressor.cs

@@ -0,0 +1,65 @@
+/*--------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See LICENSE in the project root for license information.
+ *-------------------------------------------------------------------------------------------*/
+
+using System;
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace Microsoft.Unity.Analyzers;
+
+public abstract class BaseAttributeSuppressor : DiagnosticSuppressor
+{
+
+	protected abstract Type[] SuppressableAttributeTypes { get; }
+
+	public override void ReportSuppressions(SuppressionAnalysisContext context)
+	{
+		foreach (var diagnostic in context.ReportedDiagnostics)
+		{
+			AnalyzeDiagnostic(diagnostic, context);
+		}
+	}
+
+	private static bool IsSuppressableAttribute(INamedTypeSymbol? symbol, Type type)
+	{
+		return symbol != null && symbol.Matches(type);
+	}
+
+	protected virtual bool IsSuppressableAttribute(INamedTypeSymbol? symbol)
+	{
+		return SuppressableAttributeTypes.Any(type => IsSuppressableAttribute(symbol, type));
+	}
+
+	protected virtual bool IsSuppressable(ISymbol symbol)
+	{
+		return symbol.GetAttributes().Any(a => IsSuppressableAttribute(a.AttributeClass));
+	}
+
+	protected abstract SyntaxNode? GetSuppressibleNode(Diagnostic diagnostic, SuppressionAnalysisContext context);
+
+	private void AnalyzeDiagnostic(Diagnostic diagnostic, SuppressionAnalysisContext context)
+	{
+		var node = GetSuppressibleNode(diagnostic, context);
+		if (node == null)
+			return;
+
+		var syntaxTree = diagnostic.Location.SourceTree;
+		if (syntaxTree == null)
+			return;
+
+		var model = context.GetSemanticModel(syntaxTree);
+		var symbol = model.GetDeclaredSymbol(node);
+		if (symbol == null)
+			return;
+
+		if (!IsSuppressable(symbol))
+			return;
+
+		foreach (var descriptor in SupportedSuppressions.Where(d => d.SuppressedDiagnosticId == diagnostic.Id))
+			context.ReportSuppression(Suppression.Create(descriptor, diagnostic));
+	}
+}

src/Microsoft.Unity.Analyzers/ImplicitUsageAttributeSuppressor.cs

@@ -3,56 +3,42 @@
  *  Licensed under the MIT License. See LICENSE in the project root for license information.
  *-------------------------------------------------------------------------------------------*/
 
+using System;
 using System.Collections.Immutable;
-using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.Unity.Analyzers.Resources;
+using Unity.Properties;
 
 namespace Microsoft.Unity.Analyzers;
 
 [DiagnosticAnalyzer(LanguageNames.CSharp)]
-public class ImplicitUsageAttributeSuppressor : DiagnosticSuppressor
+public class ImplicitUsageAttributeSuppressor : BaseAttributeSuppressor
 {
 	internal static readonly SuppressionDescriptor Rule = new(
 		id: "USP0019",
 		suppressedDiagnosticId: "IDE0051",
 		justification: Strings.ImplicitUsageAttributeSuppressorJustification);
 
-	public override void ReportSuppressions(SuppressionAnalysisContext context)
-	{
-		foreach (var diagnostic in context.ReportedDiagnostics)
-		{
-			AnalyzeDiagnostic(diagnostic, context);
-		}
-	}
+	protected override Type[] SuppressableAttributeTypes =>
+	[
+		typeof(JetBrains.Annotations.UsedImplicitlyAttribute),
+		typeof(UnityEngine.Scripting.PreserveAttribute),
+		typeof(CreatePropertyAttribute)
+	];
 
 	public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions => [Rule];
 
-	private static void AnalyzeDiagnostic(Diagnostic diagnostic, SuppressionAnalysisContext context)
+	protected override SyntaxNode? GetSuppressibleNode(Diagnostic diagnostic, SuppressionAnalysisContext context)
 	{
-		var methodDeclarationSyntax = context.GetSuppressibleNode<MethodDeclarationSyntax>(diagnostic);
-		if (methodDeclarationSyntax == null)
-			return;
-
-		var syntaxTree = diagnostic.Location.SourceTree;
-		if (syntaxTree == null)
-			return;
-
-		var model = context.GetSemanticModel(syntaxTree);
-		if (model.GetDeclaredSymbol(methodDeclarationSyntax) is not IMethodSymbol methodSymbol)
-			return;
-
-		if (!IsSuppressable(methodSymbol))
-			return;
-
-		context.ReportSuppression(Suppression.Create(Rule, diagnostic));
+		return context.GetSuppressibleNode<MemberDeclarationSyntax>(diagnostic) as SyntaxNode
+			?? context.GetSuppressibleNode<VariableDeclaratorSyntax>(diagnostic);
 	}
 
-	private static bool IsSuppressable(IMethodSymbol methodSymbol)
+	protected override bool IsSuppressableAttribute(INamedTypeSymbol? symbol)
 	{
 		// The Unity code stripper will consider any attribute with the exact name "PreserveAttribute", regardless of the namespace or assembly
-		return methodSymbol.GetAttributes().Any(a => a.AttributeClass != null && (a.AttributeClass.Matches(typeof(JetBrains.Annotations.UsedImplicitlyAttribute)) || a.AttributeClass.Name == nameof(UnityEngine.Scripting.PreserveAttribute)));
+		return base.IsSuppressableAttribute(symbol) || symbol is { Name: nameof(UnityEngine.Scripting.PreserveAttribute) };
 	}
 }