Add a Code Fix for converting EntitySystem event subscriptions to use attributes - #6909
Open
Tayrtahn wants to merge 33 commits into
Open
Add a Code Fix for converting EntitySystem event subscriptions to use attributes#6909Tayrtahn wants to merge 33 commits into
Tayrtahn wants to merge 33 commits into
Conversation
No longer confused by other methods with the same name
Needed so that Robust.Shared.Analyzers is available to autogenerated code.
This allows the analyzer/fixer to work on partial classes with InitializeSubsystem methods.
Saves us from needing to get the semantic model when registering the fix.
…inheritance Appears to be slightly cheaper
Contributor
|
#6904 was merged. Can you add support for it? |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Adds an analyzer and code fix to detect instances of SubscribeXEvent method calls and replace them with the event subscription attributes added by #6227.
There are quite a few limitations on which instances can be replaced automatically, but this currently covers 4794 of them in engine and content, which I think is pretty good.
The analyzer detects SubscribeXEvent method invocations that are eligible for automatic conversion and marks them with an Info diagnostic:

(They show as
...in vscode, and I believe as a green squiggle in Rider)Clicking Quick Fix or the lightbulb presents the option for the code fix:

The code fix:
[SubscribeXEvent]attribute to the event handler method.partialto the class if it doesn't already have it.using Robust.Shared.Analyzersif needed.This works even if the event handler is in a different file (for systems spread across multiple partial files).
If this deletes all the subscribe invocations from a method, it will leave behind an empty method or one that just calls its base. I investigated trying to automatically remove the method in those cases, but it wound up being quite complicated and I don't think it's worth trying to sort it all out.
There are a bunch of cases where the subscribe invocation can't be directly converted to use an attribute. The analyzer will not mark these cases; they will need manual conversion:
#if,#else, etc). These are too complicated to try to parse and work out how to automatically convert, and there are (thankfully) very few of them.SubscribeLocalEvent<MyComp, MyEvent>((u, c, ref _) => MyMethod(u, c, e));)Subscriptions withSupport added! Code fix updated!beforeandafterparameters are currently unsupported by the attributes, so the fixer ignores them. If support is added, I'll probably update this to work with it.SubscribeLocalEvent<TComp, MyEvent>(MyMethod);private MyMethod<T>(Entity<MyComp> entity, ref T args) { }private MyMethod(Entity<MyComp> entity, ref BaseMyEvent args) { }(whereBaseMyEventis an abstract type)With these restrictions, I was able to blindly run Fix All on the full engine+content solution and produce code that compiles correctly. If downstream content runs into any additional edge cases, please let me know!