-
Notifications
You must be signed in to change notification settings - Fork 43
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
initial support for running rules on Scala 3 sources #210
Conversation
interface: ScalafixInterface | ||
): Seq[String] = { | ||
if (files.isEmpty) Nil | ||
else { | ||
val errors = ListBuffer.empty[String] | ||
val hasSemanticdb = | ||
dependencies.exists(_.name.startsWith("semanticdb-scalac")) | ||
dependencies.exists(_.name.startsWith("semanticdb-scalac")) || | ||
scalacOpts.contains("-Xsemanticdb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will bring a false positive error with early dotty & 3.0.0-M* compilers, but I guess it's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to support the final release of scala 3, so I agree it's fine
lazy val command = | ||
if (isSemanticdbPluginAvailable) withSemanticdbPluginCommand | ||
else withoutSemanticdbPluginCommand | ||
|
||
private lazy val withSemanticdbPluginCommand = Command.command( | ||
"scalafixEnable", | ||
briefHelp = "Configure SemanticdbPlugin and scalaVersion for scalafix.", | ||
detail = | ||
"""1. enables SemanticdbPlugin with the right version supported by scalafix | ||
|2. sets scalaVersion to latest Scala version supported by scalafix""".stripMargin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the implementation of scalafixEnable
for sbt 1.3+ users, so it has the potential of breaking a few things, especially on complex projetcts with many modules and/or scala versions. I wonder if we could smoke-test it with scala steward that relies on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling semanticdb with the setting key semanticdbEnabled
before Scala 3, is supposed to be equivalent to adding it through enablePlugin (for sbt 1.3+). As far as I understood, it translates to adding -Xplugin:...
@adpi2 do you think we need to be more careful with this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, using SemanticdbPlugin
should be equivalent, but there are subtle changes in terms of scopes that might make it regress (see my comment below also).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the amended commit, I have reworked how SemanticdbPlugin
is enabled to make sure it's matching where semanticdb-scalac
was currently injected prior to this PR (AND on scala3 obviously). I also added a dedicated scripted to test regressions of scalafixEnable
.
|
||
//FIXME: remove when scalafixVersion is bumped to 0.9.28 | ||
resolvers += Resolver.sonatypeRepo("snapshots") | ||
dependencyOverrides += "ch.epfl.scala" % "scalafix-interfaces" % "0.9.27+52-6c9eeec9-SNAPSHOT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1 @@ | |||
sbt.version=1.5.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way scripted are tested against different versions of sbt is quite messy at the moment, but I would rather wait for scalacenter/scalafix#1333 to clean it up.
inConfig(_)(ScalafixPlugin.relaxScalacOptionsConfigSettings) | ||
) | ||
val settings = for { | ||
p <- extracted.structure.allProjectRefs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is enabling SemanticdbPlugin
on all projects of the build on purpose, since projectsWithMatchingScalaVersion
would ignore Scala 3 (as it's driven by the full Scala version semanticdb-scalac
is published against, so irrelevant as semanticdb is baked-in in the Scala 3 compiler). I can make it slightly more precise though, by adding an explicit case for Scala 3, to avoid enabling semanticdb on Scala 2.10 projects (which would fail).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in the amended commit
interface: ScalafixInterface | ||
): Seq[String] = { | ||
if (files.isEmpty) Nil | ||
else { | ||
val errors = ListBuffer.empty[String] | ||
val hasSemanticdb = | ||
dependencies.exists(_.name.startsWith("semanticdb-scalac")) | ||
dependencies.exists(_.name.startsWith("semanticdb-scalac")) || | ||
scalacOpts.contains("-Xsemanticdb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to support the final release of scala 3, so I agree it's fine
lazy val command = | ||
if (isSemanticdbPluginAvailable) withSemanticdbPluginCommand | ||
else withoutSemanticdbPluginCommand | ||
|
||
private lazy val withSemanticdbPluginCommand = Command.command( | ||
"scalafixEnable", | ||
briefHelp = "Configure SemanticdbPlugin and scalaVersion for scalafix.", | ||
detail = | ||
"""1. enables SemanticdbPlugin with the right version supported by scalafix | ||
|2. sets scalaVersion to latest Scala version supported by scalafix""".stripMargin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling semanticdb with the setting key semanticdbEnabled
before Scala 3, is supposed to be equivalent to adding it through enablePlugin (for sbt 1.3+). As far as I understood, it translates to adding -Xplugin:...
@adpi2 do you think we need to be more careful with this change ?
|
||
private lazy val isSemanticdbPluginAvailable = Try { | ||
Class.forName("sbt.plugins.SemanticdbPlugin") | ||
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is it the same as verifying that the scalacOptions contain -Xplugin:sematicdb ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is just a way of detecting that the semanticdb*
keys we redeclare above (since they are not available in the sbt 1.2.x we build against for retro compatibility) will have an effect. I find it more relevant to check for existence of that class in the classpath than the sbt version, but we could do that too:
sbt-scalafix/src/main/scala/scalafix/internal/sbt/SemanticRuleValidator.scala
Lines 41 to 45 in be4486b
val recommendedSetting = CrossVersion.partialVersion(sbtVersion) match { | |
case Some((1, n)) if n < 3 => atMostSbt12 | |
case Some((0, _)) => atMostSbt12 | |
case _ => atLeastSbt13(scalaVersion) | |
} |
(we should probably reuse the same helper for both cases actually since the intent is the same)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that code was extracted to SemanticdbPlugin
to be more explicit and shared
- use sbt 1.3+ SemanticdbPlugin to offload the choice of the right compiler options to get semanticdb output from the compiler or the compiler plugin (with the right semanticdb and Scala full version) depending on the Scala binary version - extend SemanticRuleValidator to detect Scala 3 compiler flag injected by SemanticdbPlugin - always pass the scala version for scalafix to parse with the right dialect
@@ -0,0 +1,19 @@ | |||
ThisBuild / scalafixDependencies += "ch.epfl.scala" %% "example-scalafix-rule" % "1.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contains a very simple semantic rule so it's good for an initial scripted until built-in rules pick up actual support. The only downside is that the rule does not even evaluate the semantic document, so I am not sure it really tests the generation of semanticdb compiler output... Maybe we could tag a 1.6.1 with https://github.com/scalacenter/sbt-scalafix/pull/125/files#diff-38f3b339536ccc14af957f820b7bfda9be5e1226210bb2d8938d7fab17003911R7 to lift this doubt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe!
@@ -0,0 +1,4 @@ | |||
# check that we can run a syntactic rule against a Scala 3 dialect source file | |||
-> scalafix --check LeakingImplicitClassVal | |||
> scalafix LeakingImplicitClassVal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a built-in syntactic rewriting rule that works on a scala3 source with significant indentation!
[info] [info] Running scalafix on 1 Scala sources
[info] [error] --- /tmp/sbt_704f6c79/src/main/scala/SignificantIndentation.scala
[info] [error] +++ <expected fix>
[info] [error] @@ -1,3 +1,3 @@
[info] [error] object SignificantIndentation:
[info] [error] - implicit class XtensionVal(val str: String) extends AnyVal:
[info] [error] + implicit class XtensionVal(private val str: String) extends AnyVal:
[info] [error] def doubled: String = str + str
🎉
@@ -383,6 +383,7 @@ object ScalafixPlugin extends AutoPlugin { | |||
Arg.PrintStream(errorLogger), | |||
Arg.Config(scalafixConf), | |||
Arg.Rules(shell.rules), | |||
Arg.ScalaVersion(scalaVersion.value), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that this change is required for the syntactic-only invocation in the scala-3
scripted to successfully parse a source with significant indentation.
|
||
/** Command to automatically enable semanticdb-scalac for shell session */ | ||
/** Command to automatically enable semanticdb compiler output for shell session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is getting quite complex as it currently handles:
- different sbt/Zinc versions
- 1.x with x >= 3 SemanticdbPlugin available
- 1.x with x < 3 SemanticdbPlugin unavailable
- 0.13.x SemanticdbPlugin unavailable & forcing usage of
Compat
- different scalac versions and how they may support semanticdb
- 3.x built-in
- 2.x with x >= 11 supported by semanticdb-scalac
- 2.x with x < 11 unsupported by semanticdb-scalac
@@ -1,5 +1,6 @@ | |||
version = "2.7.5" | |||
version = "3.0.0-RC2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumped in a preliminary commit to support the sources with significant indentation in the scripted tests
Showcases scalacenter/scalafix#1392
See open questions / limitations in scalacenter/scalafix#1316 (comment)
Note that most of the built-in Scalafix rules do not support Scala 3 sources at the moment.