-
Notifications
You must be signed in to change notification settings - Fork 348
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
Find References searches also for base method occurrences #1388
Find References searches also for base method occurrences #1388
Conversation
34e387d
to
87330da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to be on a good path! Thanks a lot for the great work!
I had some comments though.
metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/DocumentHighlightProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
b0f9eb8
to
c44f65d
Compare
@olafurpg It's ready to review, let me know what you think. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking closer to finished! Added some comments, mostly I think nitpicks.
metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/PositionInFile.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more small nitpicks and I also tested it myself on ZIO codebase. It seems to work reasonably well aside from a small issue. References results seem to get duplicated:
I am also worried a bit about performance of any symbols in a class extending a class from outside the workspace. First search will need to load up the classpath information, which might take a while. This might not be a big issue, but I just wanted to raise it here. I am thinking, we can just not look the entire inheritance path in case of find references. So in essence we would only look up and down the inheritance chain inside the workspace. Opinions?
metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
@tgodzik is the performance penalty for every request (even for non overridden methods) or only for requests on overridden methods? |
Currently the performance of references has not been an issue (it’s really fast) whereas missing results (and occasionally incorrect results) has been a bigger issue in my experience. |
Ok, I think we can optimize is later if needed and rename already requires loading classpath information. However, now that I think about it. Do we actually look correctly for methods coming from a dependency? Can we add a test case? |
db2158d
to
0173aee
Compare
0173aee
to
81da8a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fantastic contribution @kpbochenek! LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more comments, almost ready!
metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ReferenceProvider.scala
Outdated
Show resolved
Hide resolved
81da8a9
to
dc21706
Compare
We need to block this PR until #1287 is solved. We don't have an easy way to detect inheritance inside the dependencies and I am afraid this will cause TLDR: We need a faster and less memory heavy way to check inheritance in dependencies. |
Fixes #1063
According to the discussion in related issue it would be good to provide references of overridden / super methods when searching for references of given method.
Example code:
With current behaviour when we search for a references to 'b.fx()' we will get 2 results:
![2020-02-06-090121_290x107_scrot](https://user-images.githubusercontent.com/10478402/73917327-74938600-48bf-11ea-9878-051c2831e51d.png)
With a new behaviour searching references to the same function we will get same results plus references to overridden methods (from superclasses and subclasses):
![2020-02-06-090224_283x264_scrot](https://user-images.githubusercontent.com/10478402/73917332-778e7680-48bf-11ea-8e33-f007dd3a19b0.png)
Detailed changes:
PositionInFile
referencesSync
(only for tests) to not have to deal with async behaviour in tests.allInheritanceReferences
functionHere (https://github.com/scalameta/metals/pull/1388/files#diff-2c60d9818336621db1490d63594edff5R151) I had to add declaration to expected results, because in tests
server.references(...)
creates underneathnew ReferenceContext(true)
(where true is for includeDeclaration) which indicates declaration should be added to results.Finding references is not following inheritance tree in case of functions defined in
![2020-02-06-170525_527x179_scrot](https://user-images.githubusercontent.com/10478402/73955138-286a3500-4903-11ea-9ec4-688d9a51d99f.png)
methodsSearchedWithoutInheritance
(e.g. toString, equals, hashCode, clone)