Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow usage of rules compiled against 2.11 & 2.13 #121

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented Jun 1, 2020

Ref scalacenter/scalafix#1108

Benefits

  1. Rules using the presentation compiler can now work on 2.11 and 2.13 input as long as they are cross-compiled (helping with Make ExplicitResultTypes work with more Scala versions scalafix#998)
  2. Advanced users with local rules targeting 2.11 or 2.13 code will be able to simplify their build

2.12 remains the default until 1.0 to be backward compatible with external rules (i.e loaded through scalafixDependencies or dependency:) that are not (yet) cross-compiled.

TODO before merging

TODO in scalafix repo after merging

@bjaglin bjaglin changed the title expose scalafixScalaVersion allow usage of scalafix rules compiled against 2.11 & 2.13 Jun 2, 2020
@bjaglin bjaglin changed the title allow usage of scalafix rules compiled against 2.11 & 2.13 allow usage of rules compiled against 2.11 & 2.13 Jun 2, 2020
@bjaglin bjaglin force-pushed the scalafixScalaVersion branch from 5bc7995 to 89987be Compare June 2, 2020 20:57
settingKey[String](
"The full scala version used for the scalafix classpath. Rules must be compiled against that binary version." +
s"Defaults to the latest supported 2.12 version (${BuildInfo.scala212}). " +
"This setting is read from the global scope so it only needs to be defined once in the build."
Copy link
Collaborator Author

@bjaglin bjaglin Jun 2, 2020

Choose a reason for hiding this comment

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

We could lift this constraint if it's too restrictive.

I added it to

  1. avoid messing with the classloader structure right away, as (1) in https://github.com/scalacenter/sbt-scalafix/blob/v0.9.16/src/main/scala/scalafix/internal/sbt/ScalafixInterface.scala#L105-L120 could no longer be shared across subprojects if we were to allow per-project scala version for scalafix invocations, which would require some basic caching logic if we don't want performance to regress because of JIT warm-ups
  2. keep the door open for https://github.com/scalacenter/sbt-scalafix/pull/121/files#r434185142

@bjaglin bjaglin force-pushed the scalafixScalaVersion branch from 89987be to e6c40bd Compare June 2, 2020 21:18
@@ -119,6 +126,7 @@ object ScalafixPlugin extends AutoPlugin {
scalafixCaching := false,
scalafixResolvers := ScalafixCoursier.defaultResolvers,
scalafixDependencies := Nil,
scalafixScalaVersion := BuildInfo.scala212, // will follow scalafixVersion once community rules are cross-built
Copy link
Collaborator Author

@bjaglin bjaglin Jun 2, 2020

Choose a reason for hiding this comment

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

I wonder if it's not worth making this a TaskKey (or scalafixDependencies a SettingKey? it's already one!), to be able to support scalacenter/scalafix#998 automatically when there is no external rule with something like:

Suggested change
scalafixScalaVersion := BuildInfo.scala212, // will follow scalafixVersion once community rules are cross-built
scalafixScalaVersion := {
if (scalafixDependencies.value.isEmpty) scalaVersion.value
else BuildInfo.scala212
}

This can break invocations with non-cross-compiled rules loaded via dependency:, but maybe that's OK as long as the documentation recommends to force scalafixScalaVersion := BuildInfo.scala212 until 1.x?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, all rules need to be compiled in 2.12 to be used. Right?
In fact if you have a project in 2.13, maybe with the name of this key (scalafixScalaVersion) you will directly choose 213.
I guess we hope that most rules will be cross-compiled soon.
I don't see any solution except having good documentation for this key

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, all rules need to be compiled in 2.12 to be used. Right?

Considering the git history here which shows that sbt-scalafix has always been loading rules (built-in or external) in 2.12 and the repos references in the user documentation (https://github.com/olafurpg/example-scalafix-rule & https://github.com/olafurpg/named-literal-arguments), I think it's fair to assume that, yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we hope that most rules will be cross-compiled soon.

My suggestion in the comment above was more for the transition period: we can leverage the fact that scalafix-rules IS cross-built to 2.11/2.13, so picking it up fixes scalacenter/scalafix#998. However, it's safe to do so if there are no external rules as part of the same invocation. If there are, we can't be sure they are cross-compiled, at least not in the near future (0.9.x).

Copy link
Contributor

Choose a reason for hiding this comment

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

The scala version must match exactly in scalafix-cli_$SCALA_VERSION. I don't think we can use scalaVersion.value from the project because that would break if users have a mismatch on the patch version (2.13.1 vs 2.13.2).

There are rare cases where using scalafix 2.13.2 with for example scala version 2.13.3 could cause runtime exceptions. I think we shouldn't worry about them since the cost of building against each exact version is too high when compared to the cost of hitting these rare runtime errors (which can often be worked around on the user side by upgrading to a more recent Scala version).

Copy link
Collaborator Author

@bjaglin bjaglin Jun 4, 2020

Choose a reason for hiding this comment

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

I don't think we can use scalaVersion.value from the project because that would break if users have a mismatch on the patch version (2.13.1 vs 2.13.2).

Good point, the scripted doesn't exercise this since we use scalafix.sbt.BuildInfo.scala* as scalaVersion, I will update that.

Considering we add a projectVersionToScalafixVersion(String: scalaVersion): String which takes care of looking up the correct patch version based on the binary version the project is built with, what do you think of my (updated) proposal to address scalacenter/scalafix#998 for most of the users out-of-the box as early as possible:

in 0.9.x

Suggested change
scalafixScalaVersion := BuildInfo.scala212, // will follow scalafixVersion once community rules are cross-built
scalafixScalaVersion := {
if (scalafixDependencies.value.isEmpty) projectVersionToScalafixVersion(scalaVersion.value)
else BuildInfo.scala212
}

in 1.x

Suggested change
scalafixScalaVersion := BuildInfo.scala212, // will follow scalafixVersion once community rules are cross-built
scalafixScalaVersion := projectVersionToScalafixVersion(scalaVersion.value)

@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 2, 2020

@olafurpg this is not fully ready but it's taking shape - is that what you had in mind?

@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 2, 2020

cc @mlachkar since this leverages scalacenter/scalafix#1118

Copy link
Contributor

@mlachkar mlachkar left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Since we expect that only non-cross rules will specify the scalafixScalaVersion after v1, maybe providing good documentation will be enough to raise confusion about this key name.

I think we also need to have a more complicated rule to cross-compile, that requires code changes (example: with folders _2.11, and 2.12), to give examples to help users cross-compile.

@@ -119,6 +126,7 @@ object ScalafixPlugin extends AutoPlugin {
scalafixCaching := false,
scalafixResolvers := ScalafixCoursier.defaultResolvers,
scalafixDependencies := Nil,
scalafixScalaVersion := BuildInfo.scala212, // will follow scalafixVersion once community rules are cross-built
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, all rules need to be compiled in 2.12 to be used. Right?
In fact if you have a project in 2.13, maybe with the name of this key (scalafixScalaVersion) you will directly choose 213.
I guess we hope that most rules will be cross-compiled soon.
I don't see any solution except having good documentation for this key

@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 3, 2020

I think we also need to have a more complicated rule to cross-compile, that requires code changes (example: with folders _2.11, and 2.12), to give examples to help users cross-compile.

Indeed, advanced rules interfacing with the presentation compiler might need that. I guess the best place for this example would be something along https://github.com/olafurpg/example-scalafix-rule (or a copy of it under scalacenter as discussed in scalacenter/example-scalafix-rule#1?), since it's not really related to sbt?

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for working on this upgrade! I am really excited to be able to finally use ExplicitResultTypes in my non-2.12 projects 😄

Just brainstorming, I am wondering if the user should declare the scalafix scala version in .scalafix.conf instead of build.sbt 🤔 This information will eventually be needed by other clients to correctly resolve and fetch external rules. I think it would be nice if we can keep the build clients thin (sbt-scalafix, gradle-scalafix, metals-scalafix,...) and move as much heavy lifting as possible behind scalafix-interfaces.

src/main/scala/scalafix/sbt/ScalafixPlugin.scala Outdated Show resolved Hide resolved
@@ -119,6 +126,7 @@ object ScalafixPlugin extends AutoPlugin {
scalafixCaching := false,
scalafixResolvers := ScalafixCoursier.defaultResolvers,
scalafixDependencies := Nil,
scalafixScalaVersion := BuildInfo.scala212, // will follow scalafixVersion once community rules are cross-built
Copy link
Contributor

Choose a reason for hiding this comment

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

The scala version must match exactly in scalafix-cli_$SCALA_VERSION. I don't think we can use scalaVersion.value from the project because that would break if users have a mismatch on the patch version (2.13.1 vs 2.13.2).

There are rare cases where using scalafix 2.13.2 with for example scala version 2.13.3 could cause runtime exceptions. I think we shouldn't worry about them since the cost of building against each exact version is too high when compared to the cost of hitting these rare runtime errors (which can often be worked around on the user side by upgrading to a more recent Scala version).

@olafurpg
Copy link
Contributor

olafurpg commented Jun 4, 2020

Just to be clear, I am a big 👍 for merging this PR unblocking 2.11/2.13 support in sbt-scalafix. I don't think we should block this PR in favor of some more general solution for hypothetical use-cases (example: Metals). The sbt plugin is how 90% of users use Scalafix and I think it's totally fine to have an sbt-only solution. I'm just thinking out loud, feel free to ignore me 😅

@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 7, 2020

Just brainstorming, I am wondering if the user should declare the scalafix scala version in .scalafix.conf instead of build.sbt. This information will eventually be needed by other clients to correctly resolve and fetch external rules.

scalacenter/scalafix#1152 does handle external rules fetching, and looking at the implementation, I am not sure promoting the scalafix version to a configuration key would help. Did you have a use-case in mind?

@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 7, 2020

Putting this on hold until scalacenter/scalafix#1152 is released, as all the heavy lifting can now be done outside sbt-scalafix.

@bjaglin bjaglin changed the title allow usage of rules compiled against 2.11 & 2.13 WIP allow usage of rules compiled against 2.11 & 2.13 Jun 13, 2020
@bjaglin bjaglin force-pushed the scalafixScalaVersion branch 10 times, most recently from abc0c75 to f6578a6 Compare June 15, 2020 00:06
@bjaglin bjaglin force-pushed the scalafixScalaVersion branch from f6578a6 to 5316d91 Compare June 15, 2020 00:32
@bjaglin bjaglin changed the title WIP allow usage of rules compiled against 2.11 & 2.13 allow usage of rules compiled against 2.11 & 2.13 Jun 15, 2020
@bjaglin bjaglin marked this pull request as ready for review June 15, 2020 00:57
@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 15, 2020

Now that cross-publishing of some external rules (thanks @mlachkar) & scalacenter/scalafix#1152 are available, I have reworked the PR and it is now ready for a proper review!

@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 15, 2020

Just brainstorming, I am wondering if the user should declare the scalafix scala version in .scalafix.conf instead of build.sbt thinking This information will eventually be needed by other clients to correctly resolve and fetch external rules. I think it would be nice if we can keep the build clients thin (sbt-scalafix, gradle-scalafix, metals-scalafix,...) and move as much heavy lifting as possible behind scalafix-interfaces.

I agree that it's a bit strange to have it in build.sbt (but it's probably the same if it were in .scalafix.conf) as the 2.11/2.13 developer will face a dilemna: use ExplicitResultTypes or external rules not cross-published yet?

scalafix-interfaces could abstract the scala version(s) it uses completely with a best-effort strategy, by trying to match the provided scala version for rules that are available for that binary version, and to stick to 2.12 for the others (effectively doing 2 separate runs). That does not sound particularly easy to do though.

@bjaglin bjaglin requested review from olafurpg and mlachkar June 15, 2020 01:08
build.sbt Outdated
case "2.10" => "0.13.17"
case "2.12" => "1.2.1"
case "2.10" => "0.13.18"
case "2.12" => "1.2.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, you usually want to target the lowest possible version here to avoid binary compatibility issues on older sbt versions. I’ve had issues in the past because sbt 1.x releases are always backwards compatible but not forwards compatible

Copy link
Collaborator Author

@bjaglin bjaglin Jun 15, 2020

Choose a reason for hiding this comment

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

I also ran into sbt/sbt#5049 on another project, that's why I didn't bump the minor, just the patch release, but I agree it does not guarantee anything.

The problem is that the bump is not required for building, but as mentioned in 6e70250, for scripted to build 2.13 (I tried to use 2.11 in the local-rules scripted but ran into #127 (comment) unless I use 2.11.12, which defeats the purpose of the scripted which verifies that only the binary version matters for most rules).

AFAIK only the 0.13.18 bump was necessary, so I'll keep 1.2.1 to limit the risks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK only the 0.13.18 bump was necessary, so I'll keep 1.2.1 to limit the risks.

Actually, 1.2.7 is required to build 1.13 sources (as it's the first version that brings a Zinc with a compiler-bridge published for 2.13).

Copy link
Collaborator Author

@bjaglin bjaglin Jun 15, 2020

Choose a reason for hiding this comment

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

Nevermind, I figured out I could just override scriptedSbt to leave sbtVersion untouched! 🎉

I took the opportunity to add comments to document our discussion.

settingKey[String](
"The Scala binary version used for scalafix execution. Defaults to 2.12. " +
s"Rules must be compiled against that binary version. Advanced semantic rules such as " +
s"ExplicitResultTypes require this to match the Scala version used for compilation."
Copy link
Contributor

Choose a reason for hiding this comment

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

The last sentence may be a bit misleading since it indicates this version must match the scala version exactly (MAJOR.MINOR.PATCH), not just the binary version (MAJOR.MINOR).

Copy link
Collaborator Author

@bjaglin bjaglin Jun 15, 2020

Choose a reason for hiding this comment

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

Indeed, I am rephrasing to

        "The Scala binary version used for scalafix execution. Defaults to 2.12. " +
          s"Rules must be compiled against that binary version, or for advanced rules such as " +
          s"ExplicitResultTypes which have a full cross-version, against the corresponding full" +
          s"version that scalafix is built against."

Maybe ExplicitResultTypes is not a perfect example as scalafix-rules will always follow the patch of scalafix-cli, but I felt like an example was necessary, and I don't know of any external rule that is fully cross-versioned.

@@ -7,7 +7,8 @@ inThisBuild(
scalacOptions += "-Yrangepos",
scalacOptions += "-Ywarn-unused", // for RemoveUnused
scalafixCaching := true,
scalafixDependencies += "com.geirsson" %% "example-scalafix-rule" % "1.2.0"
scalafixDependencies += "com.nequissimus" %% "sort-imports" % "0.5.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you can use the example scala rule? Typically you want to avoid depending on 3rdparty dependencies like this in your tests so that you have more control over everything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mentioned the reason in the commit message. I wanted to move from com.geirsson to ch.epfl.scala everywhere, but couldn't do it here as I needed 2 revisions (see usage of set scalafixDependencies in the corresponding test). But I agree it does mot make sence to move from one third party (even if it's yours) to another. I'll revert changes here in that specifc reference and in test.

@bjaglin bjaglin force-pushed the scalafixScalaVersion branch from 5316d91 to ef05280 Compare June 15, 2020 06:33
The caching scripted is left with the former organization, since we
want to make sure just changing the version invalidates the cache,
and we currently have only one version of "ch.epfl.scala" %%
"example-scalafix-rule".
@bjaglin bjaglin force-pushed the scalafixScalaVersion branch from ef05280 to fb96378 Compare June 15, 2020 06:52
@bjaglin
Copy link
Collaborator Author

bjaglin commented Jun 15, 2020

Amended the commits based on the feedback and with a few cosmetic tweaks, overall diff since "Ready to review": https://github.com/scalacenter/sbt-scalafix/compare/5316d91..fd97d1a

@bjaglin bjaglin force-pushed the scalafixScalaVersion branch 2 times, most recently from 1b37d63 to e8db895 Compare June 15, 2020 08:22
The first version of Zinc that have a compiler-bridge published for
2.13 are 1.1.7 & 1.2.5, brought respectively in sbt 0.13.8 & 1.2.7.

https://github.com/sbt/sbt/blob/924d8e7/main/src/main/scala/sbt/ScriptedPlugin.scala#L61
The class loading hierarchy effectively becomes one level flatter,
as the potential (5) has now (3) as parent, which unfortunately results
in a small regression from 568f1a2 in case `refreshClasspath` is
true, which happens for invocations that either:
 (1) request external rules via CLI (`dependency:`)
 (2) use local rules (Scalafix ivy config)

In this case the potential `scalafixDependencies` are unnecessarily
fetched & reloaded each time (see `scalafixArgsFromShell()`).

If needed, we can later memoize the state of the `scalafixArguments`
instance containing that classloader across invocations if only (2)
is involved, relying on stamping local rules bytecode to detect when
invalidation is needed.

For (1), the performance penalty is not a big deal anyway as this is
probably meant to be a one-shot run, as the user with some
`scalafixDependencies` will probably move away that CLI-syntax if
it is meant to be permanent.
Benefits
1. Rules using the presentation compiler can now work on 2.11 and 2.13
   input as long as they are cross-compiled
2. Advanced users with local rules targeting 2.11 or 2.13 code will be
   able to simplify their build

2.12 remains the default until 1.0 to be backward compatible with
external rules (i.e loaded through `scalafixDependencies` or
`dependency:`) that are not (yet) cross-compiled.
@bjaglin bjaglin force-pushed the scalafixScalaVersion branch from e8db895 to fd97d1a Compare June 15, 2020 08:58
Copy link
Contributor

@mlachkar mlachkar left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants