Skip to content
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

Find All References does not find references to Deconstruct #18963

Closed
jcouv opened this issue Apr 24, 2017 · 9 comments
Closed

Find All References does not find references to Deconstruct #18963

jcouv opened this issue Apr 24, 2017 · 9 comments
Assignees
Labels
Area-Compilers Bug New Language Feature - Tuples Tuples Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Apr 24, 2017

See code below. Notice that CodeLens shows the same (0 references).

image

@jcouv
Copy link
Member Author

jcouv commented Apr 24, 2017

Cyrus gave me an overview of how Find All References works, using the parallel of foreach statements and GetEnumerable methods.
There is code for FindAllReference for ordinary methods. It has two steps: (1) querying the index to find candidate documents (that use foreach), (2) actually inspecting each candidate document.

The second step requires an API from the compiler, something like GetDeconstructionInfo, which does not presently exist.
Once that API exists, we could also make GoToDefinition work (press F12 on the equals sign in deconstruction) with little work.

Besides that, there is also work to build the index. The code is in SyntaxTreeIndex.ContextInfo. It currently stores a boolean when the document uses foreach. In the case of deconstructions, we'd store an array of cardinalities of deconstructions used.

@jcouv
Copy link
Member Author

jcouv commented Apr 25, 2017

📝 The design for a GetDeconstructInfo API is non-trivial because of nested deconstructions. Also, such nesting cases make GoToDefinition ambiguous.
That said, most deconstructions are not nested.

@CyrusNajmabadi
Copy link
Member

The design for a GetDeconstructInfo API is non-trivial because of nested deconstructions.

Can you give examples? The ones i can think of are like so:

((int i, int j), int k) = ...

In this case, i would expect to be able to get DeconstructoinInfo on (..., int k) and on (int i, int j).

In other words, even whne we have nested deconstructs, isn't there a piece of syntax we can be calling on to just see that single level of deconstruction?

@jcouv
Copy link
Member Author

jcouv commented Apr 25, 2017

I was thinking that GetDeconstructionInfo would take as input the entire deconstruction expression, just like its GetForeachStatementionInfo analog takes an entire foreach statement.

Your approach is to use parts of the left-hand-side as inputs instead. I like that!
That would allow nested cases: call GetDeconstructionInfo on syntax for (..., int k), or for (int i, int j), or for var (x, y). But it would also allow calling GetDeconstructionInfo with similar syntax from a deconstruction-foreach.

Then I think the result (DeconstructionInfo) could have a single IMethodSymbol DeconstructionMethod { get; }, and maybe a Conversion[] ElementConversions { get; }. If you want to access the nested deconstructions, you need to call GetDeconstructionInfo again on nested LHS tuples.

Two more questions:

  1. do we expect FindAllReferences to work on conversion declarations too?
  2. would you expect F12 to work on the parens of the various tuples, instead of the equals sign, then?

@Pilchie
Copy link
Member

Pilchie commented Apr 29, 2017

@jcouv - removing IDE label until the API exists, since we can't do anything here.

@jcouv
Copy link
Member Author

jcouv commented Jul 11, 2017

I discussed the API design with Neal. He convinced me that we should expose this through GetConversion instead.

Internally, deconstructions are modeled as conversions, although they are not called that in the language. A deconstruction conversion has a MethodInfo for the Deconstruct method, and an array of element-wise conversions.

Some examples to see how this approach would work:

  1. Call GetConversion on the right-hand-side of var (a, b) = (1, 2);. You'd get a tuple conversion, which would expose an array of element-wise conversions (note: this is not implemented yet).
  2. Call GetConversion on the right-hand-side of var (a, (b, c)) = deconstructable;. You'd get a deconstruction conversion, which would expose the Deconstruct method and an array of element-wise conversions (one of which is itself another deconstruction conversion).
  3. Call GetForeachStatementInfo on a foreach-deconstruction statement (foreach (var (a, b) in ...) { ... }). You'd get back a ForeachStatementInfo with an ElementConversion that is a deconstruction conversion.

@CyrusNajmabadi Would that work from the IDE perspective?

FYI @VSadov (relates to exposing tuple conversions through public API)

@CyrusNajmabadi
Copy link
Member

Sure. Seems like it would work fine. We could also generally extend this to make it so that goto-def on an equals sign would work for other conversions as well. i.e. if there was a user defined conversion from X to Y and you had: "Y y = x" then goto-def on the equals would take you to the conversion.

@jcouv jcouv modified the milestones: 15.later, 15.5 Jul 17, 2017
@jcouv
Copy link
Member Author

jcouv commented Aug 2, 2017

Relates to #11803 (asking for public API to expose tuple conversions).

@jcouv jcouv added the 4 - In Review A fix for the issue is submitted for review. label Oct 31, 2017
@jcouv
Copy link
Member Author

jcouv commented Nov 9, 2017

Merge fix, expected to ship in 15.6

@jcouv jcouv closed this as completed Nov 9, 2017
@jcouv jcouv added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug New Language Feature - Tuples Tuples Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests

4 participants