From 89fa90415da5dbe2a9ec0ecd92bcc3285698d26e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=93lafur=20P=C3=A1ll=20Geirsson?= Date: Wed, 25 Jan 2017 17:01:36 +0100 Subject: [PATCH 1/2] Organize imports with new patch system. Main changes: - Patch(from, to, replace) replaced with TokenPatch(token, newToken) with helpful combinators such as Add{Left,Right} - New high-level TreePatch datatype with can be converted to TokenPatch. - Implemented two tree patches, AddGlobalImport and RemoveGlobalImport. - Implemented OrganizeImport, which is a prerequisite to use import tree patches. - OrganizeImports - orders imports by configurable groups - removes unused imports using scalac -Ywarn-unused-import infrastructure. The implementation is requires hijacking a few private fields in g.analyzer using evil reflection hackery. - option to expand relative imports - handles renames - configurable "always used" to force keeping an import such as acyclic.file Known bugs: - introduces false import on non-existing cats.data.UUID in circe - makes an import unused by expanding relative imports to fully-qualified imports, requiring you to run scalafix twice to fully remove unused imports (e.g. in cats with -Ywarn-unused-import) - crashes on scala.meta parser bug (in akka), not scalafix problem really. Expand relative Test that patch is unchanged --- .scalafix.conf | 1 + bin/scalacfix | 1 + build.sbt | 22 +- .../main/scala/scalafix/FilterMatcher.scala | 27 +++ core/src/main/scala/scalafix/Scalafix.scala | 17 +- .../main/scala/scalafix/ScalafixConfig.scala | 56 ++++- .../scalafix/rewrite/ExplicitImplicit.scala | 6 +- .../scalafix/rewrite/ProcedureSyntax.scala | 4 +- .../main/scala/scalafix/rewrite/Rewrite.scala | 7 +- .../scala/scalafix/rewrite/RewriteCtx.scala | 2 + .../scala/scalafix/rewrite/SemanticApi.scala | 12 +- .../scalafix/rewrite/VolatileLazyVal.scala | 4 +- .../main/scala/scalafix/syntax/package.scala | 31 +++ .../scala/scalafix/util/AbsoluteFile.scala | 31 +++ .../scalafix/util/AssociatedComments.scala | 62 ++++++ .../scala/scalafix/util/CanonicalImport.scala | 112 ++++++++++ .../scala/scalafix/util/CollectionOps.scala | 18 ++ .../scala/scalafix/util/OrganizeImports.scala | 200 ++++++++++++++++++ core/src/main/scala/scalafix/util/Patch.scala | 85 +++++--- .../src/main/scala/scalafix/util/logger.scala | 11 +- .../main/scala/scalafix/util/package.scala | 3 + .../scala/scalafix/rewrite/LazyValSuite.scala | 40 ---- .../rewrite/ProcedureSyntaxSuite.scala | 77 ------- phase-order.dot | 35 +++ project/Build.scala | 1 + .../scalafix/nsc/HijackImportInfos.scala | 27 +++ .../scala/scalafix/nsc/NonRemovableMap.scala | 15 ++ .../scala/scalafix/nsc/NscSemanticApi.scala | 133 +++++++++++- .../scalafix/nsc/ScalafixNscComponent.scala | 47 ++-- .../scalafix/nsc/ScalafixNscPlugin.scala | 5 +- .../resources/ExplicitImplicit/basic.source | 40 +--- .../test/resources/syntactic/LazyVal.source | 32 +++ .../syntactic/OrganizeImportsBase.source | 73 +++++++ .../OrganizeImportsExpandRelative.source | 46 ++++ .../OrganizeImportsGroupByPrefix.source | 16 ++ .../OrganizeImportsRemoveUnused.source | 123 +++++++++++ .../syntactic/ProcedureSyntax.source | 45 ++++ .../src/test/scala/scalafix/DiffTest.scala | 18 +- .../test/scala/scalafix/SemanticTests.scala | 72 ++++--- .../scala/scalafix/rewrite/ErrorSuite.scala | 12 ++ .../scala/scalafix/rewrite/RewriteSuite.scala | 1 + .../scala/scalafix/util/DiffAssertions.scala | 7 + .../scala/scalafix/sbt/ScalafixPlugin.scala | 3 +- .../test/scala/scalafix/tests/Command.scala | 17 ++ .../tests/IntegrationPropertyTest.scala | 88 ++++---- .../test/scala/scalafix/tests/ItTest.scala | 37 ++++ scalafmt | Bin 12524 -> 12523 bytes 47 files changed, 1402 insertions(+), 320 deletions(-) create mode 100644 .scalafix.conf create mode 100755 bin/scalacfix create mode 100644 core/src/main/scala/scalafix/FilterMatcher.scala create mode 100644 core/src/main/scala/scalafix/syntax/package.scala create mode 100644 core/src/main/scala/scalafix/util/AbsoluteFile.scala create mode 100644 core/src/main/scala/scalafix/util/AssociatedComments.scala create mode 100644 core/src/main/scala/scalafix/util/CanonicalImport.scala create mode 100644 core/src/main/scala/scalafix/util/CollectionOps.scala create mode 100644 core/src/main/scala/scalafix/util/OrganizeImports.scala create mode 100644 core/src/main/scala/scalafix/util/package.scala delete mode 100644 core/src/test/scala/scalafix/rewrite/LazyValSuite.scala delete mode 100644 core/src/test/scala/scalafix/rewrite/ProcedureSyntaxSuite.scala create mode 100644 phase-order.dot create mode 100644 scalafix-nsc/src/main/scala/scalafix/nsc/HijackImportInfos.scala create mode 100644 scalafix-nsc/src/main/scala/scalafix/nsc/NonRemovableMap.scala rename {core => scalafix-nsc}/src/test/resources/ExplicitImplicit/basic.source (83%) create mode 100644 scalafix-nsc/src/test/resources/syntactic/LazyVal.source create mode 100644 scalafix-nsc/src/test/resources/syntactic/OrganizeImportsBase.source create mode 100644 scalafix-nsc/src/test/resources/syntactic/OrganizeImportsExpandRelative.source create mode 100644 scalafix-nsc/src/test/resources/syntactic/OrganizeImportsGroupByPrefix.source create mode 100644 scalafix-nsc/src/test/resources/syntactic/OrganizeImportsRemoveUnused.source create mode 100644 scalafix-nsc/src/test/resources/syntactic/ProcedureSyntax.source create mode 100644 scalafix-nsc/src/test/scala/scalafix/rewrite/ErrorSuite.scala rename {core => scalafix-nsc}/src/test/scala/scalafix/rewrite/RewriteSuite.scala (93%) rename {core => scalafix-nsc}/src/test/scala/scalafix/util/DiffAssertions.scala (91%) create mode 100644 scalafix-tests/src/test/scala/scalafix/tests/Command.scala create mode 100644 scalafix-tests/src/test/scala/scalafix/tests/ItTest.scala diff --git a/.scalafix.conf b/.scalafix.conf new file mode 100644 index 0000000000..0c723a4894 --- /dev/null +++ b/.scalafix.conf @@ -0,0 +1 @@ +imports.optimize = true diff --git a/bin/scalacfix b/bin/scalacfix new file mode 100755 index 0000000000..129e34c0d6 --- /dev/null +++ b/bin/scalacfix @@ -0,0 +1 @@ +scalac -Ywarn-unused-import -Yrangepos -Xplugin:/Users/ollie/.ivy2/local/ch.epfl.scala/scalafix-nsc_2.11/0.2.0-SNAPSHOT/jars/scalafix-nsc_2.11.jar $1 diff --git a/build.sbt b/build.sbt index 85ca821b29..ac545f56c4 100644 --- a/build.sbt +++ b/build.sbt @@ -48,6 +48,7 @@ lazy val commonSettings = Seq( triggeredMessage in ThisBuild := Watched.clearWhenTriggered, scalacOptions := compilerOptions, scalacOptions in (Compile, console) := compilerOptions :+ "-Yrepl-class-based", + test in assembly := {}, testOptions in Test += Tests.Argument("-oD") ) @@ -146,10 +147,7 @@ lazy val core = project "com.typesafe" % "config" % "1.3.1", "com.lihaoyi" %% "sourcecode" % "0.1.3", "org.scalameta" %% "scalameta" % Build.metaV, - "org.scala-lang" % "scala-reflect" % scalaVersion.value, - // Test dependencies - "org.scalatest" %% "scalatest" % Build.testV % "test", - "com.googlecode.java-diff-utils" % "diffutils" % "1.3.0" % "test" + "org.scala-lang" % "scala-reflect" % scalaVersion.value ) ) .enablePlugins(BuildInfoPlugin) @@ -160,9 +158,17 @@ lazy val `scalafix-nsc` = project scalaVersion := "2.11.8", crossScalaVersions := crossVersions, libraryDependencies ++= Seq( - "org.scala-lang" % "scala-compiler" % scalaVersion.value, - "org.scalameta" %% "scalameta" % Build.metaV % "provided", - "org.scalatest" %% "scalatest" % Build.testV % Test + "org.scala-lang" % "scala-compiler" % scalaVersion.value, + "org.scalameta" %% "scalameta" % Build.metaV % "provided", + "com.googlecode.java-diff-utils" % "diffutils" % "1.3.0" % "test", + "org.scalatest" %% "scalatest" % Build.testV % Test, + "com.lihaoyi" %% "ammonite-ops" % Build.ammoniteV % Test, + // integration property tests + "org.typelevel" %% "catalysts-platform" % "0.0.5" % Test, + "com.typesafe.slick" %% "slick" % "3.2.0-M2" % Test, + "io.circe" %% "circe-core" % "0.6.0" % Test, + "org.typelevel" %% "cats-core" % "0.7.2" % Test, + "org.scalacheck" %% "scalacheck" % "1.13.4" % Test ), // sbt does not fetch transitive dependencies of compiler plugins. // to overcome this issue, all transitive dependencies are included @@ -256,7 +262,7 @@ lazy val `scalafix-tests` = project ) .value, libraryDependencies ++= Seq( - "com.lihaoyi" %% "ammonite-ops" % "0.8.0", + "com.lihaoyi" %% "ammonite-ops" % Build.ammoniteV, "org.scalatest" %% "scalatest" % Build.testV % "test" ) ) diff --git a/core/src/main/scala/scalafix/FilterMatcher.scala b/core/src/main/scala/scalafix/FilterMatcher.scala new file mode 100644 index 0000000000..3099a405f5 --- /dev/null +++ b/core/src/main/scala/scalafix/FilterMatcher.scala @@ -0,0 +1,27 @@ +package scalafix + +import scala.util.matching.Regex +import scalafix.util.AbsoluteFile + +case class FilterMatcher(include: Regex, exclude: Regex) { + def matches(file: AbsoluteFile): Boolean = matches(file.path) + def matches(input: String): Boolean = + include.findFirstIn(input).isDefined && + exclude.findFirstIn(input).isEmpty +} + +object FilterMatcher { + val matchEverything = new FilterMatcher(".*".r, mkRegexp(Nil)) + + def mkRegexp(filters: Seq[String]): Regex = + filters match { + case Nil => "$a".r // will never match anything + case head :: Nil => head.r + case _ => filters.mkString("(", "|", ")").r + } + + def apply(includes: Seq[String], excludes: Seq[String]): FilterMatcher = + new FilterMatcher(mkRegexp(includes), mkRegexp(excludes)) + def apply(include: String): FilterMatcher = + new FilterMatcher(mkRegexp(Seq(include)), mkRegexp(Nil)) +} diff --git a/core/src/main/scala/scalafix/Scalafix.scala b/core/src/main/scala/scalafix/Scalafix.scala index fefe7a1981..5f43acbced 100644 --- a/core/src/main/scala/scalafix/Scalafix.scala +++ b/core/src/main/scala/scalafix/Scalafix.scala @@ -1,13 +1,14 @@ package scalafix -import scala.meta.parsers.Parsed +import scala.collection.immutable.Seq import scala.meta._ import scala.meta.inputs.Input +import scala.meta.parsers.Parsed import scalafix.rewrite.RewriteCtx import scalafix.rewrite.SemanticApi +import scalafix.util.AssociatedComments import scalafix.util.Patch import scalafix.util.TokenList -import scalafix.util.logger object Scalafix { def fix(code: Input, @@ -15,9 +16,15 @@ object Scalafix { semanticApi: Option[SemanticApi]): Fixed = { config.parser.apply(code, config.dialect) match { case Parsed.Success(ast) => - val ctx = RewriteCtx(config, new TokenList(ast.tokens), semanticApi) - val patches: Seq[Patch] = config.rewrites.flatMap(_.rewrite(ast, ctx)) - Fixed.Success(Patch.apply(ast.tokens, patches)) + val tokens = ast.tokens + implicit val ctx = RewriteCtx( + config, + new TokenList(ast.tokens), + AssociatedComments(tokens), + semanticApi + ) + val patches = config.rewrites.flatMap(_.rewrite(ast, ctx)) + Fixed.Success(Patch.apply(ast, patches)) case Parsed.Error(pos, msg, e) => Fixed.Failed(Failure.ParseError(pos, msg, e)) } diff --git a/core/src/main/scala/scalafix/ScalafixConfig.scala b/core/src/main/scala/scalafix/ScalafixConfig.scala index 07666a5b79..88d9e09560 100644 --- a/core/src/main/scala/scalafix/ScalafixConfig.scala +++ b/core/src/main/scala/scalafix/ScalafixConfig.scala @@ -1,20 +1,40 @@ package scalafix -import scala.meta.Dialect -import scala.meta.Tree +import scala.collection.immutable.Seq +import scala.meta._ import scala.meta.dialects.Scala211 import scala.meta.parsers.Parse import scala.util.control.NonFatal import scalafix.rewrite.Rewrite +import scalafix.syntax._ import java.io.File import com.typesafe.config.Config import com.typesafe.config.ConfigFactory +case class ImportsConfig( + expandRelative: Boolean = true, + spaceAroundCurlyBrace: Boolean = false, + organize: Boolean = true, + removeUnused: Boolean = true, + alwaysUsed: Seq[Ref] = Seq(), + groups: Seq[FilterMatcher] = Seq( + FilterMatcher("scala.language.*"), + FilterMatcher("(scala|scala\\..*)$"), + FilterMatcher("(java|java\\..*)$"), + FilterMatcher(".*") + ), + groupByPrefix: Boolean = false +) +object ImportsConfig { + def default: ImportsConfig = ImportsConfig() +} case class ScalafixConfig( rewrites: Seq[Rewrite] = Rewrite.defaultRewrites, parser: Parse[_ <: Tree] = Parse.parseSource, + imports: ImportsConfig = ImportsConfig(), + fatalWarning: Boolean = true, dialect: Dialect = Scala211 ) @@ -25,6 +45,7 @@ object ScalafixConfig { catch { case NonFatal(e) => Left(e.getMessage) } + val default = ScalafixConfig() def fromFile(file: File): Either[String, ScalafixConfig] = saferThanTypesafe(() => ConfigFactory.parseFile(file)) @@ -34,15 +55,35 @@ object ScalafixConfig { def fromConfig(config: Config): Either[String, ScalafixConfig] = { import scala.collection.JavaConverters._ + val base = ScalafixConfig( + fatalWarning = config.getBoolOrElse("fatalWarnings", + ScalafixConfig.default.fatalWarning), + imports = ImportsConfig( + expandRelative = + config.getBoolOrElse("imports.expandRelative", + ImportsConfig.default.expandRelative), + spaceAroundCurlyBrace = + config.getBoolOrElse("imports.spaceAroundCurlyBrace", + ImportsConfig.default.spaceAroundCurlyBrace), + organize = config.getBoolOrElse("imports.organize", + ImportsConfig.default.organize), + removeUnused = + config.getBoolOrElse("imports.removeUnused", + ImportsConfig.default.removeUnused), + groupByPrefix = + config.getBoolOrElse("imports.groupByPrefix", + ImportsConfig.default.groupByPrefix) + ) + ) if (config.hasPath("rewrites")) - fromNames(config.getStringList("rewrites").asScala.toList) + fromNames(config.getStringList("rewrites").asScala.toList).right + .map(rewrites => base.copy(rewrites = rewrites)) else Right(ScalafixConfig()) } - def fromNames(names: List[String]): Either[String, ScalafixConfig] = + def fromNames(names: List[String]): Either[String, Seq[Rewrite]] = names match { - case "all" :: Nil => - Right(ScalafixConfig(rewrites = Rewrite.allRewrites)) + case "all" :: Nil => Right(Rewrite.allRewrites) case _ => val invalidNames = names.filterNot(Rewrite.name2rewrite.contains) @@ -51,8 +92,7 @@ object ScalafixConfig { 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)) + Right(names.map(Rewrite.name2rewrite)) } } } diff --git a/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala b/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala index cc528cb214..6ea328a10b 100644 --- a/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala +++ b/core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala @@ -3,6 +3,8 @@ package scalafix.rewrite import scala.{meta => m} import scalafix.util.Patch import scalafix.util.Whitespace +import scala.collection.immutable.Seq +import scalafix.util.TokenPatch case object ExplicitImplicit extends Rewrite { // Don't explicitly annotate vals when the right-hand body is a single call @@ -27,8 +29,8 @@ case object ExplicitImplicit extends Rewrite { replace <- lhsTokens.reverseIterator.find(x => !x.is[Token.Equals] && !x.is[Whitespace]) typ <- semantic.typeSignature(defn) - } yield Patch(replace, replace, s"$replace: ${typ.syntax}") - }.toSeq + } yield TokenPatch.AddRight(replace, s": ${typ.syntax}") + }.to[Seq] ast.collect { case t @ m.Defn.Val(mods, _, None, body) if mods.exists(_.syntax == "implicit") && diff --git a/core/src/main/scala/scalafix/rewrite/ProcedureSyntax.scala b/core/src/main/scala/scalafix/rewrite/ProcedureSyntax.scala index bc72735628..f684aa88f8 100644 --- a/core/src/main/scala/scalafix/rewrite/ProcedureSyntax.scala +++ b/core/src/main/scala/scalafix/rewrite/ProcedureSyntax.scala @@ -7,6 +7,8 @@ import scala.meta.tokens.Token.RightBrace import scala.meta.tokens.Token.RightParen import scala.meta.tokens.Token.Space import scalafix.util.Patch +import scala.collection.immutable.Seq +import scalafix.util.TokenPatch case object ProcedureSyntax extends Rewrite { override def rewrite(ast: Tree, ctx: RewriteCtx): Seq[Patch] = { @@ -27,7 +29,7 @@ case object ProcedureSyntax extends Rewrite { if (between.nonEmpty) " " + between else "" } - Patch(next(closingParen), bodyStart, s": Unit = {$comment") + TokenPatch.AddRight(closingParen, s": Unit =") } patches } diff --git a/core/src/main/scala/scalafix/rewrite/Rewrite.scala b/core/src/main/scala/scalafix/rewrite/Rewrite.scala index e7ad73e210..a60156fa30 100644 --- a/core/src/main/scala/scalafix/rewrite/Rewrite.scala +++ b/core/src/main/scala/scalafix/rewrite/Rewrite.scala @@ -2,7 +2,9 @@ package scalafix.rewrite import scala.meta._ import scalafix.Failure.MissingSemanticApi +import scalafix.util.TreePatch import scalafix.util.Patch +import scala.collection.immutable.Seq abstract class Rewrite { def getSemanticApi(ctx: RewriteCtx): SemanticApi = ctx.semantic.getOrElse { @@ -16,7 +18,10 @@ object Rewrite { t.map(x => x.source -> x.value).toMap } - val syntaxRewrites: Seq[Rewrite] = Seq(ProcedureSyntax, VolatileLazyVal) + val syntaxRewrites: Seq[Rewrite] = Seq( + ProcedureSyntax, + VolatileLazyVal + ) val semanticRewrites: Seq[Rewrite] = Seq(ExplicitImplicit) val allRewrites: Seq[Rewrite] = syntaxRewrites ++ semanticRewrites val defaultRewrites: Seq[Rewrite] = diff --git a/core/src/main/scala/scalafix/rewrite/RewriteCtx.scala b/core/src/main/scala/scalafix/rewrite/RewriteCtx.scala index ca4f93a4f9..6b20cdd4ab 100644 --- a/core/src/main/scala/scalafix/rewrite/RewriteCtx.scala +++ b/core/src/main/scala/scalafix/rewrite/RewriteCtx.scala @@ -1,9 +1,11 @@ package scalafix.rewrite import scalafix.ScalafixConfig +import scalafix.util.AssociatedComments import scalafix.util.TokenList case class RewriteCtx( config: ScalafixConfig, tokenList: TokenList, + comments: AssociatedComments, semantic: Option[SemanticApi] ) diff --git a/core/src/main/scala/scalafix/rewrite/SemanticApi.scala b/core/src/main/scala/scalafix/rewrite/SemanticApi.scala index dfd0b5262c..1182f8cec9 100644 --- a/core/src/main/scala/scalafix/rewrite/SemanticApi.scala +++ b/core/src/main/scala/scalafix/rewrite/SemanticApi.scala @@ -1,7 +1,6 @@ package scalafix.rewrite -import scala.meta.Defn -import scala.meta.Type +import scala.meta._ /** A custom semantic api for scalafix rewrites. * @@ -18,4 +17,13 @@ trait SemanticApi { /** Returns the type annotation for given val/def. */ def typeSignature(defn: Defn): Option[Type] + + /** Returns the fully qualified name of this name, or none if unable to find it*/ + def fqn(name: Ref): Option[Ref] + + /** Returns all used refs in this compilation unit */ + def usedFqns: Seq[Ref] + + /** Returns true if importee is not used in this compilation unit, false otherwise */ + def isUnusedImport(importee: Importee): Boolean } diff --git a/core/src/main/scala/scalafix/rewrite/VolatileLazyVal.scala b/core/src/main/scala/scalafix/rewrite/VolatileLazyVal.scala index 392a1e9de4..01ee8ae3db 100644 --- a/core/src/main/scala/scalafix/rewrite/VolatileLazyVal.scala +++ b/core/src/main/scala/scalafix/rewrite/VolatileLazyVal.scala @@ -2,6 +2,8 @@ package scalafix.rewrite import scala.meta._ import scalafix.util.Patch +import scala.collection.immutable.Seq +import scalafix.util.TokenPatch case object VolatileLazyVal extends Rewrite { private object NonVolatileLazyVal { @@ -17,7 +19,7 @@ case object VolatileLazyVal extends Rewrite { override def rewrite(ast: Tree, ctx: RewriteCtx): Seq[Patch] = { ast.collect { case NonVolatileLazyVal(tok) => - Patch(tok, tok, s"@volatile ${tok.syntax}") + TokenPatch.AddLeft(tok, s"@volatile ") } } } diff --git a/core/src/main/scala/scalafix/syntax/package.scala b/core/src/main/scala/scalafix/syntax/package.scala new file mode 100644 index 0000000000..f636164fd4 --- /dev/null +++ b/core/src/main/scala/scalafix/syntax/package.scala @@ -0,0 +1,31 @@ +package scalafix + +import scala.meta.Importee +import scala.meta.tokens.Token +import scalafix.util.CanonicalImport +import scalafix.util.ImportPatch +import scalafix.util.logger + +import com.typesafe.config.Config + +package object syntax { + implicit class XtensionImporter(i: CanonicalImport) { + def supersedes(patch: ImportPatch): Boolean = + i.ref.structure == patch.importer.ref.structure && + (i.importee.is[Importee.Wildcard] || + i.importee.structure == patch.importer.importee.structure) + } + + implicit class XtensionToken(token: Token) { + def posTuple: (Int, Int) = token.start -> token.end + } + + implicit class XtensionConfig(config: Config) { + def getBoolOrElse(key: String, els: Boolean): Boolean = + if (config.hasPath(key)) config.getBoolean(key) + else els + } + implicit class XtensionString(str: String) { + def reveal: String = logger.reveal(str) + } +} diff --git a/core/src/main/scala/scalafix/util/AbsoluteFile.scala b/core/src/main/scala/scalafix/util/AbsoluteFile.scala new file mode 100644 index 0000000000..0159d8d05e --- /dev/null +++ b/core/src/main/scala/scalafix/util/AbsoluteFile.scala @@ -0,0 +1,31 @@ +package scalafix.util + +import java.io.File + +/** Wrapper around java.io.File with an absolute path. */ +sealed abstract case class AbsoluteFile(jfile: File) { + def path: String = jfile.getAbsolutePath + def /(other: String) = new AbsoluteFile(new File(jfile, other)) {} +} + +object AbsoluteFile { + def fromFiles(files: Seq[File], + workingDir: AbsoluteFile): Seq[AbsoluteFile] = { + files.map(x => fromFile(x, workingDir)) + } + private def makeAbsolute(workingDir: File)(file: File): File = + if (file.isAbsolute) file + else new File(workingDir, file.getPath) + + // If file is already absolute, then workingDir is not used. + def fromFile(file: File, workingDir: AbsoluteFile): AbsoluteFile = { + new AbsoluteFile(makeAbsolute(workingDir.jfile)(file)) {} + } + def fromPath(path: String): Option[AbsoluteFile] = { + val file = new File(path) + if (file.isAbsolute) Some(new AbsoluteFile(file) {}) + else None + } + def userDir = new AbsoluteFile(new File(System.getProperty("user.dir"))) {} + def homeDir = new AbsoluteFile(new File(System.getProperty("user.home"))) {} +} diff --git a/core/src/main/scala/scalafix/util/AssociatedComments.scala b/core/src/main/scala/scalafix/util/AssociatedComments.scala new file mode 100644 index 0000000000..cf73370976 --- /dev/null +++ b/core/src/main/scala/scalafix/util/AssociatedComments.scala @@ -0,0 +1,62 @@ +package scalafix.util + +import scala.meta.Tree +import scala.meta.tokens.Token +import scala.meta.tokens.Token.Comment +import scala.meta.tokens.Tokens +import scala.collection.immutable.Seq + +sealed abstract class AssociatedComments( + leadingMap: Map[Token, Seq[Comment]], + trailingMap: Map[Token, Seq[Comment]]) { + def leading(tree: Tree): Set[Comment] = + (for { + token <- tree.tokens.headOption + comments <- leadingMap.get(token) + } yield comments).getOrElse(Nil).toSet + + def trailing(tree: Tree): Set[Comment] = + (for { + token <- tree.tokens.lastOption + comments <- trailingMap.get(token) + } yield comments).getOrElse(Nil).toSet + + def hasComment(tree: Tree): Boolean = + trailing(tree).nonEmpty || leading(tree).nonEmpty +} + +object AssociatedComments { + + def apply(tokens: Tokens): AssociatedComments = { + import scala.meta.tokens.Token._ + val leadingBuilder = Map.newBuilder[Token, Seq[Comment]] + val trailingBuilder = Map.newBuilder[Token, Seq[Comment]] + val leading = Seq.newBuilder[Comment] + val trailing = Seq.newBuilder[Comment] + var isLeading = true + var lastToken: Token = tokens.head + tokens.foreach { + case c: Comment => + if (isLeading) leading += c + else trailing += c + case Token.LF() => isLeading = true + case Trivia() => + case currentToken => + val t = trailing.result() + if (t.nonEmpty) { + trailingBuilder += lastToken -> trailing.result() + trailing.clear() + } + val l = leading.result() + if (l.nonEmpty) { + leadingBuilder += currentToken -> leading.result() + leading.clear() + } + if (!currentToken.is[Comma]) { + lastToken = currentToken + } + isLeading = false + } + new AssociatedComments(leadingBuilder.result(), trailingBuilder.result()) {} + } +} diff --git a/core/src/main/scala/scalafix/util/CanonicalImport.scala b/core/src/main/scala/scalafix/util/CanonicalImport.scala new file mode 100644 index 0000000000..83c705e842 --- /dev/null +++ b/core/src/main/scala/scalafix/util/CanonicalImport.scala @@ -0,0 +1,112 @@ +package scalafix.util + +import scala.collection.immutable.Seq +import scala.meta._ +import scala.meta.tokens.Token.Comment +import scalafix.rewrite.RewriteCtx + +object CanonicalImport { + def apply(ref: Term.Ref, + wildcard: Importee.Wildcard, + unimports: Seq[Importee.Unimport], + renames: Seq[Importee.Rename])( + implicit ctx: RewriteCtx, + ownerImport: Import + ): CanonicalImport = + new CanonicalImport( + ref, + wildcard, + unimports, + renames, + leadingComments = ctx.comments.leading(ownerImport), + trailingComments = ctx.comments.trailing(ownerImport) ++ + (wildcard +: unimports).flatMap(ctx.comments.trailing), + None + ) {} + def apply(ref: Term.Ref, importee: Importee)( + implicit ctx: RewriteCtx, + ownerImport: Import + ): CanonicalImport = + new CanonicalImport( + ref, + importee, + Nil, + Nil, + leadingComments = ctx.comments.leading(ownerImport), + trailingComments = ctx.comments.trailing(ownerImport) ++ + ctx.comments.trailing(importee), + None + ) {} +} + +sealed case class CanonicalImport( + ref: Term.Ref, + importee: Importee, + unimports: Seq[Importee.Unimport], + renames: Seq[Importee.Rename], + leadingComments: Set[Comment], + trailingComments: Set[Comment], + fullyQualifiedRef: Option[Term.Ref] +) { + + def isRootImport: Boolean = + ref.collect { + case q"_root_.$_" => true + }.nonEmpty + + def addRootImport(ref: Term.Ref): Term.Ref = + if (!isRootImport) ref + else { + ("_root_." + ref.syntax).parse[Term].get.asInstanceOf[Term.Ref] + } + + def withFullyQualifiedRef(fqnRef: Option[Term.Ref]): CanonicalImport = + copy(fullyQualifiedRef = fqnRef.map(addRootImport)) + + def isSpecialImport: Boolean = { + val base = ref.syntax + base.startsWith("scala.language") || + base.startsWith("scala.annotation") + } + private def extraImportees = renames ++ unimports + def withoutLeading(leading: Set[Comment]): CanonicalImport = + copy(leadingComments = leadingComments.filterNot(leading)) + def tree: Import = Import(Seq(Importer(ref, unimports :+ importee))) + def syntax(implicit ctx: RewriteCtx): String = + s"${leading}import $importerSyntax$trailing" + def leading: String = + if (leadingComments.isEmpty) "" + else leadingComments.mkString("", "\n", "\n") + def trailing: String = + if (trailingComments.isEmpty) "" + else trailingComments.mkString(" ", "\n", "") + def importerSyntax(implicit ctx: RewriteCtx): String = + s"$refSyntax.$importeeSyntax" + private def curlySpace(implicit ctx: RewriteCtx) = + if (ctx.config.imports.spaceAroundCurlyBrace) " " + else "" + + def actualRef(implicit ctx: RewriteCtx): Term.Ref = + if (ctx.config.imports.expandRelative) fullyQualifiedRef.getOrElse(ref) + else ref + def refSyntax(implicit ctx: RewriteCtx): String = + actualRef.syntax + def importeeSyntax(implicit ctx: RewriteCtx): String = + if (extraImportees.nonEmpty) + s"""{$curlySpace${extraImportees + .map(_.syntax) + .mkString(", ")}, $importee$curlySpace}""" + else + importee match { + case i: Importee.Rename => s"{$curlySpace$i$curlySpace}" + case i => i.syntax + } + private def importeeOrder = importee match { + case i: Importee.Rename => (1, i.name.syntax) + case i: Importee.Wildcard => (0, i.syntax) + case i => (1, i.syntax) + } + def sortOrder(implicit ctx: RewriteCtx): (String, (Int, String)) = + (refSyntax, importeeOrder) + def structure: String = Importer(ref, Seq(importee)).structure +} diff --git a/core/src/main/scala/scalafix/util/CollectionOps.scala b/core/src/main/scala/scalafix/util/CollectionOps.scala new file mode 100644 index 0000000000..024348472a --- /dev/null +++ b/core/src/main/scala/scalafix/util/CollectionOps.scala @@ -0,0 +1,18 @@ +package scalafix.util +import scala.collection.immutable.Seq +object CollectionOps { + def partition[A, B](coll: Seq[A])( + f: PartialFunction[A, B]): (Seq[A], Seq[B]) = { + val as = Seq.newBuilder[A] + val bs = Seq.newBuilder[B] + val fopt = f.lift + coll.foreach { a => + fopt(a) match { + case Some(b) => bs += b + case None => as += a + } + } + as.result() -> bs.result() + } + +} diff --git a/core/src/main/scala/scalafix/util/OrganizeImports.scala b/core/src/main/scala/scalafix/util/OrganizeImports.scala new file mode 100644 index 0000000000..2e52920921 --- /dev/null +++ b/core/src/main/scala/scalafix/util/OrganizeImports.scala @@ -0,0 +1,200 @@ +package scalafix.util + +import scala.collection.immutable.Seq +import scala.collection.mutable +import scala.meta.Importee.Wildcard +import scala.meta._ +import scala.meta.tokens.Token.Comment +import scalafix.FilterMatcher +import scalafix.rewrite.RewriteCtx +import scalafix.syntax._ +import scalafix.util.TreePatch.AddGlobalImport +import scalafix.util.TreePatch.RemoveGlobalImport + +object OrganizeImports { + def extractImports(stats: Seq[Stat])( + implicit ctx: RewriteCtx): (Seq[Import], Seq[CanonicalImport]) = { + val imports = stats.takeWhile(_.is[Import]).collect { case i: Import => i } + val importees = imports.collect { + case imp @ Import(importers) => + implicit val currentImport = imp + importers.flatMap { importer => + val ref = importer.ref // fqnRef.getOrElse(importer.ref) + val wildcard = importer.importees.collectFirst { + case wildcard: Importee.Wildcard => wildcard + } + wildcard.fold(importer.importees.map(i => CanonicalImport(ref, i))) { + wildcard => + val unimports = importer.importees.collect { + case i: Importee.Unimport => i + } + val renames = importer.importees.collect { + case i: Importee.Rename => i + } + List(CanonicalImport(ref, wildcard, unimports, renames)) + } + } + }.flatten + imports -> importees + } + + def getLastTopLevelPkg(potPkg: Stat): Stat = potPkg match { + case Pkg(_, head +: Nil) => getLastTopLevelPkg(head) + case Pkg(_, head +: _) => head + case _ => potPkg + } + + def getGlobalImports(ast: Tree)( + implicit ctx: RewriteCtx): (Seq[Import], Seq[CanonicalImport]) = + ast match { + case Pkg(_, Seq(pkg: Pkg)) => getGlobalImports(pkg) + case Source(Seq(pkg: Pkg)) => getGlobalImports(pkg) + case Pkg(_, stats) => extractImports(stats) + case Source(stats) => extractImports(stats) + case _ => Nil -> Nil + } + + def removeDuplicates(imports: Seq[CanonicalImport]): Seq[CanonicalImport] = { + val usedSyntax = mutable.Set.empty[String] + val wildcards = imports.collect { + case c if c.importee.is[Importee.Wildcard] => c.ref.syntax + }.toSet + def isDuplicate(imp: CanonicalImport): Boolean = { + val plainSyntax = imp.tree.syntax + if (usedSyntax.contains(plainSyntax)) true + else { + usedSyntax += plainSyntax + imp.importee match { + case _: Importee.Name => wildcards.contains(imp.ref.syntax) + case _ => false + } + } + } + imports.filterNot(isDuplicate) + } + + def removeUnused(possiblyDuplicates: Seq[CanonicalImport])( + implicit ctx: RewriteCtx): Seq[CanonicalImport] = { + val imports = removeDuplicates(possiblyDuplicates) + if (!ctx.config.imports.removeUnused) imports + else { + val (usedImports, unusedImports) = + ctx.semantic + .map { semantic => + val unusedImports = + imports.partition(i => !semantic.isUnusedImport(i.importee)) + unusedImports + } + .getOrElse(possiblyDuplicates -> Nil) + usedImports + } + } + + def groupImports(imports0: Seq[CanonicalImport])( + implicit ctx: RewriteCtx): Seq[Seq[Import]] = { + val config = ctx.config.imports + def fullyQualify(imp: CanonicalImport): Option[Term.Ref] = + for { + semantic <- ctx.semantic + fqnRef <- semantic.fqn(imp.ref) + if fqnRef.is[Term.Ref] + } yield fqnRef.asInstanceOf[Term.Ref] + val imports = + imports0.map(imp => imp.withFullyQualifiedRef(fullyQualify(imp))) + val (fullyQualifiedImports, relativeImports) = + imports.partition { imp => + ctx.config.imports.expandRelative || + fullyQualify(imp).exists(_.syntax == imp.ref.syntax) + } + val groupById = + config.groups.zipWithIndex.toMap + .withDefaultValue(config.groups.length) + val grouped: Map[FilterMatcher, Seq[CanonicalImport]] = + fullyQualifiedImports + .groupBy { imp => + config.groups + .find(_.matches(imp.refSyntax)) + .getOrElse(config.groups.last) + } + (FilterMatcher("relative") -> relativeImports) + val inOrder = + grouped + .mapValues(x => x.sortBy(_.sortOrder)) + .to[Seq] + .filter(_._2.nonEmpty) + .sortBy(x => groupById(x._1)) + .collect { case (_, s) => s } + val asImports = inOrder.map { is => + if (config.groupByPrefix) { + is.groupBy(_.ref.syntax) + .to[Seq] + .map { + case (_, importers) => + Import(Seq( + Importer(importers.head.actualRef, importers.map(_.importee)))) + + } + } else { + var usedLeadingComment = Set.empty[Comment] + is.map { i => + val result = i + .withoutLeading(usedLeadingComment) + .syntax + .parse[Stat] + .get + .asInstanceOf[Import] + usedLeadingComment = usedLeadingComment ++ i.leadingComments + result + } + } + } + asImports + } + + def prettyPrint(imports: Seq[CanonicalImport])( + implicit ctx: RewriteCtx): String = { + groupImports(imports) + .map(_.map(_.syntax).mkString("\n")) + .mkString("\n\n") + } + + def organizeImports(code: Tree, patches: Seq[ImportPatch])( + implicit ctx: RewriteCtx): Seq[TokenPatch] = { + if (!ctx.config.imports.organize && patches.isEmpty) { + Nil + } else { + def combine(is: Seq[CanonicalImport], + patch: ImportPatch): Seq[CanonicalImport] = + patch match { + case add: AddGlobalImport => + if (is.exists(_.supersedes(patch))) is + else is :+ patch.importer + case remove: RemoveGlobalImport => + is.filter(_.structure == remove.importer.structure) + } + val (oldImports, globalImports) = getGlobalImports(code) + val allImports = + patches.foldLeft(removeUnused(globalImports))(combine) + groupImports(allImports) + val tokens = code.tokens + val tok = + oldImports.headOption.map(_.tokens.head).getOrElse(tokens.head) + val toRemove = for { + firstImport <- oldImports.headOption + first <- firstImport.tokens.headOption + lastImport <- oldImports.lastOption + last <- lastImport.tokens.lastOption + } yield { + tokens.toIterator + .dropWhile(_.start < first.start) + .takeWhile { x => + x.end <= last.end + } + .map(TokenPatch.Remove) + .toList + } + val toInsert = prettyPrint(allImports) + TokenPatch.AddLeft(tok, toInsert) +: + toRemove.getOrElse(Nil) + } + } +} diff --git a/core/src/main/scala/scalafix/util/Patch.scala b/core/src/main/scala/scalafix/util/Patch.scala index 45d4abda69..9338033e28 100644 --- a/core/src/main/scala/scalafix/util/Patch.scala +++ b/core/src/main/scala/scalafix/util/Patch.scala @@ -1,40 +1,71 @@ package scalafix.util +import scala.collection.immutable.Seq import scala.meta._ import scala.meta.tokens.Token import scala.meta.tokens.Token +import scalafix.ImportsConfig +import scalafix.ScalafixConfig +import scalafix.rewrite.RewriteCtx +import scalafix.util.TokenPatch.Add +import scalafix.util.TokenPatch.Remove +import scalafix.syntax._ -/** - * A patch replaces all tokens between [[from]] and [[to]] with [[replace]]. - */ -case class Patch(from: Token, to: Token, replace: String) { - def insideRange(token: Token): Boolean = - token.input.eq(from.input) && - token.end <= to.end && - token.start >= from.start +sealed abstract class Patch +abstract class TreePatch extends Patch +abstract class TokenPatch(val tok: Token, val newTok: String) + extends TreePatch { + override def toString: String = + s"TokenPatch(${logger.reveal(tok.syntax)}, ${tok.structure}, $newTok)" +} - val tokens = replace.tokenize.get.tokens.toSeq - def runOn(str: Seq[Token]): Seq[Token] = { - str.flatMap { - case `from` => tokens - case x if insideRange(x) => Nil - case x => Seq(x) - } - } +abstract class ImportPatch(val importer: CanonicalImport) extends TreePatch +object TreePatch { + case class RemoveGlobalImport(override val importer: CanonicalImport) + extends ImportPatch(importer) + case class AddGlobalImport(override val importer: CanonicalImport) + extends ImportPatch(importer) } +object TokenPatch { + case class Remove(override val tok: Token) extends TokenPatch(tok, "") + def AddRight(tok: Token, toAdd: String): TokenPatch = Add(tok, "", toAdd) + def AddLeft(tok: Token, toAdd: String): TokenPatch = Add(tok, toAdd, "") + case class Add(override val tok: Token, + addLeft: String, + addRight: String, + keepTok: Boolean = true) + extends TokenPatch(tok, + s"""$addLeft${if (keepTok) tok else ""}$addRight""") + +} object Patch { - def verifyPatches(patches: Seq[Patch]): Unit = { - // TODO(olafur) assert there's no conflicts. + def merge(a: TokenPatch, b: TokenPatch): TokenPatch = (a, b) match { + case (add1: Add, add2: Add) => + Add(add1.tok, + add1.addLeft + add2.addLeft, + add1.addRight + add2.addRight, + add1.keepTok && add2.keepTok) + case (_: Remove, add: Add) => add.copy(keepTok = false) + case (add: Add, _: Remove) => add.copy(keepTok = false) + case (rem: Remove, _: Remove) => rem + case _ => + sys.error(s"""Can't merge token patches: + |1. $a + |2. $b""".stripMargin) } - def apply(input: Seq[Token], patches: Seq[Patch]): String = { - verifyPatches(patches) - // TODO(olafur) optimize, this is SUPER inefficient - patches - .foldLeft(input) { - case (s, p) => p.runOn(s) - } - .map(_.syntax) - .mkString("") + def apply(ast: Tree, patches: Seq[Patch])(implicit ctx: RewriteCtx): String = { + val input = ast.tokens + val tokenPatches = patches.collect { case e: TokenPatch => e } + val importPatches = OrganizeImports.organizeImports(ast, patches.collect { + case e: ImportPatch => e + }) + val patchMap: Map[(Int, Int), String] = + (importPatches ++ tokenPatches) + .groupBy(_.tok.posTuple) + .mapValues(_.reduce(merge).newTok) + input.toIterator + .map(x => patchMap.getOrElse(x.posTuple, x.syntax)) + .mkString } } diff --git a/core/src/main/scala/scalafix/util/logger.scala b/core/src/main/scala/scalafix/util/logger.scala index e2ea7ca54f..fc8a8f1703 100644 --- a/core/src/main/scala/scalafix/util/logger.scala +++ b/core/src/main/scala/scalafix/util/logger.scala @@ -17,10 +17,15 @@ object logger { enclosing: sourcecode.Enclosing, showSource: Boolean): Unit = { val position = f"${new File(file.value).getName}:${line.value}" + val value = { + val str = s"${t.value}" + if (str.contains("\n")) s"\n$str" + else str + } val key = - if (showSource) s"[${t.source}]: ${t.value}" - else t.value - println(f"$logLevel%-7s $position%-25s $key") + if (showSource) s"[${t.source}]: " + else "" + println(f"$logLevel%-7s $position%-25s $key$value") } def elem(ts: sourcecode.Text[Any]*)( diff --git a/core/src/main/scala/scalafix/util/package.scala b/core/src/main/scala/scalafix/util/package.scala new file mode 100644 index 0000000000..8569ef40d5 --- /dev/null +++ b/core/src/main/scala/scalafix/util/package.scala @@ -0,0 +1,3 @@ +package scalafix + +package object util {} diff --git a/core/src/test/scala/scalafix/rewrite/LazyValSuite.scala b/core/src/test/scala/scalafix/rewrite/LazyValSuite.scala deleted file mode 100644 index 7b3ad28d72..0000000000 --- a/core/src/test/scala/scalafix/rewrite/LazyValSuite.scala +++ /dev/null @@ -1,40 +0,0 @@ -package scalafix.rewrite - -import scala.meta.inputs.Input - -class LazyValSuite extends RewriteSuite(VolatileLazyVal) { - - check( - "basic", - """|object a { - | - |val foo = 1 - | - | lazy val x = 2 - | @volatile lazy val dontChangeMe = 2 - | private lazy val x = 2 - | - | class foo { - | lazy val z = { - | reallyHardStuff() - | } - | } - |} - """.stripMargin, - """|object a { - | - |val foo = 1 - | - | @volatile lazy val x = 2 - | @volatile lazy val dontChangeMe = 2 - | @volatile private lazy val x = 2 - | - | class foo { - | @volatile lazy val z = { - | reallyHardStuff() - | } - | } - |} - """.stripMargin - ) -} diff --git a/core/src/test/scala/scalafix/rewrite/ProcedureSyntaxSuite.scala b/core/src/test/scala/scalafix/rewrite/ProcedureSyntaxSuite.scala deleted file mode 100644 index e2b25dba5a..0000000000 --- a/core/src/test/scala/scalafix/rewrite/ProcedureSyntaxSuite.scala +++ /dev/null @@ -1,77 +0,0 @@ -package scalafix.rewrite - -import scalafix.Failure -import scalafix.Fixed -import scalafix.Scalafix - -class ProcedureSyntaxSuite extends RewriteSuite(ProcedureSyntax) { - - check( - "nested function", - """ - |import /* a */ a.b.c - |import a.b.c - |// This is a comment - |@annotation - |object Main { - | def main(args: Seq[String]) { - | var number = 2 - | def increment(n: Int) { - | number += n - | } - | increment(3) - | args.foreach(println(number)) - | } - |}""".stripMargin, - """ - |import /* a */ a.b.c - |import a.b.c - |// This is a comment - |@annotation - |object Main { - | def main(args: Seq[String]): Unit = { - | var number = 2 - | def increment(n: Int): Unit = { - | number += n - | } - | increment(3) - | args.foreach(println(number)) - | } - |}""".stripMargin - ) - check( - "no right paren", - """ - |object a { - |def foo { - | println(1) - |} - |} - """.stripMargin, - """ - |object a { - |def foo: Unit = { - | println(1) - |} - |} - """.stripMargin - ) - - check( - "pathological comment", - """ - |object a { - |def main() /* unit */ { - |}} - """.stripMargin, - """ - |object a { - |def main(): Unit = { /* unit */ - |}} - """.stripMargin - ) - - test("on parse error") { - val Fixed.Failed(err: Failure.ParseError) = Scalafix.fix("object A {") - } -} diff --git a/phase-order.dot b/phase-order.dot new file mode 100644 index 0000000000..bad0cfb12b --- /dev/null +++ b/phase-order.dot @@ -0,0 +1,35 @@ +digraph G { +"inliner(0)"->"icode(0)" [color="#000000"] +"scalafix(0)"->"typer(0)" [color="#000000"] +"extmethods(0)"->"superaccessors(0)" [color="#000000"] +"refchecks(0)"->"pickler(0)" [color="#000000"] +"uncurry(0)"->"refchecks(0)" [color="#000000"] +"packageobjects(0)"->"namer(0)" [color="#0000ff"] +"jvm(0)"->"dce(0)" [color="#000000"] +"typer(0)"->"packageobjects(0)" [color="#0000ff"] +"inlinehandlers(0)"->"inliner(0)" [color="#000000"] +"dce(0)"->"closelim(0)" [color="#000000"] +"closelim(0)"->"inlinehandlers(0)" [color="#000000"] +"mixin(0)"->"flatten(0)" [color="#000000"] +"typer(0)"->"scalafix(0)" [color="#000000"] +"mixin(0)"->"constructors(0)" [color="#000000"] +"icode(0)"->"cleanup(0)" [color="#000000"] +"superaccessors(0)"->"patmat(0)" [color="#000000"] +"constopt(0)"->"closelim(0)" [color="#000000"] +"terminal(0)"->"jvm(0)" [color="#000000"] +"namer(0)"->"parser(0)" [color="#000000"] +"erasure(0)"->"explicitouter(0)" [color="#0000ff"] +"lazyvals(0)"->"erasure(0)" [color="#000000"] +"cleanup(0)"->"mixin(0)" [color="#000000"] +"patmat(0)"->"typer(0)" [color="#000000"] +"tailcalls(0)"->"uncurry(0)" [color="#000000"] +"delambdafy(0)"->"cleanup(0)" [color="#000000"] +"lambdalift(0)"->"lazyvals(0)" [color="#000000"] +"flatten(0)"->"constructors(0)" [color="#000000"] +"explicitouter(0)"->"tailcalls(0)" [color="#000000"] +"pickler(0)"->"extmethods(0)" [color="#000000"] +"constructors(0)"->"lambdalift(0)" [color="#000000"] +"posterasure(0)"->"erasure(0)" [color="#0000ff"] +"specialize(0)"->"tailcalls(0)" [color="#0000ff"] +"scalafix(0)" [color="#00ff00"] +} diff --git a/project/Build.scala b/project/Build.scala index 46329daa7f..d7935a231b 100644 --- a/project/Build.scala +++ b/project/Build.scala @@ -1,4 +1,5 @@ object Build { val metaV = "1.5.0.568" + val ammoniteV = "0.8.2" val testV = "3.0.0" } diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/HijackImportInfos.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/HijackImportInfos.scala new file mode 100644 index 0000000000..fe2d55c196 --- /dev/null +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/HijackImportInfos.scala @@ -0,0 +1,27 @@ +package scalafix.nsc +import scala.language.higherKinds + +import scala.tools.nsc.typechecker.Contexts + +trait HijackImportInfos { self: ReflectToolkit => + val allUsedSelectors: NonRemovableMap[g.analyzer.ImportInfo, + Set[g.ImportSelector]] = + new NonRemovableMap[g.analyzer.ImportInfo, Set[g.ImportSelector]](Set()) + val allImportInfos: NonRemovableMap[g.CompilationUnit, + List[g.analyzer.ImportInfo]] = + new NonRemovableMap[g.CompilationUnit, List[g.analyzer.ImportInfo]](Nil) + /** overrides private lazy maps in g.analyzer's Contexts with custom maps */ + def hijackImportInfos(): Unit = { + def hijackLazyField[T](name: String, value: T): T = { + val clazz = g.analyzer.asInstanceOf[Contexts].getClass + val field = clazz.getDeclaredFields.find(_.getName endsWith name).get + val method = clazz.getDeclaredMethods.find(_.getName endsWith name).get + field.setAccessible(true) + method.invoke(g.analyzer) // invoke lazy mechanism before setting field. + field.set(g.analyzer, value) + value + } + hijackLazyField("allUsedSelectors", allUsedSelectors) + hijackLazyField("allImportInfos", allImportInfos) + } +} diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/NonRemovableMap.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/NonRemovableMap.scala new file mode 100644 index 0000000000..4d7fb1592b --- /dev/null +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/NonRemovableMap.scala @@ -0,0 +1,15 @@ +package scalafix.nsc + +import scala.collection.mutable +import scala.util.Try + +/** Hack to get used symbols in compilation unit */ +class NonRemovableMap[K, V](default: V) extends mutable.HashMap[K, V] { + override def default(key: K): V = default + def customRemove(key: AnyRef): Option[V] = + Try(super.remove(key.asInstanceOf[K])).toOption.flatten + + // do nothing, see custom remove + override def remove(key: K): Option[V] = get(key) + override def -=(key: K): this.type = this +} diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala index 427c7ab277..f7c952c400 100644 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala @@ -4,6 +4,8 @@ import scala.collection.mutable import scala.meta.Dialect import scala.meta.Type import scala.reflect.internal.util.SourceFile +import scala.tools.nsc.Settings +import scala.util.Try import scala.{meta => m} import scalafix.Fixed import scalafix.Scalafix @@ -13,7 +15,21 @@ import scalafix.util.logger case class SemanticContext(enclosingPackage: String, inScope: List[String]) -trait NscSemanticApi extends ReflectToolkit { +trait NscSemanticApi extends ReflectToolkit with HijackImportInfos { + private implicit class XtensionGTree(tree: g.Tree) { + def structure = g.showRaw(tree) + } + private implicit class XtensionPosition( + gpos: scala.reflect.internal.util.Position) { + def matches(mpos: m.Position): Boolean = + gpos.isDefined && + gpos.start == mpos.start.offset && + gpos.end == mpos.end.offset + def inside(mpos: m.Position): Boolean = + gpos.isDefined && + gpos.start <= mpos.start.offset && + gpos.end >= mpos.end.offset + } /** Returns a map from byte offset to type name at that offset. */ private def offsetToType(gtree: g.Tree, @@ -53,7 +69,7 @@ trait NscSemanticApi extends ReflectToolkit { val parsed = dialect(gtree.toString()).parse[m.Type] parsed match { - case m.Parsed.Success(ast) => + case m.parsers.Parsed.Success(ast) => builder(gtree.pos.point) = cleanUp(ast) case _ => } @@ -97,16 +113,70 @@ trait NscSemanticApi extends ReflectToolkit { builder } - private def getSemanticApi(unit: g.CompilationUnit, - config: ScalafixConfig): SemanticApi = { - val offsets = offsetToType(unit.body, config.dialect) - if (!g.settings.Yrangepos.value) { + private def find(body: g.Tree, pos: m.Position): Option[g.Tree] = { + var result = Option.empty[g.Tree] + new g.Traverser { + override def traverse(tree: g.Tree): Unit = + if (tree.pos.matches(pos)) result = Some(tree) + else if (tree.pos.inside(pos)) super.traverse(tree) + }.traverse(body) + result + } + + private def fullNameToRef(fullName: String): Option[m.Ref] = { + import scala.meta._ + Try( + fullName.parse[Term].get + ).toOption.collect { case t: m.Ref => t } + } + private def symbolToRef(sym: g.Symbol): Option[m.Ref] = + Option(sym).flatMap(sym => fullNameToRef(sym.fullName)) + + private def assertSettingsAreValid(): Unit = { + val requiredSettings: Seq[(Settings#BooleanSetting, Boolean)] = Seq( + g.settings.Yrangepos -> true, +// g.settings.fatalWarnings -> false, + g.settings.warnUnusedImport -> true + ) + val missingSettings = requiredSettings.filterNot { + case (setting, value) => setting.value == value + } + if (missingSettings.nonEmpty) { + val (toEnable, toDisable) = missingSettings.partition(_._2) + def mkString(key: String, + settings: Seq[(Settings#BooleanSetting, Boolean)]) = + if (settings.isEmpty) "" + else s"\n$key: ${settings.map(_._1.name).mkString(", ")}" val instructions = - "Please re-compile with the scalac option -Yrangepos enabled" + s"Please re-compile with the scalac options:" + + mkString("Enabled", toEnable) + + mkString("Disabled", toDisable) val explanation = - "This option is necessary for the semantic API to function" + "This is necessary for scalafix semantic rewrites to function" sys.error(s"$instructions. $explanation") } + } + + private def getUnusedImports( + unit: g.CompilationUnit + ): List[g.ImportSelector] = { + def isMask(s: g.ImportSelector) = + s.name != g.termNames.WILDCARD && s.rename == g.termNames.WILDCARD + for { + imps <- allImportInfos.customRemove(unit).toList + imp <- imps.reverse.distinct + used = allUsedSelectors(imp) + s <- imp.tree.selectors + if !isMask(s) && !used(s) + _ = imps.foreach(allUsedSelectors.customRemove) + } yield s + } + + private def getSemanticApi(unit: g.CompilationUnit, + config: ScalafixConfig): SemanticApi = { + assertSettingsAreValid() + val offsets = offsetToType(unit.body, config.dialect) + val unused = getUnusedImports(unit) new SemanticApi { override def typeSignature(defn: m.Defn): Option[m.Type] = { @@ -119,6 +189,53 @@ trait NscSemanticApi extends ReflectToolkit { None } } + + /** Returns the fully qualified name of this name, or none if unable to find it */ + override def fqn(name: m.Ref): Option[m.Ref] = + find(unit.body, name.pos) + .map { x => +// logger.elem(x.symbol.alias, x.symbol, x.symbol.fullName, x) + x.toString() + } + .flatMap(fullNameToRef) + + def isUnusedImport(importee: m.Importee): Boolean = + unused.exists(_.namePos == importee.pos.start.offset) + def usedFqns: Seq[m.Ref] = { + logger.elem(unused) + val builder = Seq.newBuilder[m.Ref] + new g.Traverser { + override def traverse(tree: g.Tree): Unit = tree match { + case _: g.Import => + case _ => + for { + symRef <- symbolToRef(tree.symbol) + treeRef <- fullNameToRef(tree.toString()) + } { + // both tree and symbol are refs, then add both + // Why? For example tree ListBuffer.apply has symbol + // GenericCompanion.apply + builder += symRef + builder += treeRef + } + super.traverse(tree) + // for some crazy reason, the traverser does not visit annotations + // 1. annotations + if (tree.symbol != null) { + tree.symbol.annotations.foreach(x => + super.traverse(x.original)) + } + tree match { + // 2. or type trees + case t: g.TypeTree + if t.original != null && t.original.nonEmpty => + traverse(t.original) + case _ => + } + } + }.traverse(unit.body) + builder.result() + } } } diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala index 6b37240cfb..b863e2b10f 100644 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscComponent.scala @@ -1,21 +1,31 @@ package scalafix.nsc -import scala.reflect.internal.util.NoPosition +import scala.collection.mutable import scala.tools.nsc.Global import scala.tools.nsc.Phase import scala.tools.nsc.plugins.Plugin import scala.tools.nsc.plugins.PluginComponent +import scala.tools.nsc.typechecker.Contexts import scala.util.control.NonFatal -import scalafix.Fixed +import scalafix.Failure.ParseError import scalafix.ScalafixConfig import scalafix.util.FileOps +import scalafix.util.logger class ScalafixNscComponent(plugin: Plugin, val global: Global, getConfig: () => ScalafixConfig) extends PluginComponent with ReflectToolkit + with HijackImportInfos with NscSemanticApi { + + this.hijackImportInfos() + // warnUnusedImports could be set triggering a compiler error + // if fatal warnings is also enabled. + g.settings.warnUnusedImport.tryToSetFromPropertyValue("true") + g.settings.fatalWarnings.tryToSetFromPropertyValue("false") + override val phaseName: String = "scalafix" override val runsAfter: List[String] = "typer" :: Nil @@ -23,16 +33,10 @@ class ScalafixNscComponent(plugin: Plugin, if (unit.source.file.exists && unit.source.file.file.isFile && !unit.isJava) { - fix(unit, getConfig()) match { - case Fixed.Success(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 - ) + val config = getConfig() + val fixed = fix(unit, config).get + if (fixed.nonEmpty && fixed != new String(unit.source.content)) { + FileOps.writeFile(unit.source.file.file, fixed) } } } @@ -43,11 +47,20 @@ class ScalafixNscComponent(plugin: Plugin, try { runOn(unit) } catch { - case NonFatal(e) => - global.reporter.info( - NoPosition, - s"Failed to fix ${unit.source}. Error: ${e.getMessage}. $e", - force = true + case NonFatal(e) if !e.isInstanceOf[ParseError] => + val config = getConfig() + val err: (String) => Unit = + if (config.fatalWarning) + (msg: String) => g.reporter.error(unit.body.pos, msg) + else + (msg: String) => + g.reporter.info(unit.body.pos, msg, force = true) + val details = + if (config.fatalWarning) e.getStackTrace.mkString("\n", "\n", "") + else "" + err( + s"Failed to fix ${unit.source}. Error: ${e.getMessage}. $e" + + details ) } } diff --git a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala index f56cf8fea8..39c6a13aac 100644 --- a/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala +++ b/scalafix-nsc/src/main/scala/scalafix/nsc/ScalafixNscPlugin.scala @@ -13,8 +13,9 @@ class ScalafixNscPlugin(val global: Global) extends Plugin { val name = "scalafix" val description = "Refactoring tool." var config: ScalafixConfig = ScalafixConfig() - val components: List[PluginComponent] = - new ScalafixNscComponent(this, global, () => config) :: Nil + val component: ScalafixNscComponent = + new ScalafixNscComponent(this, global, () => config) + val components: List[PluginComponent] = component :: Nil override def init(options: List[String], error: (String) => Unit): Boolean = { options match { diff --git a/core/src/test/resources/ExplicitImplicit/basic.source b/scalafix-nsc/src/test/resources/ExplicitImplicit/basic.source similarity index 83% rename from core/src/test/resources/ExplicitImplicit/basic.source rename to scalafix-nsc/src/test/resources/ExplicitImplicit/basic.source index c9c44475de..a154ced1d7 100644 --- a/core/src/test/resources/ExplicitImplicit/basic.source +++ b/scalafix-nsc/src/test/resources/ExplicitImplicit/basic.source @@ -172,7 +172,7 @@ object A { implicit val x: List[D.B] = List(new D.B) } } -<<< SKIP slick tuple +<<< slick tuple object slick { case class Supplier(id: Int, name: String) implicit val supplierGetter = (arg: (Int, String)) => Supplier(arg._1, arg._2) @@ -196,41 +196,3 @@ package banana object x { implicit val f: Future[Int] = Future.successful(1) } -<<< SKIP global -class Global { - type Position = Int - class Symbol { - def pos: Position = ??? - } -} -trait Compiler { - val g: Global - val s: g.Symbol -} -trait Foo { self: Compiler => - val addons: Object { - val g: Foo.this.g.type - } - import g._ - import addons._ - implicit val x = s.pos -} ->>> -class Global { - type Position = Int - class Symbol { - def pos: Position = ??? - } -} -trait Compiler { - val g: Global - val s: g.Symbol -} -trait Foo { self: Compiler => - val addons: Object { - val g: Foo.this.g.type - } - import g._ - import addons._ - implicit val x: Position = s.pos -} diff --git a/scalafix-nsc/src/test/resources/syntactic/LazyVal.source b/scalafix-nsc/src/test/resources/syntactic/LazyVal.source new file mode 100644 index 0000000000..d3ce198138 --- /dev/null +++ b/scalafix-nsc/src/test/resources/syntactic/LazyVal.source @@ -0,0 +1,32 @@ +rewrites = [VolatileLazyVal] +<<< basic +object a { + +val foo = 1 + + lazy val x = 2 + @volatile lazy val dontChangeMe = 2 + private lazy val y = 2 + + class foo { + lazy val z = { + println() + } + } +} +>>> +object a { + +val foo = 1 + + @volatile lazy val x = 2 + @volatile lazy val dontChangeMe = 2 + @volatile private lazy val y = 2 + + class foo { + @volatile lazy val z = { + println() + } + } +} + diff --git a/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsBase.source b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsBase.source new file mode 100644 index 0000000000..6060713d4b --- /dev/null +++ b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsBase.source @@ -0,0 +1,73 @@ +rewrites = [] +imports.organize = true +imports.removeUnused = false +imports.expandRelative = false +<<< Basic +import scala.collection.immutable.{Seq, Map, List => L} +>>> +import scala.collection.immutable.{List => L} +import scala.collection.immutable.Map +import scala.collection.immutable.Seq +<<< order +import scalafix._ +import java.{util => ju} +import javax._ +import scala.collection.mutable +import scala.language.implicitConversions +>>> +import scala.language.implicitConversions + +import scala.collection.mutable + +import java.{util => ju} + +import javax._ +import scalafix._ +<<< spaces +import scala.collection.immutable.List +import scala.collection.immutable.Map +import scala.collection.immutable.Seq +trait foo +>>> +import scala.collection.immutable.List +import scala.collection.immutable.Map +import scala.collection.immutable.Seq +trait foo +<<< unimport +import scala.collection.mutable.{ ListBuffer => _, _ } +>>> +import scala.collection.mutable.{ListBuffer => _, _} +<<< comments +import scala.collection.immutable.List // comment +// leading +import scala.collection.immutable.{ + Map, // Map + Set // Set +} +trait a +>>> +import scala.collection.immutable.List // comment +// leading +import scala.collection.immutable.Map // Map +import scala.collection.immutable.Set // Set +trait a +<<< relative imports +import scalafix._ +import rewrite.ExplicitImplicit +>>> +import scalafix._ + +import rewrite.ExplicitImplicit +<<< wildcard subsumes +import scala.collection.mutable._ +import scala.collection.mutable.ListBuffer +object a { ListBuffer(1) } +>>> +import scala.collection.mutable._ +object a { ListBuffer(1) } +<<< rename + wildcard +import java.sql.{Array => SQLArray, _} +object a { val x: Array[Int] = Array(1) } +>>> +import java.sql.{Array => SQLArray, _} +object a { val x: Array[Int] = Array(1) } diff --git a/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsExpandRelative.source b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsExpandRelative.source new file mode 100644 index 0000000000..7f7dfc8925 --- /dev/null +++ b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsExpandRelative.source @@ -0,0 +1,46 @@ +rewrites = [] +imports.organize = true +imports.removeUnused = false +imports.expandRelative = true +<<< relative imports +import scalafix._ +import rewrite.ExplicitImplicit +>>> +import scalafix._ +import scalafix.rewrite.ExplicitImplicit +<<< language imports +import scala.annotation.implicitNotFound +import scala.collection.generic.CanBuild +import scala.collection.mutable.ArrayBuilder +import scala.collection.mutable.Builder +import scala.language.higherKinds +import scala.language.implicitConversions +>>> +import scala.language.higherKinds +import scala.language.implicitConversions + +import scala.annotation.implicitNotFound +import scala.collection.generic.CanBuild +import scala.collection.mutable.ArrayBuilder +import scala.collection.mutable.Builder +<<< don't rename trait +import slick.jdbc.H2Profile.api._ +>>> +import slick.jdbc.H2Profile.api._ +<<< root imports +import _root_.scalafix.rewrite.{ExplicitImplicit, ProcedureSyntax} +package object scalafix { + object a { + ExplicitImplicit.toString + ProcedureSyntax.toString + } +} +>>> +import _root_.scalafix.rewrite.ExplicitImplicit +import _root_.scalafix.rewrite.ProcedureSyntax +package object scalafix { + object a { + ExplicitImplicit.toString + ProcedureSyntax.toString + } +} diff --git a/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsGroupByPrefix.source b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsGroupByPrefix.source new file mode 100644 index 0000000000..72d1df91de --- /dev/null +++ b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsGroupByPrefix.source @@ -0,0 +1,16 @@ +rewrites = [] +imports.organize = true +imports.removeUnused = false +imports.groupByPrefix = true +imports.expandRelative = false +<<< Basic +import scala.collection.immutable.{Seq, Map, List => L} +>>> +import scala.collection.immutable.{ List => L, Map, Seq } +<<< relative imports +import scalafix.rewrite.{ ExplicitImplicit, ProcedureSyntax } +import ExplicitImplicit.rewrite +>>> +import scalafix.rewrite.{ ExplicitImplicit, ProcedureSyntax } + +import ExplicitImplicit.rewrite diff --git a/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsRemoveUnused.source b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsRemoveUnused.source new file mode 100644 index 0000000000..5af8ae06bc --- /dev/null +++ b/scalafix-nsc/src/test/resources/syntactic/OrganizeImportsRemoveUnused.source @@ -0,0 +1,123 @@ +rewrites = [] +imports.organize = true +imports.removeUnused = true +imports.expandRelative = false +<<< duplicate +import scala.collection.mutable.ListBuffer +import scala.collection.mutable.ListBuffer +object a { ListBuffer(1) } +>>> +import scala.collection.mutable.ListBuffer +object a { ListBuffer(1) } +<<< duplicate with curly +import scala.collection.mutable.ListBuffer +import scala.collection.mutable.{HashMap, ListBuffer} +object a { ListBuffer(1) } +>>> +import scala.collection.mutable.ListBuffer +object a { ListBuffer(1) } +<<< unused +import scala.collection.mutable.ListBuffer +object a { List(1) } +>>> +object a { List(1) } +<<< rename used +import scala.collection.mutable.{ListBuffer => LB} +import scala.collection.mutable.{HashMap => HM} +object a { LB(1) } +>>> +import scala.collection.mutable.{ListBuffer => LB} +object a { LB(1) } +<<< annotation +import scala.annotation.tailrec +import scala.{ specialized => sp } +object a { + @deprecated + @tailrec + def loop(sum: Int, xs: List[Int]): Int = loop(sum + xs.head, xs.tail) +} +trait CommutativeSemigroup[@sp(Int, Long, Float, Double) A] extends Any +>>> +import scala.{specialized => sp} +import scala.annotation.tailrec +object a { + @deprecated + @tailrec + def loop(sum: Int, xs: List[Int]): Int = loop(sum + xs.head, xs.tail) +} +trait CommutativeSemigroup[@sp(Int, Long, Float, Double) A] extends Any +<<< type position +import scala.collection.mutable.ListBuffer +trait a { + def lb: (Int, ListBuffer[Int]) +} +>>> +import scala.collection.mutable.ListBuffer +trait a { + def lb: (Int, ListBuffer[Int]) +} +<<< catalysts.Platform +import catalysts.Platform +object a { + def apply(): Boolean = (!Platform.isJs) && true +} +>>> +import catalysts.Platform +object a { + def apply(): Boolean = (!Platform.isJs) && true +} +<<< extends trait +import org.scalatest.FunSuiteLike +trait Foo extends FunSuiteLike +>>> +import org.scalatest.FunSuiteLike +trait Foo extends FunSuiteLike +<<< import method +import scala.collection.mutable.ListBuffer.apply +object a { val x = apply(2) } +>>> +import scala.collection.mutable.ListBuffer.apply +object a { val x = apply(2) } +<<< relative import becomes unused +import scala.collection.mutable.ListBuffer +import ListBuffer._ +object a { val x = empty[Int] } +>>> +import scala.collection.mutable.ListBuffer + +import ListBuffer._ +object a { val x = empty[Int] } +<<< pattern +import scala.concurrent.Future +import scala.util.Failure +import scala.util.Try +import scala.util.Success +object a { + Try(1) match { + case Success(a) => () + case Failure(a) => () + } +} +>>> +import scala.util.Failure +import scala.util.Success +import scala.util.Try +object a { + Try(1) match { + case Success(a) => () + case Failure(a) => () + } +} +<<< language imports +import scala.language.implicitConversions +import scala.language.higherKinds +object a { implicit def foo(x: Int): String = x.toString } +>>> +import scala.language.implicitConversions +object a { implicit def foo(x: Int): String = x.toString } +<<< SKIP macro string interpolator +import scalafix.tests.ImportMe.sc2xtensoin +object a { val x = foobar"kas" } +>>> +import scalafix.tests.ImportMe.sc2xtensoin +object a { val x = foobar"kas" } diff --git a/scalafix-nsc/src/test/resources/syntactic/ProcedureSyntax.source b/scalafix-nsc/src/test/resources/syntactic/ProcedureSyntax.source new file mode 100644 index 0000000000..8c861485bb --- /dev/null +++ b/scalafix-nsc/src/test/resources/syntactic/ProcedureSyntax.source @@ -0,0 +1,45 @@ +rewrites = [ProcedureSyntax] +<<< nested function +object Main { +// This is a comment + def main(args: Seq[String]) { + var number = 2 + def increment(n: Int) { + number += n + } + increment(3) + args.foreach(println) + } +} +>>> +object Main { +// This is a comment + def main(args: Seq[String]): Unit = { + var number = 2 + def increment(n: Int): Unit = { + number += n + } + increment(3) + args.foreach(println) + } +} +<<< right no paren +object a { +def foo { + println(1) +} +} +>>> +object a { +def foo: Unit = { + println(1) +} +} +<<< pathological comment +object a { +def main() /* unit */ { +}} +>>> +object a { +def main(): Unit = /* unit */ { +}} diff --git a/scalafix-nsc/src/test/scala/scalafix/DiffTest.scala b/scalafix-nsc/src/test/scala/scalafix/DiffTest.scala index ba20786f60..f66044c712 100644 --- a/scalafix-nsc/src/test/scala/scalafix/DiffTest.scala +++ b/scalafix-nsc/src/test/scala/scalafix/DiffTest.scala @@ -2,6 +2,7 @@ package scalafix import scala.collection.immutable.Seq import scalafix.rewrite.ExplicitImplicit +import scalafix.rewrite.ProcedureSyntax import scalafix.util.FileOps import scalafix.util.logger @@ -10,11 +11,11 @@ import java.io.File object DiffTest { def testsToRun: Seq[DiffTest] = tests.filter(testShouldRun) + private val testDir = "scalafix-nsc/src/test/resources" private def isOnly(name: String): Boolean = name.startsWith("ONLY ") private def isSkip(name: String): Boolean = name.startsWith("SKIP ") private def stripPrefix(name: String) = name.stripPrefix("SKIP ").stripPrefix("ONLY ").trim - private val testDir = "core/src/test/resources" private def apply(content: String, filename: String): Seq[DiffTest] = { val spec = filename.stripPrefix(testDir + File.separator) val moduleOnly = isOnly(content) @@ -79,6 +80,21 @@ case class DiffTest(spec: String, only: Boolean, config: ScalafixConfig) { def noWrap: Boolean = name.startsWith("NOWRAP ") + def isSyntax: Boolean = + Seq( + "syntactic", + ProcedureSyntax.toString + ).exists(spec.startsWith) + + private def packageName = name.replaceAll("[^a-zA-Z0-9]", "") + private def packagePrefix = s"package $packageName {\n" + private def packageSuffix = s" }\n" + def wrapped(code: String = original): String = + if (noWrap) original + else s"$packagePrefix$code$packageSuffix" + def unwrap(code: String): String = + if (noWrap) code + else code.stripPrefix(packagePrefix).stripSuffix(packageSuffix) val fullName = s"$spec: $name" } diff --git a/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala b/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala index 19d3ac06cf..b0873cfc62 100644 --- a/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala +++ b/scalafix-nsc/src/test/scala/scalafix/SemanticTests.scala @@ -8,26 +8,30 @@ 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.nsc.ScalafixNscPlugin import scalafix.rewrite.ExplicitImplicit import scalafix.rewrite.Rewrite +import scalafix.util.DiffAssertions +import scalafix.util.FileOps import scalafix.util.logger import org.scalatest.FunSuite -class SemanticTests extends FunSuite { +class SemanticTests extends FunSuite with DiffAssertions { self => val rewrite: Rewrite = ExplicitImplicit val parseAsCompilationUnit: Boolean = false - - private lazy val g: Global = { + private val testGlobal: Global = { def fail(msg: String) = sys.error(s"ReflectToMeta initialization failed: $msg") val classpath = System.getProperty("sbt.paths.scalafixNsc.test.classes") val pluginpath = System.getProperty("sbt.paths.plugin.jar") val scalacOptions = Seq( - "-Yrangepos" - ).mkString(" ", ", ", " ") - val options = "-cp " + classpath + " -Xplugin:" + pluginpath + ":" + classpath + " -Xplugin-require:scalafix" + scalacOptions + "-Yrangepos", + "-Ywarn-unused-import" + ).mkString(" ", " ", " ") + val options = "-cp " + classpath + " -Xplugin:" + pluginpath + ":" + + classpath + " -Xplugin-require:scalafix" + scalacOptions + val args = CommandLineParser.tokenize(options) val emptySettings = new Settings( error => fail(s"couldn't apply settings because $error")) @@ -41,20 +45,8 @@ class SemanticTests extends FunSuite { g } - private object fixer extends NscSemanticApi { - lazy val global: SemanticTests.this.g.type = SemanticTests.this.g - def apply(unit: g.CompilationUnit, config: ScalafixConfig): Fixed = - fix(unit, config) - } - - def wrap(code: String, diffTest: DiffTest): String = { - if (diffTest.noWrap) code - else { - val packageName = diffTest.name.replaceAll("[^a-zA-Z0-9]", "") - val packagedCode = s"package $packageName { $code }" - packagedCode - } - } + private val fixer = new ScalafixNscPlugin(testGlobal).component + private val g: fixer.global.type = fixer.global private def unwrap(gtree: g.Tree): g.Tree = gtree match { case g.PackageDef(g.Ident(g.TermName(_)), stat :: Nil) => stat @@ -64,17 +56,19 @@ class SemanticTests extends FunSuite { private def getTypedCompilationUnit(code: String): g.CompilationUnit = { import g._ val unit = new CompilationUnit(newSourceFile(code, "")) - val run = g.currentRun val phases = List(run.parserPhase, run.namerPhase, run.typerPhase) val reporter = new StoreReporter() g.reporter = reporter - phases.foreach(phase => { g.phase = phase g.globalPhase = phase phase.asInstanceOf[GlobalPhase].apply(unit) val errors = reporter.infos.filter(_.severity == reporter.ERROR) + val warnings = reporter.infos.filter(_.severity == reporter.WARNING) + warnings.foreach(warning => logger.warn(s"""${warning.msg} + |${warning.pos.lineContent} + |${warning.pos.lineCaret}""".stripMargin)) errors.foreach(error => fail(s"""scalac ${phase.name} error: ${error.msg} at ${error.pos} |$code""".stripMargin)) @@ -83,7 +77,7 @@ class SemanticTests extends FunSuite { } def fix(code: String, config: ScalafixConfig): String = { - val Fixed.Success(fixed) = fixer(getTypedCompilationUnit(code), config) + val Fixed.Success(fixed) = fixer.fix(getTypedCompilationUnit(code), config) fixed } case class MismatchException(details: String) extends Exception @@ -130,33 +124,41 @@ class SemanticTests extends FunSuite { getTypedCompilationUnit(code) } catch { case NonFatal(e) => - fail(s"Fixed source code does not typecheck! ${e.getMessage}", e) + e.printStackTrace() + fail( + s"""Fixed source code does not typecheck! + |Message: ${e.getMessage} + |Reveal: ${logger.reveal(code)} + |Code: $code""".stripMargin, + e + ) } } private def parse(code: String): m.Tree = { import scala.meta._ - code.parse[Source].get match { - // unwraps injected package - case m.Source(Seq(Pkg(_, stats))) => m.Source(stats) - case els => els - } + code.parse[Source].get } def check(original: String, expectedStr: String, diffTest: DiffTest): Unit = { - val fixed = fix(wrap(original, diffTest), diffTest.config) - val obtained = parse(fixed) + val fixed = fix(diffTest.wrapped(), diffTest.config) + val obtained = parse(diffTest.unwrap(fixed)) val expected = parse(expectedStr) try { checkMismatchesModuloDesugarings(obtained, expected) - if (!diffTest.noWrap) typeChecks(wrap(fixed, diffTest)) + if (diffTest.isSyntax) { + assertNoDiff(obtained.syntax, expected.syntax) + } + if (!diffTest.noWrap) { + typeChecks(diffTest.wrapped(fixed)) + } } catch { case MismatchException(details) => val header = s"scala -> meta converter error\n$details" val fullDetails = - s"""expected: + s"""${logger.header("Expected")} |${expected.syntax} - |obtained: + |${logger.header("Obtained")} |${obtained.syntax}""".stripMargin fail(s"$header\n$fullDetails") } diff --git a/scalafix-nsc/src/test/scala/scalafix/rewrite/ErrorSuite.scala b/scalafix-nsc/src/test/scala/scalafix/rewrite/ErrorSuite.scala new file mode 100644 index 0000000000..a4cb42d4fb --- /dev/null +++ b/scalafix-nsc/src/test/scala/scalafix/rewrite/ErrorSuite.scala @@ -0,0 +1,12 @@ +package scalafix.rewrite + +import scalafix.Failure +import scalafix.Fixed +import scalafix.Scalafix + +class ErrorSuite extends RewriteSuite(ProcedureSyntax) { + + test("on parse error") { + val Fixed.Failed(err: Failure.ParseError) = Scalafix.fix("object A {") + } +} diff --git a/core/src/test/scala/scalafix/rewrite/RewriteSuite.scala b/scalafix-nsc/src/test/scala/scalafix/rewrite/RewriteSuite.scala similarity index 93% rename from core/src/test/scala/scalafix/rewrite/RewriteSuite.scala rename to scalafix-nsc/src/test/scala/scalafix/rewrite/RewriteSuite.scala index 564f472cf1..091c856fb8 100644 --- a/core/src/test/scala/scalafix/rewrite/RewriteSuite.scala +++ b/scalafix-nsc/src/test/scala/scalafix/rewrite/RewriteSuite.scala @@ -4,6 +4,7 @@ import scalafix.Fixed import scalafix.Scalafix import scalafix.ScalafixConfig import scalafix.util.DiffAssertions +import scala.collection.immutable.Seq import org.scalatest.FunSuiteLike diff --git a/core/src/test/scala/scalafix/util/DiffAssertions.scala b/scalafix-nsc/src/test/scala/scalafix/util/DiffAssertions.scala similarity index 91% rename from core/src/test/scala/scalafix/util/DiffAssertions.scala rename to scalafix-nsc/src/test/scala/scalafix/util/DiffAssertions.scala index 49ea796de6..ebc666212f 100644 --- a/core/src/test/scala/scalafix/util/DiffAssertions.scala +++ b/scalafix-nsc/src/test/scala/scalafix/util/DiffAssertions.scala @@ -1,5 +1,7 @@ package scalafix.util +import scala.collection.mutable + import java.io.File import java.text.SimpleDateFormat import java.util.Date @@ -79,4 +81,9 @@ trait DiffAssertions extends FunSuiteLike { format.format(new Date(0L)) } } + import scala.collection.mutable.{HashMap => _, _} +// import scala.collection.mutable.{HashMap => _} +// import scala.collection.mutable._ + import Predef.{any2stringadd => _} + any2stringadd("") } diff --git a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala index 356e682e22..2c3ace440d 100644 --- a/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala +++ b/scalafix-sbt/src/main/scala/scalafix/sbt/ScalafixPlugin.scala @@ -97,7 +97,8 @@ object ScalafixPlugin extends AutoPlugin with ScalafixKeys { } else { val config = scalafixConfig.value.map { x => - if (!x.isFile) streams.value.log.warn(s"File does not exist: $x") + if (!x.isFile) + streams.value.log.warn(s"File does not exist: $x") s"-P:scalafix:${x.getAbsolutePath}" } scalafixInternalJar.value diff --git a/scalafix-tests/src/test/scala/scalafix/tests/Command.scala b/scalafix-tests/src/test/scala/scalafix/tests/Command.scala new file mode 100644 index 0000000000..5ceb2d533f --- /dev/null +++ b/scalafix-tests/src/test/scala/scalafix/tests/Command.scala @@ -0,0 +1,17 @@ +package scalafix.tests + +import scala.util.matching.Regex + +case class Command(cmd: String, optional: Boolean = false) + +object Command { + val testCompile = + Command("test:compile", optional = true) + val scalafixTask = + Command("scalafix", optional = true) + def default: Seq[Command] = Seq( + scalafixTask, + testCompile + ) + val RepoName: Regex = ".*/([^/].*).git".r +} \ No newline at end of file diff --git a/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala b/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala index 1868f3c247..2e20135ff7 100644 --- a/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala +++ b/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala @@ -1,51 +1,15 @@ package scalafix.tests -import scala.util.matching.Regex import scalafix.rewrite.ExplicitImplicit -import scalafix.rewrite.Rewrite +import scalafix.util.FileOps import scalafix.util.logger -import java.io.File - -import ammonite.ops.Path import ammonite.ops._ import org.scalatest.FunSuite import org.scalatest.concurrent.TimeLimits import org.scalatest.time.Minutes import org.scalatest.time.Span -object ItTest { - val root: Path = pwd / "target" / "it" -} - -object Command { - val testCompile = - Command("test:compile", optional = true) - val scalafixTask = - Command("scalafix", optional = true) - def default: Seq[Command] = Seq( - scalafixTask - ) - val RepoName: Regex = ".*/([^/].*).git".r -} -case class Command(cmd: String, optional: Boolean = false) - -case class ItTest(name: String, - repo: String, - hash: String, - cmds: Seq[Command] = Command.default, - rewrites: Seq[Rewrite] = Rewrite.defaultRewrites, - addCoursier: Boolean = true) { - def repoName: String = repo match { - case Command.RepoName(x) => x - case _ => - throw new IllegalArgumentException( - s"Unable to parse repo name from repo: $repo") - } - def workingPath: Path = ItTest.root / repoName - def parentDir: File = workingPath.toIO.getParentFile -} - // Clones the repo, adds scalafix as a plugin and tests that the // following commands success: // 1. test:compile @@ -58,8 +22,7 @@ abstract class IntegrationPropertyTest(t: ItTest, skip: Boolean = false) private val isCi = sys.props.contains("CI") private val maxTime = Span(20, Minutes) // just in case. - val hardClean = false - val comprehensiveTest = false + val hardClean = true // Clones/cleans/checkouts def setup(t: ItTest): Unit = { @@ -69,6 +32,7 @@ abstract class IntegrationPropertyTest(t: ItTest, skip: Boolean = false) } if (hardClean) { %%("git", "clean", "-fd")(t.workingPath) + %%("git", "checkout", t.hash)(t.workingPath) } else { %%("git", "checkout", "--", ".")(t.workingPath) } @@ -89,6 +53,8 @@ abstract class IntegrationPropertyTest(t: ItTest, skip: Boolean = false) write.over( t.workingPath / ".scalafix.conf", s"""rewrites = [${t.rewrites.mkString(", ")}] + |fatalWarnings = true + |${t.config} |""".stripMargin ) write.append( @@ -111,6 +77,28 @@ abstract class IntegrationPropertyTest(t: ItTest, skip: Boolean = false) logger.info(s"Running $id") failAfter(maxTime) { %("sbt", "++2.11.8", cmd)(t.workingPath) + if (t.testPatch) { + val obtainedPatch = %%("git", "diff")(t.workingPath).out.lines + val expectedPatch = read.lines( + Path( + FileOps + .getFile("scalafix-tests", + "src", + "main", + "resources", + "patches", + t.name + ".patch") + .getAbsoluteFile + ) + ) + val mismatch = obtainedPatch + .zip(expectedPatch) + .dropWhile { + case (a, b) => a == b + } + .take(5) + assert(mismatch.isEmpty) + } } logger.info(s"Completed $id") } @@ -126,13 +114,24 @@ abstract class IntegrationPropertyTest(t: ItTest, skip: Boolean = false) check() } +class Akka + extends IntegrationPropertyTest( + ItTest( + name = "akka", + repo = "https://github.com/akka/akka.git", + hash = "3936883e9ae9ef0f7a3b0eaf2ccb4c0878fcb145", + rewrites = Seq() + ), + skip = true + ) + class Circe extends IntegrationPropertyTest( ItTest( name = "circe", repo = "https://github.com/circe/circe.git", hash = "717e1d7d5d146cbd0455770771261e334f419b14", - rewrites = Seq(ExplicitImplicit) + rewrites = Seq() ), skip = true ) @@ -142,9 +141,11 @@ class Slick ItTest( name = "slick", repo = "https://github.com/slick/slick.git", + rewrites = Seq(), + testPatch = true, hash = "bd3c24be419ff2791c123067668c81e7de858915" ), - skip = true + skip = false ) class Scalaz @@ -162,8 +163,11 @@ class Cats ItTest( name = "cats", repo = "https://github.com/typelevel/cats.git", + config = ItTest.catsImportConfig, hash = "31080daf3fd8c6ddd80ceee966a8b3eada578198" - )) + ), + skip = true + ) class Monix extends IntegrationPropertyTest( diff --git a/scalafix-tests/src/test/scala/scalafix/tests/ItTest.scala b/scalafix-tests/src/test/scala/scalafix/tests/ItTest.scala new file mode 100644 index 0000000000..9da8cfba9f --- /dev/null +++ b/scalafix-tests/src/test/scala/scalafix/tests/ItTest.scala @@ -0,0 +1,37 @@ +package scalafix.tests + +import scalafix.rewrite.Rewrite +import ammonite.ops._ + +import java.io.File + +import ammonite.ops.Path + +case class ItTest(name: String, + repo: String, + hash: String, + config: String = "", + cmds: Seq[Command] = Command.default, + rewrites: Seq[Rewrite] = Rewrite.defaultRewrites, + addCoursier: Boolean = true, + testPatch: Boolean = false) { + def repoName: String = repo match { + case Command.RepoName(x) => x + case _ => + throw new IllegalArgumentException( + s"Unable to parse repo name from repo: $repo") + } + def workingPath: Path = ItTest.root / repoName + def parentDir: File = workingPath.toIO.getParentFile +} + +object ItTest { + val organizeImportsConfig: String = + """|imports.optimize = true + |imports.removeUnused = true""".stripMargin + val catsImportConfig: String = + """|imports.organize = true + |imports.removeUnused = false + |imports.groupByPrefix = true""".stripMargin + val root: Path = pwd / "target" / "it" +} diff --git a/scalafmt b/scalafmt index e6d108b032b60bce4c7d530e75bc40d033540540..e53be2cee17131ece199ff585e104a8d2a21a2a3 100755 GIT binary patch delta 194 zcmaEp_&RYzwiaXa<{Yi-jEv2bU3A=lWVeo9J;Q~)@q#N>Pgm1ubXxxEk&eYF$z)&8 zqN3@3kM_S_w8K8`_Lk)Zq3k-R@}?HnZhvXL{Mg^Xo9A{sa|&5%WS8J}&s8q$yere; zxkVM3AGT~ba^TnVEz5a=J9L~}yN|bQkGXDlbTe!6ze7D6dpKR$9&zpyvJ*NcGTS`B uo1Np?^3@HDb(^hpA2IBx4L*IGZ~xy+CqxlUEwpvWe(20098CrdE6a delta 195 zcmV;!06hQeVeDbBYbpU@vurBY0Rdr?L@Y)DVUvq2E`NaGy-yLStCuDs1))=PaTXlq zjIpL9Ik`(G@!jpB2>{gk6*KL-RJDim5Oy69Is#L}&OvwAl!|F9l zV0cgtZw;PimKa$7oc5Gq43R~sXz`fvp6lADc(Gv2-!Nk=E}_UkJi=GPBEhBDd;m~O x2MGA8MFLBIvpFr{0s&!@8!$fsVUt@hL Date: Sat, 28 Jan 2017 08:53:59 +0100 Subject: [PATCH 2/2] Clean up, refactor --- .drone.yml | 1 + .../scala/scalafix/util/CanonicalImport.scala | 32 ++-- .../scala/scalafix/util/OrganizeImports.scala | 171 ++++++++++-------- phase-order.dot | 35 ---- .../tests/IntegrationPropertyTest.scala | 22 --- 5 files changed, 115 insertions(+), 146 deletions(-) delete mode 100644 phase-order.dot diff --git a/.drone.yml b/.drone.yml index ae64e7e45c..e5197822ff 100644 --- a/.drone.yml +++ b/.drone.yml @@ -3,6 +3,7 @@ build: environment: - COURSIER_CACHE=/drone/cache/coursier commands: + - git rev-parse HEAD - export SBT_OPTS="-Xmx24G -XX:MaxPermSize=4G -Xss4M" # configuring ivy.home doesn't seem to work. Maybe related: # https://github.com/sbt/sbt/issues/1894 diff --git a/core/src/main/scala/scalafix/util/CanonicalImport.scala b/core/src/main/scala/scalafix/util/CanonicalImport.scala index 83c705e842..73ddda09fb 100644 --- a/core/src/main/scala/scalafix/util/CanonicalImport.scala +++ b/core/src/main/scala/scalafix/util/CanonicalImport.scala @@ -6,10 +6,10 @@ import scala.meta.tokens.Token.Comment import scalafix.rewrite.RewriteCtx object CanonicalImport { - def apply(ref: Term.Ref, - wildcard: Importee.Wildcard, - unimports: Seq[Importee.Unimport], - renames: Seq[Importee.Rename])( + def fromWildcard(ref: Term.Ref, + wildcard: Importee.Wildcard, + unimports: Seq[Importee.Unimport], + renames: Seq[Importee.Rename])( implicit ctx: RewriteCtx, ownerImport: Import ): CanonicalImport = @@ -23,7 +23,7 @@ object CanonicalImport { (wildcard +: unimports).flatMap(ctx.comments.trailing), None ) {} - def apply(ref: Term.Ref, importee: Importee)( + def fromImportee(ref: Term.Ref, importee: Importee)( implicit ctx: RewriteCtx, ownerImport: Import ): CanonicalImport = @@ -39,6 +39,12 @@ object CanonicalImport { ) {} } +/** A canonical imports is the minimal representation for a single import statement + * + * Only construct this class from custom constructors in the companion object. + * This class should be sealed abstract but the abstract modifier removes the + * convenient copy method. + */ sealed case class CanonicalImport( ref: Term.Ref, importee: Importee, @@ -47,7 +53,7 @@ sealed case class CanonicalImport( leadingComments: Set[Comment], trailingComments: Set[Comment], fullyQualifiedRef: Option[Term.Ref] -) { +)(implicit ctx: RewriteCtx) { def isRootImport: Boolean = ref.collect { @@ -72,7 +78,7 @@ sealed case class CanonicalImport( def withoutLeading(leading: Set[Comment]): CanonicalImport = copy(leadingComments = leadingComments.filterNot(leading)) def tree: Import = Import(Seq(Importer(ref, unimports :+ importee))) - def syntax(implicit ctx: RewriteCtx): String = + def syntax: String = s"${leading}import $importerSyntax$trailing" def leading: String = if (leadingComments.isEmpty) "" @@ -80,18 +86,18 @@ sealed case class CanonicalImport( def trailing: String = if (trailingComments.isEmpty) "" else trailingComments.mkString(" ", "\n", "") - def importerSyntax(implicit ctx: RewriteCtx): String = + def importerSyntax: String = s"$refSyntax.$importeeSyntax" - private def curlySpace(implicit ctx: RewriteCtx) = + private def curlySpace = if (ctx.config.imports.spaceAroundCurlyBrace) " " else "" - def actualRef(implicit ctx: RewriteCtx): Term.Ref = + def actualRef: Term.Ref = if (ctx.config.imports.expandRelative) fullyQualifiedRef.getOrElse(ref) else ref - def refSyntax(implicit ctx: RewriteCtx): String = + def refSyntax: String = actualRef.syntax - def importeeSyntax(implicit ctx: RewriteCtx): String = + def importeeSyntax: String = if (extraImportees.nonEmpty) s"""{$curlySpace${extraImportees .map(_.syntax) @@ -106,7 +112,7 @@ sealed case class CanonicalImport( case i: Importee.Wildcard => (0, i.syntax) case i => (1, i.syntax) } - def sortOrder(implicit ctx: RewriteCtx): (String, (Int, String)) = + def sortOrder: (String, (Int, String)) = (refSyntax, importeeOrder) def structure: String = Importer(ref, Seq(importee)).structure } diff --git a/core/src/main/scala/scalafix/util/OrganizeImports.scala b/core/src/main/scala/scalafix/util/OrganizeImports.scala index 2e52920921..7bd4035c0c 100644 --- a/core/src/main/scala/scalafix/util/OrganizeImports.scala +++ b/core/src/main/scala/scalafix/util/OrganizeImports.scala @@ -11,31 +11,38 @@ import scalafix.syntax._ import scalafix.util.TreePatch.AddGlobalImport import scalafix.util.TreePatch.RemoveGlobalImport -object OrganizeImports { - def extractImports(stats: Seq[Stat])( - implicit ctx: RewriteCtx): (Seq[Import], Seq[CanonicalImport]) = { - val imports = stats.takeWhile(_.is[Import]).collect { case i: Import => i } - val importees = imports.collect { - case imp @ Import(importers) => - implicit val currentImport = imp - importers.flatMap { importer => - val ref = importer.ref // fqnRef.getOrElse(importer.ref) - val wildcard = importer.importees.collectFirst { - case wildcard: Importee.Wildcard => wildcard - } - wildcard.fold(importer.importees.map(i => CanonicalImport(ref, i))) { - wildcard => - val unimports = importer.importees.collect { - case i: Importee.Unimport => i - } - val renames = importer.importees.collect { - case i: Importee.Rename => i - } - List(CanonicalImport(ref, wildcard, unimports, renames)) - } +private[this] class OrganizeImports private (implicit ctx: RewriteCtx) { + def extractImports(stats: Seq[Stat]): Seq[Import] = { + stats + .takeWhile(_.is[Import]) + .collect { case i: Import => i } + } + + def getCanonicalImports(imp: Import): Seq[CanonicalImport] = { + implicit val currentImport = imp + imp.importers.flatMap { importer => + val wildcard = importer.importees.collectFirst { + case wildcard: Importee.Wildcard => wildcard + } + wildcard.fold( + importer.importees.map(i => + CanonicalImport.fromImportee(importer.ref, i)) + ) { wildcard => + val unimports = importer.importees.collect { + case i: Importee.Unimport => i + } + val renames = importer.importees.collect { + case i: Importee.Rename => i } - }.flatten - imports -> importees + List( + CanonicalImport.fromWildcard( + importer.ref, + wildcard, + unimports, + renames + )) + } + } } def getLastTopLevelPkg(potPkg: Stat): Stat = potPkg match { @@ -44,14 +51,13 @@ object OrganizeImports { case _ => potPkg } - def getGlobalImports(ast: Tree)( - implicit ctx: RewriteCtx): (Seq[Import], Seq[CanonicalImport]) = + def getGlobalImports(ast: Tree): Seq[Import] = ast match { case Pkg(_, Seq(pkg: Pkg)) => getGlobalImports(pkg) case Source(Seq(pkg: Pkg)) => getGlobalImports(pkg) case Pkg(_, stats) => extractImports(stats) case Source(stats) => extractImports(stats) - case _ => Nil -> Nil + case _ => Nil } def removeDuplicates(imports: Seq[CanonicalImport]): Seq[CanonicalImport] = { @@ -73,32 +79,30 @@ object OrganizeImports { imports.filterNot(isDuplicate) } - def removeUnused(possiblyDuplicates: Seq[CanonicalImport])( - implicit ctx: RewriteCtx): Seq[CanonicalImport] = { + def removeUnused( + possiblyDuplicates: Seq[CanonicalImport]): Seq[CanonicalImport] = { val imports = removeDuplicates(possiblyDuplicates) if (!ctx.config.imports.removeUnused) imports else { val (usedImports, unusedImports) = ctx.semantic .map { semantic => - val unusedImports = - imports.partition(i => !semantic.isUnusedImport(i.importee)) - unusedImports + imports.partition(i => !semantic.isUnusedImport(i.importee)) } .getOrElse(possiblyDuplicates -> Nil) usedImports } } - def groupImports(imports0: Seq[CanonicalImport])( - implicit ctx: RewriteCtx): Seq[Seq[Import]] = { + def fullyQualify(imp: CanonicalImport): Option[Term.Ref] = + for { + semantic <- ctx.semantic + fqnRef <- semantic.fqn(imp.ref) + if fqnRef.is[Term.Ref] + } yield fqnRef.asInstanceOf[Term.Ref] + + def groupImports(imports0: Seq[CanonicalImport]): Seq[Seq[Import]] = { val config = ctx.config.imports - def fullyQualify(imp: CanonicalImport): Option[Term.Ref] = - for { - semantic <- ctx.semantic - fqnRef <- semantic.fqn(imp.ref) - if fqnRef.is[Term.Ref] - } yield fqnRef.asInstanceOf[Term.Ref] val imports = imports0.map(imp => imp.withFullyQualifiedRef(fullyQualify(imp))) val (fullyQualifiedImports, relativeImports) = @@ -150,51 +154,66 @@ object OrganizeImports { asImports } - def prettyPrint(imports: Seq[CanonicalImport])( - implicit ctx: RewriteCtx): String = { + def prettyPrint(imports: Seq[CanonicalImport]): String = { groupImports(imports) .map(_.map(_.syntax).mkString("\n")) .mkString("\n\n") } - def organizeImports(code: Tree, patches: Seq[ImportPatch])( - implicit ctx: RewriteCtx): Seq[TokenPatch] = { + def getRemovePatches(oldImports: Seq[Import], + tokens: Tokens): Seq[TokenPatch.Remove] = { + val toRemove = for { + firstImport <- oldImports.headOption + first <- firstImport.tokens.headOption + lastImport <- oldImports.lastOption + last <- lastImport.tokens.lastOption + } yield { + tokens.toIterator + .dropWhile(_.start < first.start) + .takeWhile { x => + x.end <= last.end + } + .map(TokenPatch.Remove) + .toList + } + toRemove.getOrElse(Nil) + } + + def cleanUpImports(globalImports: Seq[CanonicalImport], + patches: Seq[ImportPatch]): Seq[CanonicalImport] = { + def combine(is: Seq[CanonicalImport], + patch: ImportPatch): Seq[CanonicalImport] = + patch match { + case _: AddGlobalImport => + if (is.exists(_.supersedes(patch))) is + else is :+ patch.importer + case remove: RemoveGlobalImport => + is.filter(_.structure == remove.importer.structure) + } + patches.foldLeft(removeUnused(globalImports))(combine) + } + + def organizeImports(code: Tree, patches: Seq[ImportPatch]): Seq[TokenPatch] = { if (!ctx.config.imports.organize && patches.isEmpty) { Nil } else { - def combine(is: Seq[CanonicalImport], - patch: ImportPatch): Seq[CanonicalImport] = - patch match { - case add: AddGlobalImport => - if (is.exists(_.supersedes(patch))) is - else is :+ patch.importer - case remove: RemoveGlobalImport => - is.filter(_.structure == remove.importer.structure) - } - val (oldImports, globalImports) = getGlobalImports(code) - val allImports = - patches.foldLeft(removeUnused(globalImports))(combine) - groupImports(allImports) + val oldImports = getGlobalImports(code) + val globalImports = oldImports.flatMap(getCanonicalImports) + val cleanedUpImports = cleanUpImports(globalImports, patches) val tokens = code.tokens - val tok = - oldImports.headOption.map(_.tokens.head).getOrElse(tokens.head) - val toRemove = for { - firstImport <- oldImports.headOption - first <- firstImport.tokens.headOption - lastImport <- oldImports.lastOption - last <- lastImport.tokens.lastOption - } yield { - tokens.toIterator - .dropWhile(_.start < first.start) - .takeWhile { x => - x.end <= last.end - } - .map(TokenPatch.Remove) - .toList - } - val toInsert = prettyPrint(allImports) - TokenPatch.AddLeft(tok, toInsert) +: - toRemove.getOrElse(Nil) + val tokenToEdit = + oldImports.headOption + .map(_.tokens.head) + .getOrElse(tokens.head) + val toInsert = prettyPrint(cleanedUpImports) + TokenPatch.AddLeft(tokenToEdit, toInsert) +: + getRemovePatches(oldImports, tokens) } } } + +object OrganizeImports { + def organizeImports(code: Tree, patches: Seq[ImportPatch])( + implicit ctx: RewriteCtx): Seq[TokenPatch] = + new OrganizeImports().organizeImports(code, patches) +} diff --git a/phase-order.dot b/phase-order.dot deleted file mode 100644 index bad0cfb12b..0000000000 --- a/phase-order.dot +++ /dev/null @@ -1,35 +0,0 @@ -digraph G { -"inliner(0)"->"icode(0)" [color="#000000"] -"scalafix(0)"->"typer(0)" [color="#000000"] -"extmethods(0)"->"superaccessors(0)" [color="#000000"] -"refchecks(0)"->"pickler(0)" [color="#000000"] -"uncurry(0)"->"refchecks(0)" [color="#000000"] -"packageobjects(0)"->"namer(0)" [color="#0000ff"] -"jvm(0)"->"dce(0)" [color="#000000"] -"typer(0)"->"packageobjects(0)" [color="#0000ff"] -"inlinehandlers(0)"->"inliner(0)" [color="#000000"] -"dce(0)"->"closelim(0)" [color="#000000"] -"closelim(0)"->"inlinehandlers(0)" [color="#000000"] -"mixin(0)"->"flatten(0)" [color="#000000"] -"typer(0)"->"scalafix(0)" [color="#000000"] -"mixin(0)"->"constructors(0)" [color="#000000"] -"icode(0)"->"cleanup(0)" [color="#000000"] -"superaccessors(0)"->"patmat(0)" [color="#000000"] -"constopt(0)"->"closelim(0)" [color="#000000"] -"terminal(0)"->"jvm(0)" [color="#000000"] -"namer(0)"->"parser(0)" [color="#000000"] -"erasure(0)"->"explicitouter(0)" [color="#0000ff"] -"lazyvals(0)"->"erasure(0)" [color="#000000"] -"cleanup(0)"->"mixin(0)" [color="#000000"] -"patmat(0)"->"typer(0)" [color="#000000"] -"tailcalls(0)"->"uncurry(0)" [color="#000000"] -"delambdafy(0)"->"cleanup(0)" [color="#000000"] -"lambdalift(0)"->"lazyvals(0)" [color="#000000"] -"flatten(0)"->"constructors(0)" [color="#000000"] -"explicitouter(0)"->"tailcalls(0)" [color="#000000"] -"pickler(0)"->"extmethods(0)" [color="#000000"] -"constructors(0)"->"lambdalift(0)" [color="#000000"] -"posterasure(0)"->"erasure(0)" [color="#0000ff"] -"specialize(0)"->"tailcalls(0)" [color="#0000ff"] -"scalafix(0)" [color="#00ff00"] -} diff --git a/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala b/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala index 2e20135ff7..b7ebbf3778 100644 --- a/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala +++ b/scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala @@ -77,28 +77,6 @@ abstract class IntegrationPropertyTest(t: ItTest, skip: Boolean = false) logger.info(s"Running $id") failAfter(maxTime) { %("sbt", "++2.11.8", cmd)(t.workingPath) - if (t.testPatch) { - val obtainedPatch = %%("git", "diff")(t.workingPath).out.lines - val expectedPatch = read.lines( - Path( - FileOps - .getFile("scalafix-tests", - "src", - "main", - "resources", - "patches", - t.name + ".patch") - .getAbsoluteFile - ) - ) - val mismatch = obtainedPatch - .zip(expectedPatch) - .dropWhile { - case (a, b) => a == b - } - .take(5) - assert(mismatch.isEmpty) - } } logger.info(s"Completed $id") }