From 7a96d89e67e3dfbe898301bf1439902e39e88c2c Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Fri, 11 Oct 2024 10:53:46 -0700 Subject: [PATCH] Router: fix handling of comment-space-dot chain --- .../org/scalafmt/internal/Modification.scala | 5 +- .../scala/org/scalafmt/internal/Router.scala | 44 ++++++++-------- .../test/resources/newlines/source_fold.stat | 4 +- .../resources/newlines/source_unfold.stat | 50 ++++++++----------- .../test/scala/org/scalafmt/FormatTests.scala | 2 +- 5 files changed, 52 insertions(+), 53 deletions(-) diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Modification.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Modification.scala index cd55c98e4..3199cba47 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Modification.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/Modification.scala @@ -49,7 +49,10 @@ case class NewlineT( override val length: Int = 0 } -object Newline extends NewlineT +object Newline extends NewlineT { + def orMod(flag: Boolean, mod: => Modification): Modification = + if (flag) this else mod +} object Newline2x extends NewlineT(isDouble = true) { def apply(isDouble: Boolean): NewlineT = if (isDouble) this else Newline 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 5fb7e99e1..6b63be22a 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 @@ -1706,7 +1706,10 @@ class Router(formatOps: FormatOps) { findLastApplyAndNextSelect(rightOwner, enclosed) val (prevSelect, prevApply) = findPrevSelectAndApply(thisSelect.qual, enclosed) - val nlOnly = prevApply.exists(x => getHeadToken(x.argClause).is[T.Colon]) + val afterComment = left.is[T.Comment] + val nlOnly = + style.newlines.sourceIgnored && afterComment && hasBreak() || + prevApply.exists(x => getHeadToken(x.argClause).is[T.Colon]) val expire = getLastEnclosedToken(expireTree) val indentLen = style.indent.main @@ -1758,6 +1761,7 @@ class Router(formatOps: FormatOps) { } val ftAfterRight = tokens(ft, 2) + def modSpace = Space(afterComment) val baseSplits = style.newlines.getSelectChains match { case Newlines.classic => def getNlMod = { @@ -1772,7 +1776,7 @@ class Router(formatOps: FormatOps) { if (ok) Some(nd.left) else None } val altIndent = endSelect.map(Indent(-indentLen, _, After)) - NewlineT(alt = Some(ModExt(NoSplit).withIndentOpt(altIndent))) + NewlineT(alt = Some(ModExt(modSpace).withIndentOpt(altIndent))) } val prevChain = inSelectChain(prevSelect, thisSelect, expireTree) @@ -1787,7 +1791,7 @@ class Router(formatOps: FormatOps) { val newlinePolicy = breakOnNextDot & penalizeBreaks val ignoreNoSplit = nlOnly || hasBreak && - (left.is[T.Comment] || style.optIn.breakChainOnFirstMethodDot) + (afterComment || style.optIn.breakChainOnFirstMethodDot) val chainLengthPenalty = if ( style.newlines.penalizeSingleSelectMultiArgList && @@ -1811,13 +1815,14 @@ class Router(formatOps: FormatOps) { val nlCost = nlBaseCost + nestedPenalty + chainLengthPenalty val nlMod = getNlMod val legacySplit = Split(!prevChain, 1) { // must come first, for backwards compat - if (style.optIn.breaksInsideChains) NoSplit.orNL(noBreak) + if (style.optIn.breaksInsideChains) Newline + .orMod(hasBreak(), modSpace) else nlMod }.withPolicy(newlinePolicy).onlyFor(SplitTag.SelectChainSecondNL) val slbSplit = if (ignoreNoSplit) Split.ignored else { - val noSplit = Split(NoSplit, 0) + val noSplit = Split(modSpace, 0) if (prevChain) noSplit else chainExpire match { // allow newlines in final {} block case x: T.RightBrace => noSplit @@ -1830,50 +1835,47 @@ class Router(formatOps: FormatOps) { .withPolicy(newlinePolicy) Seq(legacySplit, slbSplit, nlSplit) } else { - val isComment = left.is[T.Comment] - val doBreak = nlOnly || isComment && hasBreak + val doBreak = nlOnly || afterComment && hasBreak Seq( Split(!prevChain, 1) { - if (style.optIn.breaksInsideChains) NoSplit.orNL(noBreak) - else if (doBreak) Newline - else getNlMod + if (style.optIn.breaksInsideChains) Newline + .orMod(hasBreak(), modSpace) + else Newline.orMod(doBreak, getNlMod) }.withPolicy(breakOnNextDot) .onlyFor(SplitTag.SelectChainSecondNL), - Split(if (doBreak) Newline else Space(isComment), 0), + Split(Newline.orMod(doBreak, modSpace), 0), ) } - case _ if left.is[T.Comment] => Seq(Split(Space.orNL(noBreak), 0)) - case Newlines.keep => if (hasBreak()) Seq(Split(Newline, 0)) else if (hasBreakAfterRightBeforeNonComment(ft)) - Seq(Split(NoSplit, 0).withPolicy( + Seq(Split(modSpace, 0).withPolicy( decideNewlinesOnlyAfterClose(Split(Newline, 0))(r), ignore = next(ft).right.is[T.Comment], )) else Seq( - Split(NoSplit, 0), + Split(modSpace, 0), Split(Newline, 1).onlyFor(SplitTag.SelectChainBinPackNL), ) case Newlines.unfold => val nlCost = if (nlOnly) 0 else 1 if (prevSelect.isEmpty && nextSelect.isEmpty) Seq( - Split(nlOnly, 0)(NoSplit).withSingleLine(getSlbEnd()), + Split(nlOnly, 0)(modSpace).withSingleLine(getSlbEnd()), Split(Newline, nlCost), ) else Seq( - Split(nlOnly, 0)(NoSplit) + Split(nlOnly, 0)(modSpace) .withSingleLine(expire, noSyntaxNL = true), - Split(NewlineT(alt = Some(NoSplit)), nlCost) + Split(NewlineT(alt = Some(modSpace)), nlCost) .withPolicy(forcedBreakOnNextDotPolicy), ) case Newlines.fold => val nlCost = if (nlOnly) 0 else 1 def nlSplitBase(implicit fileLine: FileLine) = - Split(NewlineT(alt = Some(NoSplit)), nlCost) + Split(NewlineT(alt = Some(modSpace)), nlCost) if (nextDotIfSig.isEmpty) { val noSplit = if (nlOnly) Split.ignored @@ -1881,12 +1883,12 @@ class Router(formatOps: FormatOps) { val end = nextSelect .fold(expire)(x => getLastNonTrivialToken(x.qual)) val exclude = insideBracesBlock(ft, end, parensToo = true) - Split(NoSplit, 0).withSingleLine(end, exclude = exclude) + Split(modSpace, 0).withSingleLine(end, exclude = exclude) } Seq(noSplit, nlSplitBase) } else { val policy: Policy = forcedBreakOnNextDotPolicy - val noSplit = Split(nlOnly, 0)(NoSplit).withPolicy(policy) + val noSplit = Split(nlOnly, 0)(modSpace).withPolicy(policy) Seq(noSplit, nlSplitBase.withPolicy(policy)) } } diff --git a/scalafmt-tests/shared/src/test/resources/newlines/source_fold.stat b/scalafmt-tests/shared/src/test/resources/newlines/source_fold.stat index 3525386c2..4016aa3ff 100644 --- a/scalafmt-tests/shared/src/test/resources/newlines/source_fold.stat +++ b/scalafmt-tests/shared/src/test/resources/newlines/source_fold.stat @@ -9755,8 +9755,8 @@ maxColumn = 37 === sel.getContext /*ScImportSelectors*/ .getContext.asInstanceOf[ScImportExpr].reference >>> -sel - .getContext /*ScImportSelectors*/ .getContext +sel.getContext /*ScImportSelectors*/ + .getContext .asInstanceOf[ScImportExpr] .reference <<< #4133 select chain with no-break comment before dot, long diff --git a/scalafmt-tests/shared/src/test/resources/newlines/source_unfold.stat b/scalafmt-tests/shared/src/test/resources/newlines/source_unfold.stat index 52daf02f1..0222c0cd9 100644 --- a/scalafmt-tests/shared/src/test/resources/newlines/source_unfold.stat +++ b/scalafmt-tests/shared/src/test/resources/newlines/source_unfold.stat @@ -2807,7 +2807,8 @@ class Foo { val vv = v .aaa // // - .bbb.ccc() + .bbb + .ccc() val vv = v.aaa // val vv = v.aaa } @@ -2827,7 +2828,8 @@ class Foo { .bbb // // .ccc // - .ddd.eee() + .ddd + .eee() } <<< #1334 3: continue chain indent after a comment with apply class Foo { @@ -2885,7 +2887,8 @@ val a: Vector[Array[Double]] = // another comment .map { foo => bar - }.tail + } + .tail <<< #1334 6: multiple select and a match after a comment val a: Vector[Array[Double]] = b.c // Only handle first case, others will be fixed on the next pass. @@ -2898,7 +2901,8 @@ val a: Vector[Array[Double]] = b.c val a: Vector[Array[Double]] = b.c // Only handle first case, others will be fixed on the next pass. - .headOption.a match { + .headOption + .a match { case None => case _ => } @@ -6291,10 +6295,12 @@ object a { object a { foo // c2 - .bar(baz).foo(bar, baz) + .bar(baz) + .foo(bar, baz) foo // c1 // c2 - .bar(baz).foo(bar, baz) + .bar(baz) + .foo(bar, baz) foo .bar(baz) // c1 // c2 @@ -10559,30 +10565,18 @@ maxColumn = 37 === sel.getContext /*ScImportSelectors*/ .getContext.asInstanceOf[ScImportExpr].reference >>> -BestFirstSearch:278 Failed to format -UNABLE TO FORMAT, -tok #4/14: [4] /*ScImportSelectors*/∙.: Comment(ScImportSelectors) [15..36) [ ] Dot [37..38) -policies: - (NEXTSEL1NL:[Router:1896]<48d & SLB:[Router:1868]@85!d) -splits (before policy): - c=0[0] SP:[Router:1846->Router:1846](indents=[], NoPolicy) - [SelectChainFirstNL]c=0[0] SP:[Router:1846->Router:1846](indents=[], NEXTSEL1NL:[Router:1896]<61d) -splits (after policy): - [!SelectChainFirstNL]c=0[0] SP:[Router:1846->Router:1846](indents=[], NoPolicy) - c=0[0] SP:[Router:1846->Router:1846](indents=[], NEXTSEL1NL:[Router:1896]<61d) +sel + .getContext /*ScImportSelectors*/ + .getContext + .asInstanceOf[ScImportExpr] + .reference <<< #4133 select chain with no-break comment before dot, long maxColumn = 80 === sel.getContext /*ScImportSelectors*/ .getContext.asInstanceOf[ScImportExpr].reference >>> -BestFirstSearch:278 Failed to format -UNABLE TO FORMAT, -tok #12/14: [12] .∙reference: Dot [75..76) [] Ident(reference) [76..85) -policies: - SLB:[Router:1868]@85!d - SLB:[Router:1868]@85!d - SLB:[Router:1868]@85!d -splits (before policy): - c=0[0] nS:[Router:2551->Router:2551](indents=[], NoPolicy) -splits (after policy): - c=0[0] nS:[Router:2551->Router:2551](indents=[], NoPolicy) +sel + .getContext /*ScImportSelectors*/ + .getContext + .asInstanceOf[ScImportExpr] + .reference diff --git a/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala b/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala index 9aa5cfd66..2c25dcd0a 100644 --- a/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala +++ b/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala @@ -138,7 +138,7 @@ class FormatTests extends FunSuite with CanRunTests with FormatAssertions { val explored = Debug.explored.get() logger.debug(s"Total explored: $explored") if (!onlyUnit && !onlyManual) - assertEquals(explored, 1493404, "total explored") + assertEquals(explored, 1493674, "total explored") val results = debugResults.result() // TODO(olafur) don't block printing out test results. // I don't want to deal with scalaz's Tasks :'(