Implement FindReferences for deconstructions#22934
Implement FindReferences for deconstructions#22934jcouv merged 2 commits intodotnet:post-dev15.5-contribfrom
Conversation
There was a problem hiding this comment.
📝 this is a bit unfortunate, but I don't expect deeply nested deconstructions. #Resolved
There was a problem hiding this comment.
I'd prefer an immutable array builder. we end up tending to have to avoid IEnumreables in pathological cases. #Resolved
|
@dotnet/roslyn-compiler for review. This PR provides a public API for accessing deconstruction information. See description for more details. Thanks #Resolved |
|
@dotnet/roslyn-ide for review as well. This PR implements FindReferences for |
There was a problem hiding this comment.
maybe GetDeconstructionMethods? #Resolved
There was a problem hiding this comment.
Also... on a personal level... i don't like having a single helper method that works on all these types. I think i'd prefer a GetAssignmentDeconstructionSymbols and GetForEachStatementDeconstructionSymbols. #Resolved
There was a problem hiding this comment.
avoid single letter foreach variable names. #Closed
There was a problem hiding this comment.
need to update the 'version' of SyntaxTreeINdex. Otherwise, existing indices will not be recreated and it will look like no files contain deconstructions. #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
Wrong location for these tests. Look for the more declarative FindReferencesTests written in VB. #Resolved
|
@dotnet/roslyn-compiler for review as well (adding public API to get information about deconstructions, for IDE scenario). Thanks #Resolved |
You're welcome :) Can't wait to have this. It'll be super useful! #Resolved |
There was a problem hiding this comment.
Same here. SyntaxFacts isn't for merging multiple nodes from the same language into the same concept. It's for providing a uniform syntactic API over VB and C#. I would prefer IsDeconstructionAssignment and IsDeconstructionForEachStatement #Resolved
There was a problem hiding this comment.
this would then become containsDestruction = containsDeconstruction || (syntaxFacts.IsDeconstructionAssignment || syntaxFacts.IsDeconstructionForEachStatement); #Resolved
427522d to
914c168
Compare
There was a problem hiding this comment.
Fasscinating. This is the resultant span? I guess i'm ok with that. though i think it's strange. #Resolved
There was a problem hiding this comment.
Yeah, I think something a bit narrower might be wise. Consider if there are comments in the middle of that somewhere... #Resolved
There was a problem hiding this comment.
same here.. i think i would prefer either "var (x1, x2)" to be the span, or "(x1, x2)" to be hte span. or "=" to be hte span. for the foreach case, the same (except replacing "in" for "="). #Resolved
There was a problem hiding this comment.
so just to be sure. You can' thave both .Method and .Nested available at the same time? #Resolved
There was a problem hiding this comment.
Good catch. There is a bug here. Actually, if there is a .Method, there will be a .Nested. #Resolved
There was a problem hiding this comment.
this location seems absolutely wacky :D #Resolved
There was a problem hiding this comment.
I am... barely ok with this :D i think it would be better to just select hte =, and the 'in' token though. But i would be open for arguments otherwise. #Resolved
There was a problem hiding this comment.
don't you always want to do this? i don't understand why you only do htis if you didn't find any deconstruction assignments. #Closed
There was a problem hiding this comment.
oh........... i see. yes. this is checking per node. so you couldn't have both. cute :) n/m then. #Closed
There was a problem hiding this comment.
Still worth adding a comment. I think what's throwing me off is the GetDeconstruction..Methods* name, since plural, implies that it might be doing recursive itself. Which it's not. #Resolved
6182240 to
1043e0c
Compare
|
Filed #23037 for updating ChangeSignature to handle Deconstruct methods. #Resolved |
|
@dotnet/roslyn-compiler for review of the compiler portion. Thanks #Resolved |
There was a problem hiding this comment.
Hash.CombineValues calls GetHashCode() on the ImmutableArray<T> and handles IsDefault. #Resolved
There was a problem hiding this comment.
Are IEquatable<DeconstructionInfo> and Equals needed? #Closed
There was a problem hiding this comment.
I did this for consistency with ForEachDeconstructionInfo, which begs the question.
I'll check with @gafter on public API design, and if nobody objects, I'll remove IEquatable. #Closed
|
From discussion with Chuck and Neal, we don't need IEquatable on DeconstructionInfo yet (we're not sure why ForEachStatementInfo needed it). Removed that portion and its tendrils. #Resolved |
There was a problem hiding this comment.
Gets assignment deconstruction info? #Resolved
There was a problem hiding this comment.
Gets foreach variable deconstruction info? #Resolved
There was a problem hiding this comment.
Does this throw a cast exception if the caller passes an assignment that is not a deconstruction assignment? Is that the desired behavior? #Resolved
There was a problem hiding this comment.
Good question. It's the same behavior as GetForEachStatementInfo, but does seem wrong.
For comparison, GetAwaitExpressionInfo returns default in such cases. I'll change foreach and deconstruction methods to do the same.
In reply to: 149514253 [](ancestors = 149514253)
There was a problem hiding this comment.
Hum, actually GetForEachStatementInfo is fine. The problem is just with assignment (which may or may not be a deconstruction).
In reply to: 149516672 [](ancestors = 149516672,149514253)
|
Thanks Chuck! |
There was a problem hiding this comment.
cref Conversion if that means it's a reference to the property.
There was a problem hiding this comment.
This comment implies that across the summation of all the nodes, there is one node that has a Method, but I would expect two since two methods are being invoked.
There was a problem hiding this comment.
This is an Is* method returning non-Boolean. Please revise the name. #Resolved
jasonmalinowski
left a comment
There was a problem hiding this comment.
Good other than one specific question about doc comments that look like they have a typo which makes it really confusing.
gafter
left a comment
There was a problem hiding this comment.
API and compiler changes look good to me.
|
Nice, thanks! |
jasonmalinowski
left a comment
There was a problem hiding this comment.
The docs are so clear, even a IDE developer can understand it. Thanks!
|
You mean when the docs don't say the opposite of what they should ;-) |
46f66cf to
5ae6e7b
Compare
Implements "Find All References" for
Deconstructmethods. It is similar to how FAR works on methods that participate inforeach.Fixes #18963
From the compiler side, the change is to expose information about deconstructions (assignment/declarations and foreach) via new public APIs. Since deconstructions are not conversions in the language, but they are implemented using nested conversions (like tuple conversions) with a deconstruction method, I'm using a wrapper class around deconstruction conversion objects to abstract from the implementation.
The existing
DeconstructionInfoclass (which is internal) was renamed toDeconstructMethodInfo.From the IDE side, we now keep track of whether a document uses any deconstructions (just like we do for
foreach) and we can scan through a document to find all theDeconstructmethod symbols that are references. That's the core of the "Find All References" implementation. (thanks Cyrus for the tips)Note: this fix was incomplete, it was supplemented by #24223