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 22, 2022 · 4984c64 · 4984c64
1 parent 5d61f46
commit 4984c64
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 82 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))
@@ -1127,13 +1127,15 @@ class Router(formatOps: FormatOps) {
             getMustDangleForTrailingCommas(close)
 
           val argsHeadOpt = argumentStarts.get(hash(right))
-          val onelinePolicy =
-            if (style.binPack.defnSite(isBracket) == BinPack.Unsafe.Oneline)
-              argsHeadOpt.flatMap { x =>
-                findFirstOnRight[T.Comma](tokens.getLast(x), close)
-                  .map(splitOneArgPerLineAfterCommaOnBreak)
-              }
-            else None
+          val isSingleArg = isSeqSingle(getApplyArgs(ft, false).args)
+          val oneline =
+            style.binPack.defnSite(isBracket) == BinPack.Unsafe.Oneline
+          val nlOnelinePolicy = argsHeadOpt.flatMap { x =>
+            if (isSingleArg || !oneline) None
+            else
+              findFirstOnRight[T.Comma](tokens.getLast(x), close)
+                .map(splitOneArgPerLineAfterCommaOnBreak)
+          }
 
           val mustDangle = onlyConfigStyle ||
             style.danglingParentheses.defnSite &&
@@ -1151,9 +1153,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)
+              val argPolicy = argsHeadOpt.fold(Policy.noPolicy) { x =>
+                if (oneline && isSingleArg) NoPolicy
+                else SingleLineBlock(x.tokens.last, noSyntaxNL = true)
+              }
               argPolicy & (penalizeOpens | penalizeBrackets)
             }
           val rightIsComment = right.is[T.Comment]
@@ -1174,7 +1177,7 @@ class Router(formatOps: FormatOps) {
               .withPolicy(noSplitPolicy)
               .withIndents(noSplitIndents),
             Split(nlMod, if (mustUseNL || slbOnly) 0 else nlCost)
-              .withPolicy(nlDanglePolicy & onelinePolicy & penalizeBrackets)
+              .withPolicy(nlDanglePolicy & nlOnelinePolicy & penalizeBrackets)
               .withIndent(indent)
           )
         }
@@ -1258,7 +1261,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 +1271,15 @@ class Router(formatOps: FormatOps) {
               val excludeOpen = exclude.ranges.map(_.lt).toSet
               UnindentAtExclude(excludeOpen, Num(-indentLen))
             }
+            val noSplitPolicy = if (needOnelinePolicy) {
+              val alignPolicy = if (noSplitIndents.exists(_.hasStateColumn)) {
+                nextCommaOnelinePolicy.map(_ & penalizeNewlinesPolicy)
+              } else None
+              alignPolicy.getOrElse {
+                val slbEnd = nextCommaOneline.getOrElse(close)
+                SingleLineBlock(slbEnd, noSyntaxNL = true)
+              }
+            } else penalizeNewlinesPolicy
             val indentOncePolicy =
               if (style.binPack.indentCallSiteOnce) {
                 val trigger = getIndentTrigger(leftOwner)
@@ -1279,9 +1291,8 @@ class Router(formatOps: FormatOps) {
               } else NoPolicy
             baseNoSplit
               .withOptimalTokenOpt(opt)
-              .withPolicy(penalizeNewlinesPolicy)
+              .withPolicy(noSplitPolicy)
               .andPolicy(unindentPolicy, !isSingleArg || noSplitIndents.isEmpty)
-              .andPolicyOpt(nextCommaOnelinePolicy)
               .andPolicy(indentOncePolicy)
           }
 
@@ -1482,7 +1493,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
@@ -4804,8 +4806,7 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
+  def foo(dd: DD[AA[BB], CC] =
         DDD.ddd): Bar[Baz] = {
     // c
     qux
@@ -4824,9 +4825,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDD.ddd): Bar[Baz] = {
+  def foo(dd: DD[AA[BB],
+        CC] = DDD.ddd): Bar[Baz] = {
     // c
     qux
   }
@@ -4846,9 +4846,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDDD.dddd) = {
+  def foo(dd: DD[AA[BB], CC] =
+            DDDD.dddd) = {
     // c
     qux
   }
@@ -4869,9 +4868,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDDD.dddd) = {
+  def foo(dd: DD[AA[BB],
+            CC] = DDDD.dddd) = {
     // c
     qux
   }

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
@@ -4649,9 +4651,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDDD.dddd) = {
+  def foo(dd: DD[AA[BB], CC] =
+            DDDD.dddd) = {
     // c
     qux
   }
@@ -4672,9 +4673,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDDD.dddd) = {
+  def foo(dd: DD[AA[BB], CC] =
+            DDDD.dddd) = {
     // c
     qux
   }
@@ -4755,9 +4755,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
@@ -4831,8 +4833,7 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
+  def foo(dd: DD[AA[BB], CC] =
         DDD.ddd): Bar[Baz] = {
     // c
     qux
@@ -4851,8 +4852,7 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
+  def foo(dd: DD[AA[BB], CC] =
         DDD.ddd): Bar[Baz] = {
     // c
     qux
@@ -4873,9 +4873,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDDD.dddd) = {
+  def foo(dd: DD[AA[BB], CC] =
+            DDDD.dddd) = {
     // c
     qux
   }
@@ -4896,9 +4895,8 @@ object a {
 }
 >>>
 object a {
-  def foo(
-      dd: DD[AA[BB], CC] =
-        DDDD.dddd) = {
+  def foo(dd: DD[AA[BB], CC] =
+            DDDD.dddd) = {
     // c
     qux
   }

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