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

Case Class References Missing Instantiations #403

Closed
wchau opened this issue Feb 25, 2022 · 7 comments · Fixed by #404
Closed

Case Class References Missing Instantiations #403

wchau opened this issue Feb 25, 2022 · 7 comments · Fixed by #404
Assignees

Comments

@wchau
Copy link

wchau commented Feb 25, 2022

lsif-java version: 0.7.6

Type of code: case classes in Scala

Expected behavior: Case class "Find Reference" should contain instantiations.

Actual behavior: Case class "Find References" does not contain instantiations. You can find other instantiations if you "Find Reference" any one instantiation.

If the companion object is explicitly defined, you can find instantiations there by "Find Reference".

Note that "Go to Definition" from instantiations does go to the case class if the companion object does not exist (otherwise, it would go to the companion object), but the hover references the companion object(???). Ideally, all the synthetic functions should reference the case class (and vice versa) similar to IntelliJ's code reference.

case class TestA(a: String) {}  // "Find References" missing the below instantation

...

TestA("blah")  // Hover references object TestA but "Go to Definition" jumps to case class definition
@wchau
Copy link
Author

wchau commented Feb 25, 2022

It is already a lot better than before, though, thanks! :)

@olafurpg
Copy link
Member

Thank you for reporting! I am able to reproduce and I opened a PR fixing this issue in #404

Please keep the bug reports coming, I appreciate your patience with all of the different releases. It's tricky to get this right for case classes since they generate so many different kinds of symbols (some which we want to include with "find ref" results while not others)

@olafurpg olafurpg self-assigned this Feb 25, 2022
@olafurpg
Copy link
Member

Reopening this issue since "Find references" on the case class does not show usages of the synthetic companion object. Here's a minimized test case that reproduces the problem

// relationship is_definition semanticdb maven . . minimized/Issue396#

To fix this problem, we should add an is_reference relationship alongside the is_definition relationship. I estimate this is an easy issue to fix and it does not require changes in the Sourcegraph backend.

@olafurpg olafurpg reopened this Apr 21, 2023
@olafurpg
Copy link
Member

The root cause of this issue is here where we intentionally return false for objects

I'm wondering if the fix is to remove the supportsReferenceRelationship condition where we set is_definition. As long as the names of the symbols are the same then it's OK for them to share a reference relationship.

@olafurpg olafurpg assigned keynmol and unassigned keynmol Apr 21, 2023
@olafurpg
Copy link
Member

Another possible fix, which I think might be a better solution, is to add an is_reference relationship on the synthetic apply(). on the companion object. This solution is better because it works the same regardless if there is an explicit companion or not.

@olafurpg
Copy link
Member

Concretely, the proposal is to update this line here in the snapshots to include is_reference

// relationship is_definition semanticdb maven . . minimized/Issue403#

@olafurpg
Copy link
Member

Fixed in #561

Link to reproduce https://sourcegraph.com/github.com/sourcegraph/scip-java/-/blob/tests/minimized-scala/src/main/scala/minimized/Issue403.scala?L3:12-3:20#tab=references

CleanShot 2023-04-24 at 18 14 33@2x

Note that the empty line is refers to a zero-length range for a usage of the apply() symbol. I have opened a separate issue to track that #564

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

Successfully merging a pull request may close this issue.

3 participants