Forward actions in bridge's DelegatingReporter for Scala 2

[lrytz]

The Reporter in Scala 2.13.12 will have a new doReport method with actions: List[CodeAction] that should be handled by the compiler bridge.

The change is small in principle thanks to earlier work by @eed3si9n: https://github.com/sbt/zinc/compare/develop...lrytz:zinc:codeActions?expand=1

The difficulty is how to distribute it. suggestions by @eed3si9n (scala/scala#10406 (comment))

• split the compiler bridge for 2.13.12 onwards, or

Requires adjustments in the build tools, which handle bridge compilation

https://github.com/sbt/sbt/blob/4b0b929838c7525cbacd790c2111997d80a5c72e/zinc-lm-integration/src/main/scala/sbt/internal/inc/ZincLmUtil.scala#L85-L100

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

Not sure how this would be handled - where would the file go? How would it be pulled in, would this require changes in the build tool?

• 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)

Seems not possible, we need to override a different method. The new source code doesn't compile against old compilers.

[lrytz]

Scala 2.13.12-bin-286f8d7 with the changed reporter is on https://scala-ci.typesafe.com/artifactory/scala-integration

[eed3si9n]

Yea. I saw that last night, which is helpful to get this going.

[eed3si9n]

Not sure how this would be handled - where would the file go? How would it be pulled in, would this require changes in the build tool?

I haven't figured out the exact details yet, but there's the code that does unzipping of the source-jar and invokes on-the-fly compilation of the compiler bridge. My thinking is basically add a line somewhere after unzipping to rewrite the reporter source file if Scala version matches some range.

[eed3si9n]

Here's my PR part 1 for this - #1215

[lrytz]

Thank you @eed3si9n!

So IIUC, this would create a "bridge" artifact to be released with zinc, and build tools should pick that up if the compiler is 2.13.12+, right?

Since that it requires changes in build tools, maybe it's the right moment to re-consder moving the bridge to the compiler.

There's no obvious reason why scala/scala#8531 didn't make it. I believe there was just no good reason to change the running system. But now we are at this point anyway.

[eed3si9n]

copy-pating #1215 (comment) for the record:

Currently compiler bridge serves all of 2.13.x series using https://repo1.maven.org/maven2/org/scala-sbt/compiler-bridge_2.13/1.9.2/, so the src/main would be have the default implementation that works with 2.13.11, and I was gonna implement the source swapping in ZincComponentCompiler - https://github.com/sbt/sbt/blob/1.9.x/zinc-lm-integration/src/main/scala/sbt/internal/inc/ZincComponentCompiler.scala

I have not kept track of how all Zinc consumers are picking up the compiler bridge, but generally with #1215 everything should continue to work as-is without build tools making changes. We can try to advertise this feature, and build tools not using ZincComponentCompiler.scala can copy-paste the overwrite logic.

Since that it requires changes in build tools, maybe it's the right moment to re-consider moving the bridge to the compiler.

I guess historically Scala compiler maintainers thought of the compiler bridge to be gross breach of privacy and adriaanm and others have advocated for in-sourcing it, while Zinc maintainers thought of it as the implementation details of Zinc itself, and wanted to retain freedom to change. A variant was @dwijnand I think objected also on the ground of to avoiding multi-repo update when changing Zinc. I've been 🤷 on this. Scala 3 has already taken the plunge on in-sourcing, so it's a proof that the model works.

[tgodzik]

I have not kept track of how all Zinc consumers are picking up the compiler bridge, but generally with #1215 everything should continue to work as-is without build tools making changes. We can try to advertise this feature, and build tools not using ZincComponentCompiler.scala can copy-paste the overwrite logic.

Looks like Bloop is one of tthe ones that use a custom component compiler, not really sure why. Will try to replace it with default. The only thing added seems to be Hydra, which isn't open source, so I am not super keen to keep that in the codebase if that means additional cost for us.

[lrytz]

Thanks for the clarifications.

In case we move the bridge to the compiler, build tools that are not updated also will continue to work by still compiling the bridge against the new compiler.

However, it would be a breaking change if evenutally a zinc is released without a corresponding bridge artifact. So we might be living with two copies of the bridge source code for a long time, which is not great.

[dwijnand]

A variant was @dwijnand I think objected also on the ground of to avoiding multi-repo update when changing Zinc.

Don't remember that, but I can believe that was true. I'm 🤷 on this now.

[lrytz]

I'm still going back and forth, there's pros and cons. Will take another look and let you know. In any case I don't feel strongly, your approach is certainly fine. I'm mostly trying to find out if in-sourcing the bridge would open the door for future benefits.

[lrytz]

I worked on moving the bridge sources to scala/scala and it's looking good, the binary bridge is working. Branch: https://github.com/scala/scala/compare/2.13.x...lrytz:scala:sbt-bridge-2023?expand=1. Running publishLocal on that produces a new scala2-sbt-bridge artifact.

There's another advantage to that approach (vs continuing with the bridge distributed as source code with zinc) that I didn't realize before: users can get quickfixes working with existing versions of sbt by using scalaCompilerBridgeBinaryJar. Example:

ThisBuild / scalaVersion := "2.13.12-bin-90aeb5c-SNAPSHOT"

scalacOptions ++= List("-deprecation")

scalaCompilerBridgeBinaryJar := {
  val b = (nix / Compile / dependencyClasspath).value.filter(_.data.getName == "scala2-sbt-bridge.jar").headOption
  b.map(_.data)
}

val nix = project.in(file("nix")).settings(
  autoScalaLibrary := false,
  libraryDependencies += "org.scala-lang" % "scala2-sbt-bridge" % scalaVersion.value
)

sbt:sbtproj> run
[info] compiling 1 Scala source to /Users/luc/code/tmp/sbtproj/target/scala-2.13/classes ...
[warn] /Users/luc/code/tmp/sbtproj/src/main/scala/Test.scala:6:9: procedure syntax is deprecated: instead, add `: Unit =` to explicitly declare `f`'s return type [ with quick fix ]
[warn]   def f { println() }
[warn]         ^
[warn] one warning found
[info] running Test
hi

See the [ with quick fix ] above, which I injected into the message in the bridge's DelegatingReporter in case actions are not empty.

@ckipp01 to test it in VS Code, would I need some nightly version? Or make sure to not use bloop but sbt's bsp server?

[tgodzik]

Interesting, I don't see any code actions at all, but the diagnostics are being sent multiple times. Do you have a full build.sbt ? MAybe I am doing something wrong?

[lrytz]

I don't see any code actions at all

Did you enable the binary sbt bridge (scalaCompilerBridgeBinaryJar)?

I managed to track it down it down

• the first build/publishDiagnostics is sent when the problem is reported through the DelegatingReporter
• the second one is sent when compilation is finished, in sbt's sendSuccessReport. Here the Problems are taken from the CompileAnalysis. This data structure is also populated through the compiler interface, but here the actions are discarded.

@eed3si9n it seems we need to add the actions parameter to xsbti.AnalysisCallback.problem. Can we do that with a new overload in a binary compatbile manner?

[lrytz]

As a test, I removed the AnalysisCallback.problem calls in the compiler bridge and wohoooo!! first quick fix from Scala 2 applied 🎉 The message doesn't go away after fixing it and saving, that might be due to the removed AnalysisCallback call - if I put it back, stale messages are cleared (but quick fixes don't work, as described just above).

[tgodzik]

I managed to get it working with scalacenter/bloop#2122 and the bridge fix which is in https://github.com/tgodzik/bloop/tree/scala2-sbt-bridge

I opened a PR for the first one and will work on the second one once we are closer to the release.

[eed3si9n]

Thanks for the detective work. I've split sbt stuff into #1225