Skip to content
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

[Scala3] Regression: Multiple error inlines are not shown starting 3.0.1 #3214

Open
deusaquilus opened this issue Oct 18, 2021 · 18 comments
Open
Labels
upstream-fix-needed Waiting on a fix upstream

Comments

@deusaquilus
Copy link

deusaquilus commented Oct 18, 2021

Describe the bug

When using Scala 3.0.0 if you have code that has errors in multiple levels of inlining, there is a red underline for every single one:
image


You can hover over any of them and see the errors thrown at that point:
image

In 3.0.1 you only see the last position underlined:
(although you can see that SBT still knows about the parent inline i.e. the one on line 5):
image


This functionality is badly needed because in ProtoQuill errors almost always happen in inline def quote { ... } blocks long before the run function finally splices them back into real code so having mouse-over insight onto the error in those sections is invaluable.

image

If we only have the error information in the run(...) site it is very difficult to track down the error.
image


To Reproduce

Create a simple example. Create the following macro:

object InlineMac {
  inline def matchOne(inline value: String): String = ${ matchOneImpl('value) }
  def matchOneImpl(value: Expr[String])(using Quotes): Expr[String] = {
    import quotes.reflect._
    value match
      case '{ "foo" } => '{ "foo2" }
      case _ => report.throwError("Invalid value", value)
  }

  inline def matchTwo(inline value: String): String = ${ matchTwoImpl('value) }
  def matchTwoImpl(value: Expr[String])(using Quotes): Expr[String] = {
    import quotes.reflect._
    value.asTerm.underlyingArgument.asExpr match
      case '{ "foo2" } => '{ "foo3" }
  }
}

Then use it like so:

object InlineUse {
  def main(args: Array[String]): Unit = {
    inline def splice1 = InlineMac.matchOne("foo")
    val splice2 = InlineMac.matchTwo(splice1)
  }
}

When SBT scalaVersion is 3.0.0 there will be a short red line underneath (a part of) both matchOne and matchTwo.
If SBT scalaVersion is 3.0.1 or above there will only be a red line underneath matchTwo.

Code examples can be found in the following repo:
https://github.com/getquill/protoquill-example/tree/multi-inline-example

The simple example above is here:
https://github.com/getquill/protoquill-example/tree/multi-inline-example/src/main/scala/io/getquill/simple

Expected behavior
All the places from which the code is inlined should have a red arrow underneath.

Installation:

  • Operating system: macOS/Windows/Linux
  • Editor: Visual Studio Code/Atom/Vim/Sublime/Emacs
  • Metals version: v0.10.7

Search terms

Inline. Error Highlight. Underline. Mouse Hover.

@kpodsiad
Copy link
Member

Thanks for reporting!
At first glance, it looks like the build server error. AFAIK, Metals only forwards diagnostics which they got from it. I checked and for both sbt and bloop, those diagnostics differ depending on the scala version.
For 3.0.0 I got:
Screenshot 2021-10-19 at 09 19 16
and for 3.0.1:
Screenshot 2021-10-19 at 09 19 36

I have to dig deeper into the codebase to prove my hypothesis and see what diagnostics we're getting from the build server, and what we're doing with them. Will inform about the results!

@dos65 dos65 added the upstream-fix-needed Waiting on a fix upstream label Oct 19, 2021
@dos65
Copy link
Member

dos65 commented Oct 19, 2021

I think it should be related to this - scala/scala3#12425 that was a fix for this issue #2769

Originally there was a problem that diagnostic from inline error was received for a wrong file.
We need to look if we can unline such diagnostic messages into several ones instead of into single last one.

@deusaquilus
Copy link
Author

Thanks for the feedback. If there’s something that you’d like me to test please let me know.

@deusaquilus
Copy link
Author

Curiously, is scala/scala3#12425 still a blocker for this issue? Seems like it was merged in May.
Is there still an upstream fix needed?

@dos65
Copy link
Member

dos65 commented Nov 2, 2021

@deusaquilus yep it was merged and probably that was a reason of this regression.

@deusaquilus
Copy link
Author

What does that mean? Does it mean there needs to be a new fix for something on epfl/dotty?

@dos65
Copy link
Member

dos65 commented Nov 3, 2021

@deusaquilus

Does it mean there needs to be a new fix for something on epfl/dotty?

Yes, it should fixed there.

@deusaquilus
Copy link
Author

What is the upstream fix that is needed?

@DamianReeves
Copy link

Being affected by this as well. Would be great to get it fixed.

@deusaquilus
Copy link
Author

Hey guys, sorry to pester. What is the upstream fix that is needed?

@dos65
Copy link
Member

dos65 commented Nov 17, 2021

@deusaquilus I'm going to look at it today.

@deusaquilus
Copy link
Author

Hi @dos65. Now with the release of 3.1.3 it looks like they enhanced the information provided with inline information:
https://scala-lang.org/blog/2022/06/21/scala-3.1.3-released.html#better-error-reporting-in-inlined-code

Is does this provided the feature set that you need for this kind of functionality to be implemented in Metals?

@dos65
Copy link
Member

dos65 commented Jun 21, 2022

@deusaquilus it doesn't. It' requires more changes in a bunch of interfaces.

I've seen that @ckipp01 started working on that - https://contributors.scala-lang.org/t/revisiting-dotty-diagnostics-for-tooling/5649/17

@deusaquilus
Copy link
Author

Interesting. Does metals require SBT changes to Problem.java in order to move forward?

@tanishiking
Copy link
Member

@deusaquilus Yes, and it's already done by @ckipp01 🎉 here sbt/sbt@f90b09f

@deusaquilus
Copy link
Author

Looks like the SBT changes are done. Thanks @ckipp01!!
Just curious, how does this affect Metals?

@ckipp01
Copy link
Member

ckipp01 commented Jun 23, 2022

Looks like the SBT changes are done. Thanks @ckipp01!! Just curious, how does this affect Metals?

So the idea here is that the inlined positions can be listed as DiagnosticRelatedInformation. So the actual error message would be at the specific error site, and then all of the inlined positions would be linked so that a user could click and jump right to them from the actual diagnostic. You can see a POC of this here.

Right now the compiler just spits out a giant ball of text that has the error, the inlined positions, etc all just in a string. So in Metals and really any tooling utilizing that, they have no option but to somehow parse that info out of the string to do anything meaningful with it. An alternative would be what I think they had very early on where every position had it's own diagnostic, but they weren't related in any way.

@deusaquilus
Copy link
Author

An alternative would be what I think they had very early on where every position had it's own diagnostic, but they weren't related in any way.

That totally makes sense and is in line with what I saw in early Dotty versions. Thanks for the explanation! I can’t wait for Metals and other build tools to take advantage of DiagnosticRelatedInformation!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream-fix-needed Waiting on a fix upstream
Projects
None yet
Development

No branches or pull requests

6 participants