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

Fix reference finding for tuple fields and anonymous type properties #69804

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Rekkonnect
Copy link
Contributor

@Rekkonnect Rekkonnect commented Sep 2, 2023

Fixes #24268
Fixes #52621

Related issues:
#20115

Summary

The symbol that is bound to an expression that implies the decared tuple field is always the expression itself.
See the tests and the linked issues for examples.

Also discovered a bad case with anonymous type properties, where assigning an expression accessing a name equal to the property of the anonymous type would result in binding to that anonymous type property. For example:

string a = string.Empty;
var q = new { a, a.Length };

Hovering over Length would show the property Length of the anonymous type of q, and not that of the type string.

Note: the above works by ignoring the result of the result of semanticModel.GetDeclaredSymbol on the a.Length expression, which does yield the property of the anonymous type. We only want that overriding behavior to be local for the purposes of this fix.

Related

Rename is not tested currently.

@Rekkonnect Rekkonnect requested a review from a team as a code owner September 2, 2023 22:41
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 2, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 2, 2023
void M(int x, int y)
{
var t = ($$[|{|Definition:x|}|]: x, y: y);
t = ([|x|]: y, y: x);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test of x:x as well please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x: x is included above, it's the definition, do you also want a test that doesn't involve the definition itself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test where there is also an implicit name. i.e. (x, ...)

{
int length = [|a|].Length;
var r = new { [|a|], Length = 1 };
r = new { $$[|a|], [|a|].Length };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an example where there is a named-arg for 'a'. like new { a = "", ...}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test where the $$ is on the defintion. add test where $$ is on the a in new { a = "", ...

}
]]>
</Document>
</Project>
</Workspace>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm finding the tests hard to totally understand because they don't show both implicit and explicit named-props in anonymous-types, and implicit and explicit arguments for tuples.

// Skip the tuple expression if the identifier is part of the simple identifier expression inside a tuple,
// but not the identifier of the field
// For example, we are evaluating `x` in `(x, y)`, or the expression after the colon in `(x: x, y: y)`
// but not the name of the field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't explain the 'why' this is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment ptal

case IPropertySymbol { ContainingType: ITypeSymbol { IsAnonymousType: true } }:
{
// If the span is part of the property's assignment expression, it's not our target result
// For example, we are evaluating `a.Length` in `new { a.Length }`, or in `new { Length = a.Length }`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't say 'why' though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment ptal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference highlighting incorrect with tuples Highlight References not symmetric in Tuples
2 participants