From 825209cb6f21ee9c2e05a574c45c6bbeb3e9dc8b Mon Sep 17 00:00:00 2001 From: Cheng Lian Date: Tue, 12 May 2020 13:17:12 -0700 Subject: [PATCH] Remove the intellijCompatible option (#48) --- README.adoc | 113 ------------------ ...yImportedImplicitsIntelliJCompatible.scala | 20 ---- ...itlyImportedImplicitsLanguageFeature.scala | 19 +++ ...yImportedImplicitsIntelliJCompatible.scala | 15 --- ...itlyImportedImplicitsLanguageFeature.scala | 18 +++ .../src/main/scala/fix/OrganizeImports.scala | 58 +++++---- .../scala/fix/OrganizeImportsConfig.scala | 1 - 7 files changed, 75 insertions(+), 169 deletions(-) delete mode 100644 input/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala create mode 100644 input/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala delete mode 100644 output/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala create mode 100644 output/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala diff --git a/README.adoc b/README.adoc index edc8e78e0..4d18269ad 100644 --- a/README.adoc +++ b/README.adoc @@ -765,119 +765,6 @@ import scala.concurrent.duration ---- -- -=== `intellijCompatible` - -==== Description - -When set to `true`, try to be compatible with the IntelliJ IDEA Scala import optimizer when possible. - -In some edge cases, the IntelliJ IDEA Scala import optimizer may produce broken code. For example, it does not handle the explicitly imported implicits case properly (see the <> option). In order to guarantee correctness, `OrganizeImports` may behave differently from IntelliJ in these cases. - -However, to some users, having `OrganizeImports` fighting against IntelliJ can be annoying during development. Considering that those edge cases are rarely seen, the `intellijCompatible` option provides a choice to sacrifice edge case correctness for IntelliJ compatibility. - -==== Value type - -Boolean - -==== Default value - -`false` - -==== Example - -IntelliJ compatible mode turned off:: -+ --- -In the following example, we have two explicitly imported implicit names: - -[source,scala] ----- -import scala.language.postfixOps -import scala.concurrent.ExecutionContext.Implicits.global ----- - -By default, when the IntelliJ compatible mode is turned off, to avoid the aforementioned correctness issue (see the <> option), they are moved into the last order-preserving group together with all the relative imports, if any. - -Configuration: - -[source,hocon] ----- -OrganizeImports { - groups = ["scala.", "*"] - intellijCompatible = false -} ----- - -Before: - -[source,scala] ----- -import scala.collection.mutable -import mutable.ArrayBuffer -import scala.collection.JavaConverters._ -import scala.language.postfixOps -import org.apache.spark.SparkContext -import scala.concurrent.ExecutionContext.Implicits.global ----- - -After: - -[source,scala] ----- -import scala.collection.mutable -import scala.collection.JavaConverters._ - -import org.apache.spark.SparkContext - -import mutable.ArrayBuffer -import scala.language.postfixOps -import scala.concurrent.ExecutionContext.Implicits.global ----- --- - -IntelliJ compatible mode turned on:: -+ --- -After turning on the IntelliJ compatible mode, explicitly imported implicits names are grouped in the same way as other imports (only relative imports are moved to the last group). - -Configuration: - -[source,hocon] ----- -OrganizeImports { - groups = ["scala.", "*"] - intellijCompatible = true -} ----- - -Before: - -[source,scala] ----- -import scala.collection.mutable -import mutable.ArrayBuffer -import scala.collection.JavaConverters._ -import scala.language.postfixOps -import org.apache.spark.SparkContext -import scala.concurrent.ExecutionContext.Implicits.global ----- - -After: - -[source,scala] ----- -import scala.collection.mutable -import scala.collection.JavaConverters._ -import scala.concurrent.ExecutionContext.Implicits.global -import scala.language.postfixOps - -import org.apache.spark.SparkContext - -import mutable.ArrayBuffer ----- --- - -[[remove-unused]] === `removeUnused` ==== Description diff --git a/input/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala b/input/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala deleted file mode 100644 index 471d08cee..000000000 --- a/input/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala +++ /dev/null @@ -1,20 +0,0 @@ -/* -rules = [OrganizeImports] -OrganizeImports { - importsOrder = Ascii - intellijCompatible = true -} - */ -package fix - -import scala.collection.mutable.ArrayBuffer -import fix.Implicits.a._ -import scala.concurrent.ExecutionContext.Implicits.global -import fix.Implicits.b.{i, s} - -object ExplicitlyImportedImplicitsIntelliJCompatible { - def f1()(implicit i: Int) = ??? - def f2()(implicit s: String) = ??? - f1() - f2() -} diff --git a/input/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala b/input/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala new file mode 100644 index 000000000..d54ffe18d --- /dev/null +++ b/input/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala @@ -0,0 +1,19 @@ +/* +rules = [OrganizeImports] +OrganizeImports.importsOrder = Ascii + */ +package fix + +import scala.concurrent.ExecutionContext +import fix.Implicits.b._ +import scala.languageFeature.postfixOps +import ExecutionContext.Implicits.global +import scala.languageFeature.implicitConversions +import fix.Implicits.a.{i, s} + +object ExplicitlyImportedImplicitsLanguageFeature { + def f1()(implicit i: Int) = ??? + def f2()(implicit s: String) = ??? + f1() + f2() +} diff --git a/output/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala b/output/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala deleted file mode 100644 index ed9f64095..000000000 --- a/output/src/main/scala/fix/ExplicitlyImportedImplicitsIntelliJCompatible.scala +++ /dev/null @@ -1,15 +0,0 @@ -package fix - -import scala.collection.mutable.ArrayBuffer -import scala.concurrent.ExecutionContext.Implicits.global - -import fix.Implicits.a._ -import fix.Implicits.b.i -import fix.Implicits.b.s - -object ExplicitlyImportedImplicitsIntelliJCompatible { - def f1()(implicit i: Int) = ??? - def f2()(implicit s: String) = ??? - f1() - f2() -} diff --git a/output/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala b/output/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala new file mode 100644 index 000000000..2a9b8f242 --- /dev/null +++ b/output/src/main/scala/fix/ExplicitlyImportedImplicitsLanguageFeature.scala @@ -0,0 +1,18 @@ +package fix + +import scala.concurrent.ExecutionContext +import scala.languageFeature.implicitConversions +import scala.languageFeature.postfixOps + +import fix.Implicits.b._ + +import ExecutionContext.Implicits.global +import fix.Implicits.a.i +import fix.Implicits.a.s + +object ExplicitlyImportedImplicitsLanguageFeature { + def f1()(implicit i: Int) = ??? + def f2()(implicit s: String) = ??? + f1() + f2() +} diff --git a/rules/src/main/scala/fix/OrganizeImports.scala b/rules/src/main/scala/fix/OrganizeImports.scala index 4defa3400..6d1449b73 100644 --- a/rules/src/main/scala/fix/OrganizeImports.scala +++ b/rules/src/main/scala/fix/OrganizeImports.scala @@ -146,25 +146,44 @@ class OrganizeImports(config: OrganizeImportsConfig) extends SemanticRule("Organ private def partitionImplicits( importers: Seq[Importer] )(implicit doc: SemanticDocument): (Seq[Importer], Seq[Importer]) = { - if (config.intellijCompatible) { - (Nil, importers) - } else { - val (implicits, implicitPositions) = importers.flatMap { - case importer @ Importer(_, importees) => - importees - .filter(_.is[Importee.Name]) - .filter(_.symbol.info.exists(_.isImplicit)) - .map(i => importer.copy(importees = i :: Nil) -> i.pos) - }.unzip - - val noImplicits = importers.flatMap { - filterImportees(_) { importee => - !implicitPositions.contains(importee.pos) - }.toSeq - } - - (implicits, noImplicits) + val (implicits, implicitPositions) = importers.flatMap { + case importer @ Importer(_, importees) => + importees + .filter(_.is[Importee.Name]) + .filter(_.symbol.info.exists(_.isImplicit)) + // Explicitly imported `scala.languageFeature` implicits are special cased and treated as + // normal imports due to the following reasons: + // + // 1. The IntelliJ IDEA Scala import optimizer does not handle the explicitly imported + // implicit names case (see [issue #30][1]). Moving `scala.languageFeature` to the last + // order-preserving import group produces a different result from IntelliJ, which can + // be annoying for users who use both IntelliJ and `OrganizeImports`. + // + // [1]: https://github.com/liancheng/scalafix-organize-imports/issues/30 + // + // 2. Importing `scala.languageFeature` values is almost the only commonly seen cases + // where a Scala developer imports an implicit by name explicitly. Yet, there's + // practically zero chance that an implicit of the same type can be imported to cause a + // conflict, unless the user is intentionally torturing the Scala compiler. Not + // treating them as implicits and group them as other normal imports minimizes the + // chance of behaving differently from IntelliJ without. + // + // 3. The `scala.languageFeature` values are defined as `object`s in Scala 2.12 but + // changed to implicit lazy vals in Scala 2.13. Treating them as implicits means that + // `OrganizeImports` may produce different results when `OrganizeImports` users upgrade + // their code base from Scala 2.12 to 2.13. This introduces an annoying and unnecessary + // thing to be taken care of. + .filter(_.symbol.owner.normalized != "scala.languageFeature.") + .map(i => importer.copy(importees = i :: Nil) -> i.pos) + }.unzip + + val noImplicits = importers.flatMap { + filterImportees(_) { importee => + !implicitPositions.contains(importee.pos) + }.toSeq } + + (implicits, noImplicits) } private def expandRelative(importer: Importer)(implicit doc: SemanticDocument): Importer = { @@ -350,7 +369,7 @@ object OrganizeImports { List(symbols, lowerCases, upperCases) flatMap (_ sortBy (_.syntax)) } - private def mergeImporters(importers: Seq[Importer]): Seq[Importer] = { + private def mergeImporters(importers: Seq[Importer]): Seq[Importer] = importers.groupBy(_.ref.syntax).values.toSeq.flatMap { case importer :: Nil => // If this group has only one importer, returns it as is to preserve the original source @@ -472,7 +491,6 @@ object OrganizeImports { importeesList filter (_.nonEmpty) map (Importer(ref, _)) } - } private def explodeImportees(importers: Seq[Importer]): Seq[Importer] = importers.flatMap { diff --git a/rules/src/main/scala/fix/OrganizeImportsConfig.scala b/rules/src/main/scala/fix/OrganizeImportsConfig.scala index 69f89145a..7a245876b 100644 --- a/rules/src/main/scala/fix/OrganizeImportsConfig.scala +++ b/rules/src/main/scala/fix/OrganizeImportsConfig.scala @@ -56,7 +56,6 @@ final case class OrganizeImportsConfig( ), importSelectorsOrder: ImportSelectorsOrder = ImportSelectorsOrder.Ascii, importsOrder: ImportsOrder = ImportsOrder.Ascii, - intellijCompatible: Boolean = false, removeUnused: Boolean = true )