Scalameta 4.5.0 (was 4.4.35)

[SethTisue]

In the Scala 2 community build, I noticed that 1) this upgrade hasn't happened here yet, and 2) it requires a small source change (because the deprecated allowMethodTypes was removed).

And then also I'm seeing failing tests downstream in simulacrum-scalafix, at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/3535/artifact/logs/simulacrum-scalafix-build.log :

[simulacrum-scalafix] =======
[simulacrum-scalafix] => Diff
[simulacrum-scalafix] =======
[simulacrum-scalafix] --- obtained
[simulacrum-scalafix] +++ expected
[simulacrum-scalafix] @@ -24,9 +24,9 @@
[simulacrum-scalafix] ∙
[simulacrum-scalafix] -  val fk1 = λ[FuncK[Option, List]](_.toList)
[simulacrum-scalafix] +  val fk1 = new FuncK[Option, List] { def apply[A$](a$: Option[A$]): List[A$] = a$.toList }
[simulacrum-scalafix]    val fk2 = new FuncK[Option, List] { def apply[A$](a$: Option[A$]): List[A$] = Nil }
[simulacrum-scalafix] -  val fk3 = λ[List ~> Option](_.headOption)
[simulacrum-scalafix] +  val fk3 = new (List ~> Option) { def apply[A$](a$: List[A$]): Option[A$] = a$.headOption }
[simulacrum-scalafix]    val fk4 = new (List ~> Option) { def apply[A$](xs: List[A$]): Option[A$] = xs.lastOption }
[simulacrum-scalafix]    val fk5 = new (({ type λ[α$] = Foo[α$, Int] })#λ ~> Option) { def apply[A$](a$: Foo[A$, Int]): Option[A$] = None }
[simulacrum-scalafix] -  val fk6 = λ[FuncK[List, λ[x => List[List[x]]]]](List(_))
[simulacrum-scalafix] -  val fk7 = λ[List ~> Option](_.headOption.map(identity))
[simulacrum-scalafix] +  val fk6 = new FuncK[List, ({ type λ[x] = List[List[x]] })#λ] { def apply[A$](a$: List[A$]): List[List[A$]] = List(a$) }
[simulacrum-scalafix] +  val fk7 = new (List ~> Option) { def apply[A$](a$: List[A$]): Option[A$] = a$.headOption.map(identity) }
[simulacrum-scalafix]    val fk8 = new (List ~> Option) { def apply[A$](list: List[A$]): Option[A$] = {

I don't know what to make of this. Regression in scalameta? Something in scalafix that needs to be adjusted for the new scalameta version? Or is it just something that the simulacrum-scalafix people (@sh0hei maybe?) need to adjust in their test suite?

Properly investigating isn't really something I can take on myself.

Regardless, opening this PR to get the ball rolling. (It isn't easy for the simulacrum-scalafix folks to investigate unless they have a scalafix release to use.)

[bjaglin]

I'll merge this and open a PR on simulacrum-scalafix with the resulting nightly to follow-up on this regression.

[bjaglin]

scalameta/scalameta#2196

[bjaglin]

I confirmed the breaking change, identified the root cause and updated the rule code in typelevel/simulacrum-scalafix@04f557f. However, this brings questions about compatibility.

Rule authors

Should we signal this change to rule authors dealing with Term.Placeholder through a 0.10.0 release? scalafix 0.9.0 came out with scalameta 4.0.0, so I assume there has been other breaking changes in scalameta along the way. Still, I think this one is "breaking enough" to justify this.

Running rules built against newer scalafix versions with older scalafix versions

Rules relying on Term.AnonymousFunction from scalameta 4.5.0 will fail at runtime when executed with older scalafix versions. That's because scalafix-core is already loaded when external rules are injected in a separate classloader, so their version of scalafix-core (and thus scalameta) is ignored. In other words: scalafix-interfaces forces scalafix-cli's scalameta version, there is no eviction possible here.

scalafix/scalafix-interfaces/src/main/java/scalafix/internal/interfaces/ScalafixCoursier.java

Lines 69 to 76 in 8bd700f

	// 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())

This is not new (rules matching on Scala3-specific trees such as Term.Given need a recent scalafix runner for example), but it's more annoying for existing rules that don't use new feature but just have to adapt to the new parsing output like typelevel/simulacrum-scalafix@04f557f.

To mitigate this problem, I suggest we provide an actionable error message when encountering linking exceptions ("a more recent versions of scalafix is needed to run that rule"). This will improve the scalafix user experience if/when new trees are added in scalameta in the future.

Running rules built against older scalafix versions with newer scalafix versions

Conversely, running rules built against older versions of scalafix might miss some trees (silently I believe?) when ran on the upcoming version of scalafix since they will be exposed to unexpected Term.AnonymousFunction (see the test failure that triggered this discussion).

To mitigate that, we could issue a warning when detecting that external rules are built against a different minor version of scalafix: "running external scalafix rules built for an earlier version - correctness is not guaranteed so downgrade the scalafix version or ask the rule author to release a new version". This would be very verbose though, and seems to be very specific to scalameta/scalameta#2601 so I would suggest not to do anything.

@mlachkar @olafurpg @tgodzik what's your take on all this?

[tgodzik]

To mitigate this problem, I suggest we provide an actionable error message when encountering linking exceptions ("a more recent versions of scalafix is needed to run that rule"). This will improve the scalafix user experience if/when new trees are added in scalameta in the future.

Could we have a requirement in the rules for the minimal Scalafix version? I would even fail in that situation if possible.

Conversely, running rules built against older versions of scalafix might miss some trees (silently I believe?) when ran on the upcoming version of scalafix since they will be exposed to unexpected Term.AnonymousFunction (see the test failure that triggered this discussion).

It should miss silently in most cases unless any rule depends on a very specific path of tree nodes. I think most rules traverse tree until they find what is needed, so they would skip unknown nodes. We could issue a warning in this case for sure.

[bjaglin]

Thanks for chiming-in @tgodzik !

Running rules built against newer scalafix versions with older scalafix versions

Could we have a requirement in the rules for the minimal Scalafix version? I would even fail in that situation if possible.

This can already be done on the rule side by checking https://www.javadoc.io/doc/ch.epfl.scala/scalafix-core_2.12/0.9.34/scalafix/Versions$.html. I'll experiment this in typelevel/simulacrum-scalafix#194.

For the long term, I am not convinced it's a good idea to let the client handle that (with or without a specific API allowing them to declare a version), as most of the rule authors wouldn't know which minimum scalafix/scalameta version to use. Thinking more about it, the safest would be to fail if scalafix-core tries to load rules built against a future version of it. Some community clients (mill, maven) might be lagging a few scalafix-core versions behind, but so do rule authors, so I think situations where a user cannot run a recent rule because his client is too old will be rare.

Running rules built against older scalafix versions with newer scalafix versions

I think most rules traverse tree until they find what is needed, so they would skip unknown nodes.

I think so too, yet typelevel/simulacrum-scalafix@04f557f#diff-e210d889e0da3ec13e31e14a8b9500501669f3ab159725bfcce76bae07e2668aL37 is a counter-example where we have false negative matches. It's hard to tell how common that is.

We could issue a warning in this case for sure.

The problem is that this warning would then be issued for any community rule, until a version published against the latest scalafix is released. But I don't see any other way.

[SethTisue]

The problem is that this warning would then be issued for any community rule,

I think we should bite the bullet on that. It's a (admittedly regrettable) one-time annoyance for a significant long-term benefit.