Router: disallow breaks in args in binpack=oneline

Fix bug in binpack=oneline implementation: - handling of the first arg is different from subsequent ones (which uses a single-line block no-split policy) - the single-line block policy must also exclude multiline strings.
scalameta · Jan 21, 2022 · c5d6402 · c5d6402
1 parent 6788356
commit c5d6402
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 47 deletions.
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
@@ -1105,7 +1105,7 @@ class Router(formatOps: FormatOps) {
       case ft @ FormatToken(open @ LeftParenOrBracket(), right, _)
           if !style.binPack.defnSite(open).isNever && isDefnSite(leftOwner) =>
         val close = matching(open)
-        def slbPolicy = SingleLineBlock(close, okSLC = true)
+        def slbPolicy = SingleLineBlock(close, okSLC = true, noSyntaxNL = true)
         val baseNoSplitMod = Space(style.spaces.inParentheses)
         if (close eq right)
           Seq(Split(baseNoSplitMod, 0))
@@ -1151,10 +1151,10 @@ class Router(formatOps: FormatOps) {
                     else s.map(x => if (x.isNL) x.withPenalty(p) else x)
                 }
               }
-              val argPolicy = onelinePolicy
-                .orElse(argsHeadOpt.map(x => SingleLineBlock(x.tokens.last)))
-                .getOrElse(NoPolicy)
-              argPolicy & (penalizeOpens | penalizeBrackets)
+              val argPolicy = argsHeadOpt.fold(Policy.noPolicy) { x =>
+                SingleLineBlock(x.tokens.last, noSyntaxNL = true)
+              }
+              argPolicy & onelinePolicy & (penalizeOpens | penalizeBrackets)
             }
           val rightIsComment = right.is[T.Comment]
           val mustUseNL = onlyConfigStyle ||
@@ -1258,7 +1258,7 @@ class Router(formatOps: FormatOps) {
             needOnelinePolicy && nextCommaOneline.isEmpty ||
             // multiline binpack is at odds with unfold, at least force a break
             style.newlines.source.eq(Newlines.unfold)
-          ) baseNoSplit.withSingleLine(close)
+          ) baseNoSplit.withSingleLine(close, noSyntaxNL = true)
           else {
             val opt =
               if (oneline) nextCommaOneline.orElse(Some(close))
@@ -1268,6 +1268,9 @@ class Router(formatOps: FormatOps) {
               val excludeOpen = exclude.ranges.map(_.lt).toSet
               UnindentAtExclude(excludeOpen, Num(-indentLen))
             }
+            val noSplitPolicy = nextCommaOneline.fold(penalizeNewlinesPolicy) {
+              SingleLineBlock(_, noSyntaxNL = true)
+            }
             val indentOncePolicy =
               if (style.binPack.indentCallSiteOnce) {
                 val trigger = getIndentTrigger(leftOwner)
@@ -1279,7 +1282,7 @@ class Router(formatOps: FormatOps) {
               } else NoPolicy
             baseNoSplit
               .withOptimalTokenOpt(opt)
-              .withPolicy(penalizeNewlinesPolicy)
+              .withPolicy(noSplitPolicy)
               .andPolicy(unindentPolicy, !isSingleArg || noSplitIndents.isEmpty)
               .andPolicyOpt(nextCommaOnelinePolicy)
               .andPolicy(indentOncePolicy)
@@ -1482,7 +1485,7 @@ class Router(formatOps: FormatOps) {
               style.newlines.source.eq(Newlines.keep) && newlines != 0
             Seq(
               Split(noSpace, 0)(Space)
-                .withSingleLine(endOfSingleLineBlock(optFT)),
+                .withSingleLine(endOfSingleLineBlock(optFT), noSyntaxNL = true),
               Split(Newline, 1).withPolicy(nlPolicy & indentOncePolicy)
             )
           }

diff --git a/scalafmt-tests/src/test/resources/newlines/source_classic.stat b/scalafmt-tests/src/test/resources/newlines/source_classic.stat
@@ -2743,15 +2743,17 @@ object a {
 }
 >>>
 object a {
-  val readOut =
-    new ReadOnlyOutputDirectory("main.js" -> raw"""
+  val readOut = new ReadOnlyOutputDirectory(
+    "main.js" -> raw"""
           |console.log("hello");
           |//# sourceMappingURL=main.js.map
           |// some other comment
-          |""".stripMargin, "main.js.map" -> raw"""{
+          |""".stripMargin,
+    "main.js.map" -> raw"""{
           |  "file": "main.js",
           |  "other key": 1
-          |}""".stripMargin)
+          |}""".stripMargin
+  )
 }
 <<< binPack with named parameter values, danglingParentheses
 binPack.preset = true

diff --git a/scalafmt-tests/src/test/resources/newlines/source_fold.stat b/scalafmt-tests/src/test/resources/newlines/source_fold.stat
@@ -2547,12 +2547,12 @@ object a {
 >>>
 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
+    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
   }
 }
 <<< binpack call, oneline, with syntaxNL, single arg
@@ -2593,15 +2593,17 @@ object a {
 }
 >>>
 object a {
-  val readOut =
-    new ReadOnlyOutputDirectory("main.js" -> raw"""
+  val readOut = new ReadOnlyOutputDirectory(
+    "main.js" -> raw"""
           |console.log("hello");
           |//# sourceMappingURL=main.js.map
           |// some other comment
-          |""".stripMargin, "main.js.map" -> raw"""{
+          |""".stripMargin,
+    "main.js.map" -> raw"""{
           |  "file": "main.js",
           |  "other key": 1
-          |}""".stripMargin)
+          |}""".stripMargin
+  )
 }
 <<< binPack with named parameter values, danglingParentheses
 binPack.preset = true
@@ -4671,9 +4673,8 @@ object a {
     s"Cannot parse parameter f as DummyEnum, $wrongValue is not a member of Enum (dummy-value)"
   ))
 
-  when(audienceService.publish(any[Tenant], any[Int]))
-    .thenReturn(Success[NonEmptyList[String], ApiAudience](apiAudience
-      .copy(status = AudienceStatus.Pending.name)))
+  when(audienceService.publish(any[Tenant], any[Int])).thenReturn(Success[NonEmptyList[String],
+    ApiAudience](apiAudience.copy(status = AudienceStatus.Pending.name)))
 }
 <<< unsafeCallSite = Oneline, nested with one arg, several options
 maxColumn = 100

diff --git a/scalafmt-tests/src/test/resources/newlines/source_keep.stat b/scalafmt-tests/src/test/resources/newlines/source_keep.stat
@@ -2748,15 +2748,17 @@ object a {
 }
 >>>
 object a {
-  val readOut =
-    new ReadOnlyOutputDirectory("main.js" -> raw"""
+  val readOut = new ReadOnlyOutputDirectory(
+    "main.js" -> raw"""
           |console.log("hello");
           |//# sourceMappingURL=main.js.map
           |// some other comment
-          |""".stripMargin, "main.js.map" -> raw"""{
+          |""".stripMargin,
+    "main.js.map" -> raw"""{
           |  "file": "main.js",
           |  "other key": 1
-          |}""".stripMargin)
+          |}""".stripMargin
+  )
 }
 <<< binPack with named parameter values, danglingParentheses
 binPack.preset = true

diff --git a/scalafmt-tests/src/test/resources/newlines/source_unfold.stat b/scalafmt-tests/src/test/resources/newlines/source_unfold.stat
@@ -2884,11 +2884,13 @@ object a {
 >>>
 object a {
   val readOut =
-    new ReadOnlyOutputDirectory("main.js" -> raw"""
+    new ReadOnlyOutputDirectory(
+      "main.js" -> raw"""
           |console.log("hello");
           |//# sourceMappingURL=main.js.map
           |// some other comment
-          |""".stripMargin)
+          |""".stripMargin
+    )
 }
 <<< binpack call, oneline, with syntaxNL, multiple args
 maxColumn = 60
@@ -2908,14 +2910,17 @@ object a {
 >>>
 object a {
   val readOut =
-    new ReadOnlyOutputDirectory("main.js" -> raw"""
+    new ReadOnlyOutputDirectory(
+      "main.js" -> raw"""
           |console.log("hello");
           |//# sourceMappingURL=main.js.map
           |// some other comment
-          |""".stripMargin, "main.js.map" -> raw"""{
+          |""".stripMargin,
+      "main.js.map" -> raw"""{
           |  "file": "main.js",
           |  "other key": 1
-          |}""".stripMargin)
+          |}""".stripMargin
+    )
 }
 <<< binPack with named parameter values, danglingParentheses
 binPack.preset = true

diff --git a/scalafmt-tests/src/test/resources/scalajs/Apply.stat b/scalafmt-tests/src/test/resources/scalajs/Apply.stat
@@ -241,7 +241,8 @@ object a {
 }
 >>>
 object a {
-  foo(new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name,
+  foo(
+      new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name,
           0, len, openAngleBracketOK = false))),
       new SimpleMethodName(new SimpleMethodName(validateSimpleEncodedName(name,
           0, len, openAngleBracketOK = false))))
@@ -490,10 +491,12 @@ optDef.getOrElse {
 optDef.getOrElse {
   abort(foo &&
     bar)
-  abort(foo &&
+  abort(
+      foo &&
       bar,
       baz)
-  abort(foo &&
+  abort(
+      foo &&
       bar,
       foo &&
       bar,
@@ -521,10 +524,12 @@ foo.bar {
 foo.bar {
   abort(foo &&
     bar)
-  abort(foo &&
+  abort(
+      foo &&
       bar,
       baz)
-  abort(foo &&
+  abort(
+      foo &&
       bar,
       foo &&
       bar,
@@ -739,7 +744,8 @@ optDef.getOrElse {
 optDef.getOrElse {
   abort(fooFoo &&
     barBar)
-  abort(fooFoo &&
+  abort(
+      fooFoo &&
       barBar,
       bazBaz)
 }
@@ -759,7 +765,8 @@ optDef.getOrElse {
 optDef.getOrElse {
   abort(fooFoo &&
     barBar)
-  abort(fooFoo &&
+  abort(
+      fooFoo &&
       barBar,
       bazBaz)
 }
@@ -788,13 +795,15 @@ object a {
 object a {
   Seq(foo +
     bar)
-  Seq(foo +
+  Seq(
+    foo +
       bar,
     foo +
       bar)
   Seq(foo &&
     bar)
-  Seq(foo &&
+  Seq(
+    foo &&
     bar,
     foo &&
     bar)
@@ -824,13 +833,15 @@ object a {
 object a {
   Seq(foo +
     bar)
-  Seq(foo +
+  Seq(
+    foo +
       bar,
     foo +
       bar)
   Seq(foo &&
     bar)
-  Seq(foo &&
+  Seq(
+    foo &&
     bar,
     foo &&
     bar)
@@ -859,13 +870,15 @@ object a {
 object a {
   Seq(foo +
     bar)
-  Seq(foo +
+  Seq(
+    foo +
       bar,
     foo +
       bar)
   Seq(foo &&
     bar)
-  Seq(foo &&
+  Seq(
+    foo &&
     bar,
     foo &&
     bar)