FormatOps: fix scala.js style implementation

The `scalaJs` modification of `ScalafmtConfig` is, among other things, defined as having config style enabled and dangling parens disabled. Earlier, we accidentally swapped the two flag values, so let's fix it.
scalameta · May 12, 2024 · 610eb9b · 610eb9b
1 parent cb1c28f
commit 610eb9b
Show file tree

Hide file tree

Showing 14 changed files with 157 additions and 100 deletions.
diff --git a/docs/configuration.md b/docs/configuration.md
@@ -185,40 +185,73 @@ top-level.
 
 ### Top-level presets
 
-- `preset=default`: this preset is implicit and sets all values to their
-  defaults.
-- `preset=IntelliJ`: this preset is defined as
+#### `preset=default`
+
+This preset is implicit and sets all values to their defaults.
+
+#### `preset=IntelliJ`
+
+This preset is defined as
 
 ```
     preset = default
     indent.defnSite = 2
     optIn.configStyleArguments = false
 ```
 
-- `preset=defaultWithAlign`: this preset is defined as
+#### `preset=defaultWithAlign`
+
+This preset is defined as
 
 ```
     preset = default
     align.preset = more
 ```
 
-- `preset=Scala.js`: this preset is defined as
+#### `preset=Scala.js`
+
+This preset intends to approximate the
+[style used for `scala.js`](https://github.com/scala-js/scala-js/blob/main/CODINGSTYLE.md).
+
+It uses modified detection of [config-style formatting](#newlines-config-style-formatting):
+
+- [according to Sébastien Doeraene](https://github.com/scala-js/scala-js/pull/4522#issuecomment-879168123),
+  config-style should be driven solely by presence of a dangling closing parenthesis
+- to achieve that, use a combination of
+  [`danglingParentheses.xxxSite = false`](#danglingparenthesescallsite) and
+  [`newlines.configStyleXxxSite.prefer = true`](#newlinesconfigstylexxxsiteprefer)
+
+The preset itself is defined as:
 
 ```
     preset = default
     binPack.preset = true
-    align.openParenCtrlSite = false
-    indent.callSite = 4
+    align {
+      openParenCtrlSite = false
+      arrowEnumeratorGenerator = false
+      tokens = [ caseArrow ]
+    }
+    danglingParentheses {
+      callSite = false
+      defnSite = false
+    }
     docstrings.style = Asterisk
     importSelectors = binPack
+    indent.callSite = 4
     newlines {
-      neverInResultType = true
+      avoidInResultType = true
       neverBeforeJsNative = true
       sometimesBeforeColonInMethodReturnType = false
     }
-    runner.optimizer.callSite {
-      minSpan = 500
-      minCount = 5
+    runner.optimizer {
+      callSite {
+        minSpan = 500
+        minCount = 5
+      }
+      defnSite {
+        minSpan = 500
+        minCount = 5
+      }
     }
 ```
 
@@ -4917,10 +4950,9 @@ and similarly has cross-parameter interactions:
     over binpacking
   - for `newlines.source=classic`, behaviour depends on
     [config-style](#newlinesconfigstylexxxsiteprefer):
-    - if enabled: used if [detected](#newlines-config-style-formatting), otherwise binpacked
-    - if disabled with both [`danglingParentheses.callSite`](#danglingparenthesescallsite)
-      enabled and closing parenthesis following a break: forces config-style, as described in
-      [scala.js](https://github.com/scala-js/scala-js/pull/4522#issuecomment-879168123)
+    - if enabled, config style is used if
+      - it is [detected](#newlines-config-style-formatting), or
+      - configured to use [scala.js style](#presetscalajs)
     - otherwise, uses binpacking
   - for other values of [`newlines.source`](#newlinessource),
     binpacking takes precedence

diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala
@@ -316,11 +316,7 @@ object ScalafmtConfig {
     * https://github.com/scala-js/scala-js/blob/master/CODINGSTYLE.md
     */
   val scalaJs: ScalafmtConfig = default.copy(
-    binPack = BinPack(
-      defnSite = BinPack.Site.Always,
-      callSite = BinPack.Site.Always,
-      parentConstructors = BinPack.ParentCtors.Always,
-    ),
+    binPack = BinPack.enabled,
     danglingParentheses = DanglingParentheses(false, false),
     indent = Indents(callSite = 4, defnSite = 4),
     importSelectors = ImportSelectors.binPack,

diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
@@ -2653,7 +2653,7 @@ class FormatOps(
         mustUseConfigStyle(ftAfterOpen, ftBeforeClose.hasBreak, !literalArgList)
     val scalaJsStyle = style.newlines.source == Newlines.classic &&
       configStyle == ConfigStyle.None && !literalArgList &&
-      clauseSiteFlags.dangleCloseDelim && !clauseSiteFlags.configStyle.prefer
+      !clauseSiteFlags.dangleCloseDelim && clauseSiteFlags.configStyle.prefer
     BinpackCallsiteFlags(
       literalArgList = literalArgList,
       dangleForTrailingCommas = dangleForTrailingCommas,

diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Router.scala
@@ -1183,10 +1183,10 @@ class Router(formatOps: FormatOps) {
         val singleLineOnly = style.binPack.literalsSingleLine &&
           flags.literalArgList
 
-        val scalaJsStyleNL = flags.scalaJsStyle && beforeClose.hasBreak
         val nlOpen = flags.dangleForTrailingCommas ||
           flags.configStyle != ConfigStyle.None ||
-          style.newlines.keepBreak(newlines) || scalaJsStyleNL ||
+          style.newlines.keepBreak(newlines) ||
+          flags.scalaJsStyle && beforeClose.hasBreak ||
           tokens.isRightCommentWithBreak(ft)
         val nlOnly = nlOpen && !singleLineOnly
 
@@ -1306,7 +1306,7 @@ class Router(formatOps: FormatOps) {
               styleMap.forcedBinPack(leftOwner)
             ) bothPolicies
             else configStylePolicy
-          else if (scalaJsStyleNL) configStylePolicy
+          else if (flags.scalaJsStyle) configStylePolicy
           else if (
             flags.dangleForTrailingCommas ||
             clauseSiteFlags.dangleCloseDelim &&

diff --git a/scalafmt-tests/src/test/resources/binPack/TermNameList.stat b/scalafmt-tests/src/test/resources/binPack/TermNameList.stat
@@ -88,7 +88,8 @@ somethingVeryLong(
     baz(
       qux // c1
     )
-  })
+  }
+)
 <<< unsafeCallSite forced newline, !cfg + dangle
 maxColumn = 20
 binPack.unsafeCallSite = true
@@ -102,8 +103,7 @@ somethingVeryLong(bar{baz(
 >>>
 somethingVeryLong(
   bar {
-    baz(
-      qux // c1
+    baz(qux // c1
     )
   }
 )

diff --git a/scalafmt-tests/src/test/resources/default/Apply.stat b/scalafmt-tests/src/test/resources/default/Apply.stat
@@ -1712,13 +1712,13 @@ object a {
 }
 >>>
 object a {
-  val cls = Select(
-    Select(
-      Select(
-        Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_), scalajs), js),
-        nme.annotation, x, y, z),
-      internal_, a, b, c),
-    wasPublicBeforeTyperXxx)
+  val cls =
+    Select(Select(Select(Select(Select(Select(Ident(nme.ROOTPKG), nme.scala_),
+                                       scalajs),
+                                js),
+                         nme.annotation, x, y, z),
+                  internal_, a, b, c),
+           wasPublicBeforeTyperXxx)
 }
 <<< #2633 with binPack always
 maxColumn = 70
@@ -1744,7 +1744,8 @@ object a {
       .getAnnotation(JSNameAnnotation).fold {
         sym.addAnnotation(JSNameAnnotation,
                           Literal(Constant(jsInterop.defaultJSNameOf(
-                                             symForName))))
+                                             symForName
+                                           ))))
       } { annot =>
         sym.addAnnotation(annot)
       }
@@ -1773,8 +1774,8 @@ object a {
     symForName
       .getAnnotation(JSNameAnnotation).fold {
         sym.addAnnotation(JSNameAnnotation,
-                          Literal(Constant(
-                            jsInterop.defaultJSNameOf(symForName))))
+                          Literal(Constant(jsInterop
+                            .defaultJSNameOf(symForName))))
       } { annot =>
         sym.addAnnotation(annot)
       }

diff --git a/scalafmt-tests/src/test/resources/newlines/source_classic.stat b/scalafmt-tests/src/test/resources/newlines/source_classic.stat
@@ -2801,7 +2801,10 @@ object a {
   test("foo") {
     a.b(c, d) shouldBe
       E(Seq(F(1, "v1"), F(2, "v2")),
-        G(Some(Seq(h, i)), Some(Seq(j, k)), a.b, c.d, e.f.g, h.i.j)).foo
+        G(Some(Seq(h, i)),
+          Some(Seq(j, k)), a.b, c.d,
+          e.f.g, h.i.j)
+      ).foo
   }
 }
 <<< binpack call, oneline, with syntaxNL, single arg
@@ -2889,7 +2892,8 @@ object a {
 object a {
   val foo = bar.map(x =>
     x.copy(
-      baz = Option.when(false)(x.qux)))
+      baz = Option.when(false)(x.qux)
+    ))
 }
 <<< binPack with named parameter values, !configStyleArguments + danglingParentheses
 binPack.preset = true
@@ -5063,7 +5067,8 @@ object a {
 >>>
 object a {
   def foo(bb: BB, cc: CC, dd: DD = DDD.ddd): Bar[
-    Baz] = {
+    Baz
+  ] = {
     // c
     qux
   }
@@ -7796,18 +7801,12 @@ object Main {
 }
 >>>
 object Main {
-  val bar1 = foo1(
-    10000,
-    10001,
-    10002 + 0
-  )
   val bar1 = foo1(10000,
     10001, 10002 + 0)
-  val bar1 = foo1(
-    10000,
-    10001,
-    10002 + 0
-  )
+  val bar1 = foo1(10000,
+    10001, 10002 + 0)
+  val bar1 = foo1(10000,
+    10001, 10002 + 0)
   val bar1 = foo1(10000,
     10001, 10002 + 0)
   val bar2 = foo2(0, 1, 2, 3,
@@ -7898,8 +7897,11 @@ object Main {
   )
   val bar1 = foo1(10000,
     10001, 10002 + 0)
-  val bar1 = foo1(10000,
-    10001, 10002 + 0)
+  val bar1 = foo1(
+    10000,
+    10001,
+    10002 + 0
+  )
   val bar1 = foo1(10000,
     10001, 10002 + 0)
   val bar2 = foo2(