diff --git a/build.sbt b/build.sbt index 56a0886e01..0c32d203ee 100644 --- a/build.sbt +++ b/build.sbt @@ -63,6 +63,7 @@ lazy val core = project .in(file("scalafix-core")) .settings( moduleName := "scalafix-core", + versionScheme := Some("pvp"), buildInfoSettingsForCore, libraryDependencies ++= List( scalameta, diff --git a/docs/developers/api.md b/docs/developers/api.md index ad25ec0948..0c5cc33b7d 100644 --- a/docs/developers/api.md +++ b/docs/developers/api.md @@ -4,9 +4,26 @@ title: API Overview sidebar_label: Overview --- +## Compatibility considerations + +[`scalafix-core`](https://index.scala-lang.org/scalacenter/scalafix/scalafix-core) makes the packages +below available to built-in and external rules. + +Its A.B.C version scheme follows [PVP](https://pvp.haskell.org/) for binary compatiblity and aims at +keeping source compatibility across versions. +- A.B (the major version) is increased whenever a binary incompatibility is introduced. Running a rule + built against an older major version of Scalafix may cause either runtime errors, or false + positive/negative tree selection. In that case, a warning is issued when fetching the rules(s) + artifact(s). +- C (the minor version) is increased for bugfixes or to add new features. Running a rule built against + a more recent version of Scalafix is not possible. In that case, a `ScalafixException` is raised when + fetching the rules(s) artifact(s). + +## Packages + The Scalafix public API documentation is composed of several packages. -## Scalafix v1 +### Scalafix v1 Latest Scaladoc: [v@VERSION@](https://static.javadoc.io/ch.epfl.scala/scalafix-core_2.12/@VERSION@/scalafix/v1/index.html) @@ -37,15 +54,7 @@ structures include: information such as trees/tokens and semantic information such as symbols/types/synthetics. -## Scalafix v0 - -Latest Scaladoc: -[v@VERSION@](https://static.javadoc.io/ch.epfl.scala/scalafix-core_2.12/@VERSION@/scalafix/v0/index.html) - -This is a legacy API that exists to smoothen the migration for existing Scalafix -v0.5 rules. If you are writing a new Scalafix rule, please use the v1 API. - -## Scalameta Trees +### Scalameta Trees Latest Scaladoc: [v@SCALAMETA@](https://static.javadoc.io/org.scalameta/trees_2.12/@SCALAMETA@/scala/meta/index.html) @@ -71,7 +80,7 @@ include: position". Statement position is the - `Defn`: the supertype for all definitions -## Scalameta Tokens +### Scalameta Tokens Latest Scaladoc: [v@SCALAMETA@](https://static.javadoc.io/org.scalameta/tokens_2.12/@SCALAMETA@/scala/meta/tokens/Token.html) diff --git a/scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixArgumentsImpl.scala b/scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixArgumentsImpl.scala index 82ae1ed8fd..5258148adf 100644 --- a/scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixArgumentsImpl.scala +++ b/scalafix-cli/src/main/scala/scalafix/internal/interfaces/ScalafixArgumentsImpl.scala @@ -24,6 +24,10 @@ import scalafix.Versions import scalafix.cli.ExitStatus import scalafix.interfaces._ import scalafix.internal.config.ScalaVersion +import scalafix.internal.util.Compatibility +import scalafix.internal.util.Compatibility.Compatible +import scalafix.internal.util.Compatibility.Incompatible +import scalafix.internal.util.Compatibility.Unknown import scalafix.internal.v1.Args import scalafix.internal.v1.MainOps import scalafix.internal.v1.Rules @@ -76,7 +80,48 @@ final case class ScalafixArgumentsImpl(args: Args = Args.default) customDependenciesCoordinates, Versions.scalaVersion ) - val extraURLs = customURLs.asScala ++ customDependenciesJARs.asScala + + // External rules are built against `scalafix-core` to expose `scalafix.v1.Rule` implementations. The + // classloader loading `scalafix-cli` already contains `scalafix-core` to be able to discover them (which + // is why it must be the parent of the one loading the tool classpath), so effectively, the version/instance + // in the tool classpath will not be used. This adds a sanity check to warn or prevent the user in case of + // mismatch. + val scalafixCore = coursierapi.Module.parse( + "ch.epfl.scala::scalafix-core", + coursierapi.ScalaVersion.of(Versions.scalaVersion) + ) + customDependenciesJARs.getDependencies.asScala + .find(_.getModule == scalafixCore) + .foreach { dependency => + // We only check compatibility against THE scalafix-core returned by the coursier resolution, but + // this is not exhaustive as some old rules might coexist with recent ones, so the older versions + // get evicted. coursier-interface does not provide more granularity while the native coursier + // is stuck on a 2-years old version because it no longer cross-publishes for 2.11, so for now, + // the easiest option would be to run several resolutions in isolation, which seems like a massive + // cost for just issuing a warning. + Compatibility.pvp(dependency.getVersion, Versions.nightly) match { + case Incompatible => + throw new ScalafixException( + s"Scalafix version ${Versions.nightly} cannot load the registered external rules, " + + s"please upgrade to ${dependency.getVersion} or later" + ) + case Unknown => + args.out.println( + s"""INFO: loading external rule(s) built against an old version of scalafix (${dependency.getVersion}). + |This might not be a problem, but if you run into unexpected behavior, you should either: + |1. downgrade scalafix to ${dependency.getVersion} + |2. try a more recent version of the rules(s) if available; request the rule maintainer + | to build against scalafix ${Versions.nightly} or later if that does not help + """.stripMargin + ) + case Compatible => + } + } + + val extraURLs = customURLs.asScala ++ customDependenciesJARs + .getFiles() + .asScala + .map(_.toURI().toURL()) val classLoader = new URLClassLoader( extraURLs.toArray, getClass.getClassLoader diff --git a/scalafix-cli/src/main/scala/scalafix/internal/util/Compatibility.scala b/scalafix-cli/src/main/scala/scalafix/internal/util/Compatibility.scala new file mode 100644 index 0000000000..877e1394e5 --- /dev/null +++ b/scalafix-cli/src/main/scala/scalafix/internal/util/Compatibility.scala @@ -0,0 +1,33 @@ +package scalafix.internal.util + +sealed trait Compatibility + +object Compatibility { + + case object Compatible extends Compatibility + case object Unknown extends Compatibility + case object Incompatible extends Compatibility + + private val Snapshot = """.*-SNAPSHOT""".r + private val PVP = """^([0-9]+)\.([0-9]+)\.([0-9]+)""".r // A.B.C + + def pvp(builtAgainst: String, runWith: String): Compatibility = { + (builtAgainst, runWith) match { + case (Snapshot(), _) => Unknown + case (_, Snapshot()) => Unknown + case (PVP(buildA, _, _), PVP(runA, _, _)) if buildA.toInt > runA.toInt => + Incompatible + case (PVP(buildA, _, _), PVP(runA, _, _)) if buildA.toInt < runA.toInt => + Unknown + // --- A match given the cases above + case (PVP(_, buildB, _), PVP(_, runB, _)) if buildB.toInt > runB.toInt => + Incompatible + case (PVP(_, buildB, _), PVP(_, runB, _)) if buildB.toInt < runB.toInt => + Unknown + // --- A.B match given the cases above + case (PVP(_, _, buildC), PVP(_, _, runC)) if buildC.toInt > runC.toInt => + Incompatible + case _ => Compatible + } + } +} diff --git a/scalafix-interfaces/src/main/java/scalafix/internal/interfaces/ScalafixCoursier.java b/scalafix-interfaces/src/main/java/scalafix/internal/interfaces/ScalafixCoursier.java index 83f96cca53..29ae8534c4 100644 --- a/scalafix-interfaces/src/main/java/scalafix/internal/interfaces/ScalafixCoursier.java +++ b/scalafix-interfaces/src/main/java/scalafix/internal/interfaces/ScalafixCoursier.java @@ -15,26 +15,33 @@ public class ScalafixCoursier { - private static List fetch( + private static FetchResult fetch( List repositories, List dependencies, ResolutionParams resolutionParams ) throws ScalafixException { - List jars = new ArrayList<>(); try { - List files = Fetch.create() + return Fetch.create() .withRepositories(repositories.stream().toArray(Repository[]::new)) .withDependencies(dependencies.stream().toArray(Dependency[]::new)) .withResolutionParams(resolutionParams) - .fetch(); - for (File file : files) { + .fetchResult(); + } catch (CoursierError e) { + throw new ScalafixException("Failed to fetch " + dependencies + "from " + repositories, e); + } + } + + private static List toURLs(FetchResult result) throws ScalafixException { + List urls = new ArrayList<>(); + for (File file : result.getFiles()) { + try { URL url = file.toURI().toURL(); - jars.add(url); + urls.add(url); + } catch (MalformedURLException e) { + throw new ScalafixException("Failed to load dependency " + file, e); } - } catch (CoursierError | MalformedURLException e) { - throw new ScalafixException("Failed to fetch " + dependencies + "from " + repositories, e); } - return jars; + return urls; } public static List scalafixCliJars( @@ -58,22 +65,15 @@ public static List scalafixCliJars( ScalaVersion.of(scalaVersion) ).withConfiguration("runtime") ); - return fetch(repositories, dependencies, ResolutionParams.create()); + return toURLs(fetch(repositories, dependencies, ResolutionParams.create())); } - public static List toolClasspath( + public static FetchResult toolClasspath( List repositories, List extraDependenciesCoordinates, String scalaVersion ) throws ScalafixException { - // External rules are built against `scalafix-core` to expose `scalafix.v1.Rule` implementations. The - // classloader loading `scalafix-cli` already contains `scalafix-core` to be able to discover them (which - // is why it must be the parent of the one loading the tool classpath), so effectively, the version/instance - // in the tool classpath will not be used. This is OK, as `scalafix-core` should not break binary - // compatibility, as discussed in https://github.com/scalacenter/scalafix/issues/1108#issuecomment-639853899. - Module scalafixCore = Module.parse("ch.epfl.scala::scalafix-core", ScalaVersion.of(scalaVersion)); ResolutionParams excludeDepsInParentClassloader = ResolutionParams.create() - .addExclusion(scalafixCore.getOrganization(), scalafixCore.getName()) .addExclusion("org.scala-lang", "scala-library"); List dependencies = extraDependenciesCoordinates diff --git a/scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CompatibilitySuite.scala b/scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CompatibilitySuite.scala new file mode 100644 index 0000000000..ca5aaca059 --- /dev/null +++ b/scalafix-tests/unit/src/test/scala/scalafix/tests/cli/CompatibilitySuite.scala @@ -0,0 +1,99 @@ +package scalafix.tests.cli + +import org.scalatest.funsuite.AnyFunSuite +import scalafix.internal.util.Compatibility +import scalafix.internal.util.Compatibility._ + +class CompatibilitySuite extends AnyFunSuite { + + test("PVP compatible if full match") { + assert( + Compatibility.pvp( + builtAgainst = "0.9.34", + runWith = "0.9.34" + ) == Compatible + ) + assert( + Compatibility.pvp( + builtAgainst = "1.2.3", + runWith = "1.2.3" + ) == Compatible + ) + } + + // to avoid struggles when testing nightlies + test("PVP unknown if run or build is a snapshot") { + assert( + Compatibility.pvp( + builtAgainst = "0.9.34+52-a83785c4-SNAPSHOT", + runWith = "1.2.3" + ) == Unknown + ) + assert( + Compatibility.pvp( + builtAgainst = "0.9.34", + runWith = "1.2.3+1-bfe5ccd4-SNAPSHOT" + ) == Unknown + ) + } + + // no binary breaking change in 0.9.x, 0.10.x, ... + test("PVP compatible if run is greater by minor") { + assert( + Compatibility.pvp( + builtAgainst = "0.10.0", + runWith = "0.10.20" + ) == Compatible + ) + } + + // might be false positive/negative tree matches (AST updates) + test("PVP unknown if run is greater by major") { + assert( + Compatibility.pvp( + builtAgainst = "0.9.38", + runWith = "0.10.2" + ) == Unknown + ) + assert( + Compatibility.pvp( + builtAgainst = "0.9.38", + runWith = "1.0.0" + ) == Unknown + ) + } + + // build might reference classes unknown to run + test("PVP incompatible if build is greater") { + assert( + Compatibility.pvp( + builtAgainst = "2.0.0", + runWith = "1.1.1" + ) == Incompatible + ) + assert( + Compatibility.pvp( + builtAgainst = "1.4.7", + runWith = "1.4.6" + ) == Incompatible + ) + assert( + Compatibility.pvp( + builtAgainst = "1.4.7", + runWith = "1.2.8" + ) == Incompatible + ) + assert( + Compatibility.pvp( + builtAgainst = "0.10.8", + runWith = "0.9.16" + ) == Incompatible + ) + assert( + Compatibility.pvp( + builtAgainst = "0.10.17", + runWith = "0.10.4" + ) == Incompatible + ) + } +}