scalameta / metals-feature-requests

Issue tracker for Metals feature requests
37 stars 4 forks source link

Support LSP references for better-monadic-for implicits #393

Open Z1kkurat opened 7 months ago

Z1kkurat commented 7 months ago

Describe the bug

Find references doesn't work for implicits declared with better-monadic-for. Only the reference to the implicit itself is returned. Tried in nvim and vscode.

Steps to reproduce:

  1. Example repository: https://github.com/Z1kkurat/bm4-metals-repro
  2. Open Hello.scala, put the cursor on line 6 (word bar) link
  3. Try to find references

Expected behavior

A list of references to classes/methods an implicit (Bar in the repro example) is passed to (constructors of classes Foo and Baz in the repro example)

Operating system

macOS

Editor/Extension

Nvim (nvim-metals)

Version of Metals

v.1.2.0

Extra context or search terms

Screenshot:

Screenshot 2024-02-06 at 23 01 22
kasiaMarek commented 7 months ago

Thanks for the report. It seems that bar is not the implicit synthetic symbol created by implicit0. Associating those symbols might require some special casing. We could take another look after https://github.com/scalameta/metals/pull/5940 is merged and finding references for local symbols will be handled by pc.

kasiaMarek commented 1 month ago

There seem to be 2 problems here:

  1. the usages of bar are not found,
  2. an incorrect reference enclosing the whole for comprehension is found.

1. the usages of bar are not found

In the desugared tree implicits passed to Baz constructors have different symbols then the bar value visible in the code. I think this should be fixed upstream so either all occurrences have the same symbol or are connected to the source symbol via some established way (e.g. special attachment).

E.g. the following code:

// Util.scala
package example
class Baz(implicit b: Bar)
class Bar(i: Int)
// Demo.scala
package example
object Hello {
  for {
    _ <- Some(1)
    implicit0(bar: Bar) = new Bar(1)
    baz = new Baz
    _ <- Some(2)
    baz2 = new Baz
  } yield ()
}

gets desugared into:

// Demo.scala
package example {
  object Hello extends scala.AnyRef {
    def <init>(): example.Hello.type = {
      Hello.super.<init>;
      ()
    };
    scala.Some.apply[Int](1).map[(Int, example.Bar, example.Baz)](((x$1: Int) => {
      <synthetic> <artifact> private[this] val x$3: (example.Bar, example.Bar) = (new Bar.<init>(1): example.Bar @unchecked) match {
        case (x$2 @ (bar @ _)) => scala.Tuple2.apply[example.Bar, example.Bar](x$2, bar)
      };
      val x$2: example.Bar = x$3._1;
      val bar: example.Bar = x$3._2;
      implicit <synthetic> <artifact> val bar$implicit$1: example.Bar = bar;
      val baz: example.Baz = new Baz.<init>(bar$implicit$1);
      scala.Tuple3.apply[Int, example.Bar, example.Baz](x$1, x$2, baz)
    })).flatMap[Unit](((x$6: (Int, example.Bar, example.Baz)) => (x$6: (Int, example.Bar, example.Baz) @unchecked) match {
      case (_1: Int, _2: example.Bar, _3: example.Baz): (Int, example.Bar, example.Baz)(_, (bar @ _), (baz @ _)) => {
        implicit <synthetic> <artifact> val bar$implicit$2: example.Bar = bar;
        scala.Some.apply[Int](2).map[Unit](((x$4: Int) => {
          val baz2: example.Baz = new Baz.<init>(bar$implicit$2);
          ()
        }))
      }
    }))
  }
}

2. an incorrect reference enclosing the whole for comprehension is found

In the generated tree usage of bar ( in implicit <synthetic> <artifact> val bar$implicit$1: example.Bar = bar;) has an incorrect range, which encloses the whole for comprehension. Should be fixed upstream.

tgodzik commented 1 month ago

Thanks for investigating @kasiaMarek

I don't think we can do anything as from the compiler perspective the current behaviour is correct. There is no way for us to know bar$implicit$1 and bar are the same symbol from source code perspective. This might be quite hard to fix since it requires some changes in the plugin and possibly the compiler. As such, I will move it to a feature request.