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

Test Scala 3 input/output with rule built against Scala 2.12 #19

Merged
merged 7 commits into from
May 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .scalafmt.conf
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
version = 2.7.5
project.git = true
assumeStandardLibraryStripMargin = true
project.excludeFilters = [
"main/g8"
"main/g8/.*/.*\\.scala"
"output/src"
"migration-rules/*"
]
9 changes: 6 additions & 3 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ lazy val root = (project in file("."))
val _ = (Test / g8Test).toTask("").value
val s = streams.value
s.log.info(
"running our own sbt in the copied directory to mimic the scripted test")
val p = Process(Seq("sbt", "tests/test"),
(Test / g8 / target).value / "scalafix").run()
"running our own sbt in the copied directory to mimic the scripted test"
)
val p = Process(
Seq("sbt", "tests/test"),
(Test / g8 / target).value / "scalafix"
).run()
assert(p.exitValue() == 0, "Non-zero exit from sbt tests/test")
},
scriptedLaunchOpts ++= List(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,26 @@ class Scalafixg8_v0_6 extends SyntacticRule("Scalafixg8_v0_6") {
doc.tree.traverse {
case Init(rule @ Type.Name("Rule"), _, List(List(Lit.String(_)))) =>
patch += Patch.replaceTree(rule, "SyntacticRule")
case init @ Init(Type.Name("SemanticRule"),
_,
List(List(Term.Name("index"), Lit.String(ruleName)))) =>
case init @ Init(
Type.Name("SemanticRule"),
_,
List(List(Term.Name("index"), Lit.String(ruleName)))
) =>
patch += Patch.replaceTree(init, s"""SemanticRule("$ruleName")""")
case Ctor.Primary(
_,
_,
List(
_,
_,
List(
param @ Term.Param(Nil,
Term.Name("index"),
Some(Type.Name("SemanticdbIndex")),
_)))) =>
List(
param @ Term.Param(
Nil,
Term.Name("index"),
Some(Type.Name("SemanticdbIndex")),
_
)
)
)
) =>
patch += Patch.removeTokens(param.tokens)
case defn @ Defn.Def(
_,
Expand All @@ -75,7 +82,8 @@ class Scalafixg8_v0_6 extends SyntacticRule("Scalafixg8_v0_6") {
ctx @ Term.Name("ctx"),
Some(ruleCtx @ Type.Name("RuleCtx")),
None
))
)
)
),
Some(Type.Name("Patch")),
_
Expand Down Expand Up @@ -151,11 +159,15 @@ class Scalafixg8_v0_6 extends SyntacticRule("Scalafixg8_v0_6") {
)
case Importer(ref, importees) =>
val syntax = ref.syntax
if (syntax.startsWith("scala.meta.contrib") ||
syntax.startsWith("scalafix.syntax")) {
if (
syntax.startsWith("scala.meta.contrib") ||
syntax.startsWith("scalafix.syntax")
) {
// do nothing
} else if (syntax.startsWith("org.langmeta.") ||
syntax.startsWith("scala.meta.")) {
} else if (
syntax.startsWith("org.langmeta.") ||
syntax.startsWith("scala.meta.")
) {
addImporter(importer"scala.meta._", importees)
} else if (syntax.startsWith("scalafix")) {
addImporter(importer"scalafix.v1._", importees)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ class v0_9_28 extends SemanticRule("v0_9_28") {
case t @ Importee.Name(Name("SemanticRuleSuite")) =>
Patch.removeImportee(t) +
Patch.addGlobalImport(
importer"scalafix.testkit.AbstractSemanticRuleSuite")
importer"scalafix.testkit.AbstractSemanticRuleSuite"
)
case t @ init"SemanticRuleSuite(..$_)" =>
Patch.addGlobalImport(importer"org.scalatest.FunSuiteLike") +
Patch.replaceTree(t, "AbstractSemanticRuleSuite with FunSuiteLike")
Expand Down
Binary file modified scalafmt
Binary file not shown.
5 changes: 2 additions & 3 deletions src/main/g8/default.properties
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
repo = My Repo
scalameta_version = maven(org.scalameta, scalameta_2.13)
scalafix_version = maven(ch.epfl.scala, sbt-scalafix)
repo = scalafix-my-rules
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This way, I am hoping people name their repo scalafix-*, just like sbt plugins are sbt-*. https://github.com/liancheng/scalafix-organize-imports does, but not the others.

scalafix_version = 0.9.28
101 changes: 76 additions & 25 deletions src/main/g8/scalafix/build.sbt
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
lazy val V = _root_.scalafix.sbt.BuildInfo

lazy val rulesCrossVersions = Seq(V.scala213, V.scala212, V.scala211)
lazy val scala3Version = "3.0.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As 3.x releases come out, we might want to test against different versions (although upgrading scala should not be as hard as on the 2.x era obviously). When/if that happens, we should use the full scala version as matrix suffixes, but I don't see a reason to do it now.


inThisBuild(
List(
scalaVersion := V.scala213,
crossScalaVersions := List(V.scala213, V.scala212, V.scala211),
organization := "com.example",
homepage := Some(url("https://github.com/com/example")),
licenses := List("Apache-2.0" -> url("http://www.apache.org/licenses/LICENSE-2.0")),
licenses := List(
"Apache-2.0" -> url("http://www.apache.org/licenses/LICENSE-2.0")
),
developers := List(
Developer(
"example-username",
Expand All @@ -14,41 +18,88 @@ inThisBuild(
url("https://example.com")
)
),
addCompilerPlugin(scalafixSemanticdb),
scalacOptions ++= List(
"-Yrangepos",
"-P:semanticdb:synthetics:on"
)
semanticdbEnabled := true,
semanticdbVersion := scalafixSemanticdb.revision
)
)

publish / skip := true
lazy val `$repo;format="normalize"$` = (project in file("."))
.aggregate(
rules.projectRefs ++
input.projectRefs ++
output.projectRefs ++
tests.projectRefs: _*
)
.settings(
publish / skip := true
)
Comment on lines +26 to +35
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's ugly, but without that scalafmtAll at the root wouldn't work... I am wondering if this couldn't trigger race conditions when reformatting rules* concurrently actually, but this is not specific to this PR, more to sbt-projectmatrix.


lazy val rules = project.settings(
moduleName := "scalafix",
libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % V.scalafixVersion
)
lazy val rules = projectMatrix
.settings(
moduleName := "scalafix",
libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % V.scalafixVersion
)
.defaultAxes(VirtualAxis.jvm)
.jvmPlatform(rulesCrossVersions)

lazy val input = project.settings(
publish / skip := true
)
lazy val input = projectMatrix
.settings(
publish / skip := true
)
.defaultAxes(VirtualAxis.jvm)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

suffix all projects for clarity, including the 2.13 one

.jvmPlatform(scalaVersions = rulesCrossVersions :+ scala3Version)
Copy link

Choose a reason for hiding this comment

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

There are two calls to .jvmPlatform for input, with different scalaVersions. That seems off. Shouldn't the first one already mention rulesCrossVersions :+ scala3Version and the second one be removed?

Copy link
Collaborator Author

@bjaglin bjaglin May 19, 2021

Choose a reason for hiding this comment

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

Good catch. Actually, having different settings was the goal here, since -P:semanticdb:synthetics:on is not supported by dotty. Tests were passing as the first jvmPlatform call was effectively ignored, and -P:semanticdb:synthetics:on is not mandatory (I tried to keep it as it is there on main).

I have amended to follow my initial intent, but I wonder if it makes sense to keep -P:semanticdb:synthetics:on at all by default for Scala 2.x, since:

  • it's unnecessary for rules not relying on it (obviously)
  • authors of rules relying on it will need to disable Scala 3 testing for the time being

Copy link
Collaborator Author

@bjaglin bjaglin May 20, 2021

Choose a reason for hiding this comment

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

I have now removed the -P:semanticdb:synthetics:on completely, see bfa2e49


lazy val output = project.settings(
publish / skip := true
)
lazy val output = projectMatrix
.settings(
publish / skip := true
)
.defaultAxes(VirtualAxis.jvm)
.jvmPlatform(scalaVersions = rulesCrossVersions :+ scala3Version)

lazy val testsAggregate = Project("tests", file("target/testsAggregate"))
.aggregate(tests.projectRefs: _*)
Comment on lines +59 to +60
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added to maintain backward compatibility on CI statements (for people upgrading somehow) and on docs


lazy val tests = project
lazy val tests = projectMatrix
.settings(
publish / skip := true,
libraryDependencies += "ch.epfl.scala" % "scalafix-testkit" % V.scalafixVersion % Test cross CrossVersion.full,
Compile / compile :=
(Compile / compile).dependsOn(input / Compile / compile).value,
scalafixTestkitOutputSourceDirectories :=
(output / Compile / unmanagedSourceDirectories).value,
TargetAxis
.resolve(output, Compile / unmanagedSourceDirectories)
.value,
scalafixTestkitInputSourceDirectories :=
(input / Compile / unmanagedSourceDirectories).value,
TargetAxis
.resolve(input, Compile / unmanagedSourceDirectories)
.value,
scalafixTestkitInputClasspath :=
(input / Compile / fullClasspath).value,
TargetAxis.resolve(input, Compile / fullClasspath).value,
scalafixTestkitInputScalacOptions :=
TargetAxis.resolve(input, Compile / scalacOptions).value,
scalafixTestkitInputScalaVersion :=
TargetAxis.resolve(input, Compile / scalaVersion).value
)
.defaultAxes(
rulesCrossVersions.map(VirtualAxis.scalaABIVersion) :+ VirtualAxis.jvm: _*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

omit the scala version in the suffix to focus on what is being tested - the "target"

)
.customRow(
scalaVersions = Seq(V.scala212),
Copy link
Collaborator Author

@bjaglin bjaglin May 18, 2021

Choose a reason for hiding this comment

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

axisValues = Seq(TargetAxis(scala3Version), VirtualAxis.jvm),
settings = Seq()
)
.customRow(
scalaVersions = Seq(V.scala213),
axisValues = Seq(TargetAxis(V.scala213), VirtualAxis.jvm),
settings = Seq()
)
.customRow(
scalaVersions = Seq(V.scala212),
axisValues = Seq(TargetAxis(V.scala212), VirtualAxis.jvm),
settings = Seq()
)
.customRow(
scalaVersions = Seq(V.scala211),
axisValues = Seq(TargetAxis(V.scala211), VirtualAxis.jvm),
settings = Seq()
)
.dependsOn(rules)
.enablePlugins(ScalafixTestkitPlugin)
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
rule = $repo;format="Camel"$
*/
package fix

object $repo;format="Camel"$SignificantIndentation:
Copy link
Collaborator Author

@bjaglin bjaglin May 18, 2021

Choose a reason for hiding this comment

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

This was initially a way to test that everything works, but since we have now Scala3 scripteds in sbt-scalafix, this is not really required. I do like the fact that it demonstrates the usage of scala-3 intput/output.

val a = 1
// Add code that needs fixing here.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package fix

object $repo;format="Camel"$SignificantIndentation:
val a = 1
// Add code that needs fixing here.
47 changes: 47 additions & 0 deletions src/main/g8/scalafix/project/TargetAxis.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import sbt._
import sbt.internal.ProjectMatrix
import sbtprojectmatrix.ProjectMatrixPlugin.autoImport._

/** Use on ProjectMatrix rows to tag an affinity to a custom scalaVersion */
case class TargetAxis(scalaVersion: String) extends VirtualAxis.WeakAxis {

private val scalaBinaryVersion = CrossVersion.binaryScalaVersion(scalaVersion)

override val idSuffix = s"Target\${scalaBinaryVersion.replace('.', '_')}"
override val directorySuffix = s"target\$scalaBinaryVersion"
}

object TargetAxis {

private def targetScalaVersion(virtualAxes: Seq[VirtualAxis]): String =
virtualAxes.collectFirst { case a: TargetAxis => a.scalaVersion }.get

/** When invoked on a ProjectMatrix with a TargetAxis, lookup the project
* generated by `matrix` with a scalaVersion matching the one declared in
* that TargetAxis, and resolve `key`.
*/
def resolve[T](
matrix: ProjectMatrix,
key: TaskKey[T]
): Def.Initialize[Task[T]] =
Def.taskDyn {
val sv = targetScalaVersion(virtualAxes.value)
val project = matrix.finder().apply(sv)
Def.task((project / key).value)
}

/** When invoked on a ProjectMatrix with a TargetAxis, lookup the project
* generated by `matrix` with a scalaVersion matching the one declared in
* that TargetAxis, and resolve `key`.
*/
def resolve[T](
matrix: ProjectMatrix,
key: SettingKey[T]
): Def.Initialize[T] =
Def.settingDyn {
val sv = targetScalaVersion(virtualAxes.value)
val project = matrix.finder().apply(sv)
Def.setting((project / key).value)
}

}
1 change: 1 addition & 0 deletions src/main/g8/scalafix/project/plugins.sbt
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
resolvers += Resolver.sonatypeRepo("releases")
addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "$scalafix_version$")
addSbtPlugin("com.eed3si9n" % "sbt-projectmatrix" % "0.8.0")