Implement quickfixes, aka actionable diagnostics (via CodeAction)

[eed3si9n]

Ref https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172
Fixes scala/scala-dev#844

This implements new reporter methods that take code actions as a parameter.
The purpose is to pass these along to Zinc and BSP so that the compiler can suggest code edits to the editors in a structural manner.

To show case this feature, I've added CodeActions to the following deprecations:

• Deprecation of procedure syntax
• Deprecation of empty-paren method auto-application
• Deprecation of val in for-comprehension
• Deprecation of paren-less lambda parameter

Note

Here's the flow of information

┌──────┐doReport┌────────┐
│scalac├───────►│compiler│
└──────┘        │bridge  │
                └──┬─────┘
                   │  xsbti.Problem
┌──────────┐     ┌─▼──┐
│BSP server│◄────┤Zinc│
└──┬───────┘     └────┘
   │  Diagnostic
┌──▼───────────────┐
│BSP client        │
│(Metals, IntelliJ)│
└──┬───────────────┘
   │  Diagnostic
┌──▼───────┐
│LSP Client│
└──────────┘

[lrytz]

This is looking very good, thank you!

I wonder if we can already push a bit harder to the new API, for example,

• make info0 @deprecatedInheritance
• use a default parameter in the internal classes (e.g., in PerRunReporting, FilteringReporter.doReport)
• Change StoreReporter.Info to add an actions field

Why do we need CodeAction in scala.reflect.api?

I guess the boilerplate in CodeAction is about the possibility for binary compatible evolution? Do you have a situation in mind where this could be useful?

[eed3si9n]

Why do we need CodeAction in scala.reflect.api?

Initially I thought CodeAction might leak out like position would, but now that you mention it, I can probably delete the scala.reflect.api traits.

I guess the boilerplate in CodeAction is about the possibility for binary compatible evolution? Do you have a situation in mind where this could be useful?

This was mostly copied from similar code I added in sbt, but I guess I can use normal case classes within scalac.

I wonder if we can already push a bit harder to the new API, for example, ...

I'll look into those.

[lrytz]

We're more than happy to help out, let me know.

[eed3si9n]

We're more than happy to help out, let me know.

Feel free to add commits on top (or a new PR). I am mostly interested in getting code actions out, and I trust your judgement on how implementation can be improved. I'll comment here before working on something to avoid duplication.

[eed3si9n]

I'll start with the removal of api traits, and converting to case classes.

[eed3si9n]

ok. Made a few more changes per suggestion.

[som-snytt]

I can't wait for -Xreporter RewriteReporter perhaps enabled under -rewrite.

[lrytz]

I think this should be about it for now, a List[CodeAction] paramter is available for all reporting methods in the compiler except

• Typer errors, these go through some abstractions: methods in ContextErrors, which build AbsTypeError subclasses. The actions field can be added to these subclasses as needed, they are internal anyway. But the core is there (AbsTypeError has an accessor for actions).
• Scanner errors / warnings, it seemed there are no cases there where we could provide a quick fix. Scanner deprecations have an actions parameter (unicode ⇒).

I did a basic sbt test by publishLocal and running sbt -Dstarr.version=....

[lrytz]

Now we should figure out if it breaks any tools or macros. Ping @adpi2 @tgodzik @ckipp01 @smarter @sjrd @SethTisue for some help coming up with projects to test. Maybe testing is easier after merging the PR when we have an intergration build.

[tgodzik]

I think we should be good in the projects that we use the compiler directly. Though to be 100% sure I would need to try and compile the affected projects.

[tgodzik]

I see that this is kept, so we should be asafe, I think doReport is used in scalameta/scalameta and scalameta/mdoc. Possibly also scala-cli?

[lrytz]

Calling the method is source compatible, but extending FilteringReporter is not. The abstract method used to be

  def doReport(pos: Position, msg: String, severity: Severity): Unit

and now it's

  @deprecatedOverriding("override the `doReport` overload instead", "2.13.12")
  @deprecated("use the `doReport` overload instead", "2.13.12")
  def doReport(pos: Position, msg: String, severity: Severity): Unit = doReport(pos, msg, severity, Nil)

  override def doReport(pos: Position, msg: String, severity: Severity, actions: List[CodeAction]): Unit

[tgodzik]

That will be a problem for mdoc then since we don't publish for each Scala version :/ This of course causes us problems, but on the other hand we can support all the Scala versions without separate publishing

Any idea how we should work around that? I can take a look at it after Scala Days, but I am bit terrified as I don't fully remember why we wrote it this way. I guess some of the classes were not available in the older versions and I had to work around it this way.

[lrytz]

OK, so ideally an mdoc release should be binary compatible with all Scala 2.13 versions, i.e.,

• existing ones should continue to work with new Scala releases, and
• new mdoc releases built with a new Scala version should work with older Scala versions?

[tgodzik]

That's the idea, but it's based on an assumption that the Report API wouldn't change too much 😓 which is clearly a wrong assumption.

[lrytz]

I pushed a small change that should restore binary compatibility (both ways) with existing FilteringReporter subclasses. I tested it with the following:

import scala.tools.nsc.Settings
import scala.reflect.internal.util.Position
// import scala.reflect.internal.util.CodeAction
import scala.reflect.internal.Reporter.Severity
import scala.annotation.nowarn
import scala.tools.nsc.reporters.FilteringReporter

trait MyRepT extends FilteringReporter { self: MyRep =>
  @nowarn("cat=deprecation")
  override def doReport(pos: Position, msg: String, severity: Severity): Unit =
    add(pos, msg, severity)

  // override def doReport(pos: Position, msg: String, severity: Severity, actions: List[CodeAction]): Unit =
    // add(pos, msg, severity)
}

class MyRep(val settings: Settings) extends MyRepT {
  def add(pos: Position, msg: String, severity: Severity): Unit =
    println(s"[myrep] $msg")
}

Compiling this class with 2.13.10, I can use the reporter with both 2.13.10 and this PR

➜ sandbox java -cp $(cs fetch -p org.scala-lang:scala-compiler:2.13.10):. scala.tools.nsc.Main -usejavacp -Xreporter:MyRep A.scala
[myrep] type mismatch;
 found   : String("")
 required: Int

➜ sandbox qsc -toolcp . -Xreporter:MyRep A.scala
[myrep] type mismatch;
 found   : String("")
 required: Int

I also compiled the class with this PR's compiler and it works with 2.13.10, with or without un-commenting the doReport overload.

[tgodzik]

Thanks! I think that should work for us! I can confirm 100% when there is a nightly version released

[lrytz]

This is getting ready I think.

@ckipp01 could you take a look at CodeAction.scala in this PR to see if it looks right to you?

• description: Option[String] doesn't seem to exist in LSP (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeAction)
• is it fine to use the compiler's Position in TextEdit?

One idea: instead of the actions: List[CodeAction] parameter, we could use data: Map[String, Any] and put the List[CodeAction] in there as Map("codeActions" -> List(CodeAction(...))). @eed3si9n wdyt?

[ckipp01]

This is getting ready I think.

@ckipp01 could you take a look at CodeAction.scala in this PR to see if it looks right to you?

Yup, just took a look and everything looks good! I did try to get it to work with the changes I made in Metals here, but was having a lot of issues getting it to work since I kept getting issues with Metals not being able to get semanticdb for the self-published version of Scala, even though I kept trying to turn of semanticDB in sbt. Either way, the shape that you have oulined in here seems to match what @eed3si9n made in BSP here and what I am using in Dotty here, so we should be good.

• description: Option[String] doesn't seem to exist in LSP (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeAction)

Yup, this is fine. The code action that we made here differs a bit from the LSP one. I added in the description here just in case there was something we could use it for in the future. For now I don't really know what it would be used for, but I could imagine that in a UI somewhere it could be helpful. Basically we take this and translate it into an LSP code action in Metals and other clients using LSP, and then IntelliJ can also just take this and translate it into their "action" format.

• is it fine to use the compiler's Position in TextEdit?

Yup, also fine. These Positions will get turned into a Range at least for us in the LSP world. We do the same with the Diagnostic going from a Position to Range.

One idea: instead of the actions: List[CodeAction] parameter, we could use data: Map[String, Any] and put the List[CodeAction] in there as Map("codeActions" -> List(CodeAction(...))). @eed3si9n wdyt?

Actually it started out as a Map originally but @eed3si9n had a comment here about it:

We should avoid Map since this could introduce non-determinism when persisted into JSON, and Problem is a good candidate for caching.

[lrytz]

Thanks for taking a look!

I don't mean to put the Map in the CodeAction structure.

My proposal is to use a Map in the Scala 2 compiler's Reporter methods. This would allow passing other data from the compiler to downstream tools in the future without such a big refactoring as this PR.

The compiler interface (which lives in the zinc repo for Scala 2, not in the compiler repo) would pull out the CodeActions and translate them into the zinc Problem representation (IIRC).

[eed3si9n]

To clarify this discussion I made a diagram:

┌──────┐doReport┌────────┐
│scalac├───────►│compiler│
└──────┘        │bridge  │
                └──┬─────┘
                   │  xsbti.Problem
┌──────────┐     ┌─▼──┐
│BSP server│◄────┤Zinc│
└──┬───────┘     └────┘
   │  Diagnostic
┌──▼───────────────┐
│BSP client        │
│(Metals, IntelliJ)│
└──┬───────────────┘
   │  Diagnostic
┌──▼───────┐
│LSP Client│
└──────────┘

[lrytz]

I think this is ready now. @eed3si9n would you take another look?

There is still the idea to change the actions parameter from

• actions: List[CodeAction] to
• data: Map[String, Any] with Map("codeActions" -> List(CodeAction(...)))
  but I have any concrete use case for it in mind, so it's probably better to not over-generalize.

The next step will be to adapt the compiler bridge. @eed3si9n as far as I can tell, bridge sources in zinc are by compiler major version. The current bridge sources work with the 2.13 compiler after this PR, but with the current model we'd also need to make sure the adapted bridge that forwards code actions works with older compilers. Do you already have ideas how this could be adressed?

[eed3si9n]

as far as I can tell, bridge sources in zinc are by compiler major version. The current bridge sources work with the 2.13 compiler after this PR, but with the current model we'd also need to make sure the adapted bridge that forwards code actions works with older compilers. Do you already have ideas how this could be addressed?

We could potentially:

• split the compiler bridge for 2.13.12 onwards, or
• keep the existing bridge for 2.13.x, but just ship an override DelegatingReporter.scala in Zinc itself as resource, or
• keep the existing bridge for 2.13.x, and detect 2.13.12+ in the code and split the call to structural type (reflection per compiler warning, but hopefully not too bad)

[eed3si9n]

would you take another look?

LGTM as far as the PR goes. Like tgodzik said in the comment above, we'd find out if all the pieces snap once nightly comes out.

[lrytz]

• split the compiler bridge for 2.13.12 onwards

I think I'd prefer that, it needs some work to set up. Would the dispatching be done in AnalyzingCompiler.scala?

How are the bridge sources distributed?

We could still consider moving the bridge into scala/scala, but that's beyond scope right now.

• keep the existing bridge for 2.13.x, but just ship an override DelegatingReporter.scala in Zinc itself as resource, or

If that's easier - I'd need your help

• keep the existing bridge for 2.13.x, and detect 2.13.12+ in the code and split the call to structural type (reflection per compiler warning, but hopefully not too bad)

If we're using MethodHandle the right way (instead of old school reflection), performance should be the same as a compiled method call. Then the changes would be local to the bridge source code. But development / maintenance is more difficult as we don't get help from the compiler about getting things right.

[eed3si9n]

• split the compiler bridge for 2.13.12 onwards

I think I'd prefer that, it needs some work to set up. Would the dispatching be done in AnalyzingCompiler.scala?

How are the bridge sources distributed?

compiler bridge is distributed like a normal artifact on Maven Central (https://repo1.maven.org/maven2/org/scala-sbt/compiler-bridge_2.13/1.9.2/), the downside of approach is that the dispatch is done at the build tool level - https://github.com/sbt/sbt/blob/4b0b929838c7525cbacd790c2111997d80a5c72e/zinc-lm-integration/src/main/scala/sbt/internal/inc/ZincLmUtil.scala#L85-L100. I wouldn't be surprised other build tools assume naming convention.

We could still consider moving the bridge into scala/scala, but that's beyond scope right now.

• keep the existing bridge for 2.13.x, but just ship an override DelegatingReporter.scala in Zinc itself as resource, or

If that's easier - I'd need your help

• keep the existing bridge for 2.13.x, and detect 2.13.12+ in the code and split the call to structural type (reflection per compiler warning, but hopefully not too bad)

If we're using MethodHandle the right way (instead of old school reflection), performance should be the same as a compiled method call. Then the changes would be local to the bridge source code. But development / maintenance is more difficult as we don't get help from the compiler about getting things right.

Yea, either approach works. The compiler bridge is pretty well exercised, so between two options I guess having a single DelegatingReporter.scala is slightly more preferable.

Given that we need to override a method that takes a class non-existent in 2.13.11, supplying a whole DelegatingReporter.scala might be easier actually.

[lrytz]

Moved the discussion here: sbt/zinc#1214

[SethTisue]

in the community build, over at scala/community-build#1685 , this broke scalameta:

[scalameta] [error] /Users/tisue/cb3/target-0.9.20/project-builds/scalameta-bfcdfa42ad4bddde4b08583b10292faad66d430a/scalameta/testkit/jvm/src/test/scala/scala/meta/testkit/ScalacParser.scala:51:20: method incompleteInputError overrides nothing.
[scalameta] [error] Note: the super classes of <$anon: scala.meta.testkit.ScalacParser.global.syntaxAnalyzer.UnitParser> contain the following, non final members named incompleteInputError:
[scalameta] [error] override def incompleteInputError(msg: String, actions: List[scala.reflect.internal.util.CodeAction]): Unit
[scalameta] [error]       override def incompleteInputError(msg: String) = { fail = true }
[scalameta] [error]                    ^

is it remediable here in scala/scala, or do we need to just ask the scalameta folks to adjust?

[SethTisue]

it's just test code (note jvm/src/test), but since scalameta is crossbuilt for Scala minor versions, it's not as simple as just updating the code to match the new signature, since then it will no longer compile against 2.13.11 and earlier

UPDATE: Tomasz says it is that simple, actually 👍