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 a bug related to textDocumentation/implementation #27

Merged
merged 2 commits into from
May 25, 2022

Conversation

olafurpg
Copy link
Member

Previously, doing "Find implementations" only showed one result when there were multiple results available. Now, all available results should be shown after this fix.

Opening this as a draft since it's still not working 100% and I'm gonna continue with this PR tomorrow.

Test plan

See newly added snapshot tests. I'm also manually testing this change by uploading the generated dump.lsif to Sourcegraph here https://sourcegraph.com/github.com/sourcegraph/scip-typescript@34337f2fa29d3a83c71814458d19741ae4f21db9/-/blob/snapshots/input/syntax/src/inheritance2.ts?L2:3&subtree=true#tab=implementations_typescript

olafurpg added 2 commits May 24, 2022 23:13
Previously, we emitted multiple `implementationResult` because we used
the wrong `symbolInformationIDs`.
@olafurpg olafurpg force-pushed the olafurpg/implementation-bugfix branch from 8a0c57f to d30dc38 Compare May 25, 2022 09:03
@olafurpg olafurpg marked this pull request as ready for review May 25, 2022 09:03
@olafurpg
Copy link
Member Author

It's working correctly now

CleanShot 2022-05-25 at 11 05 15@2x

The root bug was that we were always emitting fresh implementationResult vertexes because we didn't pass around symbolInformationIDs pointers. This explains why it always showed only one result for implementations.

@olafurpg
Copy link
Member Author

It's difficult to see the difference from the raw dump.lsif payloads. Here are screenshots showing the gist of the difference

CleanShot 2022-05-25 at 12 58 55
CleanShot 2022-05-25 at 12 57 44

@olafurpg olafurpg requested a review from varungandhi-src May 25, 2022 11:01
@olafurpg olafurpg self-assigned this May 25, 2022
@olafurpg olafurpg merged commit 5922ee0 into main May 25, 2022
@olafurpg olafurpg deleted the olafurpg/implementation-bugfix branch May 25, 2022 19:03
@olafurpg
Copy link
Member Author

I'm happy to do a post-merge review if there are unaddressed comments.

@olafurpg
Copy link
Member Author

I merged the PR to unblock a version bump in src-cli so that we can get this fix available asap before the blog announcements.

olafurpg added a commit to sourcegraph/src-cli that referenced this pull request May 25, 2022
olafurpg added a commit to sourcegraph/src-cli that referenced this pull request May 25, 2022
scjohns pushed a commit to sourcegraph/src-cli that referenced this pull request Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant