Skip to content

Compare symbols by metadata name when searching for eqivalent symbols in FAR engine #78656

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

Merged

Conversation

DoctorKrolic
Copy link
Contributor

Fixes: #64592

The cause was that although file-scoped types can have equal names and be in same namespaces, their metadata names are always different. I extrapolated this approach to all symbol comparisons and found-and-replaced all comparisons of Name to comparisons of MetadataName in the file. At first this doesn't make sense for most symbol kinds (e.g. namespaces or preprecessor symbols), but I believe that conceptually this is more correct. Name is mostly a user-facing thing, while MetadataName is a behind-the-scenes property which, if different on 2 symbols, 100% tells that they are not equivalent. If a symbol always has the same MetadataName as Name, the underlying symbol implementation will just return the reference to the same memory, so no waste here. But this makes implementation more future-proof; e.g. if file-scoped members are gonna be added to the language this code will still correctly handle them and tell different references apart.

@DoctorKrolic DoctorKrolic requested a review from a team as a code owner May 20, 2025 17:55
@dotnet-policy-service dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels May 20, 2025
// Avoid the expensive checks if we can fast path when the compiler just says these are equal. Also, for the
// purposes of symbol finding nullability of symbols doesn't affect things, so just use the default
// comparison.
if (searchSymbol.Equals(symbolToMatch))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not related to the core fix, but when I traced down the problem I noticed that OriginalSymbolsMatchCore method, which is invoked right below this check, has exactly the same condition in it, so there is clearly duplication of efforts going on here)

@CyrusNajmabadi
Copy link
Member

Will depend on things pass or not :)

@DoctorKrolic
Copy link
Contributor Author

@CyrusNajmabadi PTAL

@CyrusNajmabadi
Copy link
Member

Thanks.

@CyrusNajmabadi CyrusNajmabadi merged commit 8628943 into dotnet:main May 22, 2025
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 22, 2025
@DoctorKrolic DoctorKrolic deleted the compare-symbols-by-metadata-name branch May 23, 2025 06:50
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. VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codelens wrongly count multiple file-local types up from multiple files whose names are same
2 participants