Add validation for DirtyField strings (#5713)

* Add ValidateMemberAttribute, analyzer and test

* Use attribute on DirtyFields methods

* Defer member lookup

* Additional test case

* Add support for collection types

* Poke tests

* Revert "Add support for collection types"

This reverts commit 2b8f5534bd.

* break, not continue

* Cheaper attribute check with AttributeHelper

* Clean up unused helper method

---------

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

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

@@ -16,6 +16,7 @@
     <EmbeddedResource Include="..\Robust.Shared\Analyzers\PreferOtherTypeAttribute.cs" LogicalName="Robust.Shared.Analyzers.PreferOtherTypeAttribute.cs" LinkBase="Implementations" />
     <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\IoC\DependencyAttribute.cs" LogicalName="Robust.Shared.IoC.DependencyAttribute.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\GameObjects\EventBusAttributes.cs" LogicalName="Robust.Shared.GameObjects.EventBusAttributes.cs" LinkBase="Implementations" />
     <EmbeddedResource Include="..\Robust.Shared\Serialization\NetSerializableAttribute.cs" LogicalName="Robust.Shared.Serialization.NetSerializableAttribute.cs" LinkBase="Implementations" />

Robust.Analyzers.Tests/ValidateMemberAnalyzerTest.cs

@@ -0,0 +1,96 @@
+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.ValidateMemberAnalyzer, Microsoft.CodeAnalysis.Testing.DefaultVerifier>;
+
+
+namespace Robust.Analyzers.Tests;
+
+public sealed class ValidateMemberAnalyzerTest
+{
+    private static Task Verifier(string code, params DiagnosticResult[] expected)
+    {
+        var test = new CSharpAnalyzerTest<ValidateMemberAnalyzer, DefaultVerifier>()
+        {
+            TestState =
+            {
+                Sources = { code },
+            },
+        };
+
+        TestHelper.AddEmbeddedSources(
+            test.TestState,
+            "Robust.Shared.Analyzers.ValidateMemberAttribute.cs"
+        );
+
+        // ExpectedDiagnostics cannot be set, so we need to AddRange here...
+        test.TestState.ExpectedDiagnostics.AddRange(expected);
+
+        return test.RunAsync();
+    }
+
+    [Test]
+    public async Task Test()
+    {
+        const string code = """
+            using System;
+            using Robust.Shared.Analyzers;
+
+            public sealed class TestComponent
+            {
+                public int IntField;
+                public bool BoolField;
+            }
+
+            public sealed class OtherComponent
+            {
+                public float FloatField;
+                public double DoubleField;
+            }
+
+            public sealed class TestManager
+            {
+                public static void DirtyField<T>(T comp, [ValidateMember]string fieldName) { }
+                public static void DirtyTwoFields<T>(T comp, [ValidateMember]string first, [ValidateMember]string second) { }
+            }
+
+            public sealed class TestCaller
+            {
+                public void Test()
+                {
+                    var testComp = new TestComponent();
+                    var otherComp = new OtherComponent();
+
+                    TestManager.DirtyField(testComp, nameof(TestComponent.IntField));
+
+                    TestManager.DirtyField(testComp, nameof(OtherComponent.FloatField));
+
+                    TestManager.DirtyField(otherComp, nameof(TestComponent.IntField));
+
+                    TestManager.DirtyField(otherComp, nameof(OtherComponent.FloatField));
+
+                    TestManager.DirtyTwoFields(testComp, nameof(TestComponent.IntField), nameof(TestComponent.BoolField));
+
+                    TestManager.DirtyTwoFields(testComp, nameof(TestComponent.IntField), nameof(OtherComponent.FloatField));
+
+                    TestManager.DirtyTwoFields(testComp, nameof(OtherComponent.FloatField), nameof(OtherComponent.DoubleField));
+                }
+            }
+            """;
+
+        await Verifier(code,
+            // /0/Test0.cs(31,42): error RA0033: FloatField is not a member of TestComponent
+            VerifyCS.Diagnostic().WithSpan(31, 42, 31, 75).WithArguments("FloatField", "TestComponent"),
+            // /0/Test0.cs(33,43): error RA0033: IntField is not a member of OtherComponent
+            VerifyCS.Diagnostic().WithSpan(33, 43, 33, 73).WithArguments("IntField", "OtherComponent"),
+            // /0/Test0.cs(39,78): error RA0033: FloatField is not a member of TestComponent
+            VerifyCS.Diagnostic().WithSpan(39, 78, 39, 111).WithArguments("FloatField", "TestComponent"),
+            // /0/Test0.cs(41,46): error RA0033: FloatField is not a member of TestComponent
+            VerifyCS.Diagnostic().WithSpan(41, 46, 41, 79).WithArguments("FloatField", "TestComponent"),
+            // /0/Test0.cs(41,81): error RA0033: DoubleField is not a member of TestComponent
+            VerifyCS.Diagnostic().WithSpan(41, 81, 41, 115).WithArguments("DoubleField", "TestComponent")
+        );
+    }
+}

Robust.Analyzers/ITypeSymbolExtensions.cs

@@ -0,0 +1,16 @@
+using Microsoft.CodeAnalysis;
+
+namespace Robust.Analyzers;
+
+public static  class ITypeSymbolExtensions
+{
+    public static IEnumerable<ITypeSymbol> GetBaseTypesAndThis(this ITypeSymbol type)
+    {
+        var current = type;
+        while (current != null)
+        {
+            yield return current;
+            current = current.BaseType;
+        }
+    }
+}

Robust.Analyzers/ValidateMemberAnalyzer.cs

@@ -0,0 +1,95 @@
+#nullable enable
+using System.Collections.Immutable;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+using Robust.Roslyn.Shared;
+
+namespace Robust.Analyzers;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class ValidateMemberAnalyzer : DiagnosticAnalyzer
+{
+    private const string ValidateMemberType = "Robust.Shared.Analyzers.ValidateMemberAttribute";
+
+    private static readonly DiagnosticDescriptor ValidateMemberDescriptor = new(
+        Diagnostics.IdValidateMember,
+        "Invalid member name",
+        "{0} is not a member of {1}",
+        "Usage",
+        DiagnosticSeverity.Error,
+        true,
+        "Be sure the type and member name are correct.");
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [ValidateMemberDescriptor];
+
+    public override void Initialize(AnalysisContext context)
+    {
+        context.EnableConcurrentExecution();
+        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
+        context.RegisterSyntaxNodeAction(AnalyzeExpression, SyntaxKind.InvocationExpression);
+    }
+
+    private void AnalyzeExpression(SyntaxNodeAnalysisContext context)
+    {
+        if (context.Node is not InvocationExpressionSyntax node)
+            return;
+
+        if (context.SemanticModel.GetSymbolInfo(node.Expression).Symbol is not IMethodSymbol methodSymbol)
+            return;
+
+        // We need at least one type argument for context
+        if (methodSymbol.TypeArguments.Length < 1)
+            return;
+
+        // We'll be checking members of the first type argument
+        if (methodSymbol.TypeArguments[0] is not INamedTypeSymbol targetType)
+            return;
+
+        // We defer building this set until we need it later, so we don't have to build it for every single method invocation!
+        ImmutableHashSet<ISymbol>? members = null;
+
+        // Check each parameter of the method
+        foreach (var parameterContext in node.ArgumentList.Arguments)
+        {
+
+            // Get the symbol for this parameter
+            if (context.SemanticModel.GetOperation(parameterContext) is not IArgumentOperation op || op.Parameter is null)
+                continue;
+            var parameterSymbol = op.Parameter.OriginalDefinition;
+
+            // Make sure the parameter has the ValidateMember attribute
+            if (!AttributeHelper.HasAttribute(parameterSymbol, ValidateMemberType, out _))
+                continue;
+
+            // Find the value passed for this parameter.
+            // We use GetConstantValue to resolve compile-time values - i.e. the result of nameof()
+            if (context.SemanticModel.GetConstantValue(parameterContext.Expression).Value is not string fieldName)
+                continue;
+
+            // Get a set containing all the members of the target type and its ancestors
+            members ??= targetType.GetBaseTypesAndThis().SelectMany(n => n.GetMembers()).ToImmutableHashSet(SymbolEqualityComparer.Default);
+
+            // Check each member of the target type to see if it matches our passed in value
+            var found = false;
+            foreach (var member in members)
+            {
+                if (member.Name == fieldName)
+                {
+                    found = true;
+                    break;
+                }
+            }
+            // If we didn't find it, report the violation
+            if (!found)
+                context.ReportDiagnostic(Diagnostic.Create(
+                    ValidateMemberDescriptor,
+                    parameterContext.GetLocation(),
+                    fieldName,
+                    targetType.Name
+                    ));
+        }
+    }
+}

Robust.Roslyn.Shared/Diagnostics.cs

@@ -47,6 +47,7 @@ public static class Diagnostics
     public const string IdAutoGenStateParamMissing = "RA0041";
     public const string IdPrototypeRedundantType = "RA0042";
     public const string IdPrototypeEndsWithPrototype = "RA0043";
+    public const string IdValidateMember = "RA0044";
 
     public static SuppressionDescriptor MeansImplicitAssignment =>
         new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned.");

Robust.Shared/Analyzers/ValidateMemberAttribute.cs

@@ -0,0 +1,15 @@
+using System;
+
+namespace Robust.Shared.Analyzers;
+
+/// <summary>
+/// Verifies that a string parameter matches the name
+/// of a member of the first type argument.
+/// </summary>
+/// <remarks>
+/// This just does a string comparison with the member name.
+/// An identically-named member on a different class will be
+/// considered valid.
+/// </remarks>
+[AttributeUsage(AttributeTargets.Parameter)]
+public sealed class ValidateMemberAttribute : Attribute;

Robust.Shared/GameObjects/EntityManager.ComponentDeltas.cs

@@ -39,7 +39,7 @@ public abstract partial class EntityManager
         Dirty(uid, comp, metadata);
     }
 
-    public virtual void DirtyField<T>(EntityUid uid, T comp, string fieldName, MetaDataComponent? metadata = null)
+    public virtual void DirtyField<T>(EntityUid uid, T comp, [ValidateMember] string fieldName, MetaDataComponent? metadata = null)
         where T : IComponentDelta
     {
         var compReg = ComponentFactory.GetRegistration(CompIdx.Index<T>());

Robust.Shared/GameObjects/EntitySystem.Proxy.cs

@@ -155,7 +155,7 @@ public partial class EntitySystem
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    protected void DirtyField<T>(Entity<T?> entity, string fieldName, MetaDataComponent? meta = null)
+    protected void DirtyField<T>(Entity<T?> entity, [ValidateMember]string fieldName, MetaDataComponent? meta = null)
         where T : IComponentDelta
     {
         if (!Resolve(entity.Owner, ref entity.Comp))
@@ -165,7 +165,7 @@ public partial class EntitySystem
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    protected void DirtyField<T>(EntityUid uid, T component, string fieldName, MetaDataComponent? meta = null)
+    protected void DirtyField<T>(EntityUid uid, T component, [ValidateMember]string fieldName, MetaDataComponent? meta = null)
         where T : IComponentDelta
     {
         EntityManager.DirtyField(uid, component, fieldName, meta);