Find References searches also for base method occurrences

[kpbochenek]

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:

object Example {

  class A { def fx(): Unit = () }
  class B extends A { override def fx(): Unit = () }
  class C extends B { override def fx(): Unit = () }
  class D extends C
  class E extends D { override def fx(): Unit = () }

  class X { def fx(): Unit = () }

  val a = new A()
  a.fx()

  val b = new B();
  b.fx()

  val c = new C();
  c.fx()

  val d = new D();
  d.fx()

  val e = new E();
  e.fx()
  
}

With current behaviour when we search for a references to 'b.fx()' we will get 2 results:

With a new behaviour searching references to the same function we will get same results plus references to overridden methods (from superclasses and subclasses):

Detailed changes:

• fix typos (symbolOccurence -> symbolOccurrence, ineheritance -> inheritance)
• change functions to need only position and not full Params object if operate only on position
• get rid of using TextParams API object internally in favor of PositionInFile
• add referencesSync (only for tests) to not have to deal with async behaviour in tests.
• move functionality of searching for all subclasses / superclasses from RenameProvider -> ReferenceProvider and put it into allInheritanceReferences function
• reuse this function in searching for references and renaming.
• test checking mentioned functionality: https://github.com/scalameta/metals/pull/1388/files#diff-bd990ebb68b0756f1af9b16a82841ef8R216
• reference results are sorted ONLY in tests to provide deterministic results.
• fix searching for parent class when parent class exists in not currently opened file.

Here (https://github.com/scalameta/metals/pull/1388/files#diff-2c60d9818336621db1490d63594edff5R151) I had to add declaration to expected results, because in tests server.references(...) creates underneath new 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 methodsSearchedWithoutInheritance (e.g. toString, equals, hashCode, clone)

[tgodzik]

Looks to be on a good path! Thanks a lot for the great work!

I had some comments though.

[tgodzik]

@olafurpg It's ready to review, let me know what you think. :)

[tgodzik]

Looking closer to finished! Added some comments, mostly I think nitpicks.

[tgodzik]

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?

[olafurpg]

@tgodzik is the performance penalty for every request (even for non overridden methods) or only for requests on overridden methods?

[olafurpg]

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.

[tgodzik]

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?

[olafurpg]

This is a fantastic contribution @kpbochenek! LGTM 👍

[tgodzik]

Just a few more comments, almost ready!

[tgodzik]

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 find references less responsive. This can be done either by using classgraph or by our custom solution similar to searching for top levels.

TLDR: We need a faster and less memory heavy way to check inheritance in dependencies.