Implement FindReferences for deconstructions #22934

jcouv · 2017-10-31T17:59:57Z

Implements "Find All References" for Deconstruct methods. It is similar to how FAR works on methods that participate in foreach.

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 DeconstructionInfo class (which is internal) was renamed to DeconstructMethodInfo.

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 the Deconstruct method 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

jcouv · 2017-10-31T19:25:37Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+
+                foreach (var nested in deconstruction.Nested)
+                {
+                    foreach (var m in getMethods(nested))


📝 this is a bit unfortunate, but I don't expect deeply nested deconstructions. #Resolved

I'd prefer an immutable array builder. we end up tending to have to avoid IEnumreables in pathological cases. #Resolved

jcouv · 2017-10-31T19:26:56Z

@dotnet/roslyn-compiler for review. This PR provides a public API for accessing deconstruction information. See description for more details. Thanks #Resolved

jcouv · 2017-10-31T21:06:03Z

@dotnet/roslyn-ide for review as well. This PR implements FindReferences for Deconstruct methods. Thanks #Resolved

CyrusNajmabadi · 2017-11-01T21:55:56Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

@@ -227,6 +227,38 @@ public ForEachSymbols GetForEachSymbols(SemanticModel semanticModel, SyntaxNode
            }
        }

+        public ImmutableArray<IMethodSymbol> GetDeconstructionSymbols(SemanticModel semanticModel, SyntaxNode node)


maybe GetDeconstructionMethods? #Resolved

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

CyrusNajmabadi · 2017-11-01T21:56:45Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+
+                foreach (var nested in deconstruction.Nested)
+                {
+                    foreach (var m in getMethods(nested))


avoid single letter foreach variable names. #Closed

CyrusNajmabadi · 2017-11-01T21:57:49Z

...Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/OrdinaryMethodReferenceFinder.cs

@@ -106,6 +106,11 @@ private bool IsForEachMethod(IMethodSymbol methodSymbol)
                methodSymbol.Name == WellKnownMemberNames.MoveNextMethodName;
        }

+        private bool IsDeconstructMethod(IMethodSymbol methodSymbol)


nit: => #Resolved

CyrusNajmabadi · 2017-11-01T21:59:20Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex.ContextInfo.cs

@@ -24,7 +24,8 @@ private struct ContextInfo
                bool containsThisConstructorInitializer,
                bool containsBaseConstructorInitializer,
                bool containsElementAccessExpression,
-                bool containsIndexerMemberCref) :
+                bool containsIndexerMemberCref,
+                bool containsDeconstruction) :


need to update the 'version' of SyntaxTreeINdex. Otherwise, existing indices will not be recreated and it will look like no files contain deconstructions. #Resolved

Updated SerializationFormat number.

In reply to: 148396504 [](ancestors = 148396504)

CyrusNajmabadi · 2017-11-01T22:00:12Z

src/Workspaces/CoreTest/FindReferencesTests.cs

@@ -268,6 +268,68 @@ class B : C, A
            Assert.Empty(expectedMatchedLines);
        }

+        [Fact, WorkItem(18963, "https://github.com/dotnet/roslyn/issues/18963")]
+        public async Task FindReferences_Deconstruction()


Wrong location for these tests. Look for the more declarative FindReferencesTests written in VB. #Resolved

jcouv · 2017-11-03T17:40:59Z

@dotnet/roslyn-compiler for review as well (adding public API to get information about deconstructions, for IDE scenario). Thanks #Resolved

CyrusNajmabadi · 2017-11-05T21:22:39Z

(thanks Cyrus for the tips)

You're welcome :) Can't wait to have this. It'll be super useful! #Resolved

CyrusNajmabadi · 2017-11-05T21:26:13Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

@@ -246,6 +246,16 @@ public bool IsForEachStatement(SyntaxNode node)
            return node is ForEachStatementSyntax;
        }

+        public bool IsDeconstruction(SyntaxNode node)


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

CyrusNajmabadi · 2017-11-05T21:27:17Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

@@ -91,6 +92,7 @@ internal sealed partial class SyntaxTreeIndex
                            containsQueryExpression = containsQueryExpression || syntaxFacts.IsQueryExpression(node);
                            containsElementAccess = containsElementAccess || syntaxFacts.IsElementAccessExpression(node);
                            containsIndexerMemberCref = containsIndexerMemberCref || syntaxFacts.IsIndexerMemberCRef(node);
+                            containsDeconstruction = containsDeconstruction || syntaxFacts.IsDeconstruction(node);


this would then become containsDestruction = containsDeconstruction || (syntaxFacts.IsDeconstructionAssignment || syntaxFacts.IsDeconstructionForEachStatement); #Resolved

CyrusNajmabadi · 2017-11-06T19:51:54Z

src/EditorFeatures/Test2/FindReferences/FindReferencesTests.OrdinaryMethodSymbols.vb

+    public void {|Definition:Decons$$truct|}(out int x1, out int x2) { x1 = 1; x2 = 2; }
+    public void M()
+    {
+        [|var (x1, x2) =|] this;


Fasscinating. This is the resultant span? I guess i'm ok with that. though i think it's strange. #Resolved

Yeah, I think something a bit narrower might be wise. Consider if there are comments in the middle of that somewhere... #Resolved

CyrusNajmabadi · 2017-11-06T19:52:39Z

src/EditorFeatures/Test2/FindReferences/FindReferencesTests.OrdinaryMethodSymbols.vb

+    public void M()
+    {
+        [|var (x1, x2) =|] this;
+        [|foreach (var (y1, y2)|] in new[] { this }) { }


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

CyrusNajmabadi · 2017-11-06T19:53:28Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+                return;
+            }
+
+            foreach (var nested in deconstruction.Nested)


so just to be sure. You can' thave both .Method and .Nested available at the same time? #Resolved

Good catch. There is a bug here. Actually, if there is a .Method, there will be a .Nested. #Resolved

CyrusNajmabadi · 2017-11-06T19:53:39Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

@@ -246,6 +246,32 @@ public bool IsForEachStatement(SyntaxNode node)
            return node is ForEachStatementSyntax;
        }

+        public bool IsDeconstructionForEachStatement(SyntaxNode node)


nit: => #Resolved

CyrusNajmabadi · 2017-11-06T19:54:02Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

+                    return Location.Create(tree, TextSpan.FromBounds(assignment.SpanStart, assignment.OperatorToken.Span.End));
+
+                case ForEachVariableStatementSyntax @foreach:
+                    return Location.Create(tree, TextSpan.FromBounds(@foreach.SpanStart, @foreach.Variable.Span.End));


this location seems absolutely wacky :D #Resolved

CyrusNajmabadi · 2017-11-06T19:55:25Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

+            switch (node)
+            {
+                case AssignmentExpressionSyntax assignment:
+                    return Location.Create(tree, TextSpan.FromBounds(assignment.SpanStart, assignment.OperatorToken.Span.End));


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

CyrusNajmabadi · 2017-11-06T19:56:22Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs

+                var deconstructMethods = semanticFacts.GetDeconstructionAssignmentMethods(semanticModel, node);
+                if (deconstructMethods.IsEmpty)
+                {
+                    deconstructMethods = semanticFacts.GetDeconstructionForEachMethods(semanticModel, node);


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

oh........... i see. yes. this is checking per node. so you couldn't have both. cute :) n/m then. #Closed

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

jcouv · 2017-11-07T16:22:16Z

Filed #23037 for updating ChangeSignature to handle Deconstruct methods. #Resolved

jcouv · 2017-11-07T16:22:36Z

@dotnet/roslyn-compiler for review of the compiler portion. Thanks #Resolved

cston · 2017-11-07T19:09:54Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs

+            if (!underlyingConversions.IsDefault)
+            {
+                result = Hash.Combine(result, Hash.CombineValues(underlyingConversions.SelectAsArray(c => c.GetHashCode())));
+            }


Hash.CombineValues calls GetHashCode() on the ImmutableArray<T> and handles IsDefault. #Resolved

cston · 2017-11-07T19:42:29Z

src/Compilers/CSharp/Portable/Compilation/DeconstructionInfo.cs

+    /// Its second nested node has a Method (Deconstructable1.Deconstruct), no Conversion, and two Nested nodes.
+    /// Those last two nested nodes have no Method, but each have a Conversion (ImplicitNumeric, from int to long).
+    /// </summary>
+    public struct DeconstructionInfo : IEquatable<DeconstructionInfo>


Are IEquatable<DeconstructionInfo> and Equals needed? #Closed

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

jcouv · 2017-11-07T21:35:11Z

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

cston · 2017-11-07T21:43:55Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -4403,6 +4403,18 @@ private static ImmutableArray<Symbol> CreateReducedExtensionMethodIfPossible(Bou
        public abstract ForEachStatementInfo GetForEachStatementInfo(CommonForEachStatementSyntax node);

        /// <summary>
+        /// Gets for deconstruction info.


Gets assignment deconstruction info? #Resolved

cston · 2017-11-07T21:44:33Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

+        public abstract DeconstructionInfo GetDeconstructionInfo(AssignmentExpressionSyntax node);
+
+        /// <summary>
+        /// Gets for deconstruction info.


Gets foreach variable deconstruction info? #Resolved

cston · 2017-11-07T21:48:42Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

@@ -837,6 +837,34 @@ public override ForEachStatementInfo GetForEachStatementInfo(CommonForEachStatem
                enumeratorInfoOpt.CurrentConversion);
        }

+        public override DeconstructionInfo GetDeconstructionInfo(AssignmentExpressionSyntax node)
+        {
+            var boundDeconstruction = (BoundDeconstructionAssignmentOperator)GetUpperBoundNode(node);


Does this throw a cast exception if the caller passes an assignment that is not a deconstruction assignment? Is that the desired behavior? #Resolved

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)

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)

cston

Compiler changes LGTM.

jcouv · 2017-11-08T16:56:23Z

Thanks Chuck!
@dotnet/roslyn-compiler for a second review of compiler portion.

jasonmalinowski · 2017-11-08T17:51:44Z

src/Compilers/CSharp/Portable/Compilation/DeconstructionInfo.cs

+
+    /// <summary>
+    /// The representation of a deconstruction as a tree of Deconstruct methods and conversions.
+    /// Methods only appear in non-terminal nodes. All terminal nodes have a Conversion.


cref Conversion if that means it's a reference to the property.

jasonmalinowski · 2017-11-08T17:53:31Z

src/Compilers/CSharp/Portable/Compilation/DeconstructionInfo.cs

+    ///
+    /// The top-level node has no Method and no Conversion, but has two Nested nodes.
+    /// Its first nested node has no Method, but has a Conversion (Identity).
+    /// Its second nested node has a Method (Deconstructable1.Deconstruct), no Conversion, and two Nested nodes.


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.

Argh, sorry about that :-(

jasonmalinowski · 2017-11-08T17:56:20Z

src/Workspaces/CSharp/Portable/Extensions/SyntaxNodeExtensions.cs

@@ -996,5 +996,31 @@ public static ConditionalAccessExpressionSyntax GetInnerMostConditionalAccessExp
            return to.WithPrependedLeadingTrivia(finalTrivia)
                     .WithAdditionalAnnotations(Formatter.Annotation);
        }
+
+        public static SyntaxNode IsInDeconstructionLeft(this SyntaxNode node)


This is an Is* method returning non-Boolean. Please revise the name. #Resolved

jasonmalinowski

Good other than one specific question about doc comments that look like they have a typo which makes it really confusing.

gafter

API and compiler changes look good to me.

jcouv · 2017-11-09T18:49:21Z

Nice, thanks!
@jasonmalinowski I've addressed the doc comment issues. Good to go?

jasonmalinowski

The docs are so clear, even a IDE developer can understand it. Thanks!

jcouv · 2017-11-09T18:55:25Z

You mean when the docs don't say the opposite of what they should ;-)
Awesome. I'll resolve conflicts and merge once green. Thanks all.

jcouv added Area-Compilers Area-IDE labels Oct 31, 2017

jcouv added this to the 15.later milestone Oct 31, 2017

jcouv self-assigned this Oct 31, 2017

dnfclas added the cla-already-signed label Oct 31, 2017

jcouv commented Oct 31, 2017

View reviewed changes

CyrusNajmabadi reviewed Nov 1, 2017

View reviewed changes

CyrusNajmabadi reviewed Nov 5, 2017

View reviewed changes

jcouv mentioned this pull request Nov 6, 2017

Add Go To Implementation option for Deconstruct calls #23012

Closed

jcouv force-pushed the far-deconstruct branch 2 times, most recently from 427522d to 914c168 Compare November 6, 2017 19:19

jcouv requested review from sharwell, agocke and VSadov November 6, 2017 19:27

CyrusNajmabadi reviewed Nov 6, 2017

View reviewed changes

jcouv requested a review from gafter November 7, 2017 16:33

cston reviewed Nov 7, 2017

View reviewed changes

cston approved these changes Nov 7, 2017

View reviewed changes

jasonmalinowski reviewed Nov 8, 2017

View reviewed changes

jasonmalinowski requested changes Nov 8, 2017

View reviewed changes

gafter approved these changes Nov 9, 2017

View reviewed changes

jasonmalinowski approved these changes Nov 9, 2017

View reviewed changes

jcouv added 2 commits November 9, 2017 11:12

Implement FindReferences for deconstructions

4d82614

conflict

5ae6e7b

jcouv force-pushed the far-deconstruct branch from 46f66cf to 5ae6e7b Compare November 9, 2017 19:30

jcouv merged commit ee5a91f into dotnet:post-dev15.5-contrib Nov 9, 2017

jcouv deleted the far-deconstruct branch November 9, 2017 21:12

jcouv mentioned this pull request Nov 15, 2017

Rename should report a conflict when renaming Deconstruct #16102

Closed

jcouv mentioned this pull request Jan 16, 2018

FindAllReferences on Deconstruct should consider all documents #24223

Merged

jcouv mentioned this pull request Jun 30, 2018

Implement FAR for GetAwaiter methods #28230

Merged

jcouv mentioned this pull request Jan 30, 2020

Find All References does not count usages from the Implicit Range support #41310

Open

Youssef1313 mentioned this pull request Dec 25, 2020

CodeLens doesn't count up the usage on custom method GetPinnableReference #50131

Closed

Implement FindReferences for deconstructions #22934

Implement FindReferences for deconstructions #22934

Conversation

jcouv commented Oct 31, 2017 • edited Loading

jcouv Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 1, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Oct 31, 2017 • edited Loading

jcouv commented Oct 31, 2017 • edited Loading

CyrusNajmabadi Nov 1, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 5, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 1, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 1, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 1, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 6, 2017

Choose a reason for hiding this comment

CyrusNajmabadi Nov 1, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Nov 3, 2017 • edited Loading

CyrusNajmabadi commented Nov 5, 2017 • edited by jcouv Loading

CyrusNajmabadi Nov 5, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 5, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jasonmalinowski Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Nov 6, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jasonmalinowski Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Nov 7, 2017 • edited Loading

jcouv commented Nov 7, 2017 • edited Loading

cston Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv commented Nov 7, 2017 • edited Loading

cston Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 7, 2017

Choose a reason for hiding this comment

jcouv Nov 7, 2017

Choose a reason for hiding this comment

cston left a comment

Choose a reason for hiding this comment

jcouv commented Nov 8, 2017

jasonmalinowski Nov 8, 2017

Choose a reason for hiding this comment

jasonmalinowski Nov 8, 2017

Choose a reason for hiding this comment

jcouv Nov 9, 2017

Choose a reason for hiding this comment

jcouv commented Oct 31, 2017 •

edited

Loading

jcouv Oct 31, 2017 •

edited

Loading

CyrusNajmabadi Nov 1, 2017 •

edited by jcouv

Loading

jcouv commented Oct 31, 2017 •

edited

Loading

jcouv commented Oct 31, 2017 •

edited

Loading

CyrusNajmabadi Nov 1, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 5, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 1, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 1, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 1, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 1, 2017 •

edited by jcouv

Loading

jcouv commented Nov 3, 2017 •

edited

Loading

CyrusNajmabadi commented Nov 5, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 5, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 5, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

jasonmalinowski Nov 7, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

jcouv Nov 6, 2017 •

edited

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Nov 6, 2017 •

edited by jcouv

Loading

jasonmalinowski Nov 7, 2017 •

edited by jcouv

Loading

jcouv commented Nov 7, 2017 •

edited

Loading

jcouv commented Nov 7, 2017 •

edited

Loading

cston Nov 7, 2017 •

edited by jcouv

Loading

cston Nov 7, 2017 •

edited by jcouv

Loading

jcouv Nov 7, 2017 •

edited

Loading

jcouv commented Nov 7, 2017 •

edited

Loading

cston Nov 7, 2017 •

edited by jcouv

Loading

cston Nov 7, 2017 •

edited by jcouv

Loading

cston Nov 7, 2017 •

edited by jcouv

Loading

jasonmalinowski Nov 8, 2017 •

edited by gafter

Loading