Skip to content

[sbt-dotty] Use sbt loader as parent of scala instance loader #10541

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[sbt-dotty] Use sbt loader as parent of scala instance loader #10541

wants to merge 1 commit into from

Conversation

adpi2
Copy link
Member

@adpi2 adpi2 commented Nov 28, 2020 •
edited
Loading

This PR is an attempt to get rid of the CompilerClassLoader hack in sbt-bridge (see origin of the problem). The underlying purpose being to implement the new Zinc CompilerInterface2 so that we can fix a few bugs in sbt-dotty and benefit from the recent Zinc improvement in Scala 3.

In short the problem is that the ScalaInstance.loader is incompatible with the sbt loader because it loads its own xsbti.* classes.

This PR fix the problem by using the sbt ClassLoader as a parent of the ScalaInstance.loader. But we filter out all the classes that are not binary compatible with the ScalaInstance, among them the scala-library-2.12 and compiler-bridge-2.12 classes.

[test_sbt]

@adpi2 adpi2 mentioned this pull request Nov 28, 2020
Closed
@adpi2 adpi2 force-pushed the sbt-dotty branch 3 times, most recently from d675162 to 101ebf8 Compare November 29, 2020 07:45
@adpi2
Copy link
Member Author

adpi2 commented Nov 30, 2020

The new sbt-dotty works well without the CompilerClassLoader.

However Mill won't work until we apply the same logic inside it: Mill should create a ScalaInstance whose loader is compatible with the Zinc loader.

So, before merging this PR, I will have to:

  • apply the ScalaInstance loader fix in Mill
  • release Mill
  • update the community build projects that rely on Mill

@sjrd and @smarter: Do you confirm this is the right direction?

@adpi2
Copy link
Member Author

adpi2 commented Nov 30, 2020

As a side note, here is the code in Mill that create the ScalaInstance loader:
https://github.com/lihaoyi/mill/blob/aa3240a52a72cfa5fe76c4d4c747f7f61eef2f1d/scalalib/worker/src/ZincWorkerImpl.scala#L309-L321

@smarter
Copy link
Member

smarter commented Nov 30, 2020

This PR fix the problem by using the sbt ClassLoader as a parent of the ScalaInstance.loader.

If this works well enough, could this be done upstream in sbt for Scala 2 too so we don't need a special case just for Scala 3?

@adpi2
Copy link
Member Author

adpi2 commented Nov 30, 2020

@smarter

If this works well enough, could this be done upstream in sbt for Scala 2 too so we don't need a special case just for Scala 3?

Yes absolutely I test it first in sbt-dotty then I will generalize in sbt for all Scala versions.

@smarter
Copy link
Member

smarter commented Nov 30, 2020

OK, I'd be interested in hearing the opinion of the sbt maintainers on this direction.

@adpi2
Copy link
Member Author

adpi2 commented Nov 30, 2020 •
edited
Loading

@eed3si9n @eatkins

My plan is to use a filtered version of appConfiguration.provider.loader as a parent of scalaInstance.loader so that the Scala 3 compiler can load the xsbti.* classes from the same loader than sbt. A similar thing is already done inside sbt to load the jline Terminal provided by sbt in the Scala REPL (code here)

Right now I am testing this in sbt-dotty but my plan is to implement it in sbt by reusing the scalaInstanceTopLoader key. This thing will allow me to get rid of the messy CompilerClassLoader and then implement the Zinc CompilerInterface2 in the scala 3 compiler bridge.

Can you confirm that this is the right direction?

@smarter smarter mentioned this pull request Nov 30, 2020
Closed
@adpi2
Copy link
Member Author

adpi2 commented Nov 30, 2020

I found a similar implementation in Mill that is looking better to me:
https://github.com/lihaoyi/mill/blob/master/main/api/src/mill/api/ClassLoader.scala

In our case, sharedPrefixes would contain "org.jline." and "xsbti.".

I'll give a try.

@eatkins
Copy link

eatkins commented Nov 30, 2020

This approach sounds correct to me without thinking too much about it. It may be helpful to look at the code that constructs the sbt metabuild loader as well: https://github.com/sbt/sbt/blob/develop/main/src/main/java/sbt/internal/MetaBuildLoader.java. It shows how the sbt meta build classloader is constructed. You may end up wanting to make a change there as well.

@adpi2 adpi2 mentioned this pull request Nov 30, 2020
Merged
@adpi2 adpi2 changed the title [sbt-dotty] Use sbt loader as parent of scala instance loader [sbt-dotty] Use sbt loader as parent of scala instance loader [test-sbt] Nov 30, 2020
smarter
smarter reviewed Nov 30, 2020
View reviewed changes
sbt-dotty/src/dotty/tools/sbtplugin/DottyPlugin.scala Outdated
}

private class FilteringClassLoader(parent: ClassLoader) extends ClassLoader(parent) {
private val exludePackages = List(
Copy link
Member

@smarter smarter Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Listing the packages to include rather than the packages to exclude like you did in the first commit seems safer to me (for example, you're not excluding "dotty." which could be loaded by a future version of sbt). However in that commit you had to include all the JDK prefixes ("java.", "sun.", ...). Perhaps this can be avoided by replicating what CompilerClassLoader did before this PR: use null as a parent for FilteringClassLoader so super.loadClass will delegate to the system JDK classloader, and call loadClass on the sbt loader explicitly when needed instead.

@adpi2 adpi2 changed the title [sbt-dotty] Use sbt loader as parent of scala instance loader [test-sbt] [sbt-dotty] Use sbt loader as parent of scala instance loader [test_sbt] Nov 30, 2020
@adpi2 adpi2 force-pushed the sbt-dotty branch 2 times, most recently from b45548e to 3c52fa4 Compare November 30, 2020 21:05
@adpi2 adpi2 changed the title [sbt-dotty] Use sbt loader as parent of scala instance loader [test_sbt] [sbt-dotty] Use sbt loader as parent of scala instance loader Nov 30, 2020
@adpi2
Copy link
Member Author

adpi2 commented Nov 30, 2020

Related PR in Mill: com-lihaoyi/mill#1019

@adpi2 adpi2 self-assigned this Dec 1, 2020
@adpi2 adpi2 mentioned this pull request Dec 2, 2020
Merged
@adpi2 adpi2 closed this Dec 14, 2020
unkarjedy added a commit to JetBrains/intellij-scala that referenced this pull request Jul 16, 2021
@unkarjedy
update scala3-sbt-bridge from 3.0.0-M2 to 3.0.0, fix LinkageError #SC…
9a7fbfb 
…L-18861 fixed

In Scala 3.0.0-RC1 in sbt-bridge they moved to new compiler interface `xsbti.CompilerInterface2`.

Before Scala 3.0.0-RC1 `xsbt.CompilerClassLoader` was used. It resolved `xsbti.*` classes (e.g. xsbti.AnalysisCallback) to the classes from the CompileServer classloader (more specifically, they were taken from `zincRepackaged` aka `incremental-compiler.jar`).
`compiler-interface.jar` is also included into Scala 3 compiler classpath. But it was simply ignored, because the classes were already loaded by the parent classloader.

After Scala 3.0.0-RC1 `sbt.internal.inc.classpath.DualLoader` is used.
(see sbt.internal.inc.AnalyzingCompiler.compile, see also sbt.internal.inc.AnalyzingCompiler.createDualLoader)
`parentA` loader represents the compiler classloader.
Scala3 compiler uses `xsbti.*` classes directly so it so it's loader needs to resolve the those classes. And they should be resolved to the same classes used in the Compile Server.

Before this commit, the classes were resolved to the `compiler-interface` jar file in the compile classpath itself.
This caused the LinkageError.
To avoid those we delegate all `xsbti.*` classes loading to the SCS classlader.

Closely related links:
- [`[sbt-bridge] Upgrade to CompilerInterface2`](scala/scala3@5185d4d)
- [`[sbt-dotty] Use sbt loader as parent of scala instance loader #10541`](scala/scala3#10541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter left review comments

Assignees

@adpi2 adpi2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adpi2 @smarter @eatkins