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

Init Scala3 compiler bridge #947

Closed
wants to merge 5 commits into from
Closed

Conversation

adpi2
Copy link
Member

@adpi2 adpi2 commented Nov 26, 2020

First step of sbt/sbt#6080.

Minimal scala3-compiler-bridge

The scala3-compiler-bridge lives in its own module because it shares almost no sources with the Scala 2 compiler-bridge.
It is compiled by Scala 3, using sbt-dotty, because it depends on the Scala 3 compiler which is compiled in Scala 3.
As soon as sbt-dotty is merged into sbt, the scala3-compiler-bridge will be able to compiles itself.

This first minimal implementation is a direct translation of sbt-bridge in https://github.com/lampepfl/dotty:

One minor difference is that it implements the new xsbti.compile.CompilerInterface2.

Add support for VirtualFile in Scala 3

This commit add support for VirtualFiles, that has recently been added to Zinc, to the scala3-compiler-bridge.
It fixes a bug that was reported in sbt/sbt#6007.

Also it fixes a bug in the returned CompileAnalysis by populating the SourceInfos.
The reported warnings were not returned back in sbt-dotty after a succefull compilation.

Improve DualLoader for Scala 3

Change the implementation of DualLoader to fix the LinkageError exception that occurs in the Scala 3 compiler when the xsbti.* classes are loaded.
The problem is that those classes are loaded by two different ClassLoaders: the sbt loader and the scala instance loader.

In the fixed implementation, the compiler classes are loaded by the DualLoader itself, which extends URLClassLoader, so that the subsequent xsbti.* classes can be loaded by the underlying dual loader which is the sbt loader.

See https://github.com/lampepfl/dotty/blob/master/sbt-bridge/src/xsbt/CompilerClassLoader.java for more details.

The `scala3-compiler-bridge` lives in its own module because it shares
almost no sources with the Scala 2 `compiler-bridge`. It is compiled by
Scala 3, using `sbt-dotty`, because it depends on the Scala 3 compiler,
which is compiled in Scala 3. As soon as `sbt-dotty` is merged into
sbt, the `scala3-compiler-bridge` should be able to compiles itself.

This first minimal implementation is a direct translation of
`sbt-bridge` in `https://github.com/lampepfl/dotty`:
- https://github.com/lampepfl/dotty/blob/master/sbt-bridge/src/xsbt/CompilerInterface.java
- https://github.com/lampepfl/dotty/blob/master/sbt-bridge/src/xsbt/CachedCompilerImpl.java

One minor difference is that it implements the new
`xsbti.compile.CompilerInterface2`.
This commit add support for `VirtualFiles`, that has recently been added
to Zinc, to the `scala3-compiler-bridge`. It fixes a bug that was
reported in sbt/sbt#6007.

Also it fixes a bug in the bug in the returned `CompileAnalysis` by
populating the `SourceInfos`. The reported warnings were not returned
back in `sbt-dotty` after a succefull compilation.
Change the implementation of `DualLoader` to fix the `LinkageError`
exception that occurs in the Scala 3 compiler when the `xsbti.*` classes
are loaded. The problem is that those classes are loaded by two
different `ClassLoaders`: the sbt loader and the scala instance loader.

In the fixed implementation, the compiler classes are loaded by
the `DualLoader` itself, which extends `URLClassLoader`, so that the
subsequent `xsbti.*` classes can be loaded by the underlying `dual`
loader which is the sbt loader.

See https://github.com/lampepfl/dotty/blob/master/sbt-bridge/src/xsbt/CompilerClassLoader.java
for more details.
@adpi2
Copy link
Member Author

adpi2 commented Nov 26, 2020

I changed the constructor of DualLoader. So It can possibly break the downstream projects that depend on it.

Yet, since DualLoader is in the internal package, these breakages are expected to be rare. There are at least two of them:

  • one in sbt that can easily be fixed by merging Use new DualLoader implementation sbt#6172
  • the other one is in sbt-dotty here. This breakage is actually unavoidable, but it can easily be fixed by deactivating the code on future sbt 1.5.0.

I found no usage of DualLoader in Bloop.

@adpi2
Copy link
Member Author

adpi2 commented Nov 26, 2020

As intended, Mima complains about the hierachy and constructor of DualLoader:

 Error:   * the type hierarchy of class sbt.internal.inc.classpath.DualLoader is different in current version. Missing types {java.lang.ClassLoader}
Error:     filter with: ProblemFilters.exclude[MissingTypesProblem]("sbt.internal.inc.classpath.DualLoader")
Error:   * method this(java.lang.ClassLoader,scala.Function1,scala.Function1,java.lang.ClassLoader,scala.Function1,scala.Function1)Unit in class sbt.internal.inc.classpath.DualLoader does not have a correspondent in current version
Error:     filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("sbt.internal.inc.classpath.DualLoader.this")
Error:   * method this(java.lang.ClassLoader,scala.Function1,java.lang.ClassLoader,scala.Function1)Unit in class sbt.internal.inc.classpath.DualLoader's type is different in current version, where it is (Array[java.net.URL],java.lang.ClassLoader,scala.Function1,scala.Function1)Unit instead of (java.lang.ClassLoader,scala.Function1,java.lang.ClassLoader,scala.Function1)Unit
Error:     filter with: ProblemFilters.exclude[IncompatibleMethTypeProblem]("sbt.internal.inc.classpath.DualLoader.this")

Also it complains that DifferentLoaders and NullLoader have disapperead:

Error:   * class sbt.internal.inc.classpath.DifferentLoaders does not have a correspondent in current version
Error:     filter with: ProblemFilters.exclude[MissingClassProblem]("sbt.internal.inc.classpath.DifferentLoaders")
Error:   * class sbt.internal.inc.classpath.NullLoader does not have a correspondent in current version
Error:     filter with: ProblemFilters.exclude[MissingClassProblem]("sbt.internal.inc.classpath.NullLoader")

That's because I found no usage in Zinc nor sbt nor Bloop and so I decided to remove them.
We can add them back later if it causes trouble in other downstream projects.

@adpi2 adpi2 changed the title Scala3 compiler bridge Init Scala3 compiler bridge Nov 26, 2020
@adpi2
Copy link
Member Author

adpi2 commented Nov 26, 2020

This scala3-compiler-bridge can be tried out in sbt by:

  • publishing Zinc and scala3-compiler-bridge locally: publishLocal; scala3CompilerBridge/publishLocal
  • fetching and publishing locally this version of sbt
  • using sbt-dotty to have a correct ScalaInstance
  • overriding scalaCompilerBridgeBinaryJar := None to bypass the Dotty sbt-bridge
// project/build.properties
sbt.version=1.5.0-SNAPSHOT
// project/plugins.sbt
addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % "0.4.6")
// build.sbt
lazy val root = project
  .in(file("."))
  .settings(
    scalaVersion := "3.0.0-M1",
    scalaCompilerBridgeBinaryJar := None
  )

@adpi2
Copy link
Member Author

adpi2 commented Nov 26, 2020

One thing is that the current implementation does not handle suspendedUnits as the original Driver does here: https://github.com/lampepfl/dotty/blob/2f26fad3af54df76d5a898629d8d4f589c8083ce/compiler/src/dotty/tools/dotc/Driver.scala#L35-L49.

I intend to implement that very soon.

Copy link
Contributor

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before discussing the details of the PR (I haven't looked closely but it all seems to make sense), I'd like to talk about moving the compiler bridge from the Dotty repo to this repo, because I don't think moving the bridge is a good idea:

  • The compiler bridge uses internal Dotty compiler APIs, these APIs are not stable and we reserve the right to change them at anytime even in point releases, which means breaking new Scala 3 releases on existing sbt versions.
  • If we do change the API, we run into complicated bootstrapping issues: Scala 3 builds itself using sbt, but if sbt uses an incompatible bridge, we're stuck.
  • By contrast, the zinc APIs used in the compiler bridge are pure Java APIs, and an effort has been made recently to make it possible to evolve them without breaking existing users of these APIs (Reluctantly restore compatibility of xsbti.* #829, Make xsbt.CompilerInterface class name configurable #872), the exposed API is also much smaller than the entire Scala 3 compiler jar, so the effort to keep it stable should be much smaller.

So I'm in favor of maintaining the status quo here. I think the ideal solution would be to expand our pure Java APIs in scala3-interfaces to cover everything that the bridge needs to communicate with the compiler, and then it won't matter so much where we put the bridge, but until that happens I would like the bridge to stay next to the compiler.

I also see that your PR rewrote the bridge in Scala: we explicitly rewrote it in Java back in scala/scala3#5596 to avoid complicated bootstrapping issues (you need the compiler to compile the bridge, but to compile the compiler you need the bridge ...), I'd like to keep it that way if possible since it makes everything simpler in my opinion.

@smarter
Copy link
Contributor

smarter commented Nov 26, 2020

By the way, it looks like sbt/sbt#6080 already mentioned "Merge sbt-bridge in Zinc." as the proposed implementation. I didn't notice this when I commented on that issue and thus didn't raise any objection to it at the time, sorry for that.

@adpi2
Copy link
Member Author

adpi2 commented Nov 26, 2020

Thank you @smarter for telling this now, it's never too late... :)

So basically you want the compiler-bridge to be tied to the exact compiler version: sbt-bridge-3.0.0-M1, sbt-bridge-3.0.0-M2 and even for nightly versions. And of course we can implement that logic in sbt here so that we don't need sbt-dotty on future sbt versions.

It also means that the Zinc interface must be stable since the scala 3 compiler-bridge does not depend on the Zinc version anymore.

If we do change the API, we run into complicated bootstrapping issues: Scala 3 builds itself using sbt, but if sbt uses an incompatible bridge, we're stuck.

You can still maintain your own bridge in dotty for the sole purpose of bootstrapping. But I guess it is better to have one bridge for all purposes so that the Scala 3 and Zinc API converge.

By contrast, the zinc APIs used in the compiler bridge are pure Java APIs, and an effort has been made recently to make it possible to evolve them without breaking existing users of these APIs (#829, #872), the exposed API is also much smaller than the entire Scala 3 compiler jar, so the effort to keep it stable should be much smaller.

Right but the old CompilerInterface is already not precise enough for Zinc to work properly. Not only some new features (remote cache) are not supported but also some irreversible bugs where inserted, fixed for CompilerInterface2 but not for CompilerInterface.

Here the proposed implementation does extend the new CompilerInterface2 and fixes the aforementioned bugs. I would very much like to see those changes in the current sbt-bridge. But how far are we to sbt 1.4 in dotty?
(for compatibility with sbt 1.3 we can maintain both implementations but I doubt it is necessary)

I also see that your PR rewrote the bridge in Scala: we explicitly rewrote it in Java back in scala/scala3#5596 to avoid complicated bootstrapping issues

So you expect the binary compatibility between two consecutive versions of Scala 3 to be broken?

I propose the following:

  • We upgrade dotty to sbt 1.4 (@smarter how hard would this be?)
  • I translate this implementation of CompilerInterface2 to Java and I move it to dotty. Part of the implementation could go to dotty internals actually, it would lighten the bridge and clarify the API. (we can maintain the old implementation if necessary which is not buggy in sbt 1.3)
  • The fix in DualLoader can be merged into Zinc so that we don't need the ugly CompilerClassLoader for future versions of sbt.

@smarter
Copy link
Contributor

smarter commented Nov 26, 2020

But how far are we to sbt 1.4 in dotty?

Just merged: scala/scala3#10498 :)

(for compatibility with sbt 1.3 we can maintain both implementations but I doubt it is necessary)

I'm fine with dropping support for sbt 1.3.

So you expect the binary compatibility between two consecutive versions of Scala 3 to be broken?

Not now, but eventually with 3.T/4 yes: scala/scala3#10244, having the bridge compiled once and pushed to maven also means sbt doesn't have to compile it with each new compiler version, which means no complicated cache handling (I know mill at least struggled quite a bit to cache the bridge correctly)

I propose the following: [...]

Sounds good to me! Though the changes to DualLoader need to be carefully inspected since ClassLoader are always tricky business and the way they are cached in sbt/zinc is also quite intricate (and if classloader caching doesn't work right, it might still work but slow things done as the JIT ends up recompiling the same classes again and again, it helps to run the JVM with -verbose:class to verify that multiple calls to compile for example do not end up reloading the same compiler classes multiple times.)

@adpi2 adpi2 marked this pull request as draft November 26, 2020 19:38
@adpi2
Copy link
Member Author

adpi2 commented Nov 27, 2020

There is still a problem actually which is that implementing CompilerInterface2 in dotty will break compatibility of future Scala 3 versions with sbt from 1.4.0 to 1.4.4 because of the current DualLoader.

So the first thing I'll do is to fix the DualLoader for sbt 1.4.5. Then I can implement CompilerInterface2 and try to find a way to deactivate it on sbt version < 1.4.5.

@adpi2 adpi2 closed this Nov 27, 2020
@eatkins
Copy link
Contributor

eatkins commented Nov 27, 2020

I think the goal is to release a 1.5.0 that is fully scala 3 compatible before the scala 3 release. It doesn't necessarily seem worth a lot of effort to get things working with all versions of 1.4.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants