-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Make RegisterSymbolAction work on Parameters #13931
Make RegisterSymbolAction work on Parameters #13931
Conversation
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.
Cute. I like this approach. It has hte benefit that it makes the API complete, but can also be potentially implemented more efficiently if hte analysis guys want to fill in this gap in another manner.
@@ -608,6 +608,56 @@ public void RegisterSymbolAction(DiagnosticAnalyzer analyzer, Action<SymbolAnaly | |||
SymbolAnalyzerAction analyzerAction = new SymbolAnalyzerAction(action, symbolKinds, analyzer); | |||
this.GetOrCreateAnalyzerActions(analyzer).AddSymbolAction(analyzerAction); | |||
_symbolActions = _symbolActions.Add(analyzerAction); | |||
|
|||
if (symbolKinds.Contains(SymbolKind.Local)) |
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.
Comment explaining the overall idea here (and below).
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.
Done
This is a very literal interpretation of an idea we discussed last week. The weird thing is that it might actually work. |
retest windows_debug_unit64_prtest please Will file a bug if it turns out to not be my fault. |
var expectedSymbolKinds = new[] { SymbolKind.Event, SymbolKind.Field, SymbolKind.Method, SymbolKind.NamedType, SymbolKind.Namespace, SymbolKind.Property }; | ||
var expectedSymbolKinds = new[] | ||
{ | ||
SymbolKind.Event, SymbolKind.Field, SymbolKind.Local, SymbolKind.Method, SymbolKind.NamedType, SymbolKind.Namespace, SymbolKind.Parameter, SymbolKind.Property |
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.
Is this enough test coverage or do I need more specific tests? I don't know how the testing works in this area.
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.
This was not enough. See added unit tests elsewhere.
d72fad6
to
e7cc951
Compare
Removed the [Proposal] tag. Let's talk about actually doing this. Can someone point me at places to add tests, if fixing the broken one isn't sufficient and the infrastructure exists? @JohnHamby @heejaechang @mavasani @srivatsn @CyrusNajmabadi @dotnet/roslyn-analysis @dotnet/roslyn-ide |
test closed-perf please |
1 similar comment
test closed-perf please |
|
||
if (symbolKinds.Contains(SymbolKind.Parameter)) | ||
{ | ||
RegisterSymbolAction( |
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.
Registering a symbol action will have no effect until the analyzer driver actually invokes the action on each parameter. Given that analyzer driver is completely driven by symbol declared events from compilation event queue, and we don't generate compilation events for parameters, these will never be invoked.
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.
Right approach would be to avoid registering a separate action here, and instead just iterate over all parameters of the symbol here and invoke symbol actions for parameters.
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.
I think we can file a bug on optimizing these internal implementation details. Thanks to David's good testing, it should be possible to update things internally without worrying about regressions.
|
||
if (symbolKinds.Contains(SymbolKind.Local)) | ||
{ | ||
RegisterOperationAction(analyzer, opContext => |
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.
How is this preferable over actually asking the analyzer authors to register these operation actions? I am hesitant on ourselves registering new actions, as this will also skew up telemetry (for example, how many operation actions were registered).
If we do want to support local symbol kinds, then the correct approach might be to just enhance this method, which executes all operation actions, to account for registered local symbol actions, identify the set of locals for defined operations and then just directly execute symbol actions for these local symbols.
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.
How is this preferable over actually asking the analyzer authors to register these operation actions?
- We do not allow analzyers to register operation actions.
- The mapping between SymbolKind and the appropriate analyzer operations is not something that may be clear to authors. Look at the example of SymbolKind.Parameter. Getting called back for parameters means enumerating a whole host of cases. The onus should not be on all authors to figure out how to do this properly. It's just something that will lead to excess fragile code to maintain.
It seems pretty clear to me. We have RegisterSymbolAction. We should do what we can to make sure that that works properly for all the possible SymbolKinds that can be passed in.
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.
I am hesitant on putting a workaround of registering additional wrapper actions to invoke user registered actions. I don't think there is too much extra work in just invoking the local/parameter actions at the right points in the analyzer driver code itself.
@mavasani It seems like that's an implementation detail optimization that could be done in the future. I think one of hte problems we have is that currently the API isn't working properly and that's impacting consumers. THis is a simple way of getting it working with low risk and low cost. If someone is willing to sign up to do the 'right' fix deep in the analysis engine, that would work. However, that will likely be much more expensive for some people to implement cough me cough since we're not deeply comfortable with all the complexity deep in the engine. |
Implementing the parameter callbacks functionality at the right place is trivial - I was basically recommending doing the same thing that @dpoeschl is doing right now, but just at a different place, and this will avoid registering wrapper actions. Implementing the local callbacks, which we are doing via registering operation actions seems very superfluous to me. The actual work we are doing is just a single call to register a wrapper operation action with OperationKind.VariableDeclaration - I am not sure if there is any real benefit we are providing to analyzer authors here. |
I'll let david add more info, but from what i understand, we've actually tried doing the invocation there and it was quite difficult to get right. The problem was especially around: if (ShouldExecuteAction(analyzerStateOpt, symbolAction)) // line 350 The system seemed to fight reporting additional symbols when we tries to update the code here. That's why i recommend doing the simple thing here that works. Then, later on, the optimization can be done by people who can really manipulate things efficiently at these depths. |
Okay, if we can ensure that things work fine with the wrapper action approach, then I am fine with us opening a tracking item to clean this up and take the current approach. @dpoeschl - For the unit tests, look at references of CommonDiagnosticAnalyzers - you can define analyzers in that type and then use them in tests in various projects (command line, IDE driver, etc.) |
I think we're providing a lot of value. First, analyzer authors can't register this action to begin with. Second, our API right now is incomplete. We state that you can get callback hooks for symbols and yet two very important symbols (locals and parameters) you cannot actually get called back on. You can hack around that for parameters (but it's ugly, and requires you to understand the rest of our symbolic system). But there's no way to get around this for local variables. We have provided an incomplete public API. The high order bit is to make it complete and correct. Right now this seems like the cheapest way to do that. If someone can sign up to do it "better" we're happy to defer on that. But if that can't be provided, then this is a better state to be in until that work can happen at some point in the future. |
This change fills an important hole in the API. So long as it is functionally correct and can later be replaced by putting the capabilities in the engine, it's a strictly positive step. |
Yes, it was unsupported (with a scope of expansion in future) - and in fact we also have an analyzer in MS.CA.Analyzers that flags registering a symbol action with local/parameter (see here). @dpoeschl Can you also open an issue in roslyn-analyzers to add new supported symbol kinds to that analyzer? |
test perf-closed please |
e7cc951
to
6de08f7
Compare
@mavasani - Added tests and support for delegate invoke methods (in C# only, see #14062 for the VB problem). Let me know if there's more I should be testing. I was starting to go down the road of getting parameters of everything, including lambdas, anonymous methods, and local functions. However, these types of methods aren't even reported when you RegisterSymbolAction for SymbolKind.Method, so I've chosen to stay consistent and only report parameters of member-level symbols. However, I believe this gap should be fixed, especially for local functions. One proposal for how to do this is offered in #14061 |
Also an FYI to @CyrusNajmabadi about #14061 since it caused me to differ my approach here from what we had talked about offline. Let me know how disturbed you are by the change in course. |
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.
I personally think this is an acceptable approach here. I think there is room for optimization. But as this is all internal, that can come at a later point. The core issue is to get the API working properly as soon as possible so that consumers can depend on it.
|
||
if (symbolKinds.Contains(SymbolKind.Local)) | ||
{ | ||
RegisterOperationAction(analyzer, opContext => |
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.
How is this preferable over actually asking the analyzer authors to register these operation actions?
- We do not allow analzyers to register operation actions.
- The mapping between SymbolKind and the appropriate analyzer operations is not something that may be clear to authors. Look at the example of SymbolKind.Parameter. Getting called back for parameters means enumerating a whole host of cases. The onus should not be on all authors to figure out how to do this properly. It's just something that will lead to excess fragile code to maintain.
It seems pretty clear to me. We have RegisterSymbolAction. We should do what we can to make sure that that works properly for all the possible SymbolKinds that can be passed in.
|
||
if (symbolKinds.Contains(SymbolKind.Parameter)) | ||
{ | ||
RegisterSymbolAction( |
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.
I think we can file a bug on optimizing these internal implementation details. Thanks to David's good testing, it should be possible to update things internally without worrying about regressions.
throw new ArgumentException(nameof(context)); | ||
} | ||
|
||
foreach (var parameter in parameters.Where(p => !p.IsImplicitlyDeclared)) |
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.
Note: it would not be clean, but symbols do expose .Language. That means you can handle the VB implicit parameter issue here by doing a language check. As this API is purely for C#/VB, i think it's fine for you to do that.
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.
Filed #14062 to deal with the VB problem someday.
throw new ArgumentException(nameof(context)); | ||
} | ||
|
||
foreach (var parameter in parameters.Where(p => !p.IsImplicitlyDeclared)) |
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.
We're going to hit a lot of parameters. might be good to avoid the .Where allocation. YOu can just have an 'if' statement in the foreach body.
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.
Done
|
||
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics | ||
{ | ||
get |
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.
=> ?
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.
Done
Aside from @CyrusNajmabadi's comments, what else do I need to do to earn a signoff? @mavasani @JohnHamby |
@dpoeschl Can you file an issue on Area-Analyzers path for refactoring this code to optimize this further and move the callbacks into the core analyzer driver? |
@dpoeschl Can you also file an issue on roslyn-analyzers to change |
Signoff authority rests with @srivatsn . My opinion is that the API enhancement is good. The implementation strategy is slightly unsettling, but it may actually be correct and if so should be serviceable until someone has time to implement the API at a lower level. Is the testing sufficient? |
6de08f7
to
5a0f453
Compare
Responded to @CyrusNajmabadi 's feedback, and I'll file the follow-up bugs when this gets merged. |
Removed the functionality for Locals (as a separate commit so that code from the first commit can possibly be reused some day) based on a meeting where we discussed #14061 in more depth. |
Tagging @ManishJayaswal & @MattGertz for RC Ask Mode approval |
Fixes #8753
This is just a proposal to get feedback on the approach of registering more operation or symbol actions when a symbol action is registered with SymbolKind.Local or SymbolKind.Parameter. It also includes some hacks to SymbolAnalysisContext and OperationAnalysisContext to make _isSupportedDiagnostic accessible.
Additionally, we'd need to add tests (somewhere?).