Classify method group assignments as methods#57410
Conversation
| SymbolInfo symbolInfo, | ||
| ArrayBuilder<ClassifiedSpan> result) | ||
| { | ||
| // If the syntax matches "var x = m" or "x = m", and there is a candidate symbol |
There was a problem hiding this comment.
Can we add a test for the "x = m" scenario?
| symbolInfo.CandidateReason == CandidateReason.OverloadResolutionFailure && | ||
| symbolInfo.CandidateSymbols[0] is IMethodSymbol methodSymbol) | ||
| { | ||
| if (identifierName.IsRightSideOfAnyAssignExpression() || identifierName.IsVariableDeclaratorValue()) |
There was a problem hiding this comment.
Why is this being limited to only assignments? Because it looks like we have a bug here where I'm passing a method as an argument too?
There was a problem hiding this comment.
It is limited only to the bugs I knew about :)
There was a problem hiding this comment.
Also, makes me wonder if method-like (Delegate, Action, Func) identifiers should be classified as method names. Totally separate conversation from this tho.
There was a problem hiding this comment.
yeah, i agree with jason. i would even simplify this further. if we have a SymbolInfo with an error, and a single candidate, just extract that and classify whatever we have with that symbol.
There was a problem hiding this comment.
if we have a SymbolInfo with an error, and a single candidate, just extract that and classify whatever we have with that symbol.
This is a little too broad I think, and breaks a bunch of tests in not good ways. Jason's suggestion only breaks 2 tests, and in ways that seem reasonable to me.
There was a problem hiding this comment.
I'm curious what it breaks. Can you clarify?
There was a problem hiding this comment.
For example, the two attributes here are classified as classes with the change, which goes against what the comments in the test say.
There was a problem hiding this comment.
IMO. THat seems pointless. THe code is in error, and the compiler said: welp... the best thing i can come up with is that they bound to these two symbols (illegally). Things like f12 on the code will then take you to the besft-effort classes.
In those cases, i see no reason why it's a problem for classification to go: yeah... your code is wrong... but it looks like you did bind to these classes, so i'll classify this as a class.
--
In other words... i view the entire endeavor to not classify here very suspect and somewhat of a fools errand :) It's extra work to stop doing a thing that is itself not bad at all :)
| if (name is IdentifierNameSyntax identifierName && | ||
| symbolInfo.Symbol == null && | ||
| symbolInfo.CandidateSymbols.Length == 1 && | ||
| symbolInfo.CandidateReason == CandidateReason.OverloadResolutionFailure && |
There was a problem hiding this comment.
Should we just count overload resolution as another case that sends us into the ambiguous path here?
There was a problem hiding this comment.
Thanks for the suggestion, I will give it a try and see what breaks.
There is another spot where I thought a fix could go, where it allows overload resolution to still work for constructors, and I don't know why that wouldn't be the case for any other method, but I always assume that existing behaviour is there for a reason.
This PR is essentially a "is this fix reasonable?" question, because I didn't get an answer on Teams :)
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Yeah. All the test changes seem totally reasonable to me.
|
Thank you for the assistance :) |
* upstream/main: (51 commits) Disable sql when built from source Fix Reduce indirections Reduce indirections Rename files Fix nullability warning Update cloud cache in the same way Ensure only one storage service factory gets created per workspace. Remove processed typename check Simplify Equals method Build CodeAction description into table and only generate Dev17 hash Avoid recomputing expensive persistence location data that does not change. Options Refactoring - Round 3 (dotnet#57254) Merge pull request dotnet#57509 from dotnet/dev/rigibson/fix-publishdata Classify method group assignments as methods (dotnet#57410) Simplify NRT Simplify Remove unnecessary workaround code. Simplify ...
Fixes #57184