-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add support for implementation edges in our LSIF indexer #64263
Add support for implementation edges in our LSIF indexer #64263
Conversation
b7a23d0
to
e2dd6dc
Compare
e2dd6dc
to
a136715
Compare
a136715
to
c178ea6
Compare
MarkImplementationOfSymbol(overridenMember); | ||
} | ||
|
||
void MarkImplementationOfSymbol(ISymbol baseMember) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way this will make sense is if you look at the picture in #59617.
MarkImplementationOfSymbol(implementedMember); | ||
|
||
// If this overrides a method, we'll also mark it the same way. We want to chase to the base virtual method, skipping over intermediate | ||
// methods so that way all overrides of the same method point to the same virtual method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting - I would have thought we would have pointed to intermediate members which themselves point back to the base in a chain. This loses the intermediate member information, but I have no idea if that is OK or not (not sure how this is used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this and decided to go with this way for now. We saw two potential downsides of having the full chain:
- It's not clear what happens if somebody removes their method of an override in the middle of a chain, since then the ends of the chain are disconnected.
- It means a find references in the cloud has to connect all the chains together, as opposed to doing a faster lookup.
So we're going this way for now.
Fixes #59617