Configure the Scala version properly for the Scala plugin

[srdo-humio]

The Scala plugin will default to assuming a Scala version of 2.12 if not properly configured. This causes Intellij to show compilation errors when using newer features, even if such code builds fine when invoking Bazel.

The Maven and Gradle integrations in the Scala plugin figure out the Scala version by looking for the Scala library jar, and extracting the version from the file name. The Bazel plugin can do the same.

The Scala plugin library configuration can be found at https://github.com/JetBrains/intellij-scala/blob/ddf0b94cc5f9802f66a73f140657a70b2d4719fd/scala/scala-impl/src/org/jetbrains/plugins/scala/project/external/ScalaSdkUtils.scala#L59-L62

The version extraction for Gradle as an example can be found at https://github.com/JetBrains/intellij-scala/blob/ddf0b94cc5f9802f66a73f140657a70b2d4719fd/scala/integration/gradle/src/org/jetbrains/plugins/scala/project/gradle/ScalaGradleDataService.scala#L70

The relevant regexes were copied from the Scala plugin. I would have liked to simply invoke the relevant methods from the Scala plugin, but the methods can't be accessed from Java without reflecting, due to being located in a package object.

[srdo-humio]

If you prefer not copying code from the Scala plugin, I'm also happy to try submitting a patch to the Scala plugin that moves the relevant code out of the package object to somewhere reachable from Java. Making that work would then require dropping support for older Scala plugin versions. I'm not sure on the policy about that kind of thing?

[srdo-humio]

I've added a test so this doesn't break in the future.

[tpasternak]

Hey, I'm only wondering what are the implications of passing empty compilerClasspath to ScalaLibraryProperties

[tpasternak]

I think it could affect places where Scala Remote Server is spawned, for example expression evaluator @jastice

[srdo]

From a cursory reading, the compilerClasspath is mainly used in RemoteServerConnectorBase. I'm not familiar enough with the Scala plugin to know what that's for. But as far as I can tell, if you attempt to use that without setting the properties at all you will get an exception. I think that's the situation as it was before this PR. If you know how to use the remote server (or can tell me how to), it wouldn't hurt to check if it worked before and is broken with these changes.

Setting the classpath would be best, but I'm not sure leaving it as is introduces any regressions?

[jastice]

Hey, I'm only wondering what are the implications of passing empty compilerClasspath to ScalaLibraryProperties

@vasilmkd-jetbrains Can you say what the implications on compiler or debugger would be here?

[srdo]

The pre-PR code uses editor.setType, which becomes LibraryImpl.setKind, which does this

myProperties = kind.createDefaultProperties();

The ScalaLibraryType kind creates an empty ScalaLibraryProperties, where the classpaths (and version, hence the fallback to assuming Scala 2.12) are empty

https://github.com/JetBrains/intellij-scala/blob/ddf0b94cc5f9802f66a73f140657a70b2d4719fd/scala/scala-impl/src/org/jetbrains/plugins/scala/project/ScalaLibraryType.scala#L71

https://github.com/JetBrains/intellij-scala/blob/ddf0b94cc5f9802f66a73f140657a70b2d4719fd/scala/scala-impl/src/org/jetbrains/plugins/scala/project/ScalaLibraryProperties.scala#L70

So I don't think this change will break anything that wasn't already broken, because the compiler classpath was also empty before this change.

Setting the compiler classpath is a good idea, but I don't think it needs to fall under this issue, and could be handled as a separate change.

[ghost]

Hey, I'm only wondering what are the implications of passing empty compilerClasspath to ScalaLibraryProperties

@vasilmkd-jetbrains Can you say what the implications on compiler or debugger would be here?

I honestly don't know what exactly would happen, but I wouldn't be surprised if compilation becomes possible only from local sources, without dependencies. I'm not sure, it would be good to test manually. I'm sorry I can't help more.

[ghost]

It's unfortunate that the regexes are in a package object which makes it near impossible to call. We could factor that out if necessary.

[liucijus]

There's an edge case with Scala 3, which has Scala 2 library as dep on the classpath. I made a repro (intentionally messy) to demonstrate the problem: https://github.com/liucijus/rs_scala3_ij_repro

For most of the users, who use loaders like Rules Jvm External it won't be a problem as libs gets sorted in the way that Scala 3 will be before Scala 2 libs, but keep in mind that there's a possibility to have wrongly detected version due to different order.

[srdo]

@liucijus I get a 404 on your link, maybe the repo is private?

Just for reference, I took a look at how the Gradle and Maven integrations handle this:

The Gradle integration just picks whichever library happens to be at the head of the list of libraries.

https://github.com/JetBrains/intellij-scala/blob/ddf0b94cc5f9802f66a73f140657a70b2d4719fd/scala/integration/gradle/src/org/jetbrains/plugins/scala/project/gradle/ScalaGradleDataService.scala#L70

The Maven integration takes the compiler version to use from the POM of the project, and then tries to find a library with the same version in the dependencies.

https://github.com/JetBrains/intellij-scala/blob/ddf0b94cc5f9802f66a73f140657a70b2d4719fd/scala/integration/maven/src/org/jetbrains/plugins/scala/project/maven/ScalaMavenImporter.scala#L118-L132

I figure if we want to do something bulletproof here, we'd need the user to communicate which Scala version they expect for the module, in a similar way to how the Maven integration handles this by having the user specify the expected version in the POM? Otherwise I think there will be cases where we guess wrong on what the user wanted, whether we pick the min or max version available.

See for instance https://youtrack.jetbrains.com/issue/SCL-18866/sbt-module-can-wrongly-depend-on-scala-sdk-instead-of-normal-scala-library-in-multi-module-projects where someone has a project where module A depends on B, A is a intended to be a 2.13 project, and B is a Scala 3 project. In that case, A would be interpreted at being Scala 3 as well, because it gets the Scala 3 lib via B.

I'm happy to change this to pick the highest library version in the list of libraries, if you think that's better than the essentially random choice we make now?

[liucijus]

@liucijus I get a 404 on your link, maybe the repo is private?

Indeed I had it private - opened.

My opinion on the problem - I think current implementation is good enough to be used in production (I will cherry pick this for my fork if this won't get merged). Comment about the edge in the code would be nice though.

[srdo-humio]

Added a comment in the code summarizing the problem.

[srdo-humio]

@liucijus Sorry to ping you directly, but I was wondering if you would be amenable to merging this? You approved it some months ago.

We've been using this internally since then with no complaints.

[tpasternak]

Ok, thank you @srdo-humio