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

Different SCIP symbols for parameter and component property in a React function component make it seem like some references are missing #295

Open
varungandhi-src opened this issue Dec 22, 2023 · 4 comments

Comments

@varungandhi-src
Copy link
Contributor

Reported in Slack: https://sourcegraph.slack.com/archives/CHXHX7XAS/p1703179808338909

I noticed we miss a reference in this file (compare Sourcegraph vs. VS Code), in this case it was the ref I was looking for:


Here is a link to the ref panel, started at a slightly different point.

https://sourcegraph.com/github.com/sourcegraph/cody@807ea71d868fb7a1e74471bb67396b52acb704d8/-/blob/lib/ui/src/Chat.tsx?L202:5-202:28#tab=references

Explanation for the current behavior

To match the desired behavior more closely, we could potentially use the is_reference relationship in SCIP to match the local symbol with the global symbol. https://github.com/sourcegraph/scip/blob/1d0b24e10fa549e80d7c3f9f86df1238a0f5ae62/scip.proto#L439-L463

@beyang
Copy link
Member

beyang commented Dec 22, 2023

There's 2 ways of addressing this:

  1. Change the defs/refs extracted and represented in SCIP (i.e., merge the field def and argument def into one)
  2. Keep the existing data model, but in the app/UI layer, detect when a reference overlaps completely with another def, and then gather references from that def (an extra hop in the SCIP graph)

If I'm understanding correctly, you're proposing (2), which I agree is the right approach.

@varungandhi-src
Copy link
Contributor Author

I'm proposing a 3rd alternative, where we modify the SCIP data to include an extra is_reference relationship to connect the two distinct symbols for the purpose of 'Find references'. We already do this in scip-ruby for some cases.

There's no need to modify any client-side code.

@olafurpg
Copy link
Member

This PR here implements several critical changes that might address this #256

I think it's mostly ready for merging. It's an overall huge improvement in how we map properties of object literals to type members. I didn't merge it because I didn't have the bandwidth to communicate the changes or deal with feedback after publishing the change.

@olafurpg
Copy link
Member

In that PR, I opted for an encoding where we emit plain reference occurrences on the object literal properties. This encoding can be undesirable in some cases, and we can later refine it to use is_reference. The most important change in the PR is that it adds new infrastructure to map object literal properties to type members, which is a criteria to fix this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants