From c5d6402ae88e707dea7483c87cb92fb27fa1f660 Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Thu, 20 Jan 2022 07:31:26 -0800 Subject: [PATCH] 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. --- .../scala/org/scalafmt/internal/Router.scala | 19 +++++---- .../resources/newlines/source_classic.stat | 10 +++-- .../test/resources/newlines/source_fold.stat | 27 ++++++------- .../test/resources/newlines/source_keep.stat | 10 +++-- .../resources/newlines/source_unfold.stat | 15 ++++--- .../src/test/resources/scalajs/Apply.stat | 39 ++++++++++++------- 6 files changed, 73 insertions(+), 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 index 6fc03f6818..ca0c496214 100644 --- 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 index 725780b2ac..54dbbf1f62 100644 --- 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 index 1ed59e346e..f4d1c4e6af 100644 --- 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 index f46d3b9cdc..0faec85bff 100644 --- 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 index 4aa27b388a..7deb9dd81e 100644 --- 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 index e1d95dc9fe..43cbc1ee41 100644 --- 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)