From da7abc65800ba2e85c0c72d57a515673165a7c16 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Fri, 17 May 2024 01:44:03 -0400 Subject: [PATCH] Add analyzer and fixer for redundant DataField tag arguments (#5134) * Add analyzer and fixer for redundant DataField tag arguments * Share Tag autogeneration logic --- Robust.Analyzers/DataDefinitionAnalyzer.cs | 46 +++++++++++- Robust.Analyzers/DataDefinitionFixer.cs | 72 +++++++++++++++++-- Robust.Analyzers/Robust.Analyzers.csproj | 5 ++ Robust.Roslyn.Shared/Diagnostics.cs | 1 + .../Manager/Definition/DataDefinition.cs | 3 +- .../Definition/DataDefinitionUtility.cs | 12 ++++ 6 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 Robust.Shared/Serialization/Manager/Definition/DataDefinitionUtility.cs diff --git a/Robust.Analyzers/DataDefinitionAnalyzer.cs b/Robust.Analyzers/DataDefinitionAnalyzer.cs index 14910c10a..f658a2aaa 100644 --- a/Robust.Analyzers/DataDefinitionAnalyzer.cs +++ b/Robust.Analyzers/DataDefinitionAnalyzer.cs @@ -6,6 +6,7 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Robust.Roslyn.Shared; +using Robust.Shared.Serialization.Manager.Definition; namespace Robust.Analyzers; @@ -56,8 +57,18 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer "Make sure to add a setter." ); + private static readonly DiagnosticDescriptor DataFieldRedundantTagRule = new( + Diagnostics.IdDataFieldRedundantTag, + "Data field has redundant tag specified", + "Data field {0} in data definition {1} has an explicitly set tag that matches autogenerated tag", + "Usage", + DiagnosticSeverity.Info, + true, + "Make sure to remove the tag string from the data field attribute." + ); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create( - DataDefinitionPartialRule, NestedDataDefinitionPartialRule, DataFieldWritableRule, DataFieldPropertyWritableRule + DataDefinitionPartialRule, NestedDataDefinitionPartialRule, DataFieldWritableRule, DataFieldPropertyWritableRule, + DataFieldRedundantTagRule ); public override void Initialize(AnalysisContext context) @@ -125,6 +136,11 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer { context.ReportDiagnostic(Diagnostic.Create(DataFieldWritableRule, context.Node.GetLocation(), fieldSymbol.Name, type.Name)); } + + if (HasRedundantTag(fieldSymbol)) + { + context.ReportDiagnostic(Diagnostic.Create(DataFieldRedundantTagRule, context.Node.GetLocation(), fieldSymbol.Name, type.Name)); + } } } @@ -149,6 +165,11 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer { context.ReportDiagnostic(Diagnostic.Create(DataFieldPropertyWritableRule, context.Node.GetLocation(), propertySymbol.Name, type.Name)); } + + if (HasRedundantTag(propertySymbol)) + { + context.ReportDiagnostic(Diagnostic.Create(DataFieldRedundantTagRule, context.Node.GetLocation(), propertySymbol.Name, type.Name)); + } } private static bool IsReadOnlyDataField(ITypeSymbol type, ISymbol field) @@ -248,6 +269,29 @@ public sealed class DataDefinitionAnalyzer : DiagnosticAnalyzer return false; } + private static bool HasRedundantTag(ISymbol symbol) + { + if (!IsDataField(symbol, out var _, out var attribute)) + return false; + + // No args, no problem + if (attribute.ConstructorArguments.Length == 0) + return false; + + // If a tag is explicitly specified, it will be the first argument... + var tagArgument = attribute.ConstructorArguments[0]; + // ...but the first arg could also something else, since tag is optional + // so we make sure that it's a string + if (tagArgument.Value is not string explicitName) + return false; + + // Get the name that sourcegen would provide + var automaticName = DataDefinitionUtility.AutoGenerateTag(symbol.Name); + + // If the explicit name matches the sourcegen name, we have a redundancy + return explicitName == automaticName; + } + private static bool IsImplicitDataDefinition(ITypeSymbol type) { if (HasAttribute(type, ImplicitDataDefinitionNamespace)) diff --git a/Robust.Analyzers/DataDefinitionFixer.cs b/Robust.Analyzers/DataDefinitionFixer.cs index 80ba22a96..3ec8795dd 100644 --- a/Robust.Analyzers/DataDefinitionFixer.cs +++ b/Robust.Analyzers/DataDefinitionFixer.cs @@ -1,8 +1,5 @@ #nullable enable using System.Collections.Immutable; -using System.Linq; -using System.Threading; -using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; @@ -16,8 +13,11 @@ namespace Robust.Analyzers; [ExportCodeFixProvider(LanguageNames.CSharp)] public sealed class DefinitionFixer : CodeFixProvider { + private const string DataFieldAttributeName = "DataField"; + public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create( - IdDataDefinitionPartial, IdNestedDataDefinitionPartial, IdDataFieldWritable, IdDataFieldPropertyWritable + IdDataDefinitionPartial, IdNestedDataDefinitionPartial, IdDataFieldWritable, IdDataFieldPropertyWritable, + IdDataFieldRedundantTag ); public override Task RegisterCodeFixesAsync(CodeFixContext context) @@ -34,6 +34,8 @@ public sealed class DefinitionFixer : CodeFixProvider return RegisterDataFieldFix(context, diagnostic); case IdDataFieldPropertyWritable: return RegisterDataFieldPropertyFix(context, diagnostic); + case IdDataFieldRedundantTag: + return RegisterRedundantTagFix(context, diagnostic); } } @@ -72,6 +74,68 @@ public sealed class DefinitionFixer : CodeFixProvider return document.WithSyntaxRoot(root); } + private static async Task RegisterRedundantTagFix(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().First(); + + if (token == null) + return; + + // Find the DataField attribute + AttributeSyntax? dataFieldAttribute = null; + foreach (var attributeList in token.AttributeLists) + { + foreach (var attribute in attributeList.Attributes) + { + if (attribute.Name.ToString() == DataFieldAttributeName) + { + dataFieldAttribute = attribute; + break; + } + } + if (dataFieldAttribute != null) + break; + } + + if (dataFieldAttribute == null) + return; + + context.RegisterCodeFix(CodeAction.Create( + "Remove explicitly set tag", + c => RemoveRedundantTag(context.Document, dataFieldAttribute, c), + "Remove explicitly set tag" + ), diagnostic); + } + + private static async Task RemoveRedundantTag(Document document, AttributeSyntax syntax, CancellationToken cancellation) + { + var root = (CompilationUnitSyntax?) await document.GetSyntaxRootAsync(cancellation); + + if (syntax.ArgumentList == null) + return document; + + AttributeSyntax? newSyntax; + if (syntax.ArgumentList.Arguments.Count == 1) + { + // If this is the only argument, delete the ArgumentList so we don't leave empty parentheses + newSyntax = syntax.RemoveNode(syntax.ArgumentList, SyntaxRemoveOptions.KeepNoTrivia); + } + else + { + // Remove the first argument, which is the tag + var newArgs = syntax.ArgumentList.Arguments.RemoveAt(0); + var newArgList = syntax.ArgumentList.WithArguments(newArgs); + // Construct a new attribute with the tag removed + newSyntax = syntax.WithArgumentList(newArgList); + } + + 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); diff --git a/Robust.Analyzers/Robust.Analyzers.csproj b/Robust.Analyzers/Robust.Analyzers.csproj index 8bbb58f0f..e2990ffbd 100644 --- a/Robust.Analyzers/Robust.Analyzers.csproj +++ b/Robust.Analyzers/Robust.Analyzers.csproj @@ -16,6 +16,11 @@ + + + + + diff --git a/Robust.Roslyn.Shared/Diagnostics.cs b/Robust.Roslyn.Shared/Diagnostics.cs index f1bd11192..03b20162c 100644 --- a/Robust.Roslyn.Shared/Diagnostics.cs +++ b/Robust.Roslyn.Shared/Diagnostics.cs @@ -30,6 +30,7 @@ public static class Diagnostics public const string IdComponentPauseWrongTypeAttribute = "RA0024"; public const string IdDependencyFieldAssigned = "RA0025"; public const string IdUncachedRegex = "RA0026"; + public const string IdDataFieldRedundantTag = "RA0027"; public static SuppressionDescriptor MeansImplicitAssignment => new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned."); diff --git a/Robust.Shared/Serialization/Manager/Definition/DataDefinition.cs b/Robust.Shared/Serialization/Manager/Definition/DataDefinition.cs index 2f68148e8..412091cbe 100644 --- a/Robust.Shared/Serialization/Manager/Definition/DataDefinition.cs +++ b/Robust.Shared/Serialization/Manager/Definition/DataDefinition.cs @@ -65,8 +65,7 @@ namespace Robust.Shared.Serialization.Manager.Definition continue; } - var name = field.FieldInfo.Name.AsSpan(); - attribute.Tag = $"{char.ToLowerInvariant(name[0])}{name[1..]}"; + attribute.Tag = DataDefinitionUtility.AutoGenerateTag(field.FieldInfo.Name); } var dataFields = fieldDefs diff --git a/Robust.Shared/Serialization/Manager/Definition/DataDefinitionUtility.cs b/Robust.Shared/Serialization/Manager/Definition/DataDefinitionUtility.cs new file mode 100644 index 000000000..1a6adb89c --- /dev/null +++ b/Robust.Shared/Serialization/Manager/Definition/DataDefinitionUtility.cs @@ -0,0 +1,12 @@ +using System; + +namespace Robust.Shared.Serialization.Manager.Definition; + +public class DataDefinitionUtility +{ + public static string AutoGenerateTag(string name) + { + var span = name.AsSpan(); + return $"{char.ToLowerInvariant(span[0])}{span.Slice(1).ToString()}"; + } +}