improvement: Improve heuritics used for fallback symbol search #6642
Conversation
| @@ -121,7 +121,7 @@ class DefinitionLspSuite | |||
| |import org.scalatest.funsuite.AnyFunSuite/*AnyFunSuite.scala*/ | |||
| |object MainSuite/*L6*/ extends AnyFunSuite/*AnyFunSuite.scala*/ { | |||
| | test/*AnyFunSuiteLike.scala*/(testName/*<no symbol>*/) { | |||
| | val condition/*L8*/ = Main/*Main.scala:5*/.message/*Main.scala:4*/.contains/*String.java*/("Hello") | |||
| | val condition/*L8*/ = Main/*Main.scala:5*/.message/*<no symbol>*/.contains/*String.java*/("Hello") | |||
There was a problem hiding this comment.
This change is expected since there is no actual message anymore, it was changed and the code no longer parses. We would guess it previously based on outdated information.
| * the can belong to them. We don't care about things inside of | ||
| * classes, since they need to be accessed with a select on a typed | ||
| * variable instead. |
There was a problem hiding this comment.
We don't care about things inside of classes...
I didn't understand that comment at first. You just can that qualifiers in select can be only packages/objects, right?
There was a problem hiding this comment.
Reworded it to state that we only currently try to guess things inside the objects.
| val proposedCurrentPackageSymbol = | ||
| guessObjectOrClass( | ||
| trees | ||
| .packageAtPosition(path, pos) | ||
| .getOrElse("_empty_") +: proposedNameParts | ||
| ) |
There was a problem hiding this comment.
You're not handling package objects and things like:
object O {
class A
val i: A@@ = ???
}Also, in:
package a
package ba.<..> should also be findable.
But here you should prefer the inner scope instead of finding all...
There was a problem hiding this comment.
The first thing should be found by the presentation compiler without an issue, if it's not finding it the code might be too broken.
I added handling of the second case with a test, though it looks like this is also currently mostly handled by the presentation compiler correctly, but I checked manually anyway to see if the results are the same. We could have a test suite that tries to compare both, but I think it is enough to know it's working for now.
| @@ -73,7 +73,7 @@ class PcDefinitionProvider(val compiler: MetalsGlobal, params: OffsetParams) { | |||
| if ( | |||
| symbol == null || | |||
| symbol == NoSymbol || | |||
| symbol.isErroneous | |||
| symbol.isError | |||
There was a problem hiding this comment.
This is an interesting change, thanks to this we got back the symbols at definitions in the fallback change. Without this change it would not go to definition if the symbol had a type error, which from the definition standpoint is not important. What is important is to know if it's an error symbol, which would not have any position to go to.
| @@ -86,10 +86,14 @@ class PcDefinitionProvider(val compiler: MetalsGlobal, params: OffsetParams) { | |||
| symbol.pos.isDefined && | |||
| symbol.pos.source.eq(unit.source) | |||
| ) { | |||
| val namePos = | |||
There was a problem hiding this comment.
Thanks to this we will have the same result from semanticdb and from presentation compiler. I think previously it was focused, because we would get wrong ranges on parameters, but I fixed it.
Without this fixed, one of the fallback references tests started failing because go to definition would return a result that could not enclose the cursor position so for:
val iamad@@efinition = 1
presentation compiler would return:
val <<>>iamadefinition = 1
and we wouldn't be able to know if the position of the cursor is at the definition (so we should find references)
| @@ -108,9 +108,9 @@ class DefinitionLspSuite | |||
| |import java.util.concurrent.Future/*Future.java*/ // unused | |||
| |import scala.util.Failure/*Try.scala*/ // unused | |||
| |object Main/*L5*/ extends App/*App.scala*/ { | |||
| | val helloMessage/*<no symbol>*/ = Message/*Message.java:1*/.message/*Message.java:2*/ | |||
| | val helloMessage/*L6*/ = Message/*Message.java:1*/.message/*Message.java:2*/ | |||
There was a problem hiding this comment.
Thanks to the change I did presentation compiler can recover enough to know this is a separate symbol, though it will not manage to get a sensible type for it, which is fine.
|
This should be good to review, the issues I showed on the demo earlier today are not related to the changes. |
Previously, we would just use name when nothing else was working, which could lead to a lot of false positives. Now, we take into account imports, current package and fully qualified names to find the correct definition. One case this might not work is with unimports, but that should not be a big deal as both will be found only if nothing was ever compiled.
…essing Turns out Scaladoc had a similar functionality, though I improved it a tiny bit and moved it to Symbol class.
Otherwise when looking for references with only presentation compiler working we wouldn't get the correct positions we expect.
|
Ach, actually I moved the tests to slow tests to have them run for both Scala 2 and 3 |
metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala
Outdated
Show resolved
Hide resolved
Previously, we would just use name when nothing else was working, which could lead to a lot of false positives.
Now, we take into account imports, current package and fully qualified names to find the correct definition.
One case this might not work is with unimports, but that should not be a big deal as both will be found only if nothing was ever compiled.
Should fix #6641