-
Notifications
You must be signed in to change notification settings - Fork 575
Add analyzer & fixer to detect when proxy methods are not used (ProxyForAttribute)
#6026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…IComponent)" This reverts commit 1939d11.
… IComponent)" This reverts commit a65ea71. Might as well include this, since the file needs to be changed anyway.
Previously we searched with each method invocation. This also lets us skip analysis on classes with no proxy methods available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two Three bugs to be fixed:
- #6027 (comment) Analyzer needs to handle invocations within a delegate and not use variables from the outside scope.
- #6027 (comment) Code fixer somehow ate a comment above a change. Need to prevent that.
- #6027 (comment) Code fixer also ate a blank line.
… into analyzer/proxyfor
|
o lord |
| if (context.Node is not MethodDeclarationSyntax declarationSyntax) | ||
| return; | ||
|
|
||
| if (context.SemanticModel.GetDeclaredSymbol(declarationSyntax) is not { } methodSymbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad. Need to redo this to work with declaration operations instead so it doesn't need to get the semantic model for every declaration in the codebase.
An issue that often comes up in content code reviews is that contributors will miss the opportunity to use a proxy method when one is available - for example, using
EntityManager.TryGetComponentin an entity system, where they could useTryComp. These instances need to be caught and pointed out by reviewers, and addressed by PR authors, adding friction to the review process.This PR allows proxy methods to be marked with a new
ProxyForattribute which indicates that they can and should be used by derived classes instead of specified methods of other classes.ProxyForAttribute
The attribute looks like this:
The first parameter indicates the Type containing the target method and is required. The second parameter is a string (please use
nameof) with the name of the target method, and can be omitted if the proxy method name matches the target method name. As examples:TryCompdoes not matchTryGetComponentso the parameter is needed, butGetEntityQuerymatchesGetEntityQueryso it can be omitted.Warning
The analyzer will detect uses of the target method in classes where the proxy is available and raise a warning:

Fixer
Also included is a code fixer that can automatically replace the method with the proxy method.
Quick actions:
Result:
Self-checks
The analyzer also checks for proper use of the
ProxyForattribute.Redundant method name
If the proxy method name matches the target method name and the attribute redundantly specifies the target method name, a warning is raised:


This also has a code fixer to remove the redundant parameter:
Target method check
The analyzer also checks that the target method exists and will raise an error if it does not:

This verifies that the target Type has a method with the target name (either manually set by parameter or automatically obtained from the proxy method name), the same type arguments as the proxy method, and accepting the same sequence of parameters as the proxy method.
Different parameters:
Application
This PR also goes ahead and marks all possible proxy methods in
EntitySystem. The analyzer detects and flags 468 instances of those methods not being used when they could.