Add analyzer/fixer for DataFields with ReadWrite VV (#5164)

* Add analyzer/fixer for datafields with ReadWrite VV

* Nothing to see here

Robust.Analyzers.Tests/DataDefinitionAnalyzerTest.cs

@@ -0,0 +1,91 @@
+using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.CSharp.Testing;
+using Microsoft.CodeAnalysis.Testing;
+using Microsoft.CodeAnalysis.Testing.Verifiers;
+using NUnit.Framework;
+using VerifyCS =
+    Microsoft.CodeAnalysis.CSharp.Testing.NUnit.AnalyzerVerifier<Robust.Analyzers.DataDefinitionAnalyzer>;
+
+namespace Robust.Analyzers.Tests;
+
+[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)]
+[TestFixture]
+public sealed class DataDefinitionAnalyzerTest
+{
+    private static Task Verifier(string code, params DiagnosticResult[] expected)
+    {
+        var test = new CSharpAnalyzerTest<DataDefinitionAnalyzer, NUnitVerifier>()
+        {
+            TestState =
+            {
+                Sources = { code }
+            },
+        };
+
+        // 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.ViewVariables;
+            using Robust.Shared.Serialization.Manager.Attributes;
+
+            namespace Robust.Shared.ViewVariables
+            {
+                public sealed class ViewVariablesAttribute : Attribute
+                {
+                    public readonly VVAccess Access = VVAccess.ReadOnly;
+
+                    public ViewVariablesAttribute() { }
+
+                    public ViewVariablesAttribute(VVAccess access)
+                    {
+                        Access = access;
+                    }
+                }
+                public enum VVAccess : byte
+                {
+                    ReadOnly = 0,
+                    ReadWrite = 1,
+                }
+            }
+
+            namespace Robust.Shared.Serialization.Manager.Attributes
+            {
+                public class DataFieldBaseAttribute : Attribute;
+                public class DataFieldAttribute : DataFieldBaseAttribute;
+                public sealed class DataDefinitionAttribute : Attribute;
+            }
+
+            [DataDefinition]
+            public sealed partial class Foo
+            {
+                [DataField, ViewVariables(VVAccess.ReadWrite)]
+                public int Bad;
+
+                [DataField]
+                public int Good;
+
+                [DataField, ViewVariables]
+                public int Good2;
+
+                [DataField, ViewVariables(VVAccess.ReadOnly)]
+                public int Good3;
+
+                [ViewVariables]
+                public int Good4;
+            }
+            """;
+
+        await Verifier(code,
+            // /0/Test0.cs(35,5): info RA0028: Data field Bad in data definition Foo has ViewVariables attribute with ReadWrite access, which is redundant
+            VerifyCS.Diagnostic(DataDefinitionAnalyzer.DataFieldNoVVReadWriteRule).WithSpan(35, 5, 36, 20).WithArguments("Bad", "Foo")
+        );
+    }
+}

Robust.Analyzers/DataDefinitionAnalyzer.cs

@@ -7,6 +7,7 @@ using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Robust.Roslyn.Shared;
 using Robust.Shared.Serialization.Manager.Definition;
+using Robust.Shared.ViewVariables;
 
 namespace Robust.Analyzers;
 
@@ -16,6 +17,7 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer
     private const string DataDefinitionNamespace = "Robust.Shared.Serialization.Manager.Attributes.DataDefinitionAttribute";
     private const string ImplicitDataDefinitionNamespace = "Robust.Shared.Serialization.Manager.Attributes.ImplicitDataDefinitionForInheritorsAttribute";
     private const string DataFieldBaseNamespace = "Robust.Shared.Serialization.Manager.Attributes.DataFieldBaseAttribute";
+    private const string ViewVariablesNamespace = "Robust.Shared.ViewVariables.ViewVariablesAttribute";
 
     private static readonly DiagnosticDescriptor DataDefinitionPartialRule = new(
         Diagnostics.IdDataDefinitionPartial,
@@ -66,9 +68,20 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer
         true,
         "Make sure to remove the tag string from the data field attribute."
     );
+
+    public static readonly DiagnosticDescriptor DataFieldNoVVReadWriteRule = new(
+        Diagnostics.IdDataFieldNoVVReadWrite,
+        "Data field has VV ReadWrite",
+        "Data field {0} in data definition {1} has ViewVariables attribute with ReadWrite access, which is redundant",
+        "Usage",
+        DiagnosticSeverity.Info,
+        true,
+        "Make sure to remove the ViewVariables attribute."
+    );
+
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(
         DataDefinitionPartialRule, NestedDataDefinitionPartialRule, DataFieldWritableRule, DataFieldPropertyWritableRule,
-        DataFieldRedundantTagRule
+        DataFieldRedundantTagRule, DataFieldNoVVReadWriteRule
     );
 
     public override void Initialize(AnalysisContext context)
@@ -141,6 +154,11 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer
             {
                 context.ReportDiagnostic(Diagnostic.Create(DataFieldRedundantTagRule, context.Node.GetLocation(), fieldSymbol.Name, type.Name));
             }
+
+            if (HasVVReadWrite(fieldSymbol))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(DataFieldNoVVReadWriteRule, context.Node.GetLocation(), fieldSymbol.Name, type.Name));
+            }
         }
     }
 
@@ -170,6 +188,11 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer
         {
             context.ReportDiagnostic(Diagnostic.Create(DataFieldRedundantTagRule, context.Node.GetLocation(), propertySymbol.Name, type.Name));
         }
+
+        if (HasVVReadWrite(propertySymbol))
+        {
+            context.ReportDiagnostic(Diagnostic.Create(DataFieldNoVVReadWriteRule, context.Node.GetLocation(), propertySymbol.Name, type.Name));
+        }
     }
 
     private static bool IsReadOnlyDataField(ITypeSymbol type, ISymbol field)
@@ -292,6 +315,34 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer
         return explicitName == automaticName;
     }
 
+    private static bool HasVVReadWrite(ISymbol symbol)
+    {
+        if (!IsDataField(symbol, out _, out _))
+            return false;
+
+        // Make sure it has ViewVariablesAttribute
+        AttributeData? viewVariablesAttribute = null;
+        foreach (var attr in symbol.GetAttributes())
+        {
+            if (attr.AttributeClass?.ToDisplayString() == ViewVariablesNamespace)
+            {
+                viewVariablesAttribute = attr;
+            }
+        }
+        if (viewVariablesAttribute == null)
+            return false;
+
+        // Default is ReadOnly, which is fine
+        if (viewVariablesAttribute.ConstructorArguments.Length == 0)
+            return false;
+
+        var accessArgument = viewVariablesAttribute.ConstructorArguments[0];
+        if (accessArgument.Value is not byte accessByte)
+            return false;
+
+        return (VVAccess)accessByte == VVAccess.ReadWrite;
+    }
+
     private static bool IsImplicitDataDefinition(ITypeSymbol type)
     {
         if (HasAttribute(type, ImplicitDataDefinitionNamespace))

Robust.Analyzers/DataDefinitionFixer.cs

@@ -15,9 +15,11 @@ public sealed class DefinitionFixer : CodeFixProvider
 {
     private const string DataFieldAttributeName = "DataField";
 
+    private const string ViewVariablesAttributeName = "ViewVariables";
+
     public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(
         IdDataDefinitionPartial, IdNestedDataDefinitionPartial, IdDataFieldWritable, IdDataFieldPropertyWritable,
-        IdDataFieldRedundantTag
+        IdDataFieldRedundantTag, IdDataFieldNoVVReadWrite
     );
 
     public override Task RegisterCodeFixesAsync(CodeFixContext context)
@@ -36,6 +38,8 @@ public sealed class DefinitionFixer : CodeFixProvider
                     return RegisterDataFieldPropertyFix(context, diagnostic);
                 case IdDataFieldRedundantTag:
                     return RegisterRedundantTagFix(context, diagnostic);
+                case IdDataFieldNoVVReadWrite:
+                    return RegisterVVReadWriteFix(context, diagnostic);
             }
         }
 
@@ -136,6 +140,48 @@ public sealed class DefinitionFixer : CodeFixProvider
         return document.WithSyntaxRoot(root);
     }
 
+    private static async Task RegisterVVReadWriteFix(CodeFixContext context, Diagnostic diagnostic)
+    {
+        var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken);
+        var span = diagnostic.Location.SourceSpan;
+        var token = root?.FindToken(span.Start).Parent?.AncestorsAndSelf().OfType<MemberDeclarationSyntax>().First();
+
+        if (token == null)
+            return;
+
+        context.RegisterCodeFix(CodeAction.Create(
+            "Remove ViewVariables attribute",
+            c => RemoveVVAttribute(context.Document, token, c),
+            "Remove ViewVariables attribute"
+        ), diagnostic);
+    }
+
+    private static async Task<Document> RemoveVVAttribute(Document document, MemberDeclarationSyntax syntax, CancellationToken cancellation)
+    {
+        var root = (CompilationUnitSyntax?) await document.GetSyntaxRootAsync(cancellation);
+
+        var newLists = new SyntaxList<AttributeListSyntax>();
+        foreach (var attributeList in syntax.AttributeLists)
+        {
+            var attributes = new SeparatedSyntaxList<AttributeSyntax>();
+            foreach (var attribute in attributeList.Attributes)
+            {
+                if (attribute.Name.ToString() != ViewVariablesAttributeName)
+                {
+                    attributes = attributes.Add(attribute);
+                }
+            }
+            // Don't add empty lists []
+            if (attributes.Count > 0)
+                newLists = newLists.Add(attributeList.WithAttributes(attributes));
+        }
+        var newSyntax = syntax.WithAttributeLists(newLists);
+
+        root = root!.ReplaceNode(syntax, newSyntax);
+
+        return document.WithSyntaxRoot(root);
+    }
+
     private static async Task RegisterDataFieldFix(CodeFixContext context, Diagnostic diagnostic)
     {
         var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken);

Robust.Analyzers/Robust.Analyzers.csproj

@@ -19,6 +19,8 @@
   <ItemGroup>
     <!-- Needed for DataDefinitionAnalyzer. -->
     <Compile Include="..\Robust.Shared\Serialization\Manager\Definition\DataDefinitionUtility.cs" LinkBase="Implementations" />
+    <Compile Include="..\Robust.Shared\ViewVariables\ViewVariablesAttribute.cs" LinkBase="Implementations" />
+    <Compile Include="..\Robust.Shared\Serialization\NetSerializableAttribute.cs" LinkBase="Implementations" />
   </ItemGroup>
 
   <Import Project="../Robust.Roslyn.Shared/Robust.Roslyn.Shared.props" />

Robust.Roslyn.Shared/Diagnostics.cs

@@ -32,6 +32,7 @@ public static class Diagnostics
     public const string IdUncachedRegex = "RA0026";
     public const string IdDataFieldRedundantTag = "RA0027";
     public const string IdMustCallBase = "RA0028";
+    public const string IdDataFieldNoVVReadWrite = "RA0029";
 
     public static SuppressionDescriptor MeansImplicitAssignment =>
         new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned.");