Router: fix binpack=oneline continuation handling

Consistently check if there's a continuation after the last argument of an argument clause and apply a policy which requires a break after the last argument if it's multiline.
scalameta · Jun 8, 2024 · 2843423 · 2843423
1 parent 8f773f2
commit 2843423
Show file tree

Hide file tree

Showing 6 changed files with 118 additions and 96 deletions.
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
@@ -2695,19 +2695,48 @@ class FormatOps(
     private def policyOnRightDelim(
         ft: FormatToken,
         exclude: TokenRanges,
-    ): Policy = {
+    ): (Option[T], Policy) = {
       val beforeDelims = findTokenWith(ft, prev) { xft =>
         noRighDelim(xft.left, xft)
       }.merge
-      if (beforeDelims eq null) return NoPolicy
+      if (beforeDelims eq null) return (None, NoPolicy)
 
       val afterDelims = findTokenWith(ft, next) { xft =>
         noRighDelim(xft.right, xft)
       }.merge
-      if (afterDelims eq null) return NoPolicy
+      if (afterDelims eq null) return (None, NoPolicy)
+
+      def closeBreakPolicy() = {
+        @tailrec
+        def iter(currft: FormatToken): Option[Policy] = {
+          val prevft = prevNonComment(currft)
+          val tok = prevft.left
+          val breakBeforeClose = matchingOpt(tok) match {
+            case Some(open) =>
+              val cfg = styleMap.at(open)
+              def cfgStyle = cfg.configStyleCallSite.prefer
+              def dangle = cfg.danglingParentheses
+                .atCallSite(prevft.meta.leftOwner)
+              cfg.newlines.source match {
+                case Newlines.unfold => true
+                case Newlines.fold => cfgStyle ||
+                  !cfg.binPack.indentCallSiteOnce
+                case _ => !cfgStyle || dangle || prev(prevft).hasBreak
+              }
+            case _ => false
+          }
+          if (breakBeforeClose) Some(decideNewlinesOnlyBeforeClose(tok))
+          else if (prevft eq beforeDelims) None
+          else iter(prev(prevft))
+        }
+
+        iter(afterDelims)
+      }
 
-      def policyBefore(token: T): Policy = {
-        val policy = decideNewlinesOnlyBeforeToken(token)
+      def policyBefore(token: T, mayBreakBeforeToken: Boolean) = {
+        if (mayBreakBeforeToken) Some(decideNewlinesOnlyBeforeToken(token))
+        else closeBreakPolicy()
+      }.fold(Policy.noPolicy) { policy =>
         val beforeDelimsEnd = beforeDelims.right.end
         // force break if multiline and if there's no other break
         delayedBreakPolicy(Policy.End == beforeDelimsEnd, exclude)(
@@ -2718,24 +2747,40 @@ class FormatOps(
       }
 
       afterDelims.right match {
-        case c: T.Dot => policyBefore(c)
-        case LeftParenOrBracket() =>
+        case c: T.Dot => // check if Dot rule includes a break option
+          val ro = afterDelims.meta.rightOwner
+          val ok = GetSelectLike.onRightOpt(ro, afterDelims).exists { x =>
+            implicit val cfg = styleMap.at(afterDelims)
+            (cfg.newlines.getSelectChains ne Newlines.classic) || {
+              val (expireTree, nextSelect) =
+                findLastApplyAndNextSelect(ro, cfg.encloseSelectChains)
+              canStartSelectChain(x, nextSelect, expireTree)
+            }
+          }
+          (Some(c), policyBefore(c, ok))
+        case x @ LeftParenOrBracket() =>
           nextNonCommentSameLineAfter(afterDelims).right match {
-            case _: T.Comment => NoPolicy
-            case c => policyBefore(c)
+            case _: T.Comment => (None, NoPolicy)
+            case c => // check if break would cause cfg style but not otherwise
+              val cfg = styleMap.at(x)
+              val ok = cfg.newlines.sourceIgnored || ! {
+                cfg.configStyleCallSite.prefer &&
+                cfg.danglingParentheses.atCallSite(afterDelims.meta.rightOwner)
+              } || next(afterDelims).hasBreak
+              (Some(c), policyBefore(c, ok))
           }
-        case _ => NoPolicy
+        case _ => (None, NoPolicy)
       }
     }
 
     def getPolicy(isCallSite: Boolean, exclude: TokenRanges)(
         afterArg: FormatToken,
-    )(implicit fileLine: FileLine): Policy = afterArg.right match {
+    )(implicit fileLine: FileLine): (Option[T], Policy) = afterArg.right match {
       case c: T.Comma // check for trailing comma, which needs no breaks
           if !nextNonCommentAfter(afterArg).right.is[T.CloseDelim] =>
-        splitOneArgPerLineAfterCommaOnBreak(exclude)(c)
+        (None, splitOneArgPerLineAfterCommaOnBreak(exclude)(c))
       case _: T.CloseDelim if isCallSite => policyOnRightDelim(afterArg, exclude)
-      case _ => NoPolicy
+      case _ => (None, NoPolicy)
     }
   }
 

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
@@ -1164,13 +1164,8 @@ class Router(formatOps: FormatOps) {
 
         val binPack = style.binPack.callSiteFor(open)
         val oneline = binPack.isOneline
-        val nextCommaOneline =
-          if (!oneline || isSingleArg) None
-          else firstArg.map(getLast).flatMap(findComma)
-
-        val noNextCommaOneline = oneline && nextCommaOneline.isEmpty
-        val noNextCommaOnelineCurried = noNextCommaOneline &&
-          leftOwner.parent.exists(followedBySelectOrApply(_).isDefined)
+        val afterFirstArgOneline =
+          if (oneline) firstArg.map(tokenAfter) else None
 
         val indentLen = style.indent.callSite
         val indent = Indent(Num(indentLen), close, Before)
@@ -1181,30 +1176,30 @@ class Router(formatOps: FormatOps) {
         val sjsOneline = !isBracket && binPack == BinPack.Site.OnelineSjs
         val sjsExclude = exclude.getIf(sjsOneline)
 
-        val nextCommaOnelinePolicy = nextCommaOneline
-          .map(splitOneArgPerLineAfterCommaOnBreak(sjsExclude))
         val newlinesPenalty = 3 + indentLen * bracketPenalty
         val penalizeNewlinesPolicy =
           policyWithExclude(exclude, Policy.End.Before, Policy.End.On)(
             new PenalizeAllNewlines(Policy.End == close, newlinesPenalty),
           )
 
+        val (onelineCurryToken, onelinePolicy) = afterFirstArgOneline
+          .map(BinPackOneline.getPolicy(true, sjsExclude))
+          .getOrElse((None, NoPolicy))
+
         def baseNoSplit(implicit fileLine: FileLine) =
           Split(Space(style.spaces.inParentheses), 0)
         val noNLPolicy = flags.noNLPolicy
-        val slbOrNL = nlOnly || singleLineOnly || noNLPolicy == null ||
-          noNextCommaOnelineCurried
 
         val noSplit =
           if (nlOnly) Split.ignored
-          else if (slbOrNL) baseNoSplit
+          else if (singleLineOnly || noNLPolicy == null) baseNoSplit
             .withSingleLine(close, sjsExclude, noSyntaxNL = true)
           else {
-            def okSingleArgsIndents = singleArgAsInfix.isEmpty &&
-              !noNextCommaOneline && style.binPack.indentCallSiteSingleArg &&
-              (isBracket || getAssignAtSingleArgCallSite(args).isEmpty)
+            def noSingleArgIndents = oneline || singleArgAsInfix.isDefined ||
+              !style.binPack.indentCallSiteSingleArg ||
+              !isBracket && getAssignAtSingleArgCallSite(args).isDefined
             val noSplitIndents =
-              if (isSingleArg && !okSingleArgsIndents) Nil
+              if (isSingleArg && noSingleArgIndents) Nil
               else if (style.binPack.indentCallSiteOnce) {
                 @tailrec
                 def iter(tree: Tree): Option[T] = tree.parent match {
@@ -1218,20 +1213,18 @@ class Router(formatOps: FormatOps) {
                 getOpenParenAlignIndents(close)
               else Seq(indent)
 
-            def optClose = scalaJsOptClose(beforeClose, flags)
-            val opt = if (oneline) nextCommaOneline else findComma(ft)
+            val nextComma =
+              if (!oneline) findComma(ft)
+              else if (isSingleArg) None
+              else afterFirstArgOneline.map(_.right)
+            val opt = nextComma.getOrElse(scalaJsOptClose(beforeClose, flags))
 
-            val noSplitPolicy = nextCommaOnelinePolicy
-              .fold(penalizeNewlinesPolicy) { p =>
-                if (noSplitIndents.exists(_.hasStateColumn)) p &
-                  penalizeNewlinesPolicy
-                else {
-                  val end = nextCommaOneline.getOrElse(close)
-                  val slbPolicy =
-                    SingleLineBlock(end, noSyntaxNL = true, exclude = sjsExclude)
-                  slbPolicy ==> penalizeNewlinesPolicy
-                }
-              }
+            val slbArg = oneline && !noSplitIndents.exists(_.hasStateColumn)
+            val slbPolicy: Policy = (if (slbArg) nextComma else None).map {
+              SingleLineBlock(_, noSyntaxNL = true, exclude = sjsExclude)
+            }
+            val noSplitPolicy = slbPolicy ==> onelinePolicy &
+              penalizeNewlinesPolicy
             val indentPolicy = Policy ? noSplitIndents.isEmpty || {
               def unindentPolicy = Policy ? (isSingleArg || sjsOneline) &&
                 unindentAtExclude(exclude, Num(-indentLen))
@@ -1246,23 +1239,26 @@ class Router(formatOps: FormatOps) {
                 }
               unindentPolicy & indentOncePolicy
             }
-            baseNoSplit.withOptimalToken(opt.getOrElse(optClose))
+            baseNoSplit.withOptimalToken(opt)
               .withPolicy(noSplitPolicy & indentPolicy & noNLPolicy())
               .withIndents(noSplitIndents)
           }
 
         val nlPolicy: Policy = {
           def newlineBeforeClose(implicit fileLine: FileLine) =
             decideNewlinesOnlyBeforeClose(close)
+          val nlClosedOnOpenOk = onelineCurryToken.forall(x =>
+            if (x.is[T.Dot]) onelinePolicy.nonEmpty else flags.scalaJsStyle,
+          )
           val nlClosedOnOpenEffective =
-            if (!noNextCommaOnelineCurried) nlCloseOnOpen
+            if (nlClosedOnOpenOk) nlCloseOnOpen
             else if (clauseSiteFlags.configStyle.prefer) NlClosedOnOpen.Cfg
             else NlClosedOnOpen.Yes
           nlClosedOnOpenEffective match {
-            case NlClosedOnOpen.No => nextCommaOnelinePolicy
+            case NlClosedOnOpen.No => onelinePolicy
             case NlClosedOnOpen.Cfg if !styleMap.forcedBinPack(leftOwner) =>
               splitOneArgOneLine(close, leftOwner) ==> newlineBeforeClose
-            case _ => newlineBeforeClose & nextCommaOnelinePolicy
+            case _ => newlineBeforeClose & onelinePolicy
           }
         }
 
@@ -1430,9 +1426,9 @@ class Router(formatOps: FormatOps) {
             val sjsExclude =
               if (binPack ne BinPack.Site.OnelineSjs) TokenRanges.empty
               else insideBracesBlock(ft, lastTok)
-            val nlPolicy = Policy ? oneline &&
+            val onelinePolicy = Policy ? oneline &&
               nextCommaOrParen
-                .map(BinPackOneline.getPolicy(callSite, sjsExclude))
+                .map(BinPackOneline.getPolicy(callSite, sjsExclude)(_)._2)
 
             val indentOncePolicy = Policy ?
               (callSite && style.binPack.indentCallSiteOnce) && {
@@ -1443,15 +1439,18 @@ class Router(formatOps: FormatOps) {
                     s.map(x => if (x.isNL) x else x.switch(trigger, true))
                 }
               }
-            val nlSplit = Split(Newline, 1, policy = nlPolicy & indentOncePolicy)
+            val nlSplit =
+              Split(Newline, 1, policy = onelinePolicy & indentOncePolicy)
             val noSplit =
               if (style.newlines.keepBreak(newlines)) Split.ignored
               else {
                 val end = endOfSingleLineBlock(
                   nextCommaOrParen.flatMap(getEndOfResultType).getOrElse(lastFT),
                 )
-                Split(Space, 0)
-                  .withSingleLine(end, noSyntaxNL = true, exclude = sjsExclude)
+                val slbPolicy =
+                  SingleLineBlock(end, exclude = sjsExclude, noSyntaxNL = true)
+                Split(Space, 0, policy = slbPolicy ==> onelinePolicy)
+                  .withOptimalToken(end)
               }
             Seq(noSplit, nlSplit)
           }

diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/util/TreeOps.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/util/TreeOps.scala
@@ -777,22 +777,6 @@ object TreeOps {
     if (groups.isEmpty) None else Some(groups)
   }
 
-  @tailrec
-  final def followedBySelectOrApply(
-      tree: Tree,
-  )(implicit ftoks: FormatTokens): Option[Tree] = tree.parent match {
-    case Some(p: Term.New) => followedBySelectOrApply(p)
-    case Some(p: Term.Select) if p.qual eq tree => Some(tree)
-    case Some(p: Member.Infix) =>
-      if (p.lhs eq tree) Some(tree) else followedBySelectOrApply(p)
-    case Some(p: Member.Apply) if p.fun eq tree =>
-      p.argClause match {
-        case SingleArgInBraces(_) => None
-        case _ => Some(tree)
-      }
-    case _ => None
-  }
-
   // Scala syntax allows commas before right braces in weird places,
   // like constructor bodies:
   // def this() = {

diff --git a/scalafmt-tests/src/test/resources/newlines/source_classic.stat b/scalafmt-tests/src/test/resources/newlines/source_classic.stat
@@ -2803,7 +2803,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
@@ -5328,15 +5331,13 @@ object a {
 }
 >>>
 object a {
-  when(
-    service.list(ApiAudienceFilter(tenants = Contains(Seq(ATenant))), Page(0, Page.maxPageLimit),
-      Order.default)
-  ).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
+  when(service.list(ApiAudienceFilter(tenants = Contains(Seq(ATenant))), Page(0, Page.maxPageLimit),
+    Order.default))
+    .thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
 
-  when(
-    service.list(ApiAudienceFilter(tenants), Contains(Seq(ATenant)), Page(0, Page.maxPageLimit),
-      Order.default)
-  ).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
+  when(service.list(ApiAudienceFilter(tenants), Contains(Seq(ATenant)), Page(0, Page.maxPageLimit),
+    Order.default))
+    .thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
 
   when(service.list(ApiAudienceFilter(tenants), Contains(Seq(ATenant))), Page(0, Page.maxPageLimit),
     Order.default).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))

diff --git a/scalafmt-tests/src/test/resources/newlines/source_keep.stat b/scalafmt-tests/src/test/resources/newlines/source_keep.stat
@@ -5147,17 +5147,13 @@ object a {
 }
 >>>
 object a {
-  when(
-    service.list(ApiAudienceFilter(tenants = Contains(Seq(ATenant))),
-      Page(0, Page.maxPageLimit), Order.default
-    )
-  ).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
+  when(service.list(ApiAudienceFilter(tenants = Contains(Seq(ATenant))),
+    Page(0, Page.maxPageLimit), Order.default
+  )).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
 
-  when(
-    service.list(ApiAudienceFilter(tenants), Contains(Seq(ATenant)),
-      Page(0, Page.maxPageLimit), Order.default
-    )
-  ).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
+  when(service.list(ApiAudienceFilter(tenants), Contains(Seq(ATenant)),
+    Page(0, Page.maxPageLimit), Order.default
+  )).thenReturn(PageResult(Seq(audience, audience.copy(id = 2)), PageInfo(50, 0, 10)))
 
   when(service.list(ApiAudienceFilter(tenants), Contains(Seq(ATenant))),
     Page(0, Page.maxPageLimit), Order.default

diff --git a/scalafmt-tests/src/test/resources/scalajs/Apply.stat b/scalafmt-tests/src/test/resources/scalajs/Apply.stat
@@ -297,10 +297,9 @@ object a {
 }
 >>>
 object a {
-  new SimpleMethodName(
-      new SimpleMethodName(new SimpleMethodName(
-          validateSimpleEncodedName(name, 0, len, openAngleBracketOK = false)))
-  ).foo
+  new SimpleMethodName(new SimpleMethodName(new SimpleMethodName(
+      validateSimpleEncodedName(name, 0, len, openAngleBracketOK = false)
+  ))).foo
 }
 <<< binpack=oneline, nested, inner with multiple args
 maxColumn = 72
@@ -360,8 +359,8 @@ object a {
     case Apply(appMeth,
             Apply(wrapRefArrayMeth,
                 StripCast(
-                    arg @ ArrayValue(elemtpt, elems)
-                ) :: Nil) :: classTagEvidence :: Nil)
+                    arg @ ArrayValue(elemtpt,
+                        elems)) :: Nil) :: classTagEvidence :: Nil)
         if WrapArray.isClassTagBasedWrapArrayMethod(wrapRefArrayMeth.symbol) &&
           appMeth.symbol == ArrayModule_genericApply =>
       bar
@@ -406,8 +405,7 @@ object a {
   Apply(appMeth,
       Apply(wrapRefArrayMeth,
           StripCast(
-              ArrayValue(elemtpt, elems)
-          ) :: Nil) :: classTagEvidence :: Nil)
+              ArrayValue(elemtpt, elems)) :: Nil) :: classTagEvidence :: Nil)
 }
 <<< binpack=always, with infix
 maxColumn = 80
@@ -1152,7 +1150,6 @@ object a {
 >>>
 object a {
   argss.tail.foldLeft(
-    global.NewFromConstructor(ctor, argss.head: _*)
-  )(
+    global.NewFromConstructor(ctor, argss.head: _*))(
     Apply(_, _))
 }