search

sourcegraph / scip-typescript

SCIP indexer for TypeScript and JavaScript
Apache License 2.0
43 stars 17 forks source link

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 8 months ago

varungandhi-src commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

This PR here implements several critical changes that might address this https://github.com/sourcegraph/scip-typescript/pull/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 commented 8 months ago

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.