Skip to content

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Aug 11, 2021

Previously, we would need to compile the Scala compiler bridge in order to use it with a particular Scala version. Now, it's possible to download the jar, which is released together with a particular Scala version.

From other changes, a new compiler is needed together with Scala library, however it still reuses the Scala 2.13 library underneath.

Sample project this works with:

plugins {
    scala
}

repositories {
    mavenCentral()
    mavenLocal()
}

dependencies {
    implementation("org.scala-lang:scala3-library_3:3.0.1")
}

Fixes #16527

Context

Scala 3 has been released in May and a lot of its' potential users use Gradle, which is de facto standard in the JVM world. This is why

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass sanity check: ./gradlew sanityCheck
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

File libraryJar = findFile(ArtifactInfo.ScalaLibraryID, scalaClasspath);
File compilerJar = findFile(ArtifactInfo.ScalaCompilerID, scalaClasspath);
URL[] libraryUrls = new URL[]{libraryJar.toURI().tourl("")};
Copy link

Choose a reason for hiding this comment

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

This should contain both scala-library and scala3-library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't seem to break anything, but sure I can add it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@dubinsky
Copy link

dubinsky commented Aug 11, 2021

@tgodzik Wow! Thank you so much!!!
My attempt (#18003) ran into 'java.util.ServiceConfigurationError: xsbti.compile.CompilerInterface2: dotty.tools.xsbt.CompilerBridge not a subtype'; your change does some classpath manipulations that probably work around that issue - thanks!

I thought that to gain Scala 3 support, we have to use post-1.5 Zinc (that is why all the Zinc changes in my draft)...

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 11, 2021

@tgodzik Wow! Thank you so much!!!
My attempt (#18003) ran into 'java.util.ServiceConfigurationError: xsbti.compile.CompilerInterface2: dotty.tools.xsbt.CompilerBridge not a subtype'; your change does some classpath manipulations that probably work around that issue - thanks!

I thought that to gain Scala 3 support, we have to use post-1.5 Zinc (that is why all the Zinc changes in my draft)...

We don't actually need that, there is a fallback step currently, which we will want to remove, but not necessarily now. For sure we should update Zinc as the next step. I didn't want to everything in one PR, but I will most likely follow up with that.

Also, sorry I didn't notice your PR!

@dubinsky
Copy link

I thought that to gain Scala 3 support, we have to use post-1.5 Zinc (that is why all the Zinc changes in my draft)...

We don't actually need that, there is a fallback step currently, which we will want to remove, but not necessarily now. For sure we should update Zinc as the next step. I didn't want to everything in one PR, but I will most likely follow up with that.

Excellent! Thank you very much!

Also, sorry I didn't notice your PR!

You couldn't: I published mine after you did yours ;)

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 11, 2021

@dubinsky I see that you actually updated zinc, so maybe we could rebase your PR later on and update to latest zinc this way?

I see that you also spotted some places that might require some investigation (commented with // TODO: support Scala3). I think I need to take a look at ScalaBasePlugin.java and fix it here.

@dubinsky
Copy link

dubinsky commented Aug 11, 2021

@dubinsky I see that you actually updated zinc, so maybe we could rebase your PR later on and update to latest zinc this way?

Sure, I am ready to help in any way. I never used Zinc before, though, so I do not really know what I am doing.
If you want to just use parts of my draft outright - it is fine with me :)

I see that you also spotted some places that might require some investigation (commented with // TODO: support Scala3). I think I need to take a look at ScalaBasePlugin.java and fix it here.

Looking forward to it!

By the way, does IdeaScalaConfigurer need to be made Scala3-aware?
In my draft I centralized the library-finding logic so it could be re-used without code duplication, but as long as Scala 3 is supported by Gradle both on the command line and from IntelliJ Idea, I do not really care how it is done ;)

Thanks for all your work!

@luis-
Copy link

luis- commented Aug 11, 2021

@tgodzik Wow! Thank you so much!!!

My attempt (#18003) ran into 'java.util.ServiceConfigurationError: xsbti.compile.CompilerInterface2: dotty.tools.xsbt.CompilerBridge not a subtype'; your change does some classpath manipulations that probably work around that issue - thanks!

I thought that to gain Scala 3 support, we have to use post-1.5 Zinc (that is why all the Zinc changes in my draft)...

I also thought zinc 1.5 was needed. First time digging into gradle code but over the weekend I managed to swap in all the VirtualFile to uptake zinc 1.5 to add scala 3 support. Thought I was close until I ran into xsbti.* errors and then ended up butchering my working tree trying to figure it out. From looking at this and #18003 its nice to see I wasn't completely off base with my attempt. Good learning experience for me.

@GavinRay97
Copy link

Woohoo! -- huge thank-you to @tgodzik, @dubinsky and @smarter!!

🙏 🎉

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 11, 2021

The main issue wasn't actually zinc version, but wrong jars in the class loader of ScalaInstance. We can still upgrade it for sure, but it might be easier to do it in a separate pr

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 11, 2021

By the way, does IdeScalaConfigurer need to be made Scala3-aware?

Good question, I will check!

In my draft I centralized the library-finding logic so it could be re-used without code duplication, but as long as Scala 3 is supported by Gradle both on the command line and from IntelliJ Idea, I do not really care how it is done ;)

It's probably better to rework it but I wanted to keep it to a minimum, so that it is easier to review. I think We can also do a follow up.

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 12, 2021

IdeScalaConfigurer should work correctly, I added some testing and also tried use it with Intellij, which seems to work alright.

@jastice are you able to confirm that's enough to make Scala 3 work for Gradle with Intellij? Or is there something more we need to do?

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 12, 2021

Also, anyone knows how I can get the PR reviewed?

@dubinsky
Copy link

IdeScalaConfigurer should work correctly, I added some testing and also tried use it with Intellij, which seems to work alright.

Thank you!!!

@kamildoleglo
Copy link

Also, anyone knows how I can get the PR reviewed?

Maybe @eskatos could help

@dubinsky
Copy link

dubinsky commented Aug 13, 2021

@tgodzik some observations when using your branch:

  • compiler error messages sometimes give position of the offending code as character numbers (e.g., <300..312>), not line number and character in the line; I am not sure if this is the result of using version of Zinc that is not precise enough for Scala 3, or if some compiler option affects this, or if this is the new normal - but Idea does not seem equipped to handle this.
  • if I configure Idea to build the project directly instead of using Gradle (File | Settings | Build, Execution, Deployment | Build Tools | Gradle | Build and run using: IntelliJ IDEA), I get:
scalac: Error: xsbti/compile/CompilerInterface2
java.lang.NoClassDefFoundError: xsbti/compile/CompilerInterface2
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
	at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:550)
	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:458)
	at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:452)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:451)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	at sbt.internal.inc.classpath.DualLoader.loadClass(DualLoader.scala:65)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:576)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:398)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(ServiceLoader.java:1210)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1221)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator$1.run(ServiceLoader.java:1268)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator$1.run(ServiceLoader.java:1267)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1270)
	at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1300)
	at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1385)
	at sbt.internal.inc.AnalyzingCompiler.loadService(AnalyzingCompiler.scala:302)
	at sbt.internal.inc.AnalyzingCompiler.compile(AnalyzingCompiler.scala:87)
	at org.jetbrains.jps.incremental.scala.local.IdeaIncrementalCompiler.compile(IdeaIncrementalCompiler.scala:57)
	at org.jetbrains.jps.incremental.scala.local.LocalServer.compile(LocalServer.scala:43)
	at org.jetbrains.jps.incremental.scala.remote.Main$.compileLogic(Main.scala:158)
	at org.jetbrains.jps.incremental.scala.remote.Main$.$anonfun$handleCommand$1(Main.scala:141)
	at org.jetbrains.jps.incremental.scala.remote.Main$.decorated$1(Main.scala:131)
	at org.jetbrains.jps.incremental.scala.remote.Main$.handleCommand(Main.scala:138)
	at org.jetbrains.jps.incremental.scala.remote.Main$.serverLogic(Main.scala:115)
	at org.jetbrains.jps.incremental.scala.remote.Main$.nailMain(Main.scala:71)
	at org.jetbrains.jps.incremental.scala.remote.Main.nailMain(Main.scala)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.martiansoftware.nailgun.NGSession.run(NGSession.java:319)

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 13, 2021

I am getting no issues in Intellij either with Gradle or Intellij building it, are you on the newest version? The support for Scala 3 is still being improved, so best to be on the latest nightly (though I am using the latest release and seems to work fine).

As far as the positions go, I am not sure why it's pointing to characters, we might need a zinc upgrade for that or to change something in the plugin still. I will take a look how we are handling it in other build tools.

@dubinsky
Copy link

I am getting no issues in Intellij either with Gradle or Intellij building it, are you on the newest version?

This is embarrassing: I thought I was, but turns out I was not.
With the latest released Idea and Scala plugin both problems are gone.
Thank you!!!

@dubinsky
Copy link

As far as the positions go...

When Idea is configured to build directly, positions are reported correctly and messages click through to the correct source positions. When Idea builds using Gradle, positions are reported as character numbers...

@dubinsky
Copy link

dubinsky commented Aug 13, 2021

@tgodzik Positions like <1..2..3> and [1..2]

Before logging a Problem, LoggedReporter maps its Position using sourcePositionMapper supplied at the LoggedReporter's creation. Gradle Scala plugin supplies identity transformation (resulting in the positions we see); instead, sourcePositionMapper should be supplied that maps into an implementation of xsbti.Position with the desired toString().

@GavinRay97
Copy link

GavinRay97 commented Aug 14, 2021

There is one tangential issue, just as a heads up for anyone looking to use this with Metals.
I built Tomasz branch, and set up a gradle distribution a New Scala + Gradle project.

buildscript {
    repositories {
        mavenLocal()
        mavenCentral()
    }

    dependencies {
        classpath "ch.epfl.scala:gradle-bloop_2.12:1.4.8-106-8722ebfd"
    }
}

plugins {
    id "java"
    id "scala"
    id "io.quarkus"
}

// Bloop plugin uses old DSL -- MUST be applied like this
apply plugin: "bloop"

repositories {
    mavenCentral()
    mavenLocal()
}

dependencies {
    // scala3-compiler_3 will pull in corresponding scala3-library_3, which will also pull in corresponding scala-library (Scala 2), 2.13.6 in this case
    implementation "org.scala-lang:scala3-compiler_3:3.0.2-RC1"
}

group = "org.example"
version = "1.0.0-SNAPSHOT"

java {
    sourceCompatibility = JavaVersion.VERSION_16
    targetCompatibility = JavaVersion.VERSION_16
}

tasks.withType(ScalaCompile) {
    scalaCompileOptions.additionalParameters = [
        "-feature",
        "-explain",
        "-Ysafe-init",
        "-Xsemanticdb"
    ]
}

compileJava {
    options.encoding = 'UTF-8'
    options.compilerArgs << '-parameters'
}

compileTestJava {
    options.encoding = 'UTF-8'
}

Now the issue is, that this won't work with Metals. It gets detected as a Scala 2.13.6 project, and Scala 3 syntax will error in VS Code 🙁

You can check the .bloop/<project-name>.json file:

    "scala": {
      "organization": "org.scala-lang",
      "name": "scala-compiler",
      "version": "2.13.6",
      "options": [
        "-Xsemanticdb",
        "-Ysafe-init",
        "-deprecation",
        "-explain",
        "-feature",
        "-unchecked"
      ],

And Metals Doctor will show 2.13.6

I fixed this by replacing "version": "2.13.6" with "version": "3.0.2-RC1" in both .bloop/<project-name>.json and .bloop/<project-name>-test.json and then reloading the window.

But I think this might need a fix in the Gradle plugin.

Anyways, hope this is helpful for anyone that tries to test this in Metals and gets confused/runs into the same thing! 🙂

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 14, 2021

@dubinsky good catch, I will look into that!

@GavinRay97 ach yes, I was actually checking that out and haven't yet worked on a fix in the Bloop plugin. I will work on it a bit later.

@Arthurm1
Copy link

@GavinRay97

For now - adding this to your Gradle build after you apply the plugin should work...

bloop {
    stdLibName = "scala3-library_3"
}

@gradle gradle deleted a comment from ljacomet Sep 6, 2021
@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 6, 2021

It works! Thanks a lot for the help @ljacomet ! Should I do something more with the PR?

@gradle gradle deleted a comment from ljacomet Sep 6, 2021
@ljacomet
Copy link
Member

ljacomet commented Sep 6, 2021

@tgodzik Thanks a lot for the work.

If you are willing, consider adding an entry to notes.md following the template in there to have this feature properly announced in the release notes!

I also launched a longer test suite to check there are no surprises with your changes.

@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 6, 2021

@tgodzik Thanks a lot for the work.

If you are willing, consider adding an entry to notes.md following the template in there to have this feature properly announced in the release notes!

I also launched a longer test suite to check there are no surprises with your changes.

Will do in a sec!

@tgodzik tgodzik requested a review from pioterj as a code owner September 6, 2021 13:37
tgodzik and others added 5 commits September 6, 2021 15:38
Issue: gradle#16527

Previously, we would need to compile the Scala compiler bridge in order to use it with a particular Scala version. Now, it's possible to download the jar, which is released together with a particular Scala version.

From other changes, a new compiler is needed together with Scala library, however it still reuses the Scala 2.13 library underneath.

Sample project this works with:

```kotlin
plugins {
    scala
}

repositories {
    mavenCentral()
    mavenLocal()
}

dependencies {
    implementation("org.scala-lang:scala3-library_3:3.0.1")
}
```

Signed-off-by: Tomasz Godzik <tomek.godzik@gmail.com>
Signed-off-by: Tomasz Godzik <tomek.godzik@gmail.com>
…loaders for Scala 2

Signed-off-by: Tomasz Godzik <tomek.godzik@gmail.com>
Signed-off-by: tgodzik <tgodzik@virtuslab.com>
Signed-off-by: tgodzik <tgodzik@virtuslab.com>
@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 6, 2021

@ljacomet I added the additional point to release notes and the tests seemed to all pass 🎉

@ljacomet ljacomet added this to the 7.3 RC1 milestone Sep 6, 2021
@gradle gradle deleted a comment from ljacomet Sep 6, 2021
@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle
Copy link
Collaborator

Pre-tested commit build failed.

@ljacomet
Copy link
Member

ljacomet commented Sep 6, 2021

@bot-gradle test and merge

@gradle gradle deleted a comment from ljacomet Sep 6, 2021
@gradle gradle deleted a comment from ljacomet Sep 6, 2021
@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle bot-gradle merged commit e2658b4 into gradle:master Sep 6, 2021
@Arthurm1
Copy link

Arthurm1 commented Sep 7, 2021

You should be able to test this by running...

./gradlew wrapper --gradle-distribution-url=https://services.gradle.org/distributions-snapshots/gradle-7.3-20210906222431+0000-bin.zip

Thanks a lot @tgodzik and co for this PR

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.

Support for Scala3