Check that [Virtual] is not used on sealed/abstract/static classes (#6486)

* Add exclusivity check for virtual + analyzer test

* Cleanup

---------

Co-authored-by: PJB3005 <pieterjan.briers+git@gmail.com>

Robust.Analyzers.Tests/ExplicitVirtualAnalyzerTest.cs

@@ -0,0 +1,111 @@
+using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.CSharp.Testing;
+using Microsoft.CodeAnalysis.Testing;
+using NUnit.Framework;
+using VerifyCS =
+    Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier<Robust.Analyzers.ExplicitVirtualAnalyzer, Microsoft.CodeAnalysis.Testing.DefaultVerifier>;
+
+namespace Robust.Analyzers.Tests;
+
+[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)]
+[TestFixture]
+[TestOf(typeof(ExplicitVirtualAnalyzer))]
+public sealed class ExplicitVirtualAnalyzerTest
+{
+    private static Task Verifier(string code, params DiagnosticResult[] expected)
+    {
+        var test = new CSharpAnalyzerTest<ExplicitVirtualAnalyzer, DefaultVerifier>()
+        {
+            TestState =
+            {
+                Sources = { code },
+            },
+        };
+
+        TestHelper.AddEmbeddedSources(
+            test.TestState,
+            "Robust.Shared.Analyzers.VirtualAttribute.cs"
+        );
+
+        // ExpectedDiagnostics cannot be set, so we need to AddRange here...
+        test.TestState.ExpectedDiagnostics.AddRange(expected);
+
+        return test.RunAsync();
+    }
+
+    [Test]
+    [Description("Ensures that a non-sealed/abstract/static class not marked as [Virtual] raises a warning.")]
+    public async Task NoVirtualOrOther()
+    {
+        const string code = """
+            public class Foo { }
+            """;
+
+        await Verifier(code,
+            // /0/Test0.cs(1,8): warning RA0003: Class must be explicitly marked as [Virtual], abstract, static, or sealed
+            VerifyCS.Diagnostic(ExplicitVirtualAnalyzer.ExplicitVirtualRule).WithSpan(1, 8, 1, 13)
+        );
+    }
+
+    [Test]
+    [Description("Ensures that a non-sealed/abstract/static class explicitly marked as [Virtual] does not raise a warning.")]
+    public async Task OnlyVirtual()
+    {
+        const string code = """
+            using Robust.Shared.Analyzers;
+
+            [Virtual]
+            public class Foo { }
+            """;
+
+        await Verifier(code, []);
+    }
+
+    [Test]
+    [Description("Ensures that a sealed class marked as [Virtual] raises an error.")]
+    public async Task SealedVirtual()
+    {
+        const string code = """
+            using Robust.Shared.Analyzers;
+
+            [Virtual]
+            public sealed class Foo { }
+            """;
+
+        await Verifier(code,
+            // /0/Test0.cs(4,15): error RA0048: A class marked as [Virtual] cannot be abstract, static, or sealed
+            VerifyCS.Diagnostic(ExplicitVirtualAnalyzer.ExclusiveRule).WithSpan(4, 15, 4, 20));
+    }
+
+    [Test]
+    [Description("Ensures that an abstract class marked as [Virtual] raises an error.")]
+    public async Task AbstractVirtual()
+    {
+        const string code = """
+            using Robust.Shared.Analyzers;
+
+            [Virtual]
+            public abstract class Foo { }
+            """;
+
+        await Verifier(code,
+            // /0/Test0.cs(4,17): error RA0048: A class marked as [Virtual] cannot be abstract, static, or sealed
+            VerifyCS.Diagnostic(ExplicitVirtualAnalyzer.ExclusiveRule).WithSpan(4, 17, 4, 22));
+    }
+
+    [Test]
+    [Description("Ensures that a static class marked as [Virtual] raises an error.")]
+    public async Task StaticVirtual()
+    {
+        const string code = """
+            using Robust.Shared.Analyzers;
+
+            [Virtual]
+            public static class Foo { }
+            """;
+
+        await Verifier(code,
+            // /0/Test0.cs(4,15): error RA0048: A class marked as [Virtual] cannot be abstract, static, or sealed
+            VerifyCS.Diagnostic(ExplicitVirtualAnalyzer.ExclusiveRule).WithSpan(4, 15, 4, 20));
+    }
+}

Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj

@@ -18,6 +18,7 @@
     <EmbeddedResource Include="..\Robust.Shared\Analyzers\ForbidLiteralAttribute.cs" LogicalName="Robust.Shared.Analyzers.ForbidLiteralAttribute.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\Analyzers\ObsoleteInheritanceAttribute.cs" LogicalName="Robust.Shared.Analyzers.ObsoleteInheritanceAttribute.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\Analyzers\ValidateMemberAttribute.cs" LogicalName="Robust.Shared.Analyzers.ValidateMemberAttribute.cs" LinkBase="Implementations" />
+    <EmbeddedResource Include="..\Robust.Shared\Analyzers\VirtualAttribute.cs" LogicalName="Robust.Shared.Analyzers.VirtualAttribute.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\IoC\DependencyAttribute.cs" LogicalName="Robust.Shared.IoC.DependencyAttribute.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\IoC\IHasDependencies.cs" LogicalName="Robust.Shared.IoC.IHasDependencies.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\GameObjects\EventBusAttributes.cs" LogicalName="Robust.Shared.GameObjects.EventBusAttributes.cs" LinkBase="Implementations" />

Robust.Analyzers/ExplicitVirtualAnalyzer.cs

@@ -1,6 +1,5 @@
 using System.Collections.Immutable;
 using System.Diagnostics.CodeAnalysis;
-using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -15,45 +14,68 @@ public sealed class ExplicitVirtualAnalyzer : DiagnosticAnalyzer
     internal const string Attribute = "Robust.Shared.Analyzers.VirtualAttribute";
 
     [SuppressMessage("ReSharper", "RS2008")]
-    private static readonly DiagnosticDescriptor Rule = new(
+    public static readonly DiagnosticDescriptor ExplicitVirtualRule = new(
         Diagnostics.IdExplicitVirtual,
-        "Class must be explicitly marked as [Virtual], abstract, static or sealed",
-        "Class must be explicitly marked as [Virtual], abstract, static or sealed",
+        "Class must be explicitly marked as [Virtual], abstract, static, or sealed",
+        "Class must be explicitly marked as [Virtual], abstract, static, or sealed",
         "Usage",
         DiagnosticSeverity.Warning,
         isEnabledByDefault: true,
-        description: "Class must be explicitly marked as [Virtual], abstract, static or sealed.");
+        description: "Class must be explicitly marked as [Virtual], abstract, static, or sealed.");
 
-    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+    public static readonly DiagnosticDescriptor ExclusiveRule = new(
+        Diagnostics.IdExclusiveVirtual,
+        "A class marked as [Virtual] cannot be abstract, static, or sealed",
+        "A class marked as [Virtual] cannot be abstract, static, or sealed",
+        "Usage",
+        DiagnosticSeverity.Error,
+        isEnabledByDefault: true,
+        description: "A class marked as [Virtual] cannot be abstract, static, or sealed.");
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
+    [
+        ExplicitVirtualRule,
+        ExclusiveRule,
+    ];
 
     public override void Initialize(AnalysisContext context)
     {
         context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
         context.EnableConcurrentExecution();
-        context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.ClassDeclaration);
+        context.RegisterCompilationStartAction(ctx =>
+        {
+            if (ctx.Compilation.GetTypeByMetadataName(Attribute) is not INamedTypeSymbol attrSymbol)
+                return;
+
+            ctx.RegisterSyntaxNodeAction(nodeContext => AnalyzeNode(nodeContext, attrSymbol), SyntaxKind.ClassDeclaration);
+        });
     }
 
-    private static bool HasAttribute(INamedTypeSymbol namedTypeSymbol, INamedTypeSymbol attrSymbol)
+    private static void AnalyzeNode(SyntaxNodeAnalysisContext context, INamedTypeSymbol attrSymbol)
     {
-        return namedTypeSymbol.GetAttributes()
-            .Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, attrSymbol));
-    }
-
-    private static void AnalyzeNode(SyntaxNodeAnalysisContext context)
-    {
-        var attrSymbol = context.Compilation.GetTypeByMetadataName(Attribute);
         var classDecl = (ClassDeclarationSyntax)context.Node;
         var classSymbol = context.SemanticModel.GetDeclaredSymbol(classDecl);
         if (classSymbol == null)
             return;
 
-        if (classSymbol.IsSealed || classSymbol.IsAbstract || classSymbol.IsStatic)
+        var hasKeyword = classSymbol.IsSealed || classSymbol.IsAbstract || classSymbol.IsStatic;
+        var hasAttribute = AttributeHelper.HasAttribute(classSymbol, attrSymbol, out _);
+
+        if (hasAttribute && hasKeyword)
+        {
+            // Having both [Virtual] and sealed/abstract/static doesn't make sense.
+            context.ReportDiagnostic(Diagnostic.Create(
+                ExclusiveRule,
+                classDecl.Keyword.GetLocation()
+            ));
+        }
+
+        // Having just [Virtual] or sealed/abstract/static is fine.
+        if (hasKeyword || hasAttribute)
             return;
 
-        if (HasAttribute(classSymbol, attrSymbol))
-            return;
-
-        var diag = Diagnostic.Create(Rule, classDecl.Keyword.GetLocation());
+        // Having neither is bad.
+        var diag = Diagnostic.Create(ExplicitVirtualRule, classDecl.Keyword.GetLocation());
         context.ReportDiagnostic(diag);
     }
 }

Robust.Roslyn.Shared/Diagnostics.cs

@@ -56,6 +56,7 @@ public static class Diagnostics
     public const string IdHasDependenciesNotPartialParent = "RA0050";
     public const string IdHasDependenciesReadOnly = "RA0051";
     public const string IdHasDependenciesPropertyField = "RA0052";
+    public const string IdExclusiveVirtual = "RA0053";
 
     public static SuppressionDescriptor MeansImplicitAssignment =>
         new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned.");