OrganizeImports: add targetDialect for consistent syntax

scalacenter · Jan 30, 2024 · 3b78ab1 · 3b78ab1
1 parent f4fcb65
commit 3b78ab1
Show file tree

Hide file tree

Showing 72 changed files with 539 additions and 48 deletions.
diff --git a/docs/rules/OrganizeImports.md b/docs/rules/OrganizeImports.md
@@ -1328,3 +1328,147 @@ object RemoveUnused {
   val long: JLong = JLong.parseLong("0")
 }
 ```
+
+`targetDialect`
+--------------
+
+Specifies the [wildcards and renames](https://docs.scala-lang.org/scala3/reference/changed-features/imports.html)
+syntax to use and drives stripping of optional curly braces.
+
+### Value type
+
+Enum: `Auto | Scala2 | Scala3 | StandardLayout`
+
+#### `Auto`
+
+Infer the dialect from compilation settings (Scala version or `-Xsource` when
+provided) and behave like `Scala2` or `Scala3`. This is safe only for sources
+that are not cross-compiled and therefore it is NOT the default value (see
+rationale below).
+
+#### `Scala2`
+
+For all files,
+* use `_` as wildcard and `=>` for renames
+* curly braces are stripped for importers with a single regular importee
+
+#### `Scala3`
+
+For all files,
+* use `*` as wildcard and `as` for renames
+* curly braces are stripped for importers with a single importee
+
+#### `StandardLayout`
+
+For files containing `scala-3` in their path,
+* use `*` as wildcard and `as` for renames
+* curly braces are stripped for importers with a single importee
+
+For others,
+* use `_` as wildcard and `=>` for renames
+* curly braces are stripped for importers with a single regular importee
+
+### Default value
+
+`StandardLayout`
+
+Rationale: `Auto` is not a safe default for projects cross-compiling with
+Scala 3.x and Scala 2.x without `-Xsource:3`, as the Scala 3.x Scalafix run
+would introduce Scala 2.x compilation errors.
+
+### Examples
+
+#### `Scala2`  
+
+```conf
+OrganizeImports {
+  targetDialect = Scala2
+}
+```
+
+Before:
+
+```scala
+import scala.collection.immutable.{List => L}
+import scala.collection.mutable.{Map}
+import scala.collection.mutable.{Buffer => _, Seq => S, _}```
+
+After:
+
+```scala
+import scala.collection.immutable.{List => L}
+import scala.collection.mutable.Map
+import scala.collection.mutable.{Buffer => _, Seq => S, _}
+```
+
+#### `Scala3`  
+
+```conf
+OrganizeImports {
+  targetDialect = Scala3
+}
+```
+
+Before:
+
+```scala
+import scala.collection.immutable.{List => L}
+import scala.collection.mutable.{Map}
+import scala.collection.mutable.{Buffer => _, Seq => S, _}
+import scala.concurrent.ExecutionContext.Implicits.{given scala.concurrent.ExecutionContext}
+```
+
+After:
+
+```scala
+import scala.collection.immutable.List as L
+import scala.collection.mutable.Map
+import scala.collection.mutable.{Buffer as _, Seq as S, *}
+import scala.concurrent.ExecutionContext.Implicits.given scala.concurrent.ExecutionContext
+```
+
+#### `StandardLayout`  
+
+```conf
+OrganizeImports {
+  targetDialect = StandardLayout
+}
+```
+
+##### `**/scala-3/**` files
+
+Before:
+
+```scala
+import scala.collection.immutable.{List => L}
+import scala.collection.mutable.{Map}
+import scala.collection.mutable.{Buffer => _, Seq => S, _}
+import scala.concurrent.ExecutionContext.Implicits.{given scala.concurrent.ExecutionContext}
+```
+
+After:
+
+```scala
+import scala.collection.immutable.List as L
+import scala.collection.mutable.Map
+import scala.collection.mutable.{Buffer as _, Seq as S, *}
+import scala.concurrent.ExecutionContext.Implicits.given scala.concurrent.ExecutionContext
+```
+
+##### Other files
+
+Before:
+
+```scala
+import scala.collection.immutable.{List => L}
+import scala.collection.mutable.{Map}
+import scala.collection.mutable.{Buffer => _, Seq => S, _}```
+
+After:
+
+```scala
+import scala.collection.immutable.{List => L}
+import scala.collection.mutable.Map
+import scala.collection.mutable.{Buffer => _, Seq => S, _}
+```
+
diff --git a/scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala b/scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala
@@ -20,6 +20,9 @@ import scala.meta.Tree
 import scala.meta.XtensionClassifiable
 import scala.meta.XtensionCollectionLikeUI
 import scala.meta.XtensionSyntax
+import scala.meta.dialects
+import scala.meta.inputs.Input.File
+import scala.meta.inputs.Input.VirtualFile
 import scala.meta.inputs.Position
 import scala.meta.tokens.Token
 
@@ -29,6 +32,7 @@ import metaconfig.ConfEncoder
 import metaconfig.ConfOps
 import metaconfig.Configured
 import metaconfig.internal.ConfGet
+import scalafix.internal.config.ScalaVersion
 import scalafix.internal.rule.ImportMatcher.*
 import scalafix.internal.rule.ImportMatcher.---
 import scalafix.internal.rule.ImportMatcher.parse
@@ -47,11 +51,15 @@ import scalafix.v1.XtensionTreeScalafix
 class OrganizeImports(
     config: OrganizeImportsConfig,
     // shadows the default implicit always on scope (Dialect.current, matching the runtime Scala version)
-    implicit val targetDialect: Dialect
+    implicit val targetDialect: Dialect = Dialect.current,
+    scala3DialectForScala3Paths: Boolean = false
 ) extends SemanticRule("OrganizeImports") {
   import OrganizeImports._
   import ImportMatcher._
 
+  private lazy val scala3TargetDialect =
+    new OrganizeImports(config, dialects.Scala3)
+
   private val matchers = buildImportMatchers(config)
 
   private val wildcardGroupIndex: Int = matchers indexOf *
@@ -62,7 +70,7 @@ class OrganizeImports(
   private val diagnostics: ArrayBuffer[Diagnostic] =
     ArrayBuffer.empty[Diagnostic]
 
-  def this() = this(OrganizeImportsConfig(), Dialect.current)
+  def this() = this(OrganizeImportsConfig())
 
   override def description: String = "Organize import statements"
 
@@ -77,6 +85,18 @@ class OrganizeImports(
       .andThen(checkScalacOptions(_, config.scalacOptions, config.scalaVersion))
 
   override def fix(implicit doc: SemanticDocument): Patch = {
+    (doc.input, scala3DialectForScala3Paths) match {
+      case (File(path, _), true)
+          if path.toFile.getAbsolutePath.contains("scala-3/") =>
+        scala3TargetDialect.fixWithImplicitDialect(doc)
+      case (VirtualFile(path, _), true) if path.contains("scala-3/") =>
+        scala3TargetDialect.fixWithImplicitDialect(doc)
+      case _ =>
+        fixWithImplicitDialect(doc)
+    }
+  }
+
+  private def fixWithImplicitDialect(implicit doc: SemanticDocument): Patch = {
     unusedImporteePositions ++= doc.diagnostics.collect {
       case d if d.message == "Unused import" => d.position
     }
@@ -570,7 +590,12 @@ class OrganizeImports(
 
       importer match {
         case Importer(_, Importee.Wildcard() :: Nil) =>
-          syntax.patch(syntax.lastIndexOfSlice("._"), ".\u0001", 2)
+          val wildcardSyntax = Importee.Wildcard().syntax
+          syntax.patch(
+            syntax.lastIndexOfSlice(s".$wildcardSyntax"),
+            ".\u0001",
+            2
+          )
 
         case _ if importer.isCurlyBraced =>
           syntax
@@ -712,9 +737,57 @@ class OrganizeImports(
   private def importerSyntax(importer: Importer): String =
     importer.pos match {
       case pos: Position.Range =>
-        // Position found, implies that `importer` was directly parsed from the source code. Returns
-        // the original parsed text to preserve the original source level formatting.
-        pos.text
+        // Position found, implies that `importer` was directly parsed from the source code. Rewrite
+        // importees to ensure they follow the target dialect. For importers with a single importee,
+        // strip enclosing braces if they exist (or add/preserve them for Rename & Unimport on Scala 2).
+
+        val syntax = new StringBuilder(pos.text)
+        val Importer(_, importees) = importer
+
+        def patchSyntax(
+            t: Tree,
+            newSyntax: String,
+            requireBracesOnSingleImportee: Boolean
+        ) = {
+          val start = t.pos.start - pos.start
+          syntax.replace(start, t.pos.end - pos.start, newSyntax)
+
+          if (importees.length == 1) {
+            val end = t.pos.start - pos.start + newSyntax.length
+            (
+              syntax.take(start).lastIndexOf('{'),
+              syntax.indexOf('}', end),
+              requireBracesOnSingleImportee
+            ) match {
+              case (-1, -1, true) =>
+                // braces required but not detected
+                syntax.append('}')
+                syntax.insert(start, '{')
+              case (opening, closing, false)
+                  if opening != -1 && closing != -1 =>
+                // braces detected but not required
+                syntax.delete(end, closing + 1)
+                syntax.delete(opening, start)
+              case _ =>
+            }
+          }
+        }
+
+        val dialectAlwaysRequiresBraces = !targetDialect.allowAsForImportRename
+
+        // traverse & patch backwards to avoid shifting indices
+        importees.reverse.foreach {
+          case i @ Importee.Rename(_, _) =>
+            patchSyntax(i, i.copy().syntax, dialectAlwaysRequiresBraces)
+          case i @ Importee.Unimport(_) =>
+            patchSyntax(i, i.copy().syntax, dialectAlwaysRequiresBraces)
+          case i @ Importee.Wildcard() =>
+            patchSyntax(i, i.copy().syntax, false)
+          case i =>
+            patchSyntax(i, i.syntax, false)
+        }
+
+        syntax.toString
 
       case Position.None =>
         // Position not found, implies that `importer` is derived from certain existing import
@@ -736,11 +809,15 @@ class OrganizeImports(
 
   implicit private class ImporterExtension(importer: Importer) {
 
-    /** Checks whether the `Importer` is curly-braced when pretty-printed. */
+    /**
+     * Checks whether the `Importer` should be curly-braced when pretty-printed.
+     */
     def isCurlyBraced: Boolean = {
       val importees @ Importees(_, renames, unimports, _, _, _) =
         importer.importees
-      renames.nonEmpty || unimports.nonEmpty || importees.length > 1
+
+      importees.length > 1 ||
+      ((renames.length == 1 || unimports.length == 1) && !targetDialect.allowAsForImportRename)
     }
 
     /**
@@ -801,9 +878,41 @@ object OrganizeImports {
       }
     }
 
+    val (targetDialect, scala3DialectForScala3Paths) =
+      conf.targetDialect match {
+        case TargetDialect.Auto =>
+          val dialect = ScalaVersion
+            .from(scalaVersion)
+            .map { scalaVersion =>
+              def extractSuffixForScalacOption(prefix: String) = {
+                scalacOptions
+                  .filter(_.startsWith(prefix))
+                  .lastOption
+                  .map(_.stripPrefix(prefix))
+              }
+
+              // We only lookup the Scala 2 option (Scala 3 is `-source`), as the latest Scala 3
+              // dialect is used no matter what the actual minor version is anyway, and as of now,
+              // the pretty printer is just more permissive with the latest dialect.
+              val sourceScalaVersion =
+                extractSuffixForScalacOption("-Xsource:")
+                  .flatMap(ScalaVersion.from(_).toOption)
+
+              scalaVersion.dialect(sourceScalaVersion)
+            }
+            .getOrElse(Dialect.current)
+          (dialect, false)
+        case TargetDialect.Scala2 =>
+          (dialects.Scala212, false)
+        case TargetDialect.Scala3 =>
+          (dialects.Scala3, false)
+        case TargetDialect.StandardLayout =>
+          (dialects.Scala212, true)
+      }
+
     if (!conf.removeUnused || hasWarnUnused)
       Configured.ok(
-        new OrganizeImports(conf, Dialect.current)
+        new OrganizeImports(conf, targetDialect, scala3DialectForScala3Paths)
       )
     else if (hasCompilerSupport)
       Configured.error(
@@ -952,11 +1061,11 @@ object OrganizeImports {
    * Categorizes a list of `Importee`s into the following four groups:
    *
    *   - Names, e.g., `Seq`, `Option`, etc.
-   *   - Renames, e.g., `{Long => JLong}`, `{Duration => D}`, etc.
-   *   - Unimports, e.g., `{Foo => _}`.
-   *   - Givens, e.g., `{given Foo}`.
+   *   - Renames, e.g., `{Long => JLong}`, `Duration as D`, etc.
+   *   - Unimports, e.g., `{Foo => _}` or `Foo as _`.
+   *   - Givens, e.g., `given Foo`.
    *   - GivenAll, i.e., `given`.
-   *   - Wildcard, i.e., `_`.
+   *   - Wildcard, i.e., `_` or `*`.
    */
   object Importees {
     def unapply(importees: Seq[Importee]): Option[

diff --git a/scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImportsConfig.scala b/scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImportsConfig.scala
@@ -75,6 +75,19 @@ object Preset {
     ConfEncoder.instance(v => Conf.Str(v.toString))
 }
 
+sealed trait TargetDialect
+object TargetDialect {
+  case object Auto extends TargetDialect
+  case object Scala2 extends TargetDialect
+  case object Scala3 extends TargetDialect
+  case object StandardLayout extends TargetDialect
+
+  implicit def reader: ConfDecoder[TargetDialect] =
+    ReaderUtil.oneOf(Auto, Scala2, Scala3, StandardLayout)
+  implicit def writer: ConfEncoder[TargetDialect] =
+    ConfEncoder.instance(v => Conf.Str(v.toString))
+}
+
 final case class OrganizeImportsConfig(
     blankLines: BlankLines = BlankLines.Auto,
     coalesceToWildcardImportThreshold: Option[Int] = None,
@@ -88,7 +101,8 @@ final case class OrganizeImportsConfig(
     importSelectorsOrder: ImportSelectorsOrder = ImportSelectorsOrder.Ascii,
     importsOrder: ImportsOrder = ImportsOrder.Ascii,
     preset: Preset = Preset.DEFAULT,
-    removeUnused: Boolean = true
+    removeUnused: Boolean = true,
+    targetDialect: TargetDialect = TargetDialect.StandardLayout
 )
 
 object OrganizeImportsConfig {