From 2272406ea92e23320d5b0f75dbfabc834bfcfcf7 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 7 Jul 2026 16:42:47 -0400 Subject: [PATCH 01/31] Code fixer for converting SubscribeXEvents to attributes --- ...ystemSubscriptionConversionAnalyzerTest.cs | 125 ++++++++ ...tySystemSubscriptionConversionFixerTest.cs | 293 ++++++++++++++++++ ...itySystemSubscriptionConversionAnalyzer.cs | 122 ++++++++ ...EntitySystemSubscriptionConversionFixer.cs | 133 ++++++++ Robust.Roslyn.Shared/Diagnostics.cs | 1 + 5 files changed, 674 insertions(+) create mode 100644 Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs create mode 100644 Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs create mode 100644 Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs create mode 100644 Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs new file mode 100644 index 00000000000..2490f955e93 --- /dev/null +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -0,0 +1,125 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; +using NUnit.Framework; +using VerifyCS = + Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier; + +namespace Robust.Analyzers.Tests; + +[TestOf(typeof(EntitySystemSubscriptionConversionAnalyzer))] +public sealed class EntitySystemSubscriptionConversionAnalyzerTest +{ + private static Task Verifier(string code, params DiagnosticResult[] expected) + { + var test = new CSharpAnalyzerTest() + { + TestState = + { + Sources = { code } + }, + }; + + TestHelper.AddEmbeddedSources( + test.TestState//, + //"Robust.Shared.IoC.DependencyAttribute.cs" + ); + + test.TestState.Sources.Add(("TestTypeDefs.cs", TestTypeDefs)); + + // ExpectedDiagnostics cannot be set, so we need to AddRange here... + test.TestState.ExpectedDiagnostics.AddRange(expected); + + return test.RunAsync(); + } + + private const string TestTypeDefs = """ + using Robust.Shared.GameObjects; + using System; + + namespace Robust.Shared.GameObjects + { + public interface IComponent; + public abstract class Component : IComponent; + + public readonly struct EntityUid; + + public delegate void ComponentEventRefHandler(EntityUid uid, TComp component, ref TEvent args) + where TComp : IComponent + where TEvent : notnull; + + public abstract class EntitySystem + { + public virtual void Initialize() { } + public void SubscribeLocalEvent( + ComponentEventRefHandler handler, + Type[]? before = null, + Type[]? after = null) + where TComp : IComponent + where TEvent : notnull + { } + } + } + + public readonly struct TestEvent; + public readonly struct TestEvent2; + public readonly struct TestEvent3; + public sealed partial class TestComponent : IComponent; + """; + + [Test] + public async Task Test() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + ); + } + + [Test] + [Description("Tests that subscriptions with before/after parameters are not flagged as elligible for conversion.")] + // TODO: Remove this test if event subscription attributes get support for before/after parameters + public async Task IgnoreBeforeAfter() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest, before: [typeof(Component)]); + SubscribeLocalEvent(OnTest2, after: [typeof(Component)]); + SubscribeLocalEvent(OnTest3, before: [typeof(Component)], after: [typeof(Component)]); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) { } + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) { } + private void OnTest3(EntityUid uid, TestComponent comp, ref TestEvent3 args) { } + } + """; + + await Verifier(code, []); + } +} diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs new file mode 100644 index 00000000000..1b47a435f75 --- /dev/null +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -0,0 +1,293 @@ +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; +using NUnit.Framework; +using VerifyCS = + Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier; + +namespace Robust.Analyzers.Tests; + +public sealed class EntitySystemSubscriptionConversionFixerTest +{ + private static Task Verifier(string code, string fixedCode, params DiagnosticResult[] expected) + { + var test = new CSharpCodeFixTest() + { + TestState = + { + Sources = { code }, + }, + FixedState = + { + Sources = { fixedCode }, + } + }; + + test.TestState.Sources.Add(("TestTypeDefs.cs", TestTypeDefs)); + test.FixedState.Sources.Add(("TestTypeDefs.cs", TestTypeDefs)); + + test.TestState.ExpectedDiagnostics.AddRange(expected); + + return test.RunAsync(); + } + + private static Task Verifier(string[] code, string[] fixedCode, params DiagnosticResult[] expected) + { + var test = new CSharpCodeFixTest(); + + foreach (var file in code) + { + test.TestState.Sources.Add(file); + } + foreach (var file in fixedCode) + { + test.FixedState.Sources.Add(file); + } + + test.TestState.Sources.Add(("TestTypeDefs.cs", TestTypeDefs)); + test.FixedState.Sources.Add(("TestTypeDefs.cs", TestTypeDefs)); + + test.TestState.ExpectedDiagnostics.AddRange(expected); + + return test.RunAsync(); + } + + private const string TestTypeDefs = """ + using Robust.Shared.GameObjects; + using System; + + namespace Robust.Shared.GameObjects + { + public interface IComponent; + public abstract class Component : IComponent; + public sealed class SubscribeLocalEventAttribute : Attribute; + public sealed class SubscribeNetworkEventAttribute : Attribute; + public sealed class SubscribeAllEventAttribute : Attribute; + + public readonly struct EntityUid; + + public delegate void ComponentEventRefHandler(EntityUid uid, TComp component, ref TEvent args) + where TComp : IComponent + where TEvent : notnull; + public delegate void EntityEventHandler(T ev); + + public abstract class EntitySystem + { + public virtual void Initialize() { } + public void SubscribeLocalEvent( + ComponentEventRefHandler handler) + where TComp : IComponent + where TEvent : notnull + { } + protected void SubscribeNetworkEvent( + EntityEventHandler handler, + Type[]? before = null, Type[]? after = null) + where T : notnull + { } + } + } + + public readonly struct TestEvent; + public sealed partial class TestComponent : IComponent; + public sealed class TestNetworkEvent; + """; + + [Test] + public async Task ConvertLocalEvent() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + ); + } + + [Test] + public async Task ConvertLocalEvent_AddPartial() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + ); + } + + [Test] + public async Task ConvertNetworkEvent() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeNetworkEvent(OnTest); // Comment here + } + + private void OnTest(TestNetworkEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeNetworkEvent] + private void OnTest(TestNetworkEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 56) + ); + } + + [Test] + public async Task ConvertLocalEvent_WithPartials() + { + const string code1 = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + } + } + """; + + const string code2 = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixed1 = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + } + """; + + const string fixed2 = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier([code1, code2], [fixed1, fixed2], + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + ); + } +} diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs new file mode 100644 index 00000000000..85c97354cf7 --- /dev/null +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -0,0 +1,122 @@ +#nullable enable +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 EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnalyzer +{ + private const string EntitySystemTypeName = "Robust.Shared.GameObjects.EntitySystem"; + private const string InitializeMethodName = "Initialize"; + private const string SubscribeLocalEventMethodName = "SubscribeLocalEvent"; + private const string SubscribeNetworkEventMethodName = "SubscribeNetworkEvent"; + private const string SubscribeAllEventMethodName = "SubscribeAllEvent"; + private static readonly string[] SubscribeMethods = + [ + SubscribeLocalEventMethodName, + SubscribeNetworkEventMethodName, + SubscribeAllEventMethodName, + ]; + + public const string AttributeNameKey = "attribute"; + + public static readonly DiagnosticDescriptor EntitySystemSubscriptionConversionPossible = new( + Diagnostics.IdEntitySystemSubscriptionConversionPossible, + "Convert to attribute-based subscription", + "Initialize-based event subscription can be converted to attribute-based", + "Usage", + DiagnosticSeverity.Info, + true + ); + + public override ImmutableArray SupportedDiagnostics => + [ + EntitySystemSubscriptionConversionPossible, + ]; + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterCompilationStartAction(ctx => + { + if (ctx.Compilation.GetTypeByMetadataName(EntitySystemTypeName) is not { } entitySystemType) + return; + + ctx.RegisterSymbolStartAction(symbolContext => + { + // We only care about classes + if (symbolContext.Symbol is not INamedTypeSymbol typeSymbol || typeSymbol.TypeKind != TypeKind.Class) + return; + + // Must inherit from EntitySystem + if (!TypeSymbolHelper.Inherits(typeSymbol, entitySystemType)) + return; + + // Check each method definition in the class + symbolContext.RegisterOperationAction(AnalyzeMethod, OperationKind.MethodBody); + }, SymbolKind.NamedType); + }); + } + + private static void AnalyzeMethod(OperationAnalysisContext context) + { + if (context.Operation is not IMethodBodyOperation method) + return; + + // We're only looking for the Initialize method + if (context.ContainingSymbol.Name != InitializeMethodName) + return; + + if (method.BlockBody is null) + return; + + // Examine each operation within the Initialize method body + foreach (var initOperation in method.BlockBody.ChildOperations) + { + // We only care about method invocations + if (initOperation is not IExpressionStatementOperation expression + || expression.Operation is not IInvocationOperation invocation) + continue; + + // Check if the invoked method is one of the SubscribeWhateverEvent methods + if (SubscribeMethods.Contains(invocation.TargetMethod.Name)) + { + // We (currently) don't support the before and after parameters with attribute subscriptions + // so we skip any invocations that use them. + // If we do support them in the future, this check should be removed. + if (invocation.Arguments.Any( + arg => (arg.Parameter?.Name == "before" || arg.Parameter?.Name == "after") + && arg.Value is not IDefaultValueOperation)) + continue; + + var props = new Dictionary + { + { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } + }; + + // Flag this subscription as elligible for conversion + context.ReportDiagnostic(Diagnostic.Create( + EntitySystemSubscriptionConversionPossible, + invocation.Syntax.GetLocation(), + props.ToImmutableDictionary() + )); + } + } + } + + /// + /// Returns the name of the appropriate attribute to replace the given subscription method. + /// + public static string ToAttributeName(string methodName) + { + // This is currently a 1:1 match, but if it weren't, + // this would be the place to implement the remapping. + return methodName; + } +} diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs new file mode 100644 index 00000000000..f08eed63adf --- /dev/null +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -0,0 +1,133 @@ +#nullable enable +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; +using static Robust.Roslyn.Shared.Diagnostics; + +namespace Robust.Analyzers; + +[ExportCodeFixProvider(LanguageNames.CSharp)] +public sealed class EntitySystemSubscriptionConversionFixer : CodeFixProvider +{ + public override ImmutableArray FixableDiagnosticIds => + [ + IdEntitySystemSubscriptionConversionPossible + ]; + + public override FixAllProvider GetFixAllProvider() + { + return WellKnownFixAllProviders.BatchFixer; + } + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + { + foreach (var diagnostic in context.Diagnostics) + { + switch (diagnostic.Id) + { + case IdEntitySystemSubscriptionConversionPossible: + return RegisterSubscriptionConversion(context, diagnostic); + } + } + + return Task.CompletedTask; + } + + private static async Task RegisterSubscriptionConversion(CodeFixContext context, Diagnostic diagnostic) + { + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken); + var root = await semanticModel!.SyntaxTree.GetRootAsync(context.CancellationToken); + //var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken); + + var span = diagnostic.Location.SourceSpan; + var invocationSyntax = root?.FindToken(span.Start).Parent?.AncestorsAndSelf().OfType().First(); + var classSyntax = invocationSyntax?.AncestorsAndSelf().OfType().First(); + var classSymbol = semanticModel.GetDeclaredSymbol(classSyntax!); + + if (invocationSyntax is null || classSyntax is null || classSymbol is null) + return; + + if (diagnostic.Properties[EntitySystemSubscriptionConversionAnalyzer.AttributeNameKey] is not string attributeName) + return; + + context.RegisterCodeFix(CodeAction.Create( + "Convert subscription to attribute", + c => ConvertSubscription(context.Document, invocationSyntax, classSymbol, classSyntax, attributeName, c), + "Convert subscription to attribute" + ), diagnostic); + } + + private static async Task ConvertSubscription( + Document document, + InvocationExpressionSyntax invocationSyntax, + INamedTypeSymbol classSymbol, + ClassDeclarationSyntax classSyntax, + string attributeName, + CancellationToken c) + { + // Get the identifier of the event handler method. + if (invocationSyntax.ArgumentList.Arguments[0].Expression is not IdentifierNameSyntax handlerMethodIdentifer) + throw new InvalidOperationException(); + + // Use the identifier to get the symbol for the event handler method. + var handlerMethodSymbol = classSymbol.GetMembers(handlerMethodIdentifer.Identifier.Text).Single(); + + // Create a SolutionEditor to edit multiple documents without worrying about immutability. + // The Initialize method might be in a different document than the handler, thanks to partial classes. + var editor = new SolutionEditor(document.Project.Solution); + + // Get an editor for the document containing the Initialize method. + var initializeEditor = await editor.GetDocumentEditorAsync(document.Id, c); + // Make our changes to the document containing the Initialize method. + ModifyInitialize(initializeEditor, invocationSyntax); + + // Find the ID for the document containing the event handler method. + var handlerDocId = editor.OriginalSolution.GetDocumentId(handlerMethodSymbol.DeclaringSyntaxReferences.First().SyntaxTree); + + // Get an editor for the document containing the event handler method. + var handlerEditor = await editor.GetDocumentEditorAsync(handlerDocId, c); + // Make our changes to the document containing the event handler method. + ModifyHandler(handlerEditor, handlerMethodSymbol, attributeName); + + // + EnsureClassPartial(initializeEditor, classSymbol, classSyntax); + + // Return the modified solution + return editor.GetChangedSolution(); + } + + private static void ModifyInitialize( + DocumentEditor editor, + InvocationExpressionSyntax token) + { + // Remove the SubscribeWhateverEvent invocation from the Initialize method + editor.RemoveNode(token.Parent!); + } + + private static void ModifyHandler( + DocumentEditor editor, + ISymbol handlerMethodSymbol, + string attributeName) + { + // Get the syntax node for the event handler method + var handlerMethodSyntax = handlerMethodSymbol.DeclaringSyntaxReferences.First().GetSyntax() as MethodDeclarationSyntax; + + // Generate the SubscribeWhateverEvent attribute + var attr = SyntaxFactory.Attribute(SyntaxFactory.IdentifierName(attributeName)); + // Add the attribute to the event handler method + editor.AddAttribute(handlerMethodSyntax!, attr); + } + + private static void EnsureClassPartial( + DocumentEditor editor, + INamedTypeSymbol classSymbol, + ClassDeclarationSyntax classSyntax) + { + var oldModifiers = DeclarationModifiers.From(classSymbol); + editor.SetModifiers(classSyntax, oldModifiers.WithPartial(true)); + } +} diff --git a/Robust.Roslyn.Shared/Diagnostics.cs b/Robust.Roslyn.Shared/Diagnostics.cs index c521ab4832f..e62e9a5870b 100644 --- a/Robust.Roslyn.Shared/Diagnostics.cs +++ b/Robust.Roslyn.Shared/Diagnostics.cs @@ -60,6 +60,7 @@ public static class Diagnostics public const string IdInvalidAMethodSignatureForGeneratedSubscription = "RA0054"; public const string IdInvalidContainingTypeForGeneratedSubscription = "RA0055"; public const string IdNonPartialContainingTypeForGeneratedSubscription = "RA0056"; + public const string IdEntitySystemSubscriptionConversionPossible = "RA0057"; public static SuppressionDescriptor MeansImplicitAssignment => new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned."); From 8336a43ea26617c59a403a72224ed74c28e45fff Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 7 Jul 2026 16:57:12 -0400 Subject: [PATCH 02/31] Tweaks and docs --- ...EntitySystemSubscriptionConversionFixer.cs | 41 ++++++++++++++----- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index f08eed63adf..dd3d599dc5e 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -41,7 +41,6 @@ private static async Task RegisterSubscriptionConversion(CodeFixContext context, { var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken); var root = await semanticModel!.SyntaxTree.GetRootAsync(context.CancellationToken); - //var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken); var span = diagnostic.Location.SourceSpan; var invocationSyntax = root?.FindToken(span.Start).Parent?.AncestorsAndSelf().OfType().First(); @@ -51,6 +50,7 @@ private static async Task RegisterSubscriptionConversion(CodeFixContext context, if (invocationSyntax is null || classSyntax is null || classSymbol is null) return; + // Get the name of the Attribute we need to add to the event handler method. if (diagnostic.Properties[EntitySystemSubscriptionConversionAnalyzer.AttributeNameKey] is not string attributeName) return; @@ -74,7 +74,7 @@ private static async Task ConvertSubscription( throw new InvalidOperationException(); // Use the identifier to get the symbol for the event handler method. - var handlerMethodSymbol = classSymbol.GetMembers(handlerMethodIdentifer.Identifier.Text).Single(); + var handlerMethodSymbol = classSymbol.GetMembers(handlerMethodIdentifer.Identifier.Text).OfType().Single(); // Create a SolutionEditor to edit multiple documents without worrying about immutability. // The Initialize method might be in a different document than the handler, thanks to partial classes. @@ -93,41 +93,60 @@ private static async Task ConvertSubscription( // Make our changes to the document containing the event handler method. ModifyHandler(handlerEditor, handlerMethodSymbol, attributeName); - // + // Make sure the class is marked as partial. EnsureClassPartial(initializeEditor, classSymbol, classSyntax); - // Return the modified solution + // Return the modified solution. return editor.GetChangedSolution(); } + /// + /// Edits the document containing the Intialize method. + /// Removes the SubscribeWhateverEvent method invocation. + /// + /// An editor for the document containing the Initialize method. + /// The SyntaxNode for the invocation of the Initialize method. private static void ModifyInitialize( DocumentEditor editor, - InvocationExpressionSyntax token) + InvocationExpressionSyntax invocationSyntax) { - // Remove the SubscribeWhateverEvent invocation from the Initialize method - editor.RemoveNode(token.Parent!); + // Remove the SubscribeWhateverEvent invocation from the Initialize method. + editor.RemoveNode(invocationSyntax.Parent!); } + /// + /// Edits the document containing the event handler method. + /// Adds the SubscribeWhateverEventAttribute to the method. + /// + /// An editor for the document containing the event handler method. + /// The symbol for the event handler method. + /// The name of the Attribute to be added. private static void ModifyHandler( DocumentEditor editor, - ISymbol handlerMethodSymbol, + IMethodSymbol handlerMethodSymbol, string attributeName) { - // Get the syntax node for the event handler method + // Get the syntax node for the event handler method. var handlerMethodSyntax = handlerMethodSymbol.DeclaringSyntaxReferences.First().GetSyntax() as MethodDeclarationSyntax; - // Generate the SubscribeWhateverEvent attribute + // Generate the SubscribeWhateverEvent attribute. var attr = SyntaxFactory.Attribute(SyntaxFactory.IdentifierName(attributeName)); - // Add the attribute to the event handler method + + // Add the attribute to the event handler method. editor.AddAttribute(handlerMethodSyntax!, attr); } + /// + /// Marks the class as partial if it's not already. + /// private static void EnsureClassPartial( DocumentEditor editor, INamedTypeSymbol classSymbol, ClassDeclarationSyntax classSyntax) { + // Use the current modifiers as a base. var oldModifiers = DeclarationModifiers.From(classSymbol); + // Add the partial modifier if it's not already there. editor.SetModifiers(classSyntax, oldModifiers.WithPartial(true)); } } From 3be2b13821a51cf19d7a89bc2055fb35e1be0e61 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 7 Jul 2026 17:24:57 -0400 Subject: [PATCH 03/31] Test method descriptions --- .../EntitySystemSubscriptionConversionAnalyzerTest.cs | 3 ++- .../EntitySystemSubscriptionConversionFixerTest.cs | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 2490f955e93..5cf1a603384 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -68,7 +68,8 @@ public sealed partial class TestComponent : IComponent; """; [Test] - public async Task Test() + [Description("Tests that a SubscribeLocalEvent invocation in an EntitySystem Intialize method is flagged as elligible for conversion.")] + public async Task FlagSubscribeLocalEvent() { const string code = """ using Robust.Shared.GameObjects; diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index 1b47a435f75..be9147668ca 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -94,6 +94,7 @@ public sealed class TestNetworkEvent; """; [Test] + [Description("Tests that a SubscribeLocalEvent invocation is correctly converted to an attribute.")] public async Task ConvertLocalEvent() { const string code = """ @@ -140,6 +141,7 @@ await Verifier(code, fixedCode, } [Test] + [Description("Tests that a class that isn't marked partial is given the partial modifier when converted.")] public async Task ConvertLocalEvent_AddPartial() { const string code = """ @@ -186,6 +188,7 @@ await Verifier(code, fixedCode, } [Test] + [Description("Tests that a SubscribeNetworkEvent invocation is correctly converted to an attribute.")] public async Task ConvertNetworkEvent() { const string code = """ @@ -232,6 +235,7 @@ await Verifier(code, fixedCode, } [Test] + [Description("Tests that the conversion works correctly when the Initialize and event handler methods are declared in separate files (partial classes).")] public async Task ConvertLocalEvent_WithPartials() { const string code1 = """ From 34d2d8d0f52b80404636b592acc264e6dd90d5e6 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 7 Jul 2026 23:14:52 -0400 Subject: [PATCH 04/31] Add test of multiple invocations --- ...tySystemSubscriptionConversionFixerTest.cs | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index be9147668ca..3dbb0a9a1ea 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -89,6 +89,7 @@ protected void SubscribeNetworkEvent( } public readonly struct TestEvent; + public readonly struct TestEvent2; public sealed partial class TestComponent : IComponent; public sealed class TestNetworkEvent; """; @@ -140,6 +141,67 @@ await Verifier(code, fixedCode, ); } + [Test] + [Description("Tests that multiple SubscribeLocalEvent invocations are correctly converted to attributes.")] + public async Task ConvertLocalEvent_Multiple() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + SubscribeLocalEvent(OnTest2); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + + [SubscribeLocalEvent] + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62), + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 64) + ); + } + [Test] [Description("Tests that a class that isn't marked partial is given the partial modifier when converted.")] public async Task ConvertLocalEvent_AddPartial() From e5e8f56b7388c6b5a594fec3720e8996a75867df Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 7 Jul 2026 23:16:03 -0400 Subject: [PATCH 05/31] Ignore anonymous delegates --- ...ystemSubscriptionConversionAnalyzerTest.cs | 23 +++++++++++++++++++ ...itySystemSubscriptionConversionAnalyzer.cs | 6 +++++ 2 files changed, 29 insertions(+) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 5cf1a603384..6de7887d076 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -123,4 +123,27 @@ private void OnTest3(EntityUid uid, TestComponent comp, ref TestEvent3 args) { } await Verifier(code, []); } + + [Test] + [Description("Tests that subscriptions using anonymous delegates are not flagged as elligible for conversion.")] + public async Task IgnoreAnonymousDelegate() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent((u, c, ref _) => OnTest(u, c)); + } + + private void OnTest(EntityUid uid, TestComponent comp) { } + } + """; + + await Verifier(code, []); + } } diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 85c97354cf7..b4d117151ca 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -1,6 +1,7 @@ #nullable enable using System.Collections.Immutable; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; using Robust.Roslyn.Shared; @@ -95,6 +96,11 @@ private static void AnalyzeMethod(OperationAnalysisContext context) && arg.Value is not IDefaultValueOperation)) continue; + // Ignore anything that isn't a direct method reference, i.e. an anonymous delegate. + if (invocation.Arguments.SingleOrDefault(arg => arg.Parameter?.Name == "handler") is not { } handlerArg + || handlerArg.Value.Syntax is not IdentifierNameSyntax) + continue; + var props = new Dictionary { { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } From 587565a01aa0334d10166a61aeea46647ef23551 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 8 Jul 2026 10:53:58 -0400 Subject: [PATCH 06/31] Fix event handler method symbol lookup No longer confused by other methods with the same name --- ...tySystemSubscriptionConversionFixerTest.cs | 51 +++++++++++++++++++ ...EntitySystemSubscriptionConversionFixer.cs | 5 +- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index 3dbb0a9a1ea..c8b766329f3 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -249,6 +249,57 @@ await Verifier(code, fixedCode, ); } + [Test] + [Description("Tests that the conversion isn't confused by other methods with the same name as the event handler.")] + public async Task ConvertLocalEvent_HandlerOverload() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + } + + private void OnTest(string foo) { } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + private void OnTest(string foo) { } + + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + ); + } + [Test] [Description("Tests that a SubscribeNetworkEvent invocation is correctly converted to an attribute.")] public async Task ConvertNetworkEvent() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index dd3d599dc5e..49a62f7de05 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -73,8 +73,9 @@ private static async Task ConvertSubscription( if (invocationSyntax.ArgumentList.Arguments[0].Expression is not IdentifierNameSyntax handlerMethodIdentifer) throw new InvalidOperationException(); - // Use the identifier to get the symbol for the event handler method. - var handlerMethodSymbol = classSymbol.GetMembers(handlerMethodIdentifer.Identifier.Text).OfType().Single(); + var model = await document.GetSemanticModelAsync(c); + if (model.GetSymbolInfo(handlerMethodIdentifer, c).Symbol is not IMethodSymbol handlerMethodSymbol) + throw new InvalidOperationException($"Failed to find event handler method {handlerMethodIdentifer}"); // Create a SolutionEditor to edit multiple documents without worrying about immutability. // The Initialize method might be in a different document than the handler, thanks to partial classes. From 21510d2fdea74bb9cf7d4d1e5ef41c2278809087 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 8 Jul 2026 15:22:00 -0400 Subject: [PATCH 07/31] Fix whitespace weirdness with multiple invocations --- ...tySystemSubscriptionConversionFixerTest.cs | 62 +++++++++++++++++++ ...EntitySystemSubscriptionConversionFixer.cs | 4 +- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index c8b766329f3..86101fbc4cb 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -202,6 +202,68 @@ await Verifier(code, fixedCode, ); } + [Test] + [Description("Tests that multiple SubscribeLocalEvent invocations are correctly converted to attributes when there is a gap between them.")] + public async Task ConvertLocalEvent_MultipleWithGap() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + + SubscribeLocalEvent(OnTest2); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + + [SubscribeLocalEvent] + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62), + // /0/Test0.cs(11,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(11, 9, 11, 64) + ); + } + [Test] [Description("Tests that a class that isn't marked partial is given the partial modifier when converted.")] public async Task ConvertLocalEvent_AddPartial() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 49a62f7de05..1b1c07742f3 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -71,7 +71,7 @@ private static async Task ConvertSubscription( { // Get the identifier of the event handler method. if (invocationSyntax.ArgumentList.Arguments[0].Expression is not IdentifierNameSyntax handlerMethodIdentifer) - throw new InvalidOperationException(); + throw new InvalidOperationException($"Exception determining event handler method identifier for {invocationSyntax}"); var model = await document.GetSemanticModelAsync(c); if (model.GetSymbolInfo(handlerMethodIdentifer, c).Symbol is not IMethodSymbol handlerMethodSymbol) @@ -112,7 +112,7 @@ private static void ModifyInitialize( InvocationExpressionSyntax invocationSyntax) { // Remove the SubscribeWhateverEvent invocation from the Initialize method. - editor.RemoveNode(invocationSyntax.Parent!); + editor.RemoveNode(invocationSyntax.Parent!, SyntaxRemoveOptions.KeepNoTrivia); } /// From ab1b3a981fdd446a449316a5651f8e54a9cb1718 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Mon, 27 Jul 2026 14:42:30 -0400 Subject: [PATCH 08/31] Ignore any methods that contain conditional preprocessor directives --- ...ystemSubscriptionConversionAnalyzerTest.cs | 31 ++++++++++++++++++- ...itySystemSubscriptionConversionAnalyzer.cs | 6 ++++ ...EntitySystemSubscriptionConversionFixer.cs | 2 +- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 6de7887d076..e71db4e0145 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -125,7 +125,7 @@ private void OnTest3(EntityUid uid, TestComponent comp, ref TestEvent3 args) { } } [Test] - [Description("Tests that subscriptions using anonymous delegates are not flagged as elligible for conversion.")] + [Description("Tests that a subscription using an anonymous delegate is not flagged as elligible for conversion.")] public async Task IgnoreAnonymousDelegate() { const string code = """ @@ -146,4 +146,33 @@ private void OnTest(EntityUid uid, TestComponent comp) { } await Verifier(code, []); } + + [Test] + [Description("Tests that a subscription in a method containing preprocessor directives is not flagged as elligible for conversion.")] + public async Task IgnoreWithPreprocessorDirectives() + { + const string code = """ + + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + #if DEBUG + SubscribeLocalEvent(OnTest); + #else + SubscribeLocalEvent(OnTest2); + #endif + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) { } + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent args) { } + } + """; + + await Verifier(code, []); + } } diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index b4d117151ca..f45ee9060b4 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -1,6 +1,7 @@ #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; @@ -77,6 +78,11 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (method.BlockBody is null) return; + // If the Initialize method contains any sort of conditional directives, + // we consider it too complicated for automatic conversion. + if (method.Syntax.ContainsDirective(SyntaxKind.IfDirectiveTrivia | SyntaxKind.ElseDirectiveTrivia | SyntaxKind.ElifDirectiveTrivia | SyntaxKind.EndIfDirectiveTrivia)) + return; + // Examine each operation within the Initialize method body foreach (var initOperation in method.BlockBody.ChildOperations) { diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 1b1c07742f3..05bb0fcdee2 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -112,7 +112,7 @@ private static void ModifyInitialize( InvocationExpressionSyntax invocationSyntax) { // Remove the SubscribeWhateverEvent invocation from the Initialize method. - editor.RemoveNode(invocationSyntax.Parent!, SyntaxRemoveOptions.KeepNoTrivia); + editor.RemoveNode(invocationSyntax.Parent!, SyntaxRemoveOptions.KeepUnbalancedDirectives); } /// From 0e8a16a74f30d4ae864ba4e5248df1a613deb417 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 28 Jul 2026 11:15:59 -0400 Subject: [PATCH 09/31] Expand preprocessor directive filtering to check the entire class. --- .../EntitySystemSubscriptionConversionAnalyzer.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index f45ee9060b4..9afa83cfbae 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -78,9 +78,10 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (method.BlockBody is null) return; - // If the Initialize method contains any sort of conditional directives, + // If the class contains any sort of conditional directives, // we consider it too complicated for automatic conversion. - if (method.Syntax.ContainsDirective(SyntaxKind.IfDirectiveTrivia | SyntaxKind.ElseDirectiveTrivia | SyntaxKind.ElifDirectiveTrivia | SyntaxKind.EndIfDirectiveTrivia)) + var classSyntax = method.Syntax.Ancestors().OfType().First(); + if (classSyntax.ContainsDirective(SyntaxKind.IfDirectiveTrivia | SyntaxKind.ElseDirectiveTrivia | SyntaxKind.ElifDirectiveTrivia | SyntaxKind.EndIfDirectiveTrivia)) return; // Examine each operation within the Initialize method body From 0b646087dc782107ad6a89567dad0496c175efb1 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 28 Jul 2026 11:21:08 -0400 Subject: [PATCH 10/31] Add test that subscriptions in if statement blocks are not flagged. --- ...ystemSubscriptionConversionAnalyzerTest.cs | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index e71db4e0145..70664770066 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -152,7 +152,7 @@ private void OnTest(EntityUid uid, TestComponent comp) { } public async Task IgnoreWithPreprocessorDirectives() { const string code = """ - + using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -175,4 +175,32 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent args) { } await Verifier(code, []); } + + [Test] + [Description("Tests that subscriptions within if statement blocks are not flagged as elligible for conversion.")] + public async Task IgnoreWithIfStatement() + { + const string code = """ + + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + if (true) + SubscribeLocalEvent(OnTest); + else + SubscribeLocalEvent(OnTest2); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) { } + private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent args) { } + } + """; + + await Verifier(code, []); + } } From 189bf0dabe09e909b8a94e5077404b4960d72528 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 28 Jul 2026 11:55:27 -0400 Subject: [PATCH 11/31] doc tweak --- .../EntitySystemSubscriptionConversionAnalyzerTest.cs | 2 +- Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 70664770066..c2732adf806 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -98,7 +98,7 @@ await Verifier(code, [Test] [Description("Tests that subscriptions with before/after parameters are not flagged as elligible for conversion.")] - // TODO: Remove this test if event subscription attributes get support for before/after parameters + // TODO: Remove this test if event subscription attributes get support for before/after parameters (and the code fixer is made to convert to them) public async Task IgnoreBeforeAfter() { const string code = """ diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 9afa83cfbae..44816bcead2 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -97,7 +97,7 @@ private static void AnalyzeMethod(OperationAnalysisContext context) { // We (currently) don't support the before and after parameters with attribute subscriptions // so we skip any invocations that use them. - // If we do support them in the future, this check should be removed. + // If we do support them in the future (and the code fixer is improved to convert to them), this check should be removed. if (invocation.Arguments.Any( arg => (arg.Parameter?.Name == "before" || arg.Parameter?.Name == "after") && arg.Value is not IDefaultValueOperation)) From 185689683cf8e004341ba6a4ee9d022c70f4ecca Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Tue, 28 Jul 2026 12:14:13 -0400 Subject: [PATCH 12/31] Ignore invocation using generic type parameters as type args --- ...ystemSubscriptionConversionAnalyzerTest.cs | 25 +++++++++++++++++++ ...itySystemSubscriptionConversionAnalyzer.cs | 6 +++++ 2 files changed, 31 insertions(+) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index c2732adf806..d055852924a 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -176,6 +176,31 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent args) { } await Verifier(code, []); } + [Test] + [Description("Tests that a subscription using a generic type parameter is not flagged as elligible for conversion.")] + public async Task IgnoreWithGenericComponent() + { + const string code = """ + + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + where TComp : Component + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); + } + + private void OnTest(EntityUid uid, TComp comp, ref TestEvent args) { } + } + """; + + await Verifier(code, []); + } + [Test] [Description("Tests that subscriptions within if statement blocks are not flagged as elligible for conversion.")] public async Task IgnoreWithIfStatement() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 44816bcead2..4b7d574343a 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -95,6 +95,12 @@ private static void AnalyzeMethod(OperationAnalysisContext context) // Check if the invoked method is one of the SubscribeWhateverEvent methods if (SubscribeMethods.Contains(invocation.TargetMethod.Name)) { + // If any of the type arguments of the invocation is a type parameter (rather than a distinct Type), + // the attribute can't handle it, so we skip it. + // For example, RaiseLocalEvent(), where TTreeComp is a type arg to the containing class. + if (invocation.TargetMethod.TypeArguments.OfType().Any()) + continue; + // We (currently) don't support the before and after parameters with attribute subscriptions // so we skip any invocations that use them. // If we do support them in the future (and the code fixer is improved to convert to them), this check should be removed. From 08d6f2a90b1f4c1983e9f8ecaaaf61ca8276a8c4 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 29 Jul 2026 09:48:40 -0400 Subject: [PATCH 13/31] Ignore subscriptions where the handler method is generic. --- ...ystemSubscriptionConversionAnalyzerTest.cs | 29 +++++++++++++++++++ ...itySystemSubscriptionConversionAnalyzer.cs | 10 +++++++ 2 files changed, 39 insertions(+) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index d055852924a..d782086b147 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -201,6 +201,35 @@ private void OnTest(EntityUid uid, TComp comp, ref TestEvent args) { } await Verifier(code, []); } + [Test] + [Description("Tests that subscriptions using generic methods as event handlers are not flagged as elligible for conversion.")] + public async Task IgnoreWithGenericHandler() + { + const string code = """ + + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); + SubscribeLocalEvent(OnTest); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref T args) where T : TestEventArgs { } + } + + public class TestEventArgs; + public sealed class TestEventClassA : TestEventArgs; + public sealed class TestEventClassB : TestEventArgs; + """; + + await Verifier(code, []); + } + [Test] [Description("Tests that subscriptions within if statement blocks are not flagged as elligible for conversion.")] public async Task IgnoreWithIfStatement() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 4b7d574343a..a6fefef6c35 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -114,6 +114,16 @@ private static void AnalyzeMethod(OperationAnalysisContext context) || handlerArg.Value.Syntax is not IdentifierNameSyntax) continue; + // Get the symbol for the event handler method. + // We use OriginalDefinition to get the generic form if it's a generic method. + // So we get MyEventHandler instead of MyEventHandler. + if (((handlerArg.Value as IDelegateCreationOperation)?.Target as IMethodReferenceOperation)?.Method.OriginalDefinition is not { } handlerMethod) + continue; + + // If the target method is generic, we can't subscribe using the attribute. + if (handlerMethod.IsGenericMethod) + continue; + var props = new Dictionary { { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } From dbb86b9ec23753af79b0d8c0ccaf285a8be9feff Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 29 Jul 2026 09:55:13 -0400 Subject: [PATCH 14/31] Skip compilations without the subscription attribute --- .../EntitySystemSubscriptionConversionAnalyzerTest.cs | 5 +++++ .../EntitySystemSubscriptionConversionAnalyzer.cs | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index d782086b147..2c9dc158d55 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -61,6 +61,11 @@ public void SubscribeLocalEvent( } } + namespace Robust.Shared.Analyzers + { + public sealed class SubscribeLocalEventAttribute : Attribute; + } + public readonly struct TestEvent; public readonly struct TestEvent2; public readonly struct TestEvent3; diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index a6fefef6c35..2bd0bfad23a 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -13,6 +13,7 @@ namespace Robust.Analyzers; public sealed class EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnalyzer { private const string EntitySystemTypeName = "Robust.Shared.GameObjects.EntitySystem"; + private const string SubscribeLocalEventAttributeTypeName = "Robust.Shared.Analyzers.SubscribeLocalEventAttribute"; private const string InitializeMethodName = "Initialize"; private const string SubscribeLocalEventMethodName = "SubscribeLocalEvent"; private const string SubscribeNetworkEventMethodName = "SubscribeNetworkEvent"; @@ -47,6 +48,10 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationStartAction(ctx => { + // If the subscription attribute isn't available in this compilation, we can't do anything. + if (ctx.Compilation.GetTypeByMetadataName(SubscribeLocalEventAttributeTypeName) is null) + return; + if (ctx.Compilation.GetTypeByMetadataName(EntitySystemTypeName) is not { } entitySystemType) return; From ce788fba613926130b30a544a3e45d94e2740a7f Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 29 Jul 2026 13:14:57 -0400 Subject: [PATCH 15/31] Ignore virtual and abstract target methods --- .../EntitySystemSubscriptionConversionAnalyzer.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 2bd0bfad23a..1941d000e61 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -129,6 +129,11 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (handlerMethod.IsGenericMethod) continue; + // If the handler is a virtual or abstract method, we can't use the attribute + // since we would have to add it to the base class + if (handlerMethod.IsVirtual || handlerMethod.IsAbstract) + continue; + var props = new Dictionary { { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } From da40fc944f1c8a4039110e13b9b895c977218fad Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 29 Jul 2026 13:15:13 -0400 Subject: [PATCH 16/31] Add using directive if needed --- ...tySystemSubscriptionConversionFixerTest.cs | 95 ++++++++++++++++--- ...EntitySystemSubscriptionConversionFixer.cs | 12 ++- 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index 86101fbc4cb..f51ba75c3d5 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -61,9 +61,6 @@ namespace Robust.Shared.GameObjects { public interface IComponent; public abstract class Component : IComponent; - public sealed class SubscribeLocalEventAttribute : Attribute; - public sealed class SubscribeNetworkEventAttribute : Attribute; - public sealed class SubscribeAllEventAttribute : Attribute; public readonly struct EntityUid; @@ -88,6 +85,13 @@ protected void SubscribeNetworkEvent( } } + namespace Robust.Shared.Analyzers + { + public sealed class SubscribeLocalEventAttribute : Attribute; + public sealed class SubscribeNetworkEventAttribute : Attribute; + public sealed class SubscribeAllEventAttribute : Attribute; + } + public readonly struct TestEvent; public readonly struct TestEvent2; public sealed partial class TestComponent : IComponent; @@ -99,6 +103,7 @@ public sealed class TestNetworkEvent; public async Task ConvertLocalEvent() { const string code = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -118,6 +123,7 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; const string fixedCode = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -136,8 +142,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62) ); } @@ -146,6 +152,7 @@ await Verifier(code, fixedCode, public async Task ConvertLocalEvent_Multiple() { const string code = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -171,6 +178,7 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) """; const string fixedCode = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -195,10 +203,10 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62), // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 64) + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62), + // /0/Test0.cs(11,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(11, 9, 11, 64) ); } @@ -207,6 +215,7 @@ await Verifier(code, fixedCode, public async Task ConvertLocalEvent_MultipleWithGap() { const string code = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -233,6 +242,7 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) """; const string fixedCode = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -257,10 +267,10 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62), + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62), // /0/Test0.cs(11,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(11, 9, 11, 64) + VerifyCS.Diagnostic().WithSpan(12, 9, 12, 64) ); } @@ -269,6 +279,7 @@ await Verifier(code, fixedCode, public async Task ConvertLocalEvent_AddPartial() { const string code = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed class InitalizeBasedSystem : EntitySystem @@ -288,6 +299,55 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; const string fixedCode = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62) + ); + } + + [Test] + [Description("Tests that a class is given the using directive for the SubscribeLocalEventAttribute namespace when converted.")] + public async Task ConvertLocalEvent_AddUsingDirective() + { + const string code = """ + using Robust.Shared.GameObjects; + + public sealed class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); // Comment here + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -316,6 +376,7 @@ await Verifier(code, fixedCode, public async Task ConvertLocalEvent_HandlerOverload() { const string code = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -337,6 +398,7 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; const string fixedCode = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -357,8 +419,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62) ); } @@ -367,6 +429,7 @@ await Verifier(code, fixedCode, public async Task ConvertNetworkEvent() { const string code = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -386,6 +449,7 @@ private void OnTest(TestNetworkEvent args) """; const string fixedCode = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem @@ -404,8 +468,8 @@ private void OnTest(TestNetworkEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 56) + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 56) ); } @@ -452,6 +516,7 @@ public override void Initialize() """; const string fixed2 = """ + using Robust.Shared.Analyzers; using Robust.Shared.GameObjects; public sealed partial class InitalizeBasedSystem : EntitySystem diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 05bb0fcdee2..2d55807c78f 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -6,6 +6,7 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Simplification; using static Robust.Roslyn.Shared.Diagnostics; namespace Robust.Analyzers; @@ -13,6 +14,8 @@ namespace Robust.Analyzers; [ExportCodeFixProvider(LanguageNames.CSharp)] public sealed class EntitySystemSubscriptionConversionFixer : CodeFixProvider { + private const string AttributeNamespace = "Robust.Shared.Analyzers"; + public override ImmutableArray FixableDiagnosticIds => [ IdEntitySystemSubscriptionConversionPossible @@ -127,11 +130,18 @@ private static void ModifyHandler( IMethodSymbol handlerMethodSymbol, string attributeName) { + // var root = editor.OriginalRoot as CompilationUnitSyntax; + // if (!root!.Usings.Any(u => u.Name?.ToString() == AttributeNamespace)) + // { + // var newRoot = root.AddUsings(SyntaxFactory.UsingDirective(SyntaxFactory.ParseName(AttributeNamespace))); + // } + // editor.ReplaceNode + // Get the syntax node for the event handler method. var handlerMethodSyntax = handlerMethodSymbol.DeclaringSyntaxReferences.First().GetSyntax() as MethodDeclarationSyntax; // Generate the SubscribeWhateverEvent attribute. - var attr = SyntaxFactory.Attribute(SyntaxFactory.IdentifierName(attributeName)); + var attr = editor.Generator.Attribute(SyntaxFactory.IdentifierName(attributeName).WithAdditionalAnnotations(new SyntaxAnnotation("SymbolId", $"{AttributeNamespace}.{attributeName}Attribute"), Simplifier.AddImportsAnnotation)).WithAdditionalAnnotations(Simplifier.AddImportsAnnotation); // Add the attribute to the event handler method. editor.AddAttribute(handlerMethodSyntax!, attr); From 31dd718389485b13f290cb177986d6a34fdb6dc4 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 29 Jul 2026 22:40:56 -0400 Subject: [PATCH 17/31] Oops, I guess that's not a 1:1 mapping after all. --- ...tySystemSubscriptionConversionFixerTest.cs | 57 ++++++++++++++++++- ...itySystemSubscriptionConversionAnalyzer.cs | 9 ++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index f51ba75c3d5..f17a2f6af8c 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -68,6 +68,7 @@ public delegate void ComponentEventRefHandler(EntityUid uid, T where TComp : IComponent where TEvent : notnull; public delegate void EntityEventHandler(T ev); + public delegate void EntitySessionEventHandler(T msg, string foo); public abstract class EntitySystem { @@ -82,6 +83,11 @@ protected void SubscribeNetworkEvent( Type[]? before = null, Type[]? after = null) where T : notnull { } + protected void SubscribeAllEvent( + EntitySessionEventHandler handler, + Type[]? before = null, Type[]? after = null) + where T : notnull + { } } } @@ -89,7 +95,7 @@ namespace Robust.Shared.Analyzers { public sealed class SubscribeLocalEventAttribute : Attribute; public sealed class SubscribeNetworkEventAttribute : Attribute; - public sealed class SubscribeAllEventAttribute : Attribute; + public sealed class EventSubscriptionAttribute : Attribute; } public readonly struct TestEvent; @@ -473,6 +479,55 @@ await Verifier(code, fixedCode, ); } + [Test] + [Description("Tests that a SubscribeAllEvent invocation is correctly converted to an attribute.")] + public async Task ConvertAllEvent() + { + const string code = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeAllEvent(OnTest); // Comment here + } + + private void OnTest(TestNetworkEvent args, string foo) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [EventSubscription] + private void OnTest(TestNetworkEvent args, string foo) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 52) + ); + } + [Test] [Description("Tests that the conversion works correctly when the Initialize and event handler methods are declared in separate files (partial classes).")] public async Task ConvertLocalEvent_WithPartials() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 1941d000e61..8ca3ec7a899 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -18,6 +18,7 @@ public sealed class EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnaly private const string SubscribeLocalEventMethodName = "SubscribeLocalEvent"; private const string SubscribeNetworkEventMethodName = "SubscribeNetworkEvent"; private const string SubscribeAllEventMethodName = "SubscribeAllEvent"; + private const string SubscribeAllEventAttributeName = "EventSubscription"; private static readonly string[] SubscribeMethods = [ SubscribeLocalEventMethodName, @@ -154,8 +155,10 @@ private static void AnalyzeMethod(OperationAnalysisContext context) /// public static string ToAttributeName(string methodName) { - // This is currently a 1:1 match, but if it weren't, - // this would be the place to implement the remapping. - return methodName; + return methodName switch + { + SubscribeAllEventMethodName => SubscribeAllEventAttributeName, + _ => methodName + }; } } From 607c324d3d27d3f43645fbef156b3216f689a961 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 29 Jul 2026 22:48:58 -0400 Subject: [PATCH 18/31] A little bit of cleanup --- .../EntitySystemSubscriptionConversionAnalyzerTest.cs | 5 ----- .../EntitySystemSubscriptionConversionAnalyzer.cs | 2 ++ .../EntitySystemSubscriptionConversionFixer.cs | 7 ------- 3 files changed, 2 insertions(+), 12 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 2c9dc158d55..3fc44c4ffdc 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -20,11 +20,6 @@ private static Task Verifier(string code, params DiagnosticResult[] expected) }, }; - TestHelper.AddEmbeddedSources( - test.TestState//, - //"Robust.Shared.IoC.DependencyAttribute.cs" - ); - test.TestState.Sources.Add(("TestTypeDefs.cs", TestTypeDefs)); // ExpectedDiagnostics cannot be set, so we need to AddRange here... diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 8ca3ec7a899..91b105f4bc5 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -135,6 +135,8 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (handlerMethod.IsVirtual || handlerMethod.IsAbstract) continue; + // Find the name of the attribute we need to use to replace the invocation and + // pass it to the code fixer. var props = new Dictionary { { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 2d55807c78f..c7b482d2e25 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -130,13 +130,6 @@ private static void ModifyHandler( IMethodSymbol handlerMethodSymbol, string attributeName) { - // var root = editor.OriginalRoot as CompilationUnitSyntax; - // if (!root!.Usings.Any(u => u.Name?.ToString() == AttributeNamespace)) - // { - // var newRoot = root.AddUsings(SyntaxFactory.UsingDirective(SyntaxFactory.ParseName(AttributeNamespace))); - // } - // editor.ReplaceNode - // Get the syntax node for the event handler method. var handlerMethodSyntax = handlerMethodSymbol.DeclaringSyntaxReferences.First().GetSyntax() as MethodDeclarationSyntax; From 2d1741eee8699c040a85fa92dba82aa56b5c88bc Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Thu, 30 Jul 2026 11:21:39 -0400 Subject: [PATCH 19/31] Add global usings file to Robust.Shared.IntegrationTests project. Needed so that Robust.Shared.Analyzers is available to autogenerated code. --- Robust.Shared.IntegrationTests/Usings.cs | 1 + 1 file changed, 1 insertion(+) create mode 100644 Robust.Shared.IntegrationTests/Usings.cs diff --git a/Robust.Shared.IntegrationTests/Usings.cs b/Robust.Shared.IntegrationTests/Usings.cs new file mode 100644 index 00000000000..daaa0a73846 --- /dev/null +++ b/Robust.Shared.IntegrationTests/Usings.cs @@ -0,0 +1 @@ +global using Robust.Shared.Analyzers; From 84ed1ed427e93e35c4877047a12264b28a5f6e8f Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Thu, 30 Jul 2026 11:34:08 -0400 Subject: [PATCH 20/31] Break up and better document the automatic using directive stuff --- .../EntitySystemSubscriptionConversionAnalyzer.cs | 2 +- .../EntitySystemSubscriptionConversionFixer.cs | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 91b105f4bc5..a8d7a24459b 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -136,7 +136,7 @@ private static void AnalyzeMethod(OperationAnalysisContext context) continue; // Find the name of the attribute we need to use to replace the invocation and - // pass it to the code fixer. + // add it to the diagnostic so the code fixer can easily get it. var props = new Dictionary { { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index c7b482d2e25..72050294481 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -133,8 +133,17 @@ private static void ModifyHandler( // Get the syntax node for the event handler method. var handlerMethodSyntax = handlerMethodSymbol.DeclaringSyntaxReferences.First().GetSyntax() as MethodDeclarationSyntax; + // Generate an annotation containing the full name of the attribute we're adding. + // The magic string "SymbolId" makes this a SymbolAnnotation for Simplifier.AddImportsAnnotation to use. + var symbolAnnotation = new SyntaxAnnotation("SymbolId", $"{AttributeNamespace}.{attributeName}Attribute"); + + // Create the identifier for the attribute, annotating it with the full class name and AddImportsAnnotation. + // When Roslyn applies this code fix, AddImportsAnnotation tells it to add any missing using directives, + // but it needs the full name of the class to be able to do so. + var identifier = SyntaxFactory.IdentifierName(attributeName).WithAdditionalAnnotations(symbolAnnotation, Simplifier.AddImportsAnnotation); + // Generate the SubscribeWhateverEvent attribute. - var attr = editor.Generator.Attribute(SyntaxFactory.IdentifierName(attributeName).WithAdditionalAnnotations(new SyntaxAnnotation("SymbolId", $"{AttributeNamespace}.{attributeName}Attribute"), Simplifier.AddImportsAnnotation)).WithAdditionalAnnotations(Simplifier.AddImportsAnnotation); + var attr = editor.Generator.Attribute(identifier); // Add the attribute to the event handler method. editor.AddAttribute(handlerMethodSyntax!, attr); From 59e1ce5479da5dd602e62b7e195d82825524450c Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Thu, 30 Jul 2026 20:19:06 -0400 Subject: [PATCH 21/31] Remove the requirement for the method to be named Initialize This allows the analyzer/fixer to work on partial classes with InitializeSubsystem methods. --- .../EntitySystemSubscriptionConversionAnalyzer.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index a8d7a24459b..f07ecdebc4b 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -14,7 +14,6 @@ public sealed class EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnaly { private const string EntitySystemTypeName = "Robust.Shared.GameObjects.EntitySystem"; private const string SubscribeLocalEventAttributeTypeName = "Robust.Shared.Analyzers.SubscribeLocalEventAttribute"; - private const string InitializeMethodName = "Initialize"; private const string SubscribeLocalEventMethodName = "SubscribeLocalEvent"; private const string SubscribeNetworkEventMethodName = "SubscribeNetworkEvent"; private const string SubscribeAllEventMethodName = "SubscribeAllEvent"; @@ -77,10 +76,6 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (context.Operation is not IMethodBodyOperation method) return; - // We're only looking for the Initialize method - if (context.ContainingSymbol.Name != InitializeMethodName) - return; - if (method.BlockBody is null) return; From 2ae349dd91ee53c7507d436ea6bdaf530f24dff4 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Thu, 30 Jul 2026 23:13:33 -0400 Subject: [PATCH 22/31] Ignore event handlers with abstract event types --- ...ystemSubscriptionConversionAnalyzerTest.cs | 41 +++++++++++++++++++ ...itySystemSubscriptionConversionAnalyzer.cs | 6 +++ 2 files changed, 47 insertions(+) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 3fc44c4ffdc..100443fccaa 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -43,6 +43,10 @@ public delegate void ComponentEventRefHandler(EntityUid uid, T where TComp : IComponent where TEvent : notnull; + public delegate void ComponentEventHandler(EntityUid uid, TComp component, TEvent args) + where TComp : IComponent + where TEvent : notnull; + public abstract class EntitySystem { public virtual void Initialize() { } @@ -53,6 +57,14 @@ public void SubscribeLocalEvent( where TComp : IComponent where TEvent : notnull { } + + public void SubscribeLocalEvent( + ComponentEventHandler handler, + Type[]? before = null, + Type[]? after = null) + where TComp : IComponent + where TEvent : notnull + { } } } @@ -230,6 +242,35 @@ public sealed class TestEventClassB : TestEventArgs; await Verifier(code, []); } + [Test] + [Description("Tests that subscriptions using event handlers with abstract event types are not flagged as elligible for conversion.")] + public async Task IgnoreWithAbstractHandler() + { + const string code = """ + + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest); + SubscribeLocalEvent(OnTest); + } + + private void OnTest(EntityUid uid, TestComponent comp, TestEventArgs args) { } + } + + public abstract class TestEventArgs; + public sealed class TestEventClassA : TestEventArgs; + public sealed class TestEventClassB : TestEventArgs; + """; + + await Verifier(code, []); + } + [Test] [Description("Tests that subscriptions within if statement blocks are not flagged as elligible for conversion.")] public async Task IgnoreWithIfStatement() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index f07ecdebc4b..5f65cf25f56 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -125,6 +125,12 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (handlerMethod.IsGenericMethod) continue; + // If the target method's event type is abstract, we can't subscribe using the attribute, + // since the subscription needs the exact type. + var handlerEventType = handlerMethod.Parameters.Last().Type; + if (handlerEventType.IsAbstract) + continue; + // If the handler is a virtual or abstract method, we can't use the attribute // since we would have to add it to the base class if (handlerMethod.IsVirtual || handlerMethod.IsAbstract) From 987d46b62b2654066403c455e37ae854ea03f650 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Fri, 31 Jul 2026 16:39:52 -0400 Subject: [PATCH 23/31] Move class symbol lookup back into the code action. Saves us from needing to get the semantic model when registering the fix. --- .../EntitySystemSubscriptionConversionFixer.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 72050294481..944fc74c0aa 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -42,15 +42,13 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context) private static async Task RegisterSubscriptionConversion(CodeFixContext context, Diagnostic diagnostic) { - var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken); - var root = await semanticModel!.SyntaxTree.GetRootAsync(context.CancellationToken); + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken); var span = diagnostic.Location.SourceSpan; var invocationSyntax = root?.FindToken(span.Start).Parent?.AncestorsAndSelf().OfType().First(); var classSyntax = invocationSyntax?.AncestorsAndSelf().OfType().First(); - var classSymbol = semanticModel.GetDeclaredSymbol(classSyntax!); - if (invocationSyntax is null || classSyntax is null || classSymbol is null) + if (invocationSyntax is null || classSyntax is null) return; // Get the name of the Attribute we need to add to the event handler method. @@ -59,7 +57,7 @@ private static async Task RegisterSubscriptionConversion(CodeFixContext context, context.RegisterCodeFix(CodeAction.Create( "Convert subscription to attribute", - c => ConvertSubscription(context.Document, invocationSyntax, classSymbol, classSyntax, attributeName, c), + c => ConvertSubscription(context.Document, invocationSyntax, classSyntax, attributeName, c), "Convert subscription to attribute" ), diagnostic); } @@ -67,7 +65,6 @@ private static async Task RegisterSubscriptionConversion(CodeFixContext context, private static async Task ConvertSubscription( Document document, InvocationExpressionSyntax invocationSyntax, - INamedTypeSymbol classSymbol, ClassDeclarationSyntax classSyntax, string attributeName, CancellationToken c) @@ -80,6 +77,9 @@ private static async Task ConvertSubscription( if (model.GetSymbolInfo(handlerMethodIdentifer, c).Symbol is not IMethodSymbol handlerMethodSymbol) throw new InvalidOperationException($"Failed to find event handler method {handlerMethodIdentifer}"); + if (model.GetDeclaredSymbol(classSyntax) is not { } classSymbol) + throw new InvalidOperationException($"Failed to find symbol for class {classSyntax.Identifier}"); + // Create a SolutionEditor to edit multiple documents without worrying about immutability. // The Initialize method might be in a different document than the handler, thanks to partial classes. var editor = new SolutionEditor(document.Project.Solution); @@ -136,12 +136,12 @@ private static void ModifyHandler( // Generate an annotation containing the full name of the attribute we're adding. // The magic string "SymbolId" makes this a SymbolAnnotation for Simplifier.AddImportsAnnotation to use. var symbolAnnotation = new SyntaxAnnotation("SymbolId", $"{AttributeNamespace}.{attributeName}Attribute"); - + // Create the identifier for the attribute, annotating it with the full class name and AddImportsAnnotation. // When Roslyn applies this code fix, AddImportsAnnotation tells it to add any missing using directives, // but it needs the full name of the class to be able to do so. var identifier = SyntaxFactory.IdentifierName(attributeName).WithAdditionalAnnotations(symbolAnnotation, Simplifier.AddImportsAnnotation); - + // Generate the SubscribeWhateverEvent attribute. var attr = editor.Generator.Attribute(identifier); From 060d609ecf28c338760f5fde80791002dd892e4a Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Fri, 31 Jul 2026 16:51:56 -0400 Subject: [PATCH 24/31] Fancier warning message --- ...ystemSubscriptionConversionAnalyzerTest.cs | 4 +- ...tySystemSubscriptionConversionFixerTest.cs | 44 +++++++++---------- ...itySystemSubscriptionConversionAnalyzer.cs | 9 ++-- 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 100443fccaa..0d711542abd 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -103,8 +103,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + // /0/Test0.cs(9,9): info RA0058: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index f17a2f6af8c..5b5a92351e2 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -148,8 +148,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62) + // /0/Test0.cs(10,9): info RA0058: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } @@ -209,10 +209,10 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62), - // /0/Test0.cs(11,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(11, 9, 11, 64) + // /0/Test0.cs(10,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute"), + // /0/Test0.cs(11,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(11, 9, 11, 64).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } @@ -273,10 +273,10 @@ private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62), - // /0/Test0.cs(11,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(12, 9, 12, 64) + // /0/Test0.cs(10,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute"), + // /0/Test0.cs(12,9): info RA0057:Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(12, 9, 12, 64).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } @@ -324,8 +324,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62) + // /0/Test0.cs(10,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } @@ -372,8 +372,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + // /0/Test0.cs(9,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } @@ -425,8 +425,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62) + // /0/Test0.cs(10,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } @@ -474,8 +474,8 @@ private void OnTest(TestNetworkEvent args) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 56) + // /0/Test0.cs(10,9): info RA0058: Event subscription using SubscribeNetworkEvent can be converted to use SubscribeNetworkEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 56).WithArguments("SubscribeNetworkEvent", "SubscribeNetworkEventAttribute") ); } @@ -523,8 +523,8 @@ private void OnTest(TestNetworkEvent args, string foo) """; await Verifier(code, fixedCode, - // /0/Test0.cs(10,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(10, 9, 10, 52) + // /0/Test0.cs(10,9): info RA0058: Event subscription using SubscribeAllEvent can be converted to use EventSubscriptionAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 52).WithArguments("SubscribeAllEvent", "EventSubscriptionAttribute") ); } @@ -585,8 +585,8 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) """; await Verifier([code1, code2], [fixed1, fixed2], - // /0/Test0.cs(9,9): info RA0057: Initialize-based event subscription can be converted to attribute-based - VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62) + // /0/Test0.cs(9,9): info RA0057: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(9, 9, 9, 62).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") ); } } diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 5f65cf25f56..e0086711b3e 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -30,7 +30,7 @@ public sealed class EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnaly public static readonly DiagnosticDescriptor EntitySystemSubscriptionConversionPossible = new( Diagnostics.IdEntitySystemSubscriptionConversionPossible, "Convert to attribute-based subscription", - "Initialize-based event subscription can be converted to attribute-based", + "Event subscription using {0} can be converted to use {1}", "Usage", DiagnosticSeverity.Info, true @@ -138,16 +138,19 @@ private static void AnalyzeMethod(OperationAnalysisContext context) // Find the name of the attribute we need to use to replace the invocation and // add it to the diagnostic so the code fixer can easily get it. + var attributeName = ToAttributeName(invocation.TargetMethod.Name); var props = new Dictionary { - { AttributeNameKey, ToAttributeName(invocation.TargetMethod.Name) } + { AttributeNameKey, attributeName } }; // Flag this subscription as elligible for conversion context.ReportDiagnostic(Diagnostic.Create( EntitySystemSubscriptionConversionPossible, invocation.Syntax.GetLocation(), - props.ToImmutableDictionary() + props.ToImmutableDictionary(), + invocation.TargetMethod.Name, + $"{attributeName}Attribute" )); } } From dfd28141c986c96fc2ef979d31295662ed5ed064 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Sun, 2 Aug 2026 20:08:51 -0400 Subject: [PATCH 25/31] Reuse a single document editor if possible --- Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 944fc74c0aa..5e85d44f515 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -93,7 +93,8 @@ private static async Task ConvertSubscription( var handlerDocId = editor.OriginalSolution.GetDocumentId(handlerMethodSymbol.DeclaringSyntaxReferences.First().SyntaxTree); // Get an editor for the document containing the event handler method. - var handlerEditor = await editor.GetDocumentEditorAsync(handlerDocId, c); + // If the event handler is in the same document as the Initialize method, just reuse the same editor. + var handlerEditor = (handlerDocId == document.Id) ? initializeEditor : await editor.GetDocumentEditorAsync(handlerDocId, c); // Make our changes to the document containing the event handler method. ModifyHandler(handlerEditor, handlerMethodSymbol, attributeName); From fe68dfc1c43f89c2646be76f154100bb8eeaa7f6 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Sun, 2 Aug 2026 20:17:32 -0400 Subject: [PATCH 26/31] Filter for EntitySystems using checking for IEntitySystem instead of inheritance Appears to be slightly cheaper --- .../EntitySystemSubscriptionConversionAnalyzerTest.cs | 3 ++- .../EntitySystemSubscriptionConversionFixerTest.cs | 4 ++-- .../EntitySystemSubscriptionConversionAnalyzer.cs | 7 +++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 0d711542abd..87e041ae804 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -47,7 +47,8 @@ public delegate void ComponentEventHandler(EntityUid uid, T where TComp : IComponent where TEvent : notnull; - public abstract class EntitySystem + public interface IEntitySystem; + public abstract class EntitySystem : IEntitySystem { public virtual void Initialize() { } public void SubscribeLocalEvent( diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index 5b5a92351e2..cb5f9c891e2 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Testing; @@ -70,7 +69,8 @@ public delegate void ComponentEventRefHandler(EntityUid uid, T public delegate void EntityEventHandler(T ev); public delegate void EntitySessionEventHandler(T msg, string foo); - public abstract class EntitySystem + public interface IEntitySystem; + public abstract class EntitySystem : IEntitySystem { public virtual void Initialize() { } public void SubscribeLocalEvent( diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index e0086711b3e..ad933f9b1dd 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -12,7 +12,7 @@ namespace Robust.Analyzers; [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnalyzer { - private const string EntitySystemTypeName = "Robust.Shared.GameObjects.EntitySystem"; + private const string EntitySystemTypeName = "Robust.Shared.GameObjects.IEntitySystem"; private const string SubscribeLocalEventAttributeTypeName = "Robust.Shared.Analyzers.SubscribeLocalEventAttribute"; private const string SubscribeLocalEventMethodName = "SubscribeLocalEvent"; private const string SubscribeNetworkEventMethodName = "SubscribeNetworkEvent"; @@ -48,7 +48,7 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationStartAction(ctx => { - // If the subscription attribute isn't available in this compilation, we can't do anything. + // If the subscription attributes aren't available in this compilation, we can't do anything. if (ctx.Compilation.GetTypeByMetadataName(SubscribeLocalEventAttributeTypeName) is null) return; @@ -61,8 +61,7 @@ public override void Initialize(AnalysisContext context) if (symbolContext.Symbol is not INamedTypeSymbol typeSymbol || typeSymbol.TypeKind != TypeKind.Class) return; - // Must inherit from EntitySystem - if (!TypeSymbolHelper.Inherits(typeSymbol, entitySystemType)) + if (!typeSymbol.AllInterfaces.Contains(entitySystemType)) return; // Check each method definition in the class From 9b6f90fbbcdf6ce2b0f3fce232e0373259fb11d1 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Sun, 2 Aug 2026 20:46:25 -0400 Subject: [PATCH 27/31] Docs cleanup and tweaks --- .../EntitySystemSubscriptionConversionAnalyzer.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index ad933f9b1dd..dec5a7ba604 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -25,6 +25,9 @@ public sealed class EntitySystemSubscriptionConversionAnalyzer : DiagnosticAnaly SubscribeAllEventMethodName, ]; + /// + /// The key used to access the needed replacement attribute name in the diagnostic's Properties dictionary. + /// public const string AttributeNameKey = "attribute"; public static readonly DiagnosticDescriptor EntitySystemSubscriptionConversionPossible = new( @@ -61,6 +64,7 @@ public override void Initialize(AnalysisContext context) if (symbolContext.Symbol is not INamedTypeSymbol typeSymbol || typeSymbol.TypeKind != TypeKind.Class) return; + // Filter out anything that isn't an EntitySystem if (!typeSymbol.AllInterfaces.Contains(entitySystemType)) return; @@ -126,12 +130,14 @@ private static void AnalyzeMethod(OperationAnalysisContext context) // If the target method's event type is abstract, we can't subscribe using the attribute, // since the subscription needs the exact type. + // This assumes that the event is the last parameter in the handler's signature, + // which seems like a reasonable assumption at the time of writing. var handlerEventType = handlerMethod.Parameters.Last().Type; if (handlerEventType.IsAbstract) continue; // If the handler is a virtual or abstract method, we can't use the attribute - // since we would have to add it to the base class + // since we would have to add it to the base class. if (handlerMethod.IsVirtual || handlerMethod.IsAbstract) continue; From 0f5f95c073b26dd0b7058705943a95bfa2bf4dd2 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 5 Aug 2026 13:25:17 -0400 Subject: [PATCH 28/31] First pass adding before/after support --- ...ystemSubscriptionConversionAnalyzerTest.cs | 54 ++++++++-------- ...tySystemSubscriptionConversionFixerTest.cs | 64 +++++++++++++++++-- ...itySystemSubscriptionConversionAnalyzer.cs | 14 ++-- ...EntitySystemSubscriptionConversionFixer.cs | 53 ++++++++++++++- 4 files changed, 144 insertions(+), 41 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 87e041ae804..54e18aa3d03 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -109,33 +109,33 @@ await Verifier(code, ); } - [Test] - [Description("Tests that subscriptions with before/after parameters are not flagged as elligible for conversion.")] - // TODO: Remove this test if event subscription attributes get support for before/after parameters (and the code fixer is made to convert to them) - public async Task IgnoreBeforeAfter() - { - const string code = """ - using Robust.Shared.GameObjects; - - public sealed partial class InitalizeBasedSystem : EntitySystem - { - public override void Initialize() - { - base.Initialize(); - - SubscribeLocalEvent(OnTest, before: [typeof(Component)]); - SubscribeLocalEvent(OnTest2, after: [typeof(Component)]); - SubscribeLocalEvent(OnTest3, before: [typeof(Component)], after: [typeof(Component)]); - } - - private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) { } - private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) { } - private void OnTest3(EntityUid uid, TestComponent comp, ref TestEvent3 args) { } - } - """; - - await Verifier(code, []); - } + // [Test] + // [Description("Tests that subscriptions with before/after parameters are not flagged as elligible for conversion.")] + // // TODO: Remove this test if event subscription attributes get support for before/after parameters (and the code fixer is made to convert to them) + // public async Task IgnoreBeforeAfter() + // { + // const string code = """ + // using Robust.Shared.GameObjects; + + // public sealed partial class InitalizeBasedSystem : EntitySystem + // { + // public override void Initialize() + // { + // base.Initialize(); + + // SubscribeLocalEvent(OnTest, before: [typeof(Component)]); + // SubscribeLocalEvent(OnTest2, after: [typeof(Component)]); + // SubscribeLocalEvent(OnTest3, before: [typeof(Component)], after: [typeof(Component)]); + // } + + // private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) { } + // private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) { } + // private void OnTest3(EntityUid uid, TestComponent comp, ref TestEvent3 args) { } + // } + // """; + + // await Verifier(code, []); + // } [Test] [Description("Tests that a subscription using an anonymous delegate is not flagged as elligible for conversion.")] diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index cb5f9c891e2..c7edb9114df 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -74,7 +74,8 @@ public abstract class EntitySystem : IEntitySystem { public virtual void Initialize() { } public void SubscribeLocalEvent( - ComponentEventRefHandler handler) + ComponentEventRefHandler handler, + Type[]? before = null, Type[]? after = null) where TComp : IComponent where TEvent : notnull { } @@ -93,9 +94,9 @@ protected void SubscribeAllEvent( namespace Robust.Shared.Analyzers { - public sealed class SubscribeLocalEventAttribute : Attribute; - public sealed class SubscribeNetworkEventAttribute : Attribute; - public sealed class EventSubscriptionAttribute : Attribute; + public sealed class SubscribeLocalEventAttribute(Type[]? before = null, Type[]? after = null) : Attribute; + public sealed class SubscribeNetworkEventAttribute(Type[]? before = null, Type[]? after = null) : Attribute; + public sealed class EventSubscriptionAttribute(Type[]? before = null, Type[]? after = null) : Attribute; } public readonly struct TestEvent; @@ -280,6 +281,61 @@ await Verifier(code, fixedCode, ); } + [Test] + [Description("Tests that a SubscribeLocalEvent invocation with before and after parameters is correctly converted to an attribute.")] + public async Task ConvertLocalEvent_WithBeforeAfter() + { + const string code = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest, before: [typeof(SomeOtherSystemA)], after: new[] { typeof(SomeOtherSystemB) }); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + + public sealed class SomeOtherSystemA : EntitySystem; + public sealed class SomeOtherSystemB : EntitySystem; + """; + + const string fixedCode = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent(before: [typeof(SomeOtherSystemA)], after: [typeof(SomeOtherSystemB)])] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + + public sealed class SomeOtherSystemA : EntitySystem; + public sealed class SomeOtherSystemB : EntitySystem; + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(10,9): info RA0058: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 141).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") + ); + } + [Test] [Description("Tests that a class that isn't marked partial is given the partial modifier when converted.")] public async Task ConvertLocalEvent_AddPartial() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index dec5a7ba604..1cc2371bdf9 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -105,13 +105,13 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (invocation.TargetMethod.TypeArguments.OfType().Any()) continue; - // We (currently) don't support the before and after parameters with attribute subscriptions - // so we skip any invocations that use them. - // If we do support them in the future (and the code fixer is improved to convert to them), this check should be removed. - if (invocation.Arguments.Any( - arg => (arg.Parameter?.Name == "before" || arg.Parameter?.Name == "after") - && arg.Value is not IDefaultValueOperation)) - continue; + // // We (currently) don't support the before and after parameters with attribute subscriptions + // // so we skip any invocations that use them. + // // If we do support them in the future (and the code fixer is improved to convert to them), this check should be removed. + // if (invocation.Arguments.Any( + // arg => (arg.Parameter?.Name == "before" || arg.Parameter?.Name == "after") + // && arg.Value is not IDefaultValueOperation)) + // continue; // Ignore anything that isn't a direct method reference, i.e. an anonymous delegate. if (invocation.Arguments.SingleOrDefault(arg => arg.Parameter?.Name == "handler") is not { } handlerArg diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 5e85d44f515..e3b14472a44 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -6,6 +6,7 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Operations; using Microsoft.CodeAnalysis.Simplification; using static Robust.Roslyn.Shared.Diagnostics; @@ -80,6 +81,12 @@ private static async Task ConvertSubscription( if (model.GetDeclaredSymbol(classSyntax) is not { } classSymbol) throw new InvalidOperationException($"Failed to find symbol for class {classSyntax.Identifier}"); + if (model?.GetOperation(invocationSyntax) is not IInvocationOperation invocationOperation) + throw new InvalidOperationException($"Failed to find invocation operation"); + + var beforeTypes = GetTypesList(invocationOperation, "before"); + var afterTypes = GetTypesList(invocationOperation, "after"); + // Create a SolutionEditor to edit multiple documents without worrying about immutability. // The Initialize method might be in a different document than the handler, thanks to partial classes. var editor = new SolutionEditor(document.Project.Solution); @@ -96,7 +103,7 @@ private static async Task ConvertSubscription( // If the event handler is in the same document as the Initialize method, just reuse the same editor. var handlerEditor = (handlerDocId == document.Id) ? initializeEditor : await editor.GetDocumentEditorAsync(handlerDocId, c); // Make our changes to the document containing the event handler method. - ModifyHandler(handlerEditor, handlerMethodSymbol, attributeName); + ModifyHandler(handlerEditor, handlerMethodSymbol, attributeName, beforeTypes, afterTypes); // Make sure the class is marked as partial. EnsureClassPartial(initializeEditor, classSymbol, classSyntax); @@ -129,7 +136,10 @@ private static void ModifyInitialize( private static void ModifyHandler( DocumentEditor editor, IMethodSymbol handlerMethodSymbol, - string attributeName) + string attributeName, + IEnumerable beforeTypes, + IEnumerable afterTypes + ) { // Get the syntax node for the event handler method. var handlerMethodSyntax = handlerMethodSymbol.DeclaringSyntaxReferences.First().GetSyntax() as MethodDeclarationSyntax; @@ -141,11 +151,16 @@ private static void ModifyHandler( // Create the identifier for the attribute, annotating it with the full class name and AddImportsAnnotation. // When Roslyn applies this code fix, AddImportsAnnotation tells it to add any missing using directives, // but it needs the full name of the class to be able to do so. - var identifier = SyntaxFactory.IdentifierName(attributeName).WithAdditionalAnnotations(symbolAnnotation, Simplifier.AddImportsAnnotation); + var identifier = editor.Generator.IdentifierName(attributeName).WithAdditionalAnnotations(symbolAnnotation, Simplifier.AddImportsAnnotation); // Generate the SubscribeWhateverEvent attribute. var attr = editor.Generator.Attribute(identifier); + var before = BuildArgument(beforeTypes, "before"); + var after = BuildArgument(afterTypes, "after"); + + attr = editor.Generator.AddAttributeArguments(attr, [before, after]); + // Add the attribute to the event handler method. editor.AddAttribute(handlerMethodSyntax!, attr); } @@ -163,4 +178,36 @@ private static void EnsureClassPartial( // Add the partial modifier if it's not already there. editor.SetModifiers(classSyntax, oldModifiers.WithPartial(true)); } + + private static IEnumerable GetTypesList(IInvocationOperation invocationOperation, string parameter) + { + var arg = invocationOperation.Arguments.Where(arg => arg.Parameter?.Name == parameter).SingleOrDefault(); + if (arg.Value is IDefaultValueOperation or null) + return []; + var expression = (arg.Syntax as ArgumentSyntax)?.Expression; + return expression switch + { + CollectionExpressionSyntax collection => collection.Elements.OfType().Select(e => e.Expression), + ArrayCreationExpressionSyntax arrayCreation => arrayCreation.Initializer?.Expressions ?? [], + ImplicitArrayCreationExpressionSyntax implicitArrayCreation => implicitArrayCreation.Initializer.Expressions, + _ => throw new InvalidOperationException("Invalid types list") + }; + //var node = arg.Value.Syntax; + //var node = invocationOperation.Arguments.Where(arg => arg.Parameter?.Name == parameter).Select(arg => arg.Value.Syntax).SingleOrDefault(); + // return node switch + // { + // ImplicitArrayCreationExpressionSyntax implicitArrayCreation => implicitArrayCreation.Initializer.Expressions.Cast(), + // CollectionExpressionSyntax collection => collection.Elements.Select(el => (el as ExpressionElementSyntax)?.Expression).Cast(), + // null => [], + // _ => throw new InvalidOperationException("Invalid types list") + // }; + } + + private static AttributeArgumentSyntax BuildArgument(IEnumerable types, string name) + { + var nameColon = SyntaxFactory.NameColon(name); + var syntaxList = SyntaxFactory.SeparatedList(types.Select(SyntaxFactory.ExpressionElement)); + var collection = SyntaxFactory.CollectionExpression(syntaxList); + return SyntaxFactory.AttributeArgument(null, nameColon, collection); + } } From 75a5e0d3bf5175fa118180194b066b4035b10571 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 5 Aug 2026 22:39:57 -0400 Subject: [PATCH 29/31] Cleanup and docs --- ...ystemSubscriptionConversionAnalyzerTest.cs | 28 ------------- ...itySystemSubscriptionConversionAnalyzer.cs | 8 ---- ...EntitySystemSubscriptionConversionFixer.cs | 39 +++++++++++++------ 3 files changed, 27 insertions(+), 48 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs index 54e18aa3d03..3c5fdb4c339 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionAnalyzerTest.cs @@ -109,34 +109,6 @@ await Verifier(code, ); } - // [Test] - // [Description("Tests that subscriptions with before/after parameters are not flagged as elligible for conversion.")] - // // TODO: Remove this test if event subscription attributes get support for before/after parameters (and the code fixer is made to convert to them) - // public async Task IgnoreBeforeAfter() - // { - // const string code = """ - // using Robust.Shared.GameObjects; - - // public sealed partial class InitalizeBasedSystem : EntitySystem - // { - // public override void Initialize() - // { - // base.Initialize(); - - // SubscribeLocalEvent(OnTest, before: [typeof(Component)]); - // SubscribeLocalEvent(OnTest2, after: [typeof(Component)]); - // SubscribeLocalEvent(OnTest3, before: [typeof(Component)], after: [typeof(Component)]); - // } - - // private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) { } - // private void OnTest2(EntityUid uid, TestComponent comp, ref TestEvent2 args) { } - // private void OnTest3(EntityUid uid, TestComponent comp, ref TestEvent3 args) { } - // } - // """; - - // await Verifier(code, []); - // } - [Test] [Description("Tests that a subscription using an anonymous delegate is not flagged as elligible for conversion.")] public async Task IgnoreAnonymousDelegate() diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs index 1cc2371bdf9..358caf99d15 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionAnalyzer.cs @@ -105,14 +105,6 @@ private static void AnalyzeMethod(OperationAnalysisContext context) if (invocation.TargetMethod.TypeArguments.OfType().Any()) continue; - // // We (currently) don't support the before and after parameters with attribute subscriptions - // // so we skip any invocations that use them. - // // If we do support them in the future (and the code fixer is improved to convert to them), this check should be removed. - // if (invocation.Arguments.Any( - // arg => (arg.Parameter?.Name == "before" || arg.Parameter?.Name == "after") - // && arg.Value is not IDefaultValueOperation)) - // continue; - // Ignore anything that isn't a direct method reference, i.e. an anonymous delegate. if (invocation.Arguments.SingleOrDefault(arg => arg.Parameter?.Name == "handler") is not { } handlerArg || handlerArg.Value.Syntax is not IdentifierNameSyntax) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index e3b14472a44..7f8c76d76c7 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -156,8 +156,8 @@ IEnumerable afterTypes // Generate the SubscribeWhateverEvent attribute. var attr = editor.Generator.Attribute(identifier); - var before = BuildArgument(beforeTypes, "before"); - var after = BuildArgument(afterTypes, "after"); + var before = GenerateTypesArgument(beforeTypes, "before"); + var after = GenerateTypesArgument(afterTypes, "after"); attr = editor.Generator.AddAttributeArguments(attr, [before, after]); @@ -179,35 +179,50 @@ private static void EnsureClassPartial( editor.SetModifiers(classSyntax, oldModifiers.WithPartial(true)); } + /// + /// Extracts an enumerable containing the nodes passed to the named + /// parameter of an invocation. + /// + /// The method invocation the argument is being passed to. + /// The name of the parameter ("before" or "after") + /// + /// Thrown if the passed value is not a valid type of expression. + /// The passed value must be either a collection expression or an array literal. + /// private static IEnumerable GetTypesList(IInvocationOperation invocationOperation, string parameter) { + // Get the operation representing the argument we're looking for. var arg = invocationOperation.Arguments.Where(arg => arg.Parameter?.Name == parameter).SingleOrDefault(); + // If the argument is omitted, the operation will be a DefaultValueOperation. if (arg.Value is IDefaultValueOperation or null) return []; + // The way of getting the set of elements varies depending on the syntax that was used. var expression = (arg.Syntax as ArgumentSyntax)?.Expression; return expression switch { + // SubscribeLocalEvent(MyMethod, before: [typeof(MyOtherSystem)]) CollectionExpressionSyntax collection => collection.Elements.OfType().Select(e => e.Expression), + // SubscribeLocalEvent(MyMethod, before: new Type[] { typeof(MyOtherSystem) }) ArrayCreationExpressionSyntax arrayCreation => arrayCreation.Initializer?.Expressions ?? [], + // SubscribeLocalEvent(MyMethod, before: new[] { typeof(MyOtherSystem) }) ImplicitArrayCreationExpressionSyntax implicitArrayCreation => implicitArrayCreation.Initializer.Expressions, _ => throw new InvalidOperationException("Invalid types list") }; - //var node = arg.Value.Syntax; - //var node = invocationOperation.Arguments.Where(arg => arg.Parameter?.Name == parameter).Select(arg => arg.Value.Syntax).SingleOrDefault(); - // return node switch - // { - // ImplicitArrayCreationExpressionSyntax implicitArrayCreation => implicitArrayCreation.Initializer.Expressions.Cast(), - // CollectionExpressionSyntax collection => collection.Elements.Select(el => (el as ExpressionElementSyntax)?.Expression).Cast(), - // null => [], - // _ => throw new InvalidOperationException("Invalid types list") - // }; } - private static AttributeArgumentSyntax BuildArgument(IEnumerable types, string name) + /// + /// Returns a syntax node representing an attribute argument passing a collection expression of typeof expressions. + /// + /// The typeof expressions to populate the collection. + /// The name of the method parameter this argument is being passed to ("before" or "after"). + private static AttributeArgumentSyntax GenerateTypesArgument(IEnumerable types, string name) { + // Explicitly naming the parameters is much nicer for readability, especially with optional parameters. var nameColon = SyntaxFactory.NameColon(name); + // Throw our list of typeof expressions into a collection expression. var syntaxList = SyntaxFactory.SeparatedList(types.Select(SyntaxFactory.ExpressionElement)); var collection = SyntaxFactory.CollectionExpression(syntaxList); + // Return the complete argument to be added to the attribute. return SyntaxFactory.AttributeArgument(null, nameColon, collection); } } From f937ca97cf5c6f65429d437ff192e21d2eb0d6d1 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 5 Aug 2026 22:57:47 -0400 Subject: [PATCH 30/31] Don't add empty type arrays --- ...EntitySystemSubscriptionConversionFixer.cs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs index 7f8c76d76c7..28b9f02e0b6 100644 --- a/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs +++ b/Robust.Analyzers/EntitySystemSubscriptionConversionFixer.cs @@ -137,8 +137,8 @@ private static void ModifyHandler( DocumentEditor editor, IMethodSymbol handlerMethodSymbol, string attributeName, - IEnumerable beforeTypes, - IEnumerable afterTypes + IEnumerable? beforeTypes, + IEnumerable? afterTypes ) { // Get the syntax node for the event handler method. @@ -156,10 +156,17 @@ IEnumerable afterTypes // Generate the SubscribeWhateverEvent attribute. var attr = editor.Generator.Attribute(identifier); + // Generate attribute argument syntax nodes for the before and after arguments. var before = GenerateTypesArgument(beforeTypes, "before"); var after = GenerateTypesArgument(afterTypes, "after"); - attr = editor.Generator.AddAttributeArguments(attr, [before, after]); + // Remove either or both if they are null (meaning they weren't in the original invocation). + var args = new[]{before, after}.Where(arg => arg is not null); + + // If either or both are non-null, add them as arguments to the attribute. + // If both are null, we don't add anything otherwise we get empty parentheses on the attribute. + if (args.Any()) + attr = editor.Generator.AddAttributeArguments(attr, args!); // Add the attribute to the event handler method. editor.AddAttribute(handlerMethodSyntax!, attr); @@ -189,13 +196,13 @@ private static void EnsureClassPartial( /// Thrown if the passed value is not a valid type of expression. /// The passed value must be either a collection expression or an array literal. /// - private static IEnumerable GetTypesList(IInvocationOperation invocationOperation, string parameter) + private static IEnumerable? GetTypesList(IInvocationOperation invocationOperation, string parameter) { // Get the operation representing the argument we're looking for. var arg = invocationOperation.Arguments.Where(arg => arg.Parameter?.Name == parameter).SingleOrDefault(); // If the argument is omitted, the operation will be a DefaultValueOperation. if (arg.Value is IDefaultValueOperation or null) - return []; + return null; // The way of getting the set of elements varies depending on the syntax that was used. var expression = (arg.Syntax as ArgumentSyntax)?.Expression; return expression switch @@ -215,8 +222,10 @@ private static IEnumerable GetTypesList(IInvocationOperation i /// /// The typeof expressions to populate the collection. /// The name of the method parameter this argument is being passed to ("before" or "after"). - private static AttributeArgumentSyntax GenerateTypesArgument(IEnumerable types, string name) + private static AttributeArgumentSyntax? GenerateTypesArgument(IEnumerable? types, string name) { + if (types is null) + return null; // Explicitly naming the parameters is much nicer for readability, especially with optional parameters. var nameColon = SyntaxFactory.NameColon(name); // Throw our list of typeof expressions into a collection expression. From 3b171db152d6aeae8524e499ebee8529486637a9 Mon Sep 17 00:00:00 2001 From: Tayrtahn Date: Wed, 5 Aug 2026 23:03:59 -0400 Subject: [PATCH 31/31] More tests --- ...tySystemSubscriptionConversionFixerTest.cs | 107 +++++++++++++++++- 1 file changed, 101 insertions(+), 6 deletions(-) diff --git a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs index c7edb9114df..2b1c237d17f 100644 --- a/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs +++ b/Robust.Analyzers.Tests/EntitySystemSubscriptionConversionFixerTest.cs @@ -103,6 +103,9 @@ public sealed class EventSubscriptionAttribute(Type[]? before = null, Type[]? af public readonly struct TestEvent2; public sealed partial class TestComponent : IComponent; public sealed class TestNetworkEvent; + + public sealed class SomeOtherSystemA : EntitySystem; + public sealed class SomeOtherSystemB : EntitySystem; """; [Test] @@ -303,9 +306,6 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) // Do something } } - - public sealed class SomeOtherSystemA : EntitySystem; - public sealed class SomeOtherSystemB : EntitySystem; """; const string fixedCode = """ @@ -325,9 +325,6 @@ private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) // Do something } } - - public sealed class SomeOtherSystemA : EntitySystem; - public sealed class SomeOtherSystemB : EntitySystem; """; await Verifier(code, fixedCode, @@ -336,6 +333,104 @@ await Verifier(code, fixedCode, ); } + [Test] + [Description("Tests that a SubscribeLocalEvent invocation a before parameter is correctly converted to an attribute.")] + public async Task ConvertLocalEvent_WithBefore() + { + const string code = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest, before: new[] { typeof(SomeOtherSystemA) }); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent(before: [typeof(SomeOtherSystemA)])] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(10,9): info RA0058: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 106).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") + ); + } + + [Test] + [Description("Tests that a SubscribeLocalEvent invocation an after parameter is correctly converted to an attribute.")] + public async Task ConvertLocalEvent_WithAfter() + { + const string code = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + + SubscribeLocalEvent(OnTest, after: [typeof(SomeOtherSystemA), typeof(SomeOtherSystemB)]); + } + + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + const string fixedCode = """ + using Robust.Shared.Analyzers; + using Robust.Shared.GameObjects; + + public sealed partial class InitalizeBasedSystem : EntitySystem + { + public override void Initialize() + { + base.Initialize(); + } + + [SubscribeLocalEvent(after: [typeof(SomeOtherSystemA), typeof(SomeOtherSystemB)])] + private void OnTest(EntityUid uid, TestComponent comp, ref TestEvent args) + { + // Do something + } + } + """; + + await Verifier(code, fixedCode, + // /0/Test0.cs(10,9): info RA0058: Event subscription using SubscribeLocalEvent can be converted to use SubscribeLocalEventAttribute + VerifyCS.Diagnostic().WithSpan(10, 9, 10, 123).WithArguments("SubscribeLocalEvent", "SubscribeLocalEventAttribute") + ); + } + [Test] [Description("Tests that a class that isn't marked partial is given the partial modifier when converted.")] public async Task ConvertLocalEvent_AddPartial()