Separate versioning of sbt-scalafix and scalafix

[mlachkar]

• Separate versioning of sbt-scalafix and scalafix-core similar to sbt-scalafmt and scalafmt-core. Users should declare the Scalafix version in .scalafix.conf so that clients like Metals or maybe IntelliJ can load the correct version of Scalafix without having to inspect project/plugins.sbt

• It will make easier the release of scalafix, which is now depending on sbt-scalafix.

#fixes #1084

[bjaglin]

From my experience with scalafmt / sbt-scalafmt: it can be confusing for the users if the release cycles are distinct but the versions remain in the same vicinity. I don't have a good solution for that though, as we might want to signal 1.0 at the same time for scalafix-core and sbt-scalafix.

[mlachkar]

I agree that it's quite annoying to specify the sbt-scalafix version, and then add a version inside the scalafix.conf from the user point of view, that looks almost the same. Moreover, I wonder if people will update the version inside .scalafix.conf (except if scala steward is able to manage this).

On the other hand, I think that those two versions should be independent since semantically they are different and they could occur independently. It will make easier the release management.

I wonder if it's possible to have a default value for scalafix version, provided by the sbt plugin, which will need a commit to evolve, but not an urgent one (avoiding the error when fetching artifacts for really early adopters ^^ ). The scafix-version config, will be used only for the advanced user (metals for example).

So my idea is not to show directly this version conf to avoid the confusion that raises from "versions remaining almost the same". What do you think ?

[olafurpg]

I think it's a good idea to have clearly distinct version numbers for sbt-scalafix and scalafix-core, for example sbt-scalafix:20.0.0 and scalafix-core:1.0.0. It might be a bit weird at first sight but it helps clarify that the two versions are independent. I regret we didn't do that with sbt-scalafmt (it was proposed).

I would require the version field in .scalafix.conf from scalafix-core:1.0.0 going forward. Otherwise I am concerned many users will stay on old scalafix versions using the latest default from scalafix.

One problem I see with this scheme is that sbt-scalafix won't know what version of SemanticDB to install since it's provided by scalafix-interfaces. The options I see to fix this problem are

• keep sbt-scalafix and scalafix-core versions in sync and pay the complexity price for releases
• dynamically fetch and load resources for scalafix-interfaces.jar that matches the version in .scalafix.conf
• require users to additionally specify semanticdb version in .scalafix.conf

I'm not sure what the best solution is 🤔

[mlachkar]

why couldn't we evolve the default value of scalafix with releases?

I don't think users should specify semanticdb version, it's internal to scalafix (they don't know much about which one is the correct one).

[bjaglin]

dynamically fetch and load resources for scalafix-interfaces.jar that matches the version in .scalafix.conf

That looks like the best option to me.

Implementation-wise, https://github.com/scalacenter/sbt-scalafix/blob/9d7d17743208a4a04fac5138d947f5c1ecfabe93/src/main/scala/scalafix/sbt/ScalafixPlugin.scala#L54-L55 could be wrapped in a key (backed by Scalafix.classloadInstance().scalametaVersion). However, sbt 1.3.x's semanticdbVersion being a setting key, we would need it to be a setting key too, causing an eager lookup at sbt init time...addCompilerPlugin does not have this limitation though (so the lookup could remain lazy in a task key).

[bjaglin]

I think it's a good idea to have clearly distinct version numbers for sbt-scalafix and scalafix-core, for example sbt-scalafix:20.0.0 and scalafix-core:1.0.0. It might be a bit weird at first sight but it helps clarify that the two versions are independent. I regret we didn't do that with sbt-scalafmt (it was proposed).

:+1:

[olafurpg]

The biggest motivation to separate versioning of sbt-scalafix and scalafix-core is that it means other integrations (mill, gradle, maybe metals) also don't need to cut a new release for every new Scalafix release. It would be hugely beneficial if all clients can use scalafix-interfaces to support any version of Scalafix. The current situation is the sbt-scalafix and scalafix-core must be released together, and it usually means that other clients end up staying on old Scalafix versions.

That looks like the best option to me.

I agree.

One option to avoid the eager sbt init lookup is to have users install sbt-scalafix in the meta-meta-build project/project/plugins.sbt. We could read .scalafix.conf from there and expose the version in project/plugins.sbt. I'm still not a fan of that approach because it's unorthodox I am concerned it would be confusing for users.

[bjaglin]

I'm still not a fan of that approach because it's unorthodox I am concerned it would be confusing for users.

Indeed, although technically it's a good workaround!

Would it be an option to challenge in sbt the fact that semanticdbVersion is a setting key? By challenging, I mean keeping it around for compatibility but introducing a semanticdbCompilerPluginVersion task key with the exact same semantics.

[olafurpg]

@bjaglin it looks like semanticdbVersion is eventually used by allDependencies which is a task key so it might be possible to make that change upstream in sbt (cc/ @eed3si9n ). However, such an approach would only work with new versions of sbt, which is a bit unsatisfying.

[bjaglin]

Yes I don't think there is a need for it to be a setting key. Regarding sbt 1.3.x: I know it's possible to declare a task key twice, so maybe we could have the "new one" in autoImports and affect libraryDependencies ourselves. Not sure how the duplicated key in scope for sbt 1.4.x clients would play though.

[olafurpg]

As a starting point, I think it's fine to fetch the semanticdb version via scalafix-interfaces.jar using coursier resolution during sbt init. There will be minor overhead from the resolution but I estimate it will only be a few milliseconds when cached (if I had to take a wild guess, around 20ms). This overhead is unavoidable regardless of the approach we take.

[bjaglin]

I started looking into that for good and quickly faced another challenge: since scalafix-cli is published with full cross version (one per binary version), scalafix-interfaces needs to find that exact full Scala version.

Since https://github.com/scalacenter/scalafix/commit/3694af653dde8c6c05a2af5051540a3f7f7870a6, clients are passing a binary Scala version, that scalafix-interfaces can easily map to the full Scala version since the information is baked-in. As scalafix-interfaces will now be able to load a future scalafix-cli published with an unknown full Scala version, this is no longer possible.

I see 3 options:

1. Require end-users to provide a full scalaVersion in .scalafmt.comf, along the scalafix version
   • easy to implement but makes end-users upgrades a bit cumbersome as the scalaVersion needs to be bumped sometimes
2. Keep asking clients to provide a binary Scala version 2.n when calling scalafix-interfaces, and increment m until a ch.epfl.scala:scalafix-cli_2.n.m:version is found (I assume there is no other way than brute-force, but maybe ivy provides some?)
   • costly, particularly in the sbt context where we need to do that at loading time to inject the right semanticDB compiler plugin - I guess we would need an extra layer of caching to avoid involving Coursier at all except the first time
   • since the semanticDB compiler plugin needs to match the full Scala version of the build environment, there is not much flexibility brought by the usage of the binary Scala version
3. Ask clients to provide a full Scala version (matching the build environment) when calling scalafix-interfaces
   • for that to be effectively usable, scalafix-cli should be published for all (or several?) full Scala versions for a given Scala binary version
   • we'll also have to to keep track and expose the latest semanticDB compiler plugin for each full Scala version - that's doable at build-time though so it's not a big deal

I am thinking about opening a PR to implement (3) as a preliminary step to this ticket. WDYT @olafurpg @mlachkar ?

[olafurpg]

Approach 3 sounds like the best option.

I’m wondering, would this change no longer be necessary if we could automatically release sbt-Scalafix together with Scalafix-cli? Maybe it’s possible to merge the repos somehow

[bjaglin]

I’m wondering, would this change no longer be necessary if we could automatically release sbt-Scalafix together with Scalafix-cli? Maybe it’s possible to merge the repos somehow

I didn't go as far as challenging this issue itself as I thought the need for it was already discussed before https://github.com/scalacenter/scalafix/issues/1108, but it's interesting indeed. I have seen that the sbt plugin used to be part of the main repo - what was the rationale at that time to spin it off? Merging it back would of course makes things easier (and make all discussions in this thread irrelevant), but I like the fact that it's currently separated as it forces us to think like other scalafix-interface clients (IDE or build tool) do.

[bjaglin]

I tried to sum up the current constraints about Scala versions before embarking on (3), and I could not find any (new) blocker. Please correct me if I got something wrong!

1. The client MUST be provided a semanticdb-scalac_2.n.m matching the Scala full version it starts the compiler with
   1. sbt-scalafix currently assumes that the version exposed as BuildInfo.scalametaVersion by scalafix-interfaces is available for the right Scalafix full version. scalafixEnable does it the other way around, forcing the build to be compiled with the Scala full version used in the Scalafix build when publishing scalafix-interfaces.
   2. Unlike what's mentioned in (3), this does not NEED to change in the context of what's discussed here, but we could provide a version for each Scala full version available when scalafix-interfaces is published, allowing scalafix to be "ahead" of the project's full version (but not the other way around)
2. The client MUST classload a scalafix-cli_2.n.m that matches the Scala binary version it compiled the target sources with so that the semantic rules in the transitive scalafix-rules_2.n can parse target class files
   1. Exposed in scalafix-interfaces since https://github.com/scalacenter/scalafix/pull/1152
3. The client MUST classload a scalafix-cli_2.n.m that matches the Scala binary version it compiled the target sources with so that the semantic rules compiled on the fly by the transitive scalafix-reflect_2.n.m can parse target class files
   1. Exposed in scalafix-interfaces since https://github.com/scalacenter/scalafix/pull/1152
4. The client SHOULD classload a scalafix-cli_2.n.m that matches the Scala full version it compiled the target sources with so that the advanced semantic rules in the transitive scalafix-rules_2.n interacts with a scala-compiler:2.n.m & semanticdb-scalac-core_2.n.m compatible with the one used to generate target class files
   1. This cannot be guaranteed at configuration time at the moment since scalafix-interfaces only accepts a Scala binary version, but https://github.com/scalacenter/scalafix/pull/1174 effectively soften this constraint via testing

[bjaglin]

~Writing this left me with an open question about scalafix-rule_2.n which depends on semanticdb-scalac-core_2.n.m: since some external rules depend on it for ScalafixGlobal and is currently loaded in the same classloader, isn't there a risk for conflicting semanticdb-scalac-core as no eviction will happen if the Scala suffix differs? Of course, ScalafixGlobal is not part of the public API and it is well documented that scalafix-core should be used for rule authors to build against, but still...~

Edited: not a problem, see 2 messages below.

[mlachkar]

Usually, you depend on scalafix-rules in your own-rule project, then you use/classload the rule in another projet/module which depends on scalafix-interfaces. If I understand the issue: scalafix-interacaces will now be responsible for the resolution of semanticdb version, which may conflict with scalafix-rule_2.n semancdb-scalac-core ?

[bjaglin]

@mlachkar not really, this isn't related to scalafix-interfaces. Actually while trying to rephrase I realized there was no problem... An external rule depending on a scalafix-rule_2.a will bring in a semanticdb-scalac-core_2.a.b, which I thought could conflict with a potential semantidb-scalac-core_2.a.c brought in by a newer scalafix-rule_2.a, but the latter would evict the latter so the transitive dependencies of the latter won't be around!

[bjaglin]

I will try to find some time over the next few weeks to resume my work on this.

[bjaglin]

It has been a few weeks now 😅 so before getting back to that, I wanted to re-evaluate (3) at the light of

• the removal of on-demand compilation when semanticdb files are missing, limiting the usage of the compiler to advanced semantic rules (for the presentation compiler) and for scalafix-reflect to compile source rules.
• the discussions we've had for Scala 3: publishing scalafix-core for Scala 3 is not trivial (metaconfig, scalameta, etc), so we will likely go the Java API way to abstract the Scala 3 compiler (as originally suggested in https://github.com/scalacenter/scalafix/issues/998) to run ExplicitResultTypes against Scala 3 sources from a Scala 2.x rule
• ~The ongoing work on -Yscala-release~ I had a deeper look at it and it's actually irrelevant in our case

In a nutshell, I'd like to see if we could work towards removing the need for passing a Scala version (even binary) to scalafix-interface instead of asking the client to pass a full one

• Currently, only -reflect, -cli & -testkit are published with the full Scala version (-rules & -core are not, even though -rules does use scala.reflect.internal). From my understanding, the rationale for using the full suffix was to avoid getting scala-compiler & scala-reflect artifacts evicted as there is no compatibility guarantee. With scala 2.x mature and 3.x bring more guarantees from one minor to the other, maybe we can stop publishing with the full Scala version?
• scalafix-interface could then load scalafix-cli_2.13
• ExplicitResultTypes would need to run in another classloader to work against 2.11 and 2.12, with a Java API bridge (not part of the Scalafix API), but that's what we will need anyway for 3.x support

What do you think @olafurpg & @mlachkar? Maybe it's worth getting an opinion from someone at the scalacenter involved in the compiler?

[olafurpg]

I tried to catch up on the discussion but it's not clear to me what is the exact goal here.

I'd like to see if we could work towards removing the need for passing a Scala version (even binary) to scalafix-interface

Can you elaborate on the motivation for this? The full Scala version is passed along to rules via withConfiguration() and rules may rely on this information to function correctly (for example, ExplicitResultTypes).

https://github.com/scalacenter/scalafix/blob/25569fdc54038a796053b07ce531d1ae44574a11/scalafix-core/src/main/scala/scalafix/v1/Configuration.scala#L7

I had assumed the Scala version was relatively easy for clients to provide.

With scala 2.x mature and 3.x bring more guarantees from one minor to the other, maybe we can stop publishing with the full Scala version?

Compiler APIs tend to break in minor ways so I would be careful about relying on compatibility guarantees between minor 3.x releases. Some rules like ExplicitResultTypes may also want to poke into compiler internals, which requires targeting a specific Scala version.

Not sure if this is relevant but I recently implemented a resource generator for the sbt-sourcegraph plugin to identify the latest SemanticDB version for all Scala 2.x versions.

https://github.com/sourcegraph/sbt-sourcegraph/blob/d0c4b2a33d8f091d8ca82f8fc1e801a5afb75654/src/main/scala/com/sourcegraph/sbtsourcegraph/Versions.scala#L36-L61

A similar solution based on coursier complete might simplify identifying the latest scalafix-cli version supporting an exact Scala version 🤔 Beware that coursier complete doesn't reliably work on private artifactory instances so it's best to generate this file against Maven Central and embed it as a resource.

[sjrd]

Currently, only -reflect, -cli & -testkit are published with the full Scala version (-rules & -core are not, even though -rules does use scala.reflect.internal). From my understanding, the rationale for using the full suffix was to avoid getting scala-compiler & scala-reflect artifacts evicted as there is no compatibility guarantee. With scala 2.x mature and 3.x bring more guarantees from one minor to the other, maybe we can stop publishing with the full Scala version?

The compilers, both Scala 2 and 3, make no guarantee whatsoever about their binary or source API. Same goes for scala.reflect.internal. Even a refactoring in that codebase could break code that depends on it. And by "could" I mean "will". This happened to Scala.js as recently as 2.13.5. Scala 3.x gives more compatibility guarantees for libraries and for TASTy, not for the compiler APIs nor the internal implementation of tasty-reflect.

Publishing against only the binary-version (as opposed to the full-version) something that depends on the compiler is a recipe for disaster later. In the worst case (like for the checkClassType change I mentioned above), there may be no way to reconcile the old and new APIs at the binary level. The only way to keep supporting both is to have binaries that are cross-versioned. If you install a scheme were you publish only for the binary version, and then something like the above pops again, you'll be stuck in a position where you have no escape except a) break the older versions, or b) revert all your changes and go back to a scheme with full-version suffixes.

[bjaglin]

Thanks @olafurpg & @sjrd for pointing out the rationale behind full-version publishing!

I tried to catch up on the discussion but it's not clear to me what is the exact goal here.

Indeed it's drifting from the original goal - I am just trying to challenge all versioning schemes to anticipate any other potential breaking change for users and/or rule authors so that we can batch it with that one.

Publishing against only the binary-version (as opposed to the full-version) something that depends on the compiler is a recipe for disaster later

If we stricly follow this guideline, I believe we should publish scalafix-rules with the full-version. What's confusing me is that scalameta does not seem to follow it:

➜  ~ cs resolve --what-depends-on org.scala-lang:scala-compiler ch.epfl.scala:scalafix-rules_2.12:0.9.34
  Result:
└─ org.scala-lang:scala-compiler:2.12.15
   ├─ ch.epfl.scala:scalafix-rules_2.12:0.9.34
   ├─ org.scala-lang:scalap:2.12.15
   │  └─ org.scalameta:scalameta_2.12:4.4.32
   │     ├─ ch.epfl.scala:scalafix-core_2.12:0.9.34
   │     │  └─ ch.epfl.scala:scalafix-rules_2.12:0.9.34
   │     └─ org.scalameta:semanticdb-scalac-core_2.12.15:4.4.32
   │        └─ ch.epfl.scala:scalafix-rules_2.12:0.9.34
   └─ org.scalameta:semanticdb-scalac-core_2.12.15:4.4.32
      └─ ch.epfl.scala:scalafix-rules_2.12:0.9.34

If I understand well, that could be a source of binary incompatibility when rules built against different versions of scalameta run in the same classloader (which can become a problem as the number of community rules increase) ? Using a full-version for scalafix-core could mitigate this I guess, but would require either

1. rule authors to start cross-building all patch versions just so that scalafix-cli can pick "the right one" matching its runtime
2. each rule to be evaluated in separate classloader with its own scala version (probably heavy on the JVM to JIT several scala-library JARs, and complicated because v1.Rule has scala types in its signatures)

[tgodzik]

I think the dependency on semanticdb-scalac-core is to do with the semanticdb, but not really with any interaction with the compiler. scalameta itself doesn't interact with it.

[bjaglin]

I think the dependency on semanticdb-scalac-core is to do with the semanticdb, but not really with any interaction with the compiler. scalameta itself doesn't interact with it.

Indeed, that part looks safe.

But what about the dependency from scalameta to scalap? Even if the scalap API remains effectively stable (despite not having a MiMa setup), usage of scala-compiler from scalap might break if their versions are not aligned.

[olafurpg]

With Scala 3 out and with no plans for Scala 2.14 then I'd try to avoid complicating the existing cross-publishing schemes. I think it's OK for rules to report a Scalafix error if they require a specific Scala patch version.

But what about the dependency from scalameta to scalap?

This should be a a low-risk dependency. The transitive dependency on scala-compiler via scalameta->scalap should also not be a concern.

probably heavy on the JVM to JIT several scala-library JARs, and complicated because v1.Rule has scala types in its signatures

FWIW, you can layer the classloaders so that scala-library types are shared in the parent classloader.