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

Promote synthetic definitions into normal definitions if they share names #404

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

olafurpg
Copy link
Member

Fixes #403

Previously, we generated synthetic definitions for all synthetic
definitions. However, this meant that "find references" never showed
results for synthetic symbols share the same name as the non-synthetic
definition. This commit changes that so synthetic definitions that share
the same name as the non-synthetic symbol are included with "find
references" results.

While testing this change I noticed that we don't handle synthetic
constructor parameters so I added support for those along the way.

Test plan

Snapshot tests included in the diff. I also tested the change manually in this URL here https://sourcegraph.com/github.com/sourcegraph/sbt-sourcegraph@e2f71270ea518766c65097b1f17e97b55059d792/-/blob/example/foo.scala?L3:13#tab=references

CleanShot 2022-02-25 at 15 20 44@2x
CleanShot 2022-02-25 at 15 20 47@2x

CleanShot 2022-02-25 at 15 20 51@2x

…ames

Previously, we generated synthetic definitions for all synthetic
definitions. However, this meant that "find references" never showed
results for synthetic symbols share the same name as the non-synthetic
definition. This commit changes that so synthetic definitions that share
the same name as the non-synthetic symbol are included with "find
references" results.

While testing this change I noticed that we don't handle synthetic
constructor parameters so I added support for those along the way.
@olafurpg
Copy link
Member Author

@bobheadxi The pr-auditor check failed with a cryptic error message https://github.com/sourcegraph/lsif-java/runs/5334788397?check_suite_focus=true#step:4:34

![CleanShot 2022-02-25 at 15 20 51@2x](https://user-images.githubusercontent.com/1408093/155731053-74904d20-db13-4e49-b25a-2ebe55f1f41f.png) Error:<nil>}
[35](https://github.com/sourcegraph/lsif-java/runs/5334788397?check_suite_focus=true#step:4:35)
2022/02/25 14:23:17 preMergeAudit: CreateStatus: POST https://api.github.com/repos/sourcegraph/lsif-java/statuses/0c51ae30ffd4e50b1f5fd0a2dbf47e039a3c6b09: 401 Bad credentials []

Any advice on how to troubleshoot that?

@bobheadxi
Copy link
Member

@olafurpg we use sourcegraph-bot to make API requests, does the bot have access to this repo?

@bobheadxi
Copy link
Member

Ah I see - @olafurpg this PR seems to be from a fork, I'm not sure how API access to set status updates on a fork's commits works

@wchau
Copy link

wchau commented Feb 25, 2022

In https://sourcegraph.com/github.com/sourcegraph/sbt-sourcegraph@e2f71270ea518766c65097b1f17e97b55059d792/-/blob/example/foo.scala?L3:13#tab=references

When I hover

B

in

case class B

It sometimes gives case class B... and sometimes gives object B.

Screen Shot 2022-02-25 at 3 24 34 PM

Screen Shot 2022-02-25 at 3 24 57 PM

@olafurpg
Copy link
Member Author

@wchau I'm able to reproduce! That's an issue outside of lsif-java that needs to be fixed in the Sourcegraph backend. Both the case class and the object are defined at the same location and there appears to b non-determinism related to which symbol docstring gets displayed in the hover. I've added a reminder to report an upstream issue in the sourcegraph/sourcegraph erpo

@olafurpg
Copy link
Member Author

Ah I see - @olafurpg this PR seems to be from a fork, I'm not sure how API access to set status updates on a fork's commits works

I can open PRs from the main repo but what should the pr-auditor do for PRs from external contributors? I have a preference for opening PRs from my fork instead of the main repo because I like push a lot of WIP branches while keeping the main repo clean. I don't like cloning a repo with lots of WIP branches from the maintainers.

@olafurpg
Copy link
Member Author

olafurpg commented Mar 2, 2022

@wchau I opened a followup issue to address the non-deterministic hovers https://github.com/sourcegraph/sourcegraph/issues/32044

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.

Case Class References Missing Instantiations
4 participants