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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 20 commits into from

Conversation

Rekkonnect
Copy link
Contributor

@Rekkonnect Rekkonnect commented Sep 2, 2023 •
edited
Loading

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
CyrusNajmabadi
CyrusNajmabadi reviewed Sep 3, 2023
View reviewed changes
src/EditorFeatures/Test2/FindReferences/FindReferencesTests.Tuples.vb
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, ...)

CyrusNajmabadi
CyrusNajmabadi reviewed Sep 7, 2023
View reviewed changes
src/EditorFeatures/Test2/FindReferences/FindReferencesTests.AnonymousTypeSymbols.vb
{
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 = "", ...

CyrusNajmabadi
CyrusNajmabadi reviewed Sep 7, 2023
View reviewed changes
src/EditorFeatures/Test2/FindReferences/FindReferencesTests.Tuples.vb
}
]]>
</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.

CyrusNajmabadi
CyrusNajmabadi reviewed Sep 7, 2023
View reviewed changes
...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs Outdated
// 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

CyrusNajmabadi
CyrusNajmabadi reviewed Sep 7, 2023
View reviewed changes
...s/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs Outdated
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

@build-analysis build-analysis bot mentioned this pull request Aug 28, 2024
Open
3 tasks
@Rekkonnect
Copy link
Contributor Author

@CyrusNajmabadi ptal, I'll check the failing checks soon

CyrusNajmabadi
CyrusNajmabadi reviewed Oct 19, 2024
View reviewed changes
src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs Outdated
protected static async ValueTask DiscoverImpliedSymbolsAsync(
TSymbol symbol, Solution solution, ArrayBuilder<ISymbol> symbolBuilder, CancellationToken cancellationToken)
{
var name = symbol.Name;
Copy link
Member

Choose a reason for hiding this comment

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

this whole method needs comments.

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 comments ptal

@build-analysis build-analysis bot mentioned this pull request Oct 19, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi Awaiting requested review from CyrusNajmabadi

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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