OrganizeImports.targetDialect: wildcard/rename syntax & curly braces stripping #1896

bjaglin · 2023-11-23T00:18:22Z

Context

Before this PR, Scala2 syntax (_ & Foo => F, surrounded by curly braces even for single-importee importers) was used, except for importers not semantically updated by an OrganizeImports run (allowing Scala3 syntax * & Foo as F, without surrounding braces for single-importee importers).

Goal of this PR

This adds a new configuration flag which controls which dialect is used consistently, for all importers. Optional braces are stripped whenever possible.

Impact

That new flag defaults to StandardLayout to maximize compatibility. Note however that is NOT similar to the previous behavior, as it's more strict: Scala 3 syntax for wildcard and rename importees (* & Foo as F) is forced for scala-3 sources.

Scala3-only builds (i.e not cross-compiling to Scala2 withtout -Xsource;3) should switch to Scala3. For example, this is the effect of targetDialect = Scala3 vs scalafix 0.11.1 on the dotty codebase (see also a note on Ascii ordering).

Testing strategy

Leaving the default (StandardLayout) for most of the tests to reduce the amount of files, unless the test is exercising renames and/or multiple importees, in which case we use Auto in the version-agnostic input, and with version-specific outputs.

bjaglin · 2023-11-23T00:50:59Z

scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala

-      case Position.None =>
-        // Position not found, implies that `importer` is derived from certain existing import
-        // statement(s). Pretty-prints it.
-        val syntax = importer.syntax


This was capturing the implicit Dialect always in scope (Dialect.current) which matches the runtime Scalafix Scala version. This one is not necessarily the Scala version of the sources (as Scalafix never runs in Scala 3 yet, and as we can have -Xsource:3 in Scala 2.13).

Moving it outside the companion object allows to capture the freshly-added instance dialect, inferred from the Configuration.

bjaglin · 2023-11-23T00:58:04Z

scalafix-tests/output/src/main/scala-3/test/organizeImports/DeduplicateImportees.scala

+import scala.collection.immutable.{Map => Dict}
+import scala.collection.immutable.{Set as _, *}


The first line is preserved while the second is generated by Scalafix, thus the inconsistency. We need to fix that - just not sure if it has to be in this PR as it does not look trivial.

Actually, preserving original syntax is also what is causing a few tests to fail as:

the sorting is done on pretty-printed Importers even though the original syntax remains

foo.{bar => qux} is rewritten foo.bar as qux with the Scala 3 dialect

As a result, the order is changed for no apparent reason: a preserved foo.{bar => qux} no longer follows import foo.wiz if the source is Scala 3.

--- obtained +++ expected import test.organizeImports.GivenImports.Beta -import test.organizeImports.GivenImports.{alpha => _} import test.organizeImports.GivenImports.given Alpha +import test.organizeImports.GivenImports.{alpha => _} import test.organizeImports.GivenImports.{beta => _, given}

bjaglin · 2023-11-23T22:36:37Z

scalafix-tests/input/src/main/scala/test/organizeImports/ExplodeImportsFormatPreserving.scala

@@ -7,6 +7,6 @@ OrganizeImports.groupedImports = Explode
 package test.organizeImports

 import test.organizeImports.ExplodeImports.FormatPreserving.g1.{ a, b }
-import test.organizeImports.ExplodeImports.FormatPreserving.g2.{ c => C, _ }
+import test.organizeImports.ExplodeImports.FormatPreserving.g2.{ c  =>  C, _ }


add whitespaces on purpose to see how rewriting with as behaves when the patch does not have the same length as what's being replaced - it also documents the behavior that surrounding whitespaces are preserved, but whitespaces within the importee are not

scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala

bjaglin · 2023-11-23T23:12:20Z

scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala

+          if (stripEnclosingBraces)
+            (
+              syntax.take(start).lastIndexOf('{'),
+              syntax.indexOf('}', end)
+            ) match {
+              case (from, to) if from != -1 && to != -1 =>
+                syntax.delete(end, to + 1)
+                syntax.delete(from, start)
+              case _ =>
+            }


-import foo.{qux as bar} +import foo.qux as bar

Still unsure if we want to force that as it goes a bit against the format-preserving features, although it seems that these ones were more about whether you would leave spaces or not around brackets - I don't know if there is a demand for keeping single elements wrapped in brackets.

If we don't strip, with the current implementation, we end up having unexpected sorting in Scala 3 as we use pretty-printed brace-less syntax as sort key:

import foo.{qux as bar} // sorted as foo.qux import foo.{bar, qux}

I think stripping {} here is fine. It's actually much easier to just do as, so I would recommend that

bjaglin · 2024-01-21T01:25:56Z

docs/rules/OrganizeImports.md

+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.


It's really a shame we can't default to Auto 😞

@tgodzik maybe metals has more information and could force Auto whenever we know for sure that sources won't be cross-compiled with Scala2? I am not convinced as we would get a lot of false negatives with the checks I have in mind

source files rooted in a scala-3 directory

source files using other Scala 3 features (I am not sure we can pull this information for the tree - as far as I know the only way would be to try and fail to parse with a Scala 2 dialect...)

We could do something similar in sbt-scalafix (although this would be the first time we mingle with rule configuration) with fairly low risk when detecting that we both have

no usage of sbt-projectmatrix

no crossScalaVersions set

I don't think we have that information even in Metals especially that with crossScalaVersion only one version would be imported.

The alternative to Auto would be to have StandardLayout which would do Scala2 target under scala and scala-2, Scala3 for scala-3 directories.

Anyway, I think having Scala2 as default makes sense

It turned out StandardLayout wasn't so hard to implement so I added it and made it the default. Thanks 👍

Awesome! Thanks!

bjaglin · 2024-01-21T23:45:28Z

scalafix-tests/input/src/main/scala-2-xsource3/test/organizeImports/TargetDialectAuto.scala

+rules = [OrganizeImports]
+OrganizeImports {
+  removeUnused = false
+  targetDialect = Auto


testing -Xsource:3 detection thanks to #1666

bjaglin · 2024-01-28T21:34:29Z

scalafix-tests/input/src/main/scala-3/test/organizeImports/ExpandGiven.scala

 */
 package test.organizeImports

 import test.organizeImports.GivenImports.Beta
 import test.organizeImports.GivenImports.Alpha
-import test.organizeImports.GivenImports.{given Beta, given Alpha}
+import test.organizeImports.GivenImports.{given Beta, given test.organizeImports.GivenImports.Gamma, given Alpha}


exercising a given with FQDN

bjaglin · 2024-01-28T22:52:38Z

@tgodzik I am gonna merge this soon, ahead of the upcoming release for the new Scala2/3 releases. I updated the PR description and rule docs to try to spell out the changes. Let me know if you see any negative impact for Metals.

tgodzik

LGTM! I think this makes sense.

This allows rules to inspect whether files are in `scala` or `scala-3` (like they would do with regular Input.File), at the cost of some verbosity in test names.

Even when a * symbol exists, Scala 3 (and the Scalameta parser with the Scala 3 dialect) will treat imports with that syntax as wildcards. For projects cross-building in Scala 2 and 3 and explicitly referencing such a symbol in the importers, runs of OrganizeImports forcing Scala 2 syntax will yield inconsistent results, as * would confusingly (but not so incorrectly) be downgraded to _ in the Scala 3 run, would it will remain a reference to the symbol in the Scala 2 run. I attempted to align the Scala 3 run behavior with the Scala 2 run one, but after a while it became clear that it would require a big redesign of the flow (because the original syntax behind the wildcard gets lost across tree copies) and would add a lot of complexity for very little value. Also, I believe the current behavior is acceptable as the Scala 3 run output is stable against a Scala 2 run. That is why I decided to simply document the behavior via a test (which should hopefully prevent changes that would remove that property in the future) instead of fixing it.

bjaglin force-pushed the scala3-import-wildcard branch from 6ff8755 to 4493993 Compare November 23, 2023 00:35

bjaglin closed this Nov 23, 2023

bjaglin force-pushed the scala3-import-wildcard branch from 4493993 to e054ea7 Compare November 23, 2023 00:49

bjaglin reopened this Nov 23, 2023

bjaglin commented Nov 23, 2023

View reviewed changes

bjaglin force-pushed the scala3-import-wildcard branch from a9ffbb0 to 5db1849 Compare November 23, 2023 08:26

bjaglin changed the title ~~OrganizeImports: favor Scala 3 syntax on Scala 3 sources~~ OrganizeImports: use new Scala 3 syntax on Scala 3 sources Nov 23, 2023

bjaglin force-pushed the scala3-import-wildcard branch from 5db1849 to 689323b Compare November 23, 2023 22:33

bjaglin commented Nov 23, 2023

View reviewed changes

bjaglin force-pushed the scala3-import-wildcard branch 2 times, most recently from ae41c23 to 6ba3390 Compare November 23, 2023 22:49

bjaglin commented Nov 23, 2023

View reviewed changes

scalafix-rules/src/main/scala/scalafix/internal/rule/OrganizeImports.scala Outdated Show resolved Hide resolved

bjaglin commented Nov 23, 2023

View reviewed changes

bjaglin force-pushed the scala3-import-wildcard branch from 6ba3390 to 9899d70 Compare November 23, 2023 23:28

bjaglin force-pushed the scala3-import-wildcard branch 5 times, most recently from cf6255e to 2be66d5 Compare January 20, 2024 17:55

bjaglin changed the title ~~OrganizeImports: use new Scala 3 syntax on Scala 3 sources~~ OrganizeImports: introduce targetDialect to force Scala 3 syntax Jan 20, 2024

bjaglin force-pushed the scala3-import-wildcard branch from 2be66d5 to fb6d6a6 Compare January 20, 2024 18:03

bjaglin changed the title ~~OrganizeImports: introduce targetDialect to force Scala 3 syntax~~ OrganizeImports: add targetDialect to allow forcing Scala 3 wildcards/renames syntax Jan 20, 2024

bjaglin changed the title ~~OrganizeImports: add targetDialect to allow forcing Scala 3 wildcards/renames syntax~~ OrganizeImports: expose Scala 3 wildcards/renames syntax via targetDialect Jan 20, 2024

bjaglin changed the title ~~OrganizeImports: expose Scala 3 wildcards/renames syntax via targetDialect~~ OrganizeImports: expose Scala3 wildcards/renames syntax via targetDialect Jan 20, 2024

bjaglin changed the title ~~OrganizeImports: expose Scala3 wildcards/renames syntax via targetDialect~~ OrganizeImports: expose new Scala3 imports syntax via targetDialect Jan 20, 2024

bjaglin changed the title ~~OrganizeImports: expose new Scala3 imports syntax via targetDialect~~ OrganizeImports: expose new Scala3 syntax via targetDialect Jan 20, 2024

bjaglin force-pushed the scala3-import-wildcard branch 2 times, most recently from a714b6a to 68dc414 Compare January 20, 2024 18:42

bjaglin mentioned this pull request Jan 20, 2024

OrganizeImports with coalesceToWildcardImportThreshold > 0 uses wildcard pattern "_" instead of "*" in Scala 3 #1894

Closed

bjaglin force-pushed the scala3-import-wildcard branch 3 times, most recently from e673412 to 2e5aabc Compare January 21, 2024 01:07

bjaglin commented Jan 21, 2024

View reviewed changes

bjaglin marked this pull request as ready for review January 21, 2024 01:26

bjaglin requested a review from tgodzik January 21, 2024 01:26

bjaglin force-pushed the scala3-import-wildcard branch 2 times, most recently from 692d679 to 462466f Compare January 21, 2024 23:44

bjaglin commented Jan 21, 2024

View reviewed changes

This was referenced Jan 22, 2024

Testing OrganizeImports.targetDialect = Scala3 bjaglin/dotty#1

Draft

Testing OrganizeImports.targetDialect = Scala2 bjaglin/dotty#2

Draft

bjaglin force-pushed the scala3-import-wildcard branch from 462466f to 6415ed1 Compare January 28, 2024 21:29

bjaglin commented Jan 28, 2024

View reviewed changes

bjaglin force-pushed the scala3-import-wildcard branch from 6415ed1 to 6d32959 Compare January 28, 2024 22:05

bjaglin changed the title ~~OrganizeImports: expose new Scala3 syntax via targetDialect~~ OrganizeImports.targetDialect: wildcard/rename syntax & curly braces stripping Jan 28, 2024

bjaglin force-pushed the scala3-import-wildcard branch from 6d32959 to c9926b0 Compare January 28, 2024 22:41

tgodzik approved these changes Jan 30, 2024

View reviewed changes

bjaglin added 2 commits January 30, 2024 14:31

testkit: include last subpath of test input source directory

2876430

This allows rules to inspect whether files are in `scala` or `scala-3` (like they would do with regular Input.File), at the cost of some verbosity in test names.

OrganizeImports: gain control over dialect used for .syntax

f4fcb65

bjaglin force-pushed the scala3-import-wildcard branch from c9926b0 to c45e7bf Compare January 30, 2024 14:28

bjaglin mentioned this pull request Jan 30, 2024

run scalafix on Scala 3 sources bjaglin/scalafix#4

Closed

bjaglin force-pushed the scala3-import-wildcard branch 3 times, most recently from 3b78ab1 to 0b9c628 Compare January 30, 2024 14:47

bjaglin added 2 commits January 30, 2024 22:41

OrganizeImports: add targetDialect for consistent syntax

9ff33fb

bjaglin force-pushed the scala3-import-wildcard branch from b10a634 to 3329013 Compare February 3, 2024 09:29

bjaglin merged commit 8fcea0d into scalacenter:main Feb 3, 2024
8 checks passed

bjaglin mentioned this pull request Feb 3, 2024

run scalafix on Scala 3 sources #1918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OrganizeImports.targetDialect: wildcard/rename syntax & curly braces stripping #1896

OrganizeImports.targetDialect: wildcard/rename syntax & curly braces stripping #1896

bjaglin commented Nov 23, 2023 •

edited

Loading

bjaglin Nov 23, 2023

bjaglin Nov 23, 2023

bjaglin Nov 23, 2023 •

edited

Loading

bjaglin Nov 23, 2023 •

edited

Loading

bjaglin Nov 23, 2023 •

edited

Loading

tgodzik Nov 24, 2023

bjaglin Jan 21, 2024

tgodzik Jan 30, 2024

bjaglin Jan 30, 2024

tgodzik Jan 30, 2024

bjaglin Jan 21, 2024

bjaglin Jan 28, 2024

bjaglin commented Jan 28, 2024

tgodzik left a comment

		import scala.collection.immutable.{Map => Dict}
		import scala.collection.immutable.{Set as _, *}

OrganizeImports.targetDialect: wildcard/rename syntax & curly braces stripping #1896

OrganizeImports.targetDialect: wildcard/rename syntax & curly braces stripping #1896

Conversation

bjaglin commented Nov 23, 2023 • edited Loading

Context

Goal of this PR

Impact

Testing strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaglin Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

bjaglin Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

bjaglin Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaglin commented Jan 28, 2024

tgodzik left a comment

Choose a reason for hiding this comment

bjaglin commented Nov 23, 2023 •

edited

Loading

bjaglin Nov 23, 2023 •

edited

Loading

bjaglin Nov 23, 2023 •

edited

Loading

bjaglin Nov 23, 2023 •

edited

Loading