From c73b54862e8e99cf96331e523e6d231f0c6a7604 Mon Sep 17 00:00:00 2001 From: Pieter-Jan Briers Date: Thu, 26 Jun 2025 22:24:23 +0200 Subject: [PATCH] Add analyzers to detect some prototype misuse (#6048) * Add analyzers to detect some prototype misuse Detects people marking prototype as NetSerializable. Detects people creating new prototype instances themselves. * Update Robust.Analyzers/PrototypeNetSerializableAnalyzer.cs Co-authored-by: Tayrtahn --------- Co-authored-by: Tayrtahn --- .../PrototypeInstantiationAnalyzerTest.cs | 64 ++++++++++++++++ .../PrototypeNetSerializableAnalyzerTest.cs | 61 +++++++++++++++ Robust.Analyzers.Tests/RTAnalyzerTest.cs | 17 +++++ .../Robust.Analyzers.Tests.csproj | 4 + .../PrototypeInstantiationAnalyzer.cs | 48 ++++++++++++ .../PrototypeNetSerializableAnalyzer.cs | 76 +++++++++++++++++++ Robust.Roslyn.Shared/AttributeHelper.cs | 18 +++++ Robust.Roslyn.Shared/Diagnostics.cs | 3 + Robust.Roslyn.Shared/TypeSymbolHelper.cs | 15 +++- Robust.Shared/Prototypes/Attributes.cs | 6 ++ Robust.Shared/Prototypes/IPrototype.cs | 12 +-- .../Manager/Attributes/DataFieldAttribute.cs | 4 + 12 files changed, 320 insertions(+), 8 deletions(-) create mode 100644 Robust.Analyzers.Tests/PrototypeInstantiationAnalyzerTest.cs create mode 100644 Robust.Analyzers.Tests/PrototypeNetSerializableAnalyzerTest.cs create mode 100644 Robust.Analyzers.Tests/RTAnalyzerTest.cs create mode 100644 Robust.Analyzers/PrototypeInstantiationAnalyzer.cs create mode 100644 Robust.Analyzers/PrototypeNetSerializableAnalyzer.cs diff --git a/Robust.Analyzers.Tests/PrototypeInstantiationAnalyzerTest.cs b/Robust.Analyzers.Tests/PrototypeInstantiationAnalyzerTest.cs new file mode 100644 index 000000000..6be2114d9 --- /dev/null +++ b/Robust.Analyzers.Tests/PrototypeInstantiationAnalyzerTest.cs @@ -0,0 +1,64 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Testing; +using NUnit.Framework; +using VerifyCS = + Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier; + +namespace Robust.Analyzers.Tests; + +[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)] +[TestFixture] +[TestOf(typeof(PrototypeInstantiationAnalyzer))] +public sealed class PrototypeInstantiationAnalyzerTest +{ + private static Task Verifier(string code, params DiagnosticResult[] expected) + { + var test = new RTAnalyzerTest() + { + TestState = + { + Sources = { code } + }, + }; + + TestHelper.AddEmbeddedSources( + test.TestState, + "Robust.Shared.Prototypes.Attributes.cs", + "Robust.Shared.Prototypes.IPrototype.cs", + "Robust.Shared.Serialization.Manager.Attributes.DataFieldAttribute.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 Robust.Shared.Serialization; + using Robust.Shared.Prototypes; + + [Prototype] + public sealed class FooPrototype : IPrototype + { + [IdDataField] + public string ID { get; private set; } = default!; + } + + public static class Bad + { + public static FooPrototype Real() + { + return new FooPrototype(); + } + } + """; + + await Verifier(code, + // /0/Test0.cs(15,16): warning RA0039: Do not instantiate prototypes directly. Prototypes should always be instantiated by the prototype manager. + VerifyCS.Diagnostic().WithSpan(15, 16, 15, 34)); + } +} diff --git a/Robust.Analyzers.Tests/PrototypeNetSerializableAnalyzerTest.cs b/Robust.Analyzers.Tests/PrototypeNetSerializableAnalyzerTest.cs new file mode 100644 index 000000000..9727d9c71 --- /dev/null +++ b/Robust.Analyzers.Tests/PrototypeNetSerializableAnalyzerTest.cs @@ -0,0 +1,61 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Testing; +using NUnit.Framework; +using VerifyCS = + Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier; + +namespace Robust.Analyzers.Tests; + +[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)] +[TestFixture] +[TestOf(typeof(PrototypeNetSerializableAnalyzer))] +public sealed class PrototypeNetSerializableAnalyzerTest +{ + private static Task Verifier(string code, params DiagnosticResult[] expected) + { + var test = new RTAnalyzerTest() + { + TestState = + { + Sources = { code } + }, + }; + + TestHelper.AddEmbeddedSources( + test.TestState, + "Robust.Shared.Serialization.NetSerializableAttribute.cs", + "Robust.Shared.Prototypes.Attributes.cs", + "Robust.Shared.Prototypes.IPrototype.cs", + "Robust.Shared.Serialization.Manager.Attributes.DataFieldAttribute.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.Serialization; + using Robust.Shared.Prototypes; + + [Prototype] + [Serializable, NetSerializable] + public sealed class FooPrototype : IPrototype + { + [IdDataField] + public string ID { get; private set; } = default!; + } + """; + + await Verifier(code, + // /0/Test0.cs(7,21): warning RA0037: Type FooPrototype is a prototype and marked as [NetSerializable]. Prototypes should not be directly sent over the network, send their IDs instead. + VerifyCS.Diagnostic(PrototypeNetSerializableAnalyzer.RuleNetSerializable).WithSpan(7, 21, 7, 33).WithArguments("FooPrototype"), + // /0/Test0.cs(7,21): warning RA0038: Type FooPrototype is a prototype and marked as [Serializable]. Prototypes should not be directly sent over the network, send their IDs instead. + VerifyCS.Diagnostic(PrototypeNetSerializableAnalyzer.RuleSerializable).WithSpan(7, 21, 7, 33).WithArguments("FooPrototype")); + } +} diff --git a/Robust.Analyzers.Tests/RTAnalyzerTest.cs b/Robust.Analyzers.Tests/RTAnalyzerTest.cs new file mode 100644 index 000000000..79dceb131 --- /dev/null +++ b/Robust.Analyzers.Tests/RTAnalyzerTest.cs @@ -0,0 +1,17 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Testing; + +namespace Robust.Analyzers.Tests; + +public sealed class RTAnalyzerTest : CSharpAnalyzerTest + where TAnalyzer : DiagnosticAnalyzer, new() +{ + protected override ParseOptions CreateParseOptions() + { + var baseOptions = (CSharpParseOptions) base.CreateParseOptions(); + return baseOptions.WithPreprocessorSymbols("ROBUST_ANALYZERS_TEST"); + } +} diff --git a/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj b/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj index f3d2d52ea..8f0f1f8f9 100644 --- a/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj +++ b/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj @@ -17,6 +17,10 @@ + + + + diff --git a/Robust.Analyzers/PrototypeInstantiationAnalyzer.cs b/Robust.Analyzers/PrototypeInstantiationAnalyzer.cs new file mode 100644 index 000000000..361138009 --- /dev/null +++ b/Robust.Analyzers/PrototypeInstantiationAnalyzer.cs @@ -0,0 +1,48 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using Robust.Roslyn.Shared; + +namespace Robust.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class PrototypeInstantiationAnalyzer : DiagnosticAnalyzer +{ + private const string PrototypeInterfaceType = "Robust.Shared.Prototypes.IPrototype"; + + public static readonly DiagnosticDescriptor Rule = new( + Diagnostics.IdPrototypeInstantiation, + "Do not instantiate prototypes directly", + "Do not instantiate prototypes directly. Prototypes should always be instantiated by the prototype manager.", + "Usage", + DiagnosticSeverity.Warning, + true); + + public override ImmutableArray SupportedDiagnostics => [Rule]; + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterCompilationStartAction(static ctx => + { + var prototypeInterface = ctx.Compilation.GetTypeByMetadataName(PrototypeInterfaceType); + if (prototypeInterface == null) + return; + + ctx.RegisterOperationAction(symContext => Check(prototypeInterface, symContext), OperationKind.ObjectCreation); + }); + } + + private static void Check(INamedTypeSymbol prototypeInterface, OperationAnalysisContext ctx) + { + if (ctx.Operation is not IObjectCreationOperation { Type: { } resultType } creationOp) + return; + + if (!TypeSymbolHelper.ImplementsInterface(resultType, prototypeInterface)) + return; + + ctx.ReportDiagnostic(Diagnostic.Create(Rule, creationOp.Syntax.GetLocation())); + } +} diff --git a/Robust.Analyzers/PrototypeNetSerializableAnalyzer.cs b/Robust.Analyzers/PrototypeNetSerializableAnalyzer.cs new file mode 100644 index 000000000..cdb1f6d53 --- /dev/null +++ b/Robust.Analyzers/PrototypeNetSerializableAnalyzer.cs @@ -0,0 +1,76 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Robust.Roslyn.Shared; + +namespace Robust.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class PrototypeNetSerializableAnalyzer : DiagnosticAnalyzer +{ + private const string PrototypeInterfaceType = "Robust.Shared.Prototypes.IPrototype"; + private const string NetSerializableAttributeType = "Robust.Shared.Serialization.NetSerializableAttribute"; + + public static readonly DiagnosticDescriptor RuleNetSerializable = new( + Diagnostics.IdPrototypeNetSerializable, + "Prototypes should not be [NetSerializable]", + "Type {0} is a prototype and marked as [NetSerializable]. Prototypes should not be directly sent over the network, send their IDs instead.", + "Usage", + DiagnosticSeverity.Warning, + true); + + + public static readonly DiagnosticDescriptor RuleSerializable = new( + Diagnostics.IdPrototypeSerializable, + "Prototypes should not be [Serializable]", + "Type {0} is a prototype and marked as [Serializable]. Prototypes should not be directly sent over the network, send their IDs instead.", + "Usage", + DiagnosticSeverity.Warning, + true); + + public override ImmutableArray SupportedDiagnostics => [ + RuleNetSerializable, + RuleSerializable + ]; + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterCompilationStartAction(static ctx => + { + var prototypeInterface = ctx.Compilation.GetTypeByMetadataName(PrototypeInterfaceType); + var netSerializableAttribute = ctx.Compilation.GetTypeByMetadataName(NetSerializableAttributeType); + + if (prototypeInterface == null || netSerializableAttribute == null) + return; + + ctx.RegisterSymbolAction(symbolContext => CheckClass(prototypeInterface, netSerializableAttribute, symbolContext), SymbolKind.NamedType); + }); + } + + private static void CheckClass( + INamedTypeSymbol prototypeInterface, + INamedTypeSymbol netSerializableAttribute, + SymbolAnalysisContext symbolContext) + { + if (symbolContext.Symbol is not INamedTypeSymbol symbol) + return; + + if (!TypeSymbolHelper.ImplementsInterface(symbol, prototypeInterface)) + return; + + if (AttributeHelper.HasAttribute(symbol, netSerializableAttribute, out _)) + { + symbolContext.ReportDiagnostic( + Diagnostic.Create(RuleNetSerializable, symbol.Locations[0], symbol.ToDisplayString())); + } + + if (symbol.IsSerializable) + { + symbolContext.ReportDiagnostic( + Diagnostic.Create(RuleSerializable, symbol.Locations[0], symbol.ToDisplayString())); + } + } +} diff --git a/Robust.Roslyn.Shared/AttributeHelper.cs b/Robust.Roslyn.Shared/AttributeHelper.cs index 7a8174b1f..d907a8025 100644 --- a/Robust.Roslyn.Shared/AttributeHelper.cs +++ b/Robust.Roslyn.Shared/AttributeHelper.cs @@ -43,4 +43,22 @@ public static class AttributeHelper return defaultValue; } + + public static bool HasAttribute( + INamedTypeSymbol symbol, + INamedTypeSymbol attribute, + [NotNullWhen(true)] out AttributeData? matchedAttribute) + { + matchedAttribute = null; + foreach (var typeAttribute in symbol.GetAttributes()) + { + if (SymbolEqualityComparer.Default.Equals(typeAttribute.AttributeClass, attribute)) + { + matchedAttribute = typeAttribute; + return true; + } + } + + return false; + } } diff --git a/Robust.Roslyn.Shared/Diagnostics.cs b/Robust.Roslyn.Shared/Diagnostics.cs index 3d18aeff6..6525bbfd3 100644 --- a/Robust.Roslyn.Shared/Diagnostics.cs +++ b/Robust.Roslyn.Shared/Diagnostics.cs @@ -40,6 +40,9 @@ public static class Diagnostics public const string IdObsoleteInheritance = "RA0034"; public const string IdObsoleteInheritanceWithMessage = "RA0035"; public const string IdDataFieldYamlSerializable = "RA0036"; + public const string IdPrototypeNetSerializable = "RA0037"; + public const string IdPrototypeSerializable = "RA0038"; + public const string IdPrototypeInstantiation = "RA0039"; public static SuppressionDescriptor MeansImplicitAssignment => new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned."); diff --git a/Robust.Roslyn.Shared/TypeSymbolHelper.cs b/Robust.Roslyn.Shared/TypeSymbolHelper.cs index cb969139c..70c0b28b1 100644 --- a/Robust.Roslyn.Shared/TypeSymbolHelper.cs +++ b/Robust.Roslyn.Shared/TypeSymbolHelper.cs @@ -6,7 +6,7 @@ namespace Robust.Roslyn.Shared; public static class TypeSymbolHelper { - public static bool ShittyTypeMatch(INamedTypeSymbol type, string attributeMetadataName) + public static bool ShittyTypeMatch(ITypeSymbol type, string attributeMetadataName) { // Doing it like this only allocates when the type actually matches, which is good enough for me right now. if (!attributeMetadataName.EndsWith(type.Name)) @@ -15,7 +15,7 @@ public static class TypeSymbolHelper return type.ToDisplayString() == attributeMetadataName; } - public static bool ImplementsInterface(INamedTypeSymbol type, string interfaceTypeName) + public static bool ImplementsInterface(ITypeSymbol type, string interfaceTypeName) { foreach (var interfaceType in type.AllInterfaces) { @@ -25,4 +25,15 @@ public static class TypeSymbolHelper return false; } + + public static bool ImplementsInterface(ITypeSymbol type, INamedTypeSymbol interfaceType) + { + foreach (var @interface in type.AllInterfaces) + { + if (SymbolEqualityComparer.Default.Equals(@interface, interfaceType)) + return true; + } + + return false; + } } diff --git a/Robust.Shared/Prototypes/Attributes.cs b/Robust.Shared/Prototypes/Attributes.cs index d5dddc403..cc26fbfc3 100644 --- a/Robust.Shared/Prototypes/Attributes.cs +++ b/Robust.Shared/Prototypes/Attributes.cs @@ -1,6 +1,8 @@ using System; +#if !ROBUST_ANALYZERS_TEST using JetBrains.Annotations; using Robust.Shared.Serialization.Manager.Attributes; +#endif namespace Robust.Shared.Prototypes; @@ -9,10 +11,12 @@ namespace Robust.Shared.Prototypes; /// To prevent needing to instantiate it because interfaces can't declare statics. /// [AttributeUsage(AttributeTargets.Class, Inherited = false)] +#if !ROBUST_ANALYZERS_TEST [BaseTypeRequired(typeof(IPrototype))] [MeansImplicitUse] [MeansDataDefinition] [Virtual] +#endif public class PrototypeAttribute : Attribute { /// @@ -35,10 +39,12 @@ public class PrototypeAttribute : Attribute } [AttributeUsage(AttributeTargets.Class, Inherited = false)] +#if !ROBUST_ANALYZERS_TEST [BaseTypeRequired(typeof(IPrototype))] [MeansImplicitUse] [MeansDataDefinition] [MeansDataRecord] +#endif public sealed class PrototypeRecordAttribute : PrototypeAttribute { public PrototypeRecordAttribute(string type, int loadPriority = 1) : base(type, loadPriority) diff --git a/Robust.Shared/Prototypes/IPrototype.cs b/Robust.Shared/Prototypes/IPrototype.cs index bb9e99210..9cb4b03a8 100644 --- a/Robust.Shared/Prototypes/IPrototype.cs +++ b/Robust.Shared/Prototypes/IPrototype.cs @@ -1,11 +1,8 @@ using System; -using JetBrains.Annotations; using Robust.Shared.Serialization.Manager.Attributes; -using Robust.Shared.Serialization.TypeSerializers.Implementations.Custom.Prototype; -using Robust.Shared.Serialization.TypeSerializers.Interfaces; +#if !ROBUST_ANALYZERS_TEST using Robust.Shared.ViewVariables; -using YamlDotNet.Core.Tokens; -using YamlDotNet.RepresentationModel; +#endif namespace Robust.Shared.Prototypes { @@ -22,7 +19,10 @@ namespace Robust.Shared.Prototypes /// An ID for this prototype instance. /// If this is a duplicate, an error will be thrown. /// - [ViewVariables(VVAccess.ReadOnly)] string ID { get; } +#if !ROBUST_ANALYZERS_TEST + [ViewVariables(VVAccess.ReadOnly)] +#endif + string ID { get; } } public interface IInheritingPrototype diff --git a/Robust.Shared/Serialization/Manager/Attributes/DataFieldAttribute.cs b/Robust.Shared/Serialization/Manager/Attributes/DataFieldAttribute.cs index cf1d79890..fcde5823d 100644 --- a/Robust.Shared/Serialization/Manager/Attributes/DataFieldAttribute.cs +++ b/Robust.Shared/Serialization/Manager/Attributes/DataFieldAttribute.cs @@ -1,12 +1,16 @@ using System; +#if !ROBUST_ANALYZERS_TEST using JetBrains.Annotations; +#endif namespace Robust.Shared.Serialization.Manager.Attributes { [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)] +#if !ROBUST_ANALYZERS_TEST [MeansImplicitAssignment] [MeansImplicitUse(ImplicitUseKindFlags.Assign)] [Virtual] +#endif public class DataFieldAttribute : DataFieldBaseAttribute { ///