Incorrect position of error reported from macro in inline methods chain

[dos65]

This issue is based on scalameta/metals#3214

The position of error that was produced in inline context is unwrapped to the outermost one.
In case if it's a chain of inline calls, it's unclear where the actual error happens.

Example:

// InlineMac.scala
object InlineMac:
  
  inline def sample(inline expr: String): Int =
    ${ sampleImpl('expr) }
  
  def sampleImpl(expr: Expr[String])(using Quotes): Expr[Int] =
      import quotes.reflect.*
      report.errorAndAbort("Error", expr)
      
// Main.scala
object Main:
  def main(args: Array: String): Unit =
     inline def v2 = InlineMac.sample("foo")  // <- the actual error position was here
     inline def v1 = v2
     
     v2  // [error] Error <- reported error position is here - wrong

There is a difference between in error message between direct compiler usage and using bsp-client.
The default compiler reportee additionally adds lines like: This location contains code that was inlined from Main.scala:27 but I don't think that it helps a lot in this case.

[dos65]

After looking a bit more at inline errors, I noticed that it's a broader topic than just a reported case in this issue.

Even if the described case would have a more accurate position, it might be hard to understand the source of the error as it might depend on a parameter.

For example - a chain of inline defs with a summonInline with some specific type at the end:

inline def second[A]: Int = 
   summonInline[Foo[A]].foo

inline def first[A]: Int
  second[A] + 42
  
def foo =       // It's hard to say what is wrong by this error even having a correct position
  first[String] // [error] no implicit argument of  type Foo[A] was found

I've also noticed that InlinedCalls property has a similarity with stacktraces which fits this case. Also, as the nature of inlining is a kind of interpretation so probably having errors with traces wouldn't be a completely insane idea.

From Metals viewpoint, if there were stacktraces from inlineContext we could show them to the user at the place of error + provide additional navigation.
I did a quick test and attached a fake trace to hover to check if it's possible.

So, I'm wondering if it's possible to extend Diagnostic interface to add an option to carry such stacktraces from inlining.
I think that would make these error more clear.

Could anyone dive into this thing? I don't know who should I ping to start a discussion.

[nicolasstucki]

This example

trait Foo[X]:
  def foo: Int

inline def second[A]: Int = compiletime.summonInline[Foo[A]].foo

inline def first[A]: Int = second[A] + 42

def foo = first[String]

fails with

8 |def foo = first[String]
  |          ^^^^^^^^^^^^^
  |          no implicit argument of type Foo[String] was found
  | This location contains code that was inlined from Foo.scala:4
  | This location contains code that was inlined from Foo.scala:6

which seems to be correct. The summonInline was executed at inline site which is at the call of first[String]. Then we output the inline stack which points to the call of second[A] and summonInline[Foo[A]].

[nicolasstucki]

For the first case

// Main.scala
object Main:
  def main(args: Array[String]): Unit =
     inline def v2 =
       InlineMac.sample(
        "foo" // <- the actual error position was here
       )
     inline def v1 = v2

     v2  // [error] Error <- reported error position is here

the error is

0 |     v2  // [error] Error <- reported error position is here - wrong
   |     ^^
   |     Error
   | This location contains code that was inlined from Foo2.scala:6
   | This location contains code that was inlined from Foo2.scala:5

which points to the macro expansion site. This is the place where the error happened. It also points out the was reported on the "foo" string which was in code inlined by InlineMac.sample(...). It seems to have all the correct positions.

[nicolasstucki]

What we can improve is how the inline stack is reported. We could show the code and not only the line number.

[dos65]

@nicolasstucki

  | This location contains code that was inlined from Foo.scala:4
  | This location contains code that was inlined from Foo.scala:6

By some reason this part isn't rendered for bsp-clients(bloop/sbt-bsp) so Metals doesn't show it.
I can look why but I don't think that it will make it clear enough.

The problem from the editor and user viewpoint that inline error are different from a usual one.
It's attached only to the outermost position that triggers the first inline method.
The difference is that with an usual error it would be enough to look at signature of method that you trying to apply but the same approach won't work with inline one.
For example:

trait Foo[X]:
  def foo: Int

// you can look at the `first` signature and check what is required to call it 
object non-inlined:
   def second[A](using f: Foo[A]): Int = f.foo

   def first[A: Foo]: Int = second[A] + 42

   def foo = first[String] // [error] 

// looking at `first` signature/implementation won't be helpful
// there is also no errors on `second[A]` / `summonInline[A]`
// for methods larger than in this sample it might be hard to say what you need to fix
object inlined:

   inline def second[A]: Int = compiletime.summonInline[Foo[A]].foo

   inline def first[A]: Int = second[A] + 42

   def foo = first[String] // [error]

The solution with a better error message with code might improve the situation.
However, looking at the scalameta/metals#3214 , it seems that for IDE we need to have a way to provide a better navigation to the source of error / do some additional highlighting for inner positions.

I don't know what might be the final solution but at least having an extended information with a full trace would allow us to experiment with this thing.

[deusaquilus]

@dos65 @nicolasstucki Is #14002 supposed to be the solution to this issue? Or is there something else needed?

[nicolasstucki]

From the dotty perspective #14002 is the solution. But metals also needs to do a similar improvement.

[tgodzik]

With #14002 will it be possible to get all the locations or would Metals need to parse the messages for it?

[dos65]

@tgodzik The plan was to extend the diagnostic structure to carry this additional info for inline error, but I haven't had time to prototype this thing yet.

[nicolasstucki]

#14002 does not change anything for metals. The positions already contain the necessary information, it is a matter of displaying it correctly. The code in #14002 can be used as a reference to implement the metals version of this.