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

Add goto implementation #934

Merged
merged 14 commits into from Oct 4, 2019
Merged

Add goto implementation #934

merged 14 commits into from Oct 4, 2019

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Sep 19, 2019

This adds the functionality for goto implementation which is done by:

  • moving handling of semanticDB file changes out of ReferencesProvider
  • creating and additional class that handles extracting semantic data on changes and delegates them to other providers that handle concrete indexes
  • adding a new ImplementationProvider with new index for class hierarchies
…s an additional index for class hierachies, which is later used to figure out the class implementations.
@tgodzik tgodzik requested a review from olafurpg Sep 19, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementation branch 2 times, most recently from ff2edf3 to 76b80c4 Sep 19, 2019
@tgodzik tgodzik requested a review from marek1840 Sep 19, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 76b80c4 to 84a0f97 Sep 20, 2019
@tgodzik tgodzik requested a review from marek1840 Sep 20, 2019
Copy link
Member

olafurpg left a comment

This is exciting! A few critical questions about the shape of the index and how to handle multiple source files. Otherwise mostly just nitpick 😅

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Sep 20, 2019

@olafurpg @marek1840 Thanks for the reviews! Managed to find a lot of issues thanks to them.

@tgodzik tgodzik requested review from olafurpg and marek1840 Sep 20, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementation branch 2 times, most recently from 5681fc1 to 986b058 Sep 20, 2019
…ences to use proper token distance
@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 986b058 to f6a9e0c Sep 20, 2019
…mplementations are defined in a single string
@tgodzik tgodzik requested a review from olafurpg Sep 24, 2019
olafurpg added a commit to olafurpg/metals that referenced this pull request Sep 24, 2019
olafurpg added a commit to olafurpg/metals that referenced this pull request Sep 24, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 4ff40e0 to 9c09ae2 Sep 25, 2019
…ods and calculate possible method implementations.
@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 9c09ae2 to 38a8a65 Sep 26, 2019
@tgodzik tgodzik changed the title Add goto implementation for classes Add goto implementation Sep 26, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 84916bc to e2489ae Sep 26, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Sep 27, 2019

Need to add support for non-workspace classes still - working on that.

@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 5390106 to b1848b4 Sep 27, 2019
@tgodzik tgodzik force-pushed the tgodzik:implementation branch from e27a4c9 to 7be79ea Sep 29, 2019
Copy link
Member

olafurpg left a comment

I just tried this locally and it's looking super good! I'm very excited to see that it now handles method implementations even they're overloaded and have generics 😻 Nice work 👏

I found one case where it still doesn't find implementations of external libraries via an intermediary type

Screenshot 2019-09-29 at 22 41 46

I expected it to include `class Rex` as an implementation since `RuntimeException` extends `Exception`.

If we add class Bar extends Exception it works as expected
Screenshot 2019-09-29 at 22 42 05

@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 295b1e2 to 2d1dcfb Sep 30, 2019
@tgodzik tgodzik requested review from marek1840 and olafurpg Oct 1, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 1, 2019

Alright, I managed to fix the issue reported by @olafurpg and added some tests. Then added some more tests and now I am out of ideas for what might not work anymore. Let me know if anyone can think of more corner cases in the tests.

Tried to tidy the code as much as possible, let me know if something is not clear.
I would appreciate every possible feedback.

tgodzik added 2 commits Oct 1, 2019
Copy link
Member

olafurpg left a comment

I just tried this out on the Akka build and I amazed how well it works. Fanstastic work @tgodzik! LGTM 👍

SemanticDB indexing time doesn't seem to be negatively affected

time: updated build targets in 95ms
time: started file watcher in 2.4s
time: indexed library classpath in 0.59s
time: indexed workspace SemanticDBs in 0.39s
time: indexed workspace sources in 2.64s
time: indexed library sources in 0.24s
time: indexed workspace in 6.37s

It's able to find 500-1000 implementations of akka Actor, Actor.receive, Exception somewhere between 0.5-2 seconds

[Trace - 00:06:22 PM] Sending response 'textDocument/implementation - (106)'. Processing request took 848ms

I'm able to find implementations even with generics and overloading 😍

Screenshot 2019-10-03 at 11 49 41

This feature unblocks "rename symbol", which I'm very excited to try out!

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 3, 2019

I hit on one case in Akka where I expected results but got "no implementation found"

Screenshot 2019-10-03 at 12 22 30

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 3, 2019

I hit on one case in Akka where I expected results but got "no implementation found"

Screenshot 2019-10-03 at 12 22 30

Checking!

@tgodzik tgodzik force-pushed the tgodzik:implementation branch from 3a4ba5e to d837625 Oct 3, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 3, 2019

I hit on one case in Akka where I expected results but got "no implementation found"
Screenshot 2019-10-03 at 12 22 30

Checking!

That's fixed, problem was when symbol was not found in GlobalSymbolTable

@tgodzik tgodzik merged commit bad1fc0 into scalameta:master Oct 4, 2019
2 checks passed
2 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tgodzik tgodzik deleted the tgodzik:implementation branch Oct 4, 2019
@jvican

This comment has been minimized.

Copy link
Collaborator

jvican commented Oct 4, 2019

Congratulations on getting this one merged, I'm excited to try this out, looks fantastic 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.