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

Kotlin find references shows refs for wrong symbol #678

Open
beyang opened this issue Jan 23, 2024 · 6 comments · Fixed by #682
Open

Kotlin find references shows refs for wrong symbol #678

beyang opened this issue Jan 23, 2024 · 6 comments · Fixed by #682

Comments

@beyang
Copy link
Member

beyang commented Jan 23, 2024

  • Sourcegraph version: sourcegraph.com

Steps to reproduce:

  1. This should show refs for PromptPanel, but instead shows refs for JLayeredPane and Component

image

@beyang
Copy link
Member Author

beyang commented Jan 24, 2024

@varungandhi-src varungandhi-src transferred this issue from sourcegraph/sourcegraph-public-snapshot Jan 24, 2024
@varungandhi-src
Copy link
Contributor

varungandhi-src commented Jan 24, 2024

Here, the problem is that we're emitting reference relationships for the type symbol.

However, that's incorrect, we should only be emitting an implementation relationship.

This is covered in the SCIP documentation. https://github.com/sourcegraph/scip/blob/main/scip.proto#L439-L472

  // When resolving "Find references", this field documents what other symbols
  // should be included together with this symbol. For example, consider the
  // following TypeScript code that defines two symbols `Animal#sound()` and
  // `Dog#sound()`:
  // ```ts
  // interface Animal {
  //           ^^^^^^ definition Animal#
  //   sound(): string
  //   ^^^^^ definition Animal#sound()
  // }
  // class Dog implements Animal {
  //       ^^^ definition Dog#, relationships = [{symbol: "Animal#", is_implementation: true}]
  //   public sound(): string { return "woof" }
  //          ^^^^^ definition Dog#sound(), references_symbols = Animal#sound(), relationships = [{symbol: "Animal#sound()", is_implementation:true, is_reference: true}]
  // }
  // const animal: Animal = new Dog()
  //               ^^^^^^ reference Animal#
  // console.log(animal.sound())
  //                    ^^^^^ reference Animal#sound()
  // ```
  // Doing "Find references" on the symbol `Animal#sound()` should return
  // references to the `Dog#sound()` method as well. Vice-versa, doing "Find
  // references" on the `Dog#sound()` method should include references to the
  // `Animal#sound()` method as well.
  bool is_reference = 2;
  // Similar to `is_reference` but for "Find implementations".
  // It's common for `is_implementation` and `is_reference` to both be true but
  // it's not always the case.
  // In the TypeScript example above, observe that `Dog#` has an
  // `is_implementation` relationship with `"Animal#"` but not `is_reference`.
  // This is because "Find references" on the "Animal#" symbol should not return
  // "Dog#". We only want "Dog#" to return as a result for "Find
  // implementations" on the "Animal#" symbol.
  bool is_implementation = 3;

Maybe all that is needed is to add a call to supportsReferenceRelationship (https://sourcegraph.com/github.com/sourcegraph/scip-java/-/blob/scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/ScipSemanticdb.java?L442:26-442:55) check during the appropriate setIsReference calls. (maybe this one)

@olafurpg
Copy link
Member

@varungandhi-src the root bug seems to be that semanticdb-kotlinc is setting Kind.TYPE here instead of Kind.Class https://sourcegraph.com/github.com/sourcegraph/scip-kotlin@cbbf452c9bf0165d8144ba17a6f2026e00257fd6/-/blob/semanticdb-kotlinc/src/main/kotlin/com/sourcegraph/semanticdb_kotlinc/SymbolsCache.kt?L167:62-167:66

Alternatively, we can update scip-java to add an extra case for case Kind.TYPE: return false in the pattern match you linked to. Should be an easy fix, thank you @beyang for reporting!

olafurpg added a commit that referenced this issue Feb 2, 2024
Fixes #678

Previously, doing "Find references" on a symbol with kind `INTERFACE` or
`TYPE` returned results for all references to symbols that *implement*
that symbol. This bug was particularly noticeable for Kotlin sources because
scip-kotlin emits the `TYPE` kind for classes. Now, we no longer emit
reference relationships so the implementations only appear in "Find
implementation" and not "Find references".
@olafurpg
Copy link
Member

olafurpg commented Feb 2, 2024

Used the opportunity while fixing Kotlin 1.9 support to also fix this bug #682

@olafurpg
Copy link
Member

olafurpg commented Feb 2, 2024

I'm trying to confirm 100% that this bug has been fixed but now I'm hitting on another bug in our code search UI. Reported here https://sourcegraph.slack.com/archives/C05MHAP318B/p1706882647263079

I'm not able to manually upload the SCIP index anymore because I don't have site-admin access.

Either way, we should be able to confirm it's fixed as soon as the main branch in the JB repo merges a new PR https://sourcegraph.com/github.com/sourcegraph/jetbrains/-/blob/src/main/java/com/sourcegraph/cody/PromptPanel.kt?L30:7-30:18#tab=references

@varungandhi-src
Copy link
Contributor

Re-opening since this was reported again in Slack.

Sourcegraph link

image

The commit for the index is quite new, so either auto-indexing is not picking up the version of the scip-java with this fix, or the bug is not fully fixed.

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

Successfully merging a pull request may close this issue.

4 participants