From 582fe2651cf03ad3817623cbd58b6d79dc63e609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Fri, 2 Dec 2016 13:43:56 +0100 Subject: [PATCH 1/5] Change design of sbt plugin, again. The previous implementation was problematic because the compiler plugin didn't always load. This approach uses a similar design as scoverage, enabling/disabling the plugin via scalacOptions. --- build.sbt | 2 + core/src/main/scala/scalafix/Scalafix.scala | 1 + .../main/scala/scalafix/ScalafixConfig.scala | 22 +++++- core/src/main/scala/scalafix/Versions.scala | 2 +- .../src/main/resources/scalac-plugin.xml | 2 +- .../main/scala/scalafix/nsc/ScalafixNsc.scala | 14 ---- .../scalafix/nsc/ScalafixNscComponent.scala | 10 ++- .../scalafix/nsc/ScalafixNscPlugin.scala | 28 ++++++++ .../scala/scalafix/sbt/ScalafixPlugin.scala | 71 +++++++++---------- .../src/sbt-test/sbt-scalafix/basic/build.sbt | 3 +- .../sbt-scalafix/basic/project/plugins.sbt | 2 +- .../basic/src/main/scala/Test.scala | 1 + .../src/sbt-test/sbt-scalafix/basic/test | 2 + 13 files changed, 97 insertions(+), 63 deletions(-) delete mode 100644 scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNsc.scala create mode 100644 scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala diff --git a/build.sbt b/build.sbt index d7d4ed377..e6b7863b4 100644 --- a/build.sbt +++ b/build.sbt @@ -174,6 +174,7 @@ lazy val `scalafix-sbt` = project.settings( ScriptedPlugin.scriptedSettings, sbtPlugin := true, scalaVersion := "2.10.5", + moduleName := "sbt-scalafix", sources in Compile += baseDirectory.value / "../core/src/main/scala/scalafix/Versions.scala", scriptedLaunchOpts := Seq( @@ -234,3 +235,4 @@ def exposePaths(projectName: String, } ) } + diff --git a/core/src/main/scala/scalafix/Scalafix.scala b/core/src/main/scala/scalafix/Scalafix.scala index f3fcb8710..fefe7a198 100644 --- a/core/src/main/scala/scalafix/Scalafix.scala +++ b/core/src/main/scala/scalafix/Scalafix.scala @@ -7,6 +7,7 @@ import scalafix.rewrite.RewriteCtx import scalafix.rewrite.SemanticApi import scalafix.util.Patch import scalafix.util.TokenList +import scalafix.util.logger object Scalafix { def fix(code: Input, diff --git a/core/src/main/scala/scalafix/ScalafixConfig.scala b/core/src/main/scala/scalafix/ScalafixConfig.scala index 2d510c817..a8ac8d8b5 100644 --- a/core/src/main/scala/scalafix/ScalafixConfig.scala +++ b/core/src/main/scala/scalafix/ScalafixConfig.scala @@ -7,7 +7,27 @@ import scala.meta.parsers.Parse import scalafix.rewrite.Rewrite case class ScalafixConfig( - rewrites: Seq[Rewrite] = Rewrite.syntaxRewrites, + rewrites: Seq[Rewrite] = Rewrite.allRewrites, parser: Parse[_ <: Tree] = Parse.parseSource, dialect: Dialect = Scala211 ) + +object ScalafixConfig { + def fromNames(names: List[String]): Either[String, ScalafixConfig] = { + names match { + case "all" :: Nil => + Right(ScalafixConfig(rewrites = Rewrite.allRewrites)) + case _ => + val invalidNames = + names.filterNot(Rewrite.name2rewrite.contains) + if (invalidNames.nonEmpty) { + Left( + s"Invalid rewrite rule: ${invalidNames.mkString(",")}. " + + s"Valid rules are: ${Rewrite.name2rewrite.keys.mkString(",")}") + } else { + val rewrites = names.map(Rewrite.name2rewrite) + Right(ScalafixConfig(rewrites = rewrites)) + } + } + } +} diff --git a/core/src/main/scala/scalafix/Versions.scala b/core/src/main/scala/scalafix/Versions.scala index 112323f20..9172ca4b1 100644 --- a/core/src/main/scala/scalafix/Versions.scala +++ b/core/src/main/scala/scalafix/Versions.scala @@ -2,6 +2,6 @@ package scalafix object Versions { val nightly = "0.2.0-SNAPSHOT" - val stable = nightly + val stable: String = nightly val scala = "2.11.8" } diff --git a/scalafix-nsc/src/main/resources/scalac-plugin.xml b/scalafix-nsc/src/main/resources/scalac-plugin.xml index 0426e4605..d3d16e8be 100644 --- a/scalafix-nsc/src/main/resources/scalac-plugin.xml +++ b/scalafix-nsc/src/main/resources/scalac-plugin.xml @@ -1,4 +1,4 @@ scalafix - scalafix.nsc.ScalafixNsc + scalafix.nsc.ScalafixNscPlugin diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNsc.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNsc.scala deleted file mode 100644 index 82b24060d..000000000 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNsc.scala +++ /dev/null @@ -1,14 +0,0 @@ -package scalafix.nsc - -import scala.meta.Defn -import scala.meta.Type -import scala.tools.nsc.Global -import scala.tools.nsc.plugins.Plugin -import scala.tools.nsc.plugins.PluginComponent - -class ScalafixNsc(val global: Global) extends Plugin { - val name = "scalafix" - val description = "Refactoring tool." - val components: List[PluginComponent] = - new ScalafixNscComponent(this, global) :: Nil -} diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala index 675e7755d..16090718c 100644 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala @@ -6,11 +6,11 @@ import scala.tools.nsc.plugins.Plugin import scala.tools.nsc.plugins.PluginComponent import scalafix.Fixed import scalafix.ScalafixConfig -import scalafix.rewrite.Rewrite import scalafix.util.FileOps -import scalafix.util.logger -class ScalafixNscComponent(plugin: Plugin, val global: Global) +class ScalafixNscComponent(plugin: Plugin, + val global: Global, + getConfig: () => ScalafixConfig) extends PluginComponent with ReflectToolkit with NscSemanticApi { @@ -23,9 +23,7 @@ class ScalafixNscComponent(plugin: Plugin, val global: Global) if (unit.source.file.exists && unit.source.file.file.isFile && !unit.isJava) { - // TODO(olafur) pull out rewrite rules from configuration flags. - val rewrites = Rewrite.semanticRewrites - fix(unit, ScalafixConfig(rewrites = rewrites)) match { + fix(unit, getConfig()) match { case Fixed.Success(fixed) => FileOps.writeFile(unit.source.file.file, fixed) case Fixed.Failed(e) => diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala new file mode 100644 index 000000000..e13bdd356 --- /dev/null +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala @@ -0,0 +1,28 @@ +package scalafix.nsc + +import scala.meta.Defn +import scala.meta.Type +import scala.tools.nsc.Global +import scala.tools.nsc.plugins.Plugin +import scala.tools.nsc.plugins.PluginComponent +import scalafix.ScalafixConfig +import scalafix.rewrite.Rewrite + +class ScalafixNscPlugin(val global: Global) extends Plugin { + val name = "scalafix" + val description = "Refactoring tool." + var config: ScalafixConfig = ScalafixConfig(rewrites = Rewrite.allRewrites) + val components: List[PluginComponent] = + new ScalafixNscComponent(this, global, () => config) :: Nil + + override def init(options: List[String], error: (String) => Unit): Boolean = { + ScalafixConfig.fromNames(options) match { + case Left(msg) => + error(msg) + false + case Right(userConfig) => + config = userConfig + true + } + } +} diff --git a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index f844eeafb..fe19aa9ff 100644 --- a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -5,55 +5,50 @@ import sbt._ import sbt.plugins.JvmPlugin trait ScalafixKeys { - val scalafixCompile: TaskKey[Unit] = - taskKey[Unit]( - "Fix Scala sources using scalafix. Note, this task runs clean.") + val scalafixRewrites: SettingKey[Seq[String]] = + settingKey[Seq[String]]("Which scalafix rules should run?") + val scalafixEnabled: SettingKey[Boolean] = + settingKey[Boolean]("Is scalafix enabled?") +} - val scalafixConfigurations: SettingKey[Seq[Configuration]] = - settingKey[Seq[Configuration]]( - "Which configurations should scalafix run in?" + - " Defaults to Compile and Test.") +object rewrite { + // TODO(olafur) share these with scalafix-core. + val ProcedureSyntax = "ProcedureSyntax" + val ExplicitImplicit = "ExplicitImplicit" + val VolatileLazyVal = "VolatileLazyVal" } object ScalafixPlugin extends AutoPlugin with ScalafixKeys { + object autoImport extends ScalafixKeys + private val nightlyVersion = _root_.scalafix.Versions.nightly + private val disabled = sys.props.contains("scalafix.disable") override def requires = JvmPlugin override def trigger: PluginTrigger = AllRequirements - val nightlyVersion: String = _root_.scalafix.Versions.nightly - val scalafix: Command = Command.command( - name = "scalafix", - briefHelp = "Run scalafix rewrite rules. Note, will clean the build.", - detail = - "Injects the scalafix compiler plugin into your sbt session " + - "and then clean compiles all source files. " + - "Scalafix rewrite rules are executed from within the compiler " + - "plugin during compilation. " + - "Once scalafix has completed, the session is clear." - ) { state => - // TODO(olafur) Is there a cleaner way to accomplish the same? - // Requirements: the plugin must be enabled only during the scalafix task/command. - val addScalafixCompilerPluginSetting: String = - s"""libraryDependencies in ThisBuild += - | compilerPlugin("ch.epfl.scala" %% "scalafix-nsc" % "$nightlyVersion")""".stripMargin - s"set $addScalafixCompilerPluginSetting" :: - "scalafixCompile" :: - s"session clear" :: + + val scalafix: Command = Command.command("scalafix") { state => + s"set scalafixEnabled in Global := true" :: + "clean" :: + "test:compile" :: + s"set scalafixEnabled in Global := false" :: state } override def projectSettings: Seq[Def.Setting[_]] = Seq( + addCompilerPlugin("ch.epfl.scala" %% "scalafix-nsc" % nightlyVersion), commands += scalafix, - scalafixConfigurations := Seq(Compile, Test), - scalafixCompile := Def.taskDyn { - val cleanCompileInAllConfigurations = - scalafixConfigurations.value.map(x => - (compile in x).dependsOn(clean in x)) - Def - .task(()) - .dependsOn(cleanCompileInAllConfigurations: _*) - }.value + scalafixRewrites := Seq( + rewrite.ExplicitImplicit, + rewrite.ProcedureSyntax + ), + scalafixEnabled in Global := false, + scalacOptions ++= { + val rewrites = scalafixRewrites.value + if (!scalafixEnabled.value && rewrites.isEmpty) Nil + else { + val prefixed = rewrites.map(x => s"scalafix:$x") + Seq(s"-P:${prefixed.mkString(",")}") + } + } ) - - object autoImport extends ScalafixKeys - } diff --git a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt index 8398a3914..9f2cc0cae 100644 --- a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt +++ b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt @@ -32,7 +32,8 @@ TaskKey[Unit]("check") := { """ |object Main { | implicit val x: Int = 23 - | def main(args: Array[String]) { + | lazy val y = 2 + | def main(args: Array[String]): Unit = { | println("hello") | } |} diff --git a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/project/plugins.sbt b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/project/plugins.sbt index 41c33fbc1..843726faf 100644 --- a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/project/plugins.sbt +++ b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/project/plugins.sbt @@ -1 +1 @@ -addSbtPlugin("ch.epfl.scala" % "scalafix-sbt" % sys.props("plugin.version")) +addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % sys.props("plugin.version")) diff --git a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/src/main/scala/Test.scala b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/src/main/scala/Test.scala index e2bb8c8a3..f775b1060 100644 --- a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/src/main/scala/Test.scala +++ b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/src/main/scala/Test.scala @@ -1,5 +1,6 @@ object Main { implicit val x = 23 + lazy val y = 2 def main(args: Array[String]) { println("hello") } diff --git a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/test b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/test index 2848a5303..bb99e9ff9 100644 --- a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/test +++ b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/test @@ -1,3 +1,5 @@ +# compile should not affect scalafix +> compile > scalafix > check From 8328b20ae3ad72452327cebf1d3e5664f8d3da2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Mon, 5 Dec 2016 11:18:58 +0100 Subject: [PATCH 2/5] Try to fix plugin loading error --- bin/testAll.sh | 2 +- build.sbt | 4 ++++ .../src/main/scala/scalafix/sbt/ScalafixPlugin.scala | 7 +++---- scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt | 2 ++ 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/bin/testAll.sh b/bin/testAll.sh index 5c155a2ca..85ef3595b 100755 --- a/bin/testAll.sh +++ b/bin/testAll.sh @@ -1,3 +1,3 @@ #!/bin/bash set -e -sbt clean test publishLocal scripted +sbt clean test scripted diff --git a/build.sbt b/build.sbt index e6b7863b4..845de9e63 100644 --- a/build.sbt +++ b/build.sbt @@ -173,6 +173,10 @@ lazy val `scalafix-sbt` = project.settings( allSettings, ScriptedPlugin.scriptedSettings, sbtPlugin := true, + scripted := scripted.dependsOn( + publishLocal in `scalafix-nsc`, + publishLocal in core + ).evaluated, scalaVersion := "2.10.5", moduleName := "sbt-scalafix", sources in Compile += diff --git a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index fe19aa9ff..4758579c8 100644 --- a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -27,10 +27,9 @@ object ScalafixPlugin extends AutoPlugin with ScalafixKeys { val scalafix: Command = Command.command("scalafix") { state => s"set scalafixEnabled in Global := true" :: - "clean" :: - "test:compile" :: - s"set scalafixEnabled in Global := false" :: - state + "test:compile" :: + s"set scalafixEnabled in Global := false" :: + state } override def projectSettings: Seq[Def.Setting[_]] = diff --git a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt index 9f2cc0cae..5278b57ee 100644 --- a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt +++ b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt @@ -1,4 +1,6 @@ scalaVersion in ThisBuild := "2.11.8" +scalacOptions in ThisBuild += "-Ydebug" + lazy val root = project .in(file(".")) .aggregate(p1) From c635507971a9ac3173047420a210e92d62690c69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Mon, 5 Dec 2016 14:05:35 +0100 Subject: [PATCH 3/5] Use same setup as scoverage --- .../scala/scalafix/sbt/ScalafixPlugin.scala | 44 +++++++++++++++---- .../src/sbt-test/sbt-scalafix/basic/build.sbt | 1 - 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 4758579c8..f91b72d82 100644 --- a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -22,19 +22,29 @@ object ScalafixPlugin extends AutoPlugin with ScalafixKeys { object autoImport extends ScalafixKeys private val nightlyVersion = _root_.scalafix.Versions.nightly private val disabled = sys.props.contains("scalafix.disable") + private val ScalafixPluginConfig = config("scalafix").hide override def requires = JvmPlugin override def trigger: PluginTrigger = AllRequirements val scalafix: Command = Command.command("scalafix") { state => s"set scalafixEnabled in Global := true" :: - "test:compile" :: - s"set scalafixEnabled in Global := false" :: - state + "clean" :: + "test:compile" :: + s"set scalafixEnabled in Global := false" :: + state } override def projectSettings: Seq[Def.Setting[_]] = Seq( - addCompilerPlugin("ch.epfl.scala" %% "scalafix-nsc" % nightlyVersion), + ivyConfigurations += ScalafixPluginConfig, + libraryDependencies ++= { + if (!scalafixEnabled.value) Nil + else { + Seq( + "ch.epfl.scala" %% "scalafix-nsc" % nightlyVersion % ScalafixPluginConfig + ) + } + }, commands += scalafix, scalafixRewrites := Seq( rewrite.ExplicitImplicit, @@ -42,11 +52,29 @@ object ScalafixPlugin extends AutoPlugin with ScalafixKeys { ), scalafixEnabled in Global := false, scalacOptions ++= { - val rewrites = scalafixRewrites.value - if (!scalafixEnabled.value && rewrites.isEmpty) Nil + // scalafix should not affect compilations outside of the scalafix task. + // The approach taken here is the same as scoverage uses, see: + // https://github.com/scoverage/sbt-scoverage/blob/45ac49583f5a32dfebdce23b94c5336da4906e59/src/main/scala/scoverage/ScoverageSbtPlugin.scala#L70-L83 + if (!scalafixEnabled.value) Nil else { - val prefixed = rewrites.map(x => s"scalafix:$x") - Seq(s"-P:${prefixed.mkString(",")}") + val scalafixNscPluginJar = update.value + .matching(configurationFilter(ScalafixPluginConfig.name)) + .find(_.getAbsolutePath.matches(".*scalafix-nsc[^-]*.jar$")) + .getOrElse { + throw new IllegalStateException( + "Unable to find scalafix-nsc in library dependencies!") + } + val rewrites = scalafixRewrites.value + val config: Option[String] = + if (rewrites.isEmpty) None + else { + val prefixed = rewrites.map(x => s"scalafix:$x") + Some(s"-P:${prefixed.mkString(",")}") + } + Seq( + Some(s"-Xplugin:${scalafixNscPluginJar.getAbsolutePath}"), + config + ).flatten } } ) diff --git a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt index 5278b57ee..09b858bb4 100644 --- a/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt +++ b/scalafix-sbt/src/sbt-test/sbt-scalafix/basic/build.sbt @@ -1,5 +1,4 @@ scalaVersion in ThisBuild := "2.11.8" -scalacOptions in ThisBuild += "-Ydebug" lazy val root = project .in(file(".")) From da08340d105824175eba888d27e9becc6a787d76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Mon, 5 Dec 2016 15:29:04 +0100 Subject: [PATCH 4/5] Don't annotate vals with implicitly[T] rhs --- .../scala/scalafix/rewrite/ExplicitImplicit.scala | 11 ++++++++++- core/src/test/resources/ExplicitImplicit/basic.source | 10 ++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala b/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala index 45a9babdb..cc528cb21 100644 --- a/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala +++ b/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala @@ -5,6 +5,14 @@ import scalafix.util.Patch import scalafix.util.Whitespace case object ExplicitImplicit extends Rewrite { + // Don't explicitly annotate vals when the right-hand body is a single call + // to `implicitly`. Prevents ambiguous implicit. Not annotating in such cases, + // this a common trick employed implicit-heavy code to workaround SI-2712. + // Context: https://gitter.im/typelevel/cats?at=584573151eb3d648695b4a50 + private def isImplicitly(term: m.Term): Boolean = term match { + case m.Term.ApplyType(m.Term.Name("implicitly"), _) => true + case _ => false + } override def rewrite(ast: m.Tree, ctx: RewriteCtx): Seq[Patch] = { import scala.meta._ val semantic = getSemanticApi(ctx) @@ -23,7 +31,8 @@ case object ExplicitImplicit extends Rewrite { }.toSeq ast.collect { case t @ m.Defn.Val(mods, _, None, body) - if mods.exists(_.syntax == "implicit") => + if mods.exists(_.syntax == "implicit") && + !isImplicitly(body) => fix(t, body) case t @ m.Defn.Def(mods, _, _, _, None, body) if mods.exists(_.syntax == "implicit") => diff --git a/core/src/test/resources/ExplicitImplicit/basic.source b/core/src/test/resources/ExplicitImplicit/basic.source index 0b045e985..ae103be02 100644 --- a/core/src/test/resources/ExplicitImplicit/basic.source +++ b/core/src/test/resources/ExplicitImplicit/basic.source @@ -90,3 +90,13 @@ package foo { class B { implicit val x: foo.A[Int] = new foo.A(10) } +<<< implicitly 2712 trick +class A { + implicit val s = "string" + implicit val x = implicitly[String] +} +>>> +class A { + implicit val s: String = "string" + implicit val x = implicitly[String] +} From f317b226e71fb14f3993bae87e48f18309373cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Mon, 5 Dec 2016 18:57:44 +0100 Subject: [PATCH 5/5] Polish type stripping --- .../resources/ExplicitImplicit/basic.source | 72 +++++++++++++++ .../scala/scalafix/nsc/NscSemanticApi.scala | 89 +++++++++++++++---- .../scalafix/nsc/ScalafixNscComponent.scala | 6 +- .../test/scala/scalafix/SemanticTests.scala | 15 +++- 4 files changed, 160 insertions(+), 22 deletions(-) diff --git a/core/src/test/resources/ExplicitImplicit/basic.source b/core/src/test/resources/ExplicitImplicit/basic.source index ae103be02..b60df6650 100644 --- a/core/src/test/resources/ExplicitImplicit/basic.source +++ b/core/src/test/resources/ExplicitImplicit/basic.source @@ -100,3 +100,75 @@ class A { implicit val s: String = "string" implicit val x = implicitly[String] } +<<< shorten imported name +import scala.collection.immutable.Map +class A { + implicit val x = Map(1 -> "") +} +>>> +import scala.collection.immutable.Map +class A { + implicit val x: Map[Int, String] = Map(1 -> "") +} +<<< shorten imported name 2 +import scala.collection.immutable._ +class A { + implicit val x = Map(1 -> "") +} +>>> +import scala.collection.immutable._ +class A { + implicit val x: Map[Int, String] = Map(1 -> "") +} +<<< enclosing package strip is last +package b { class B } +package a { + import b.B + class A { + implicit val x = new B + } +} +>>> +package b { class B } +package a { + import b.B + class A { + implicit val x: B = new B + } +} +<<< inner inner object +object A { + object B { + class C + object C { + implicit val x = List(new C) + } + } +} +>>> +object A { + object B { + class C + object C { + implicit val x: List[C] = List(new C) + } + } +} +<<< sibling classes +object D { + class B +} +object A { + class C { + implicit val x = List(new D.B) + } +} +>>> +object D { + class B +} +object A { + class C { + implicit val x: List[D.B] = List(new D.B) + } +} diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala index 45bc37285..1523cac07 100644 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala @@ -2,50 +2,101 @@ package scalafix.nsc import scala.collection.mutable import scala.meta.Dialect +import scala.meta.Type import scala.reflect.internal.util.SourceFile import scala.{meta => m} import scalafix.Fixed import scalafix.Scalafix import scalafix.ScalafixConfig import scalafix.rewrite.SemanticApi +import scalafix.util.logger -trait NscSemanticApi extends ReflectToolkit { +case class SemanticContext(enclosingPackage: String, inScope: List[String]) - /** Removes redudant Foo.this.ActualType prefix from a type */ - private def stripRedundantThis(typ: m.Type): m.Type = - typ.transform { - case m.Term.Select(m.Term.This(m.Name.Indeterminate(_)), qual) => qual - case m.Type.Select(m.Term.This(m.Name.Indeterminate(_)), qual) => qual - }.asInstanceOf[m.Type] +trait NscSemanticApi extends ReflectToolkit { /** Returns a map from byte offset to type name at that offset. */ private def offsetToType(gtree: g.Tree, dialect: Dialect): mutable.Map[Int, m.Type] = { + // TODO(olafur) Come up with more principled approach, this is hacky as hell. + // Operating on strings is definitely the wrong way to approach this + // problem. Ideally, this implementation uses the tree/symbol api. However, + // while I'm just trying to get something simple working I feel a bit more + // productive with this hack. val builder = mutable.Map.empty[Int, m.Type] - def add(gtree: g.Tree, enclosingPackage: String): Unit = { - // TODO(olafur) Come up with more principled approach, this is hacky as hell. - val typename = gtree.toString().stripPrefix(enclosingPackage) - val parsed = dialect(typename).parse[m.Type] + def add(gtree: g.Tree, ctx: SemanticContext): Unit = { + + /** Removes redudant Foo.this.ActualType prefix from a type */ + val stripRedundantThis: m.Type => m.Type = _.transform { + case m.Term.Select(m.Term.This(m.Name.Indeterminate(_)), qual) => + qual + case m.Type.Select(m.Term.This(m.Name.Indeterminate(_)), qual) => + qual + }.asInstanceOf[m.Type] + + val stripImportedPrefix: m.Type => m.Type = _.transform { + case prefix @ m.Type.Select(_, name) + if ctx.inScope.contains(prefix.syntax) => + name + }.asInstanceOf[m.Type] + + val stripEnclosingPackage: m.Type => m.Type = _.transform { + case typ: m.Type.Ref => + import scala.meta._ +// logger.elem(ctx.enclosingPackage, typ.syntax) + typ.syntax.stripPrefix(ctx.enclosingPackage).parse[m.Type].get + }.asInstanceOf[m.Type] + + val cleanUp: (Type) => Type = + stripRedundantThis andThen + stripImportedPrefix andThen + stripEnclosingPackage + + val typename = gtree.toString() + val toImport: String = + if (ctx.inScope.contains(typename)) typename.replace(".*\\.", "") + else typename + val parsed = dialect(toImport).parse[m.Type] +// logger.elem(g.showRaw(gtree), typename, toImport, ctx.inScope) parsed match { case m.Parsed.Success(ast) => - builder(gtree.pos.point) = stripRedundantThis(ast) + builder(gtree.pos.point) = cleanUp(ast) case _ => } } - def foreach(gtree: g.Tree, enclosingPkg: String): Unit = { + def evaluate(ctx: SemanticContext, gtree: g.Tree): SemanticContext = { gtree match { - case g.ValDef(_, _, tpt, _) if tpt.nonEmpty => add(tpt, enclosingPkg) - case g.DefDef(_, _, _, _, tpt, _) => add(tpt, enclosingPkg) + case g.ValDef(_, _, tpt, _) if tpt.nonEmpty => add(tpt, ctx) + case g.DefDef(_, _, _, _, tpt, _) => add(tpt, ctx) case _ => } gtree match { case g.PackageDef(pid, _) => - gtree.children.foreach(x => foreach(x, pid.symbol.fullName + ".")) - case _ => - gtree.children.foreach(x => foreach(x, enclosingPkg)) + val newCtx = ctx.copy(enclosingPackage = pid.symbol.fullName + ".") + gtree.children.foldLeft(newCtx)(evaluate) + ctx // leaving pkg scope + case t: g.Template => + val newCtx = + ctx.copy(inScope = t.symbol.owner.fullName :: ctx.inScope) + gtree.children.foldLeft(newCtx)(evaluate) + ctx + case g.Import(expr, selectors) => + val newNames: Seq[String] = selectors.collect { + case g.ImportSelector(from, _, to, _) if from == to => + Seq(s"${expr.symbol.fullName}.$from") + case g.ImportSelector(from, _, null, _) => + expr.tpe.members.collect { + case x if !x.fullName.contains("$") => + x.fullName + } + }.flatten + ctx.copy(inScope = ctx.inScope ++ newNames) + case els => +// logger.elem(els, g.showRaw(els)) + gtree.children.foldLeft(ctx)(evaluate) } } - foreach(gtree, "") + evaluate(SemanticContext("", Nil), gtree) builder } diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala index 16090718c..60e53023d 100644 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala @@ -8,6 +8,8 @@ import scalafix.Fixed import scalafix.ScalafixConfig import scalafix.util.FileOps +import java.util + class ScalafixNscComponent(plugin: Plugin, val global: Global, getConfig: () => ScalafixConfig) @@ -25,7 +27,9 @@ class ScalafixNscComponent(plugin: Plugin, !unit.isJava) { fix(unit, getConfig()) match { case Fixed.Success(fixed) => - FileOps.writeFile(unit.source.file.file, fixed) + if (fixed.nonEmpty && fixed != new String(unit.source.content)) { + FileOps.writeFile(unit.source.file.file, fixed) + } case Fixed.Failed(e) => g.reporter.warning(unit.body.pos, "Failed to run scalafix. " + e.getMessage) diff --git a/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala b/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala index 7c0b718e2..3381d9ee3 100644 --- a/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala +++ b/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala @@ -6,10 +6,12 @@ import scala.tools.nsc.CompilerCommand import scala.tools.nsc.Global import scala.tools.nsc.Settings import scala.tools.nsc.reporters.StoreReporter +import scala.util.control.NonFatal import scala.{meta => m} import scalafix.nsc.NscSemanticApi import scalafix.rewrite.ExplicitImplicit import scalafix.rewrite.Rewrite +import scalafix.util.logger import org.scalatest.FunSuite @@ -84,7 +86,7 @@ class SemanticTests extends FunSuite { tree } def wrap(code: String, name: String): String = { - val packageName = name.replaceAll("[^a-zA-Z]", "") + val packageName = name.replaceAll("[^a-zA-Z0-9]", "") val packagedCode = s"package $packageName { $code }" packagedCode } @@ -158,6 +160,13 @@ class SemanticTests extends FunSuite { } loop(obtained, expected) } + private def typeChecks(code: String): Unit = { + try { + getTypedCompilationUnit(code) + } catch { + case NonFatal(e) => fail("Fixed source code does not typecheck!", e) + } + } private def parse(code: String): m.Tree = { import scala.meta._ @@ -169,10 +178,12 @@ class SemanticTests extends FunSuite { } def check(original: String, expectedStr: String, diffTest: DiffTest): Unit = { - val obtained = parse(fix(wrap(original, diffTest.name))) + val fixed = fix(wrap(original, diffTest.name)) + val obtained = parse(fixed) val expected = parse(expectedStr) try { checkMismatchesModuloDesugarings(obtained, expected) + typeChecks(wrap(fixed, diffTest.name)) } catch { case MismatchException(details) => val header = s"scala -> meta converter error\n$details"