From 5015f57126afa80dbb83f3703a440d6ac0b21377 Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Fri, 11 Aug 2023 09:22:35 +0300 Subject: [PATCH] FormatWriter: accumulate align shift correctly Alignment logic accumulates alignment shift for matching lines, with the assumption that the extra space will definitely be added. However, we don't add extra spaces to no-split cases, instead attempting to delay them until the next token... but if the next token is also a no-split, then the extra alignment space is lost, leading to incorrect formatting. One such example is `(` which typically has no spaces before or after. --- docs/configuration.md | 8 ++-- .../org/scalafmt/internal/FormatWriter.scala | 47 ++++++++++--------- .../resources/align/DefaultWithAlign.stat | 6 +-- .../scala/org/scalafmt/util/HasTests.scala | 5 +- 4 files changed, 38 insertions(+), 28 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 00fc950649..c93aaaa20d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -948,9 +948,11 @@ Apart from a few special cases, the way alignment works is as follows: - both owners belong to the same "statement container"; this is determined internally and usually selects the nearest containing block, template, match, argument or parameter group. -- if two tokens match: - - if there's a space before them, they themselves will be aligned on the right - - if there's a space after them, the next tokens will be aligned on the left +- for each token that has matches in the surrounding lines: + - we'll determine the amount of extra space needed to be added _before_ + that token, to align it _on the right_ with matching tokens + - however, if there was no space before the token, that extra space will be + added to the next space on its line, aligning subsequent token _on the left_. Align has several nested fields, which you can customize. However, it comes with four possible presets: none, some, more, & most. diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatWriter.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatWriter.scala index 5a96edab65..4e1f775621 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatWriter.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatWriter.scala @@ -31,6 +31,7 @@ class FormatWriter(formatOps: FormatOps) { val locations = getFormatLocations(state) styleMap.init.runner.event(FormatEvent.Written(locations)) + var delayedAlign = 0 locations.foreach { entry => val location = entry.curr implicit val style: ScalafmtConfig = location.style @@ -94,7 +95,7 @@ class FormatWriter(formatOps: FormatOps) { } else if (location.missingBracesOpenOrTuck) sb.append(" {") - if (!skipWs) entry.formatWhitespace + if (!skipWs) delayedAlign = entry.formatWhitespace(delayedAlign) } sb.toString() @@ -425,13 +426,12 @@ class FormatWriter(formatOps: FormatOps) { val tokenAligns: Map[Int, Int] = alignmentTokens def foreach(f: Entry => Unit): Unit = { - val iterator = Iterator.range(0, locations.length).map(new Entry(_)) - iterator.filter(_.curr.isNotRemoved).foreach(f) + Iterator.range(0, locations.length).foreach { i => + val entry = new Entry(i) + if (entry.curr.isNotRemoved) f(entry) + } } - private def getAlign(tok: FormatToken, alignOffset: Int = 0): Int = - tokenAligns.get(tok.meta.idx).fold(0)(_ + alignOffset) - class Entry(val i: Int) { val curr = locations(i) private implicit val style: ScalafmtConfig = curr.style @@ -440,17 +440,13 @@ class FormatWriter(formatOps: FormatOps) { @inline def tok = curr.formatToken @inline def state = curr.state @inline def prevState = curr.state.prev - @inline def lastModification = prevState.split.modExt.mod - - def getWhitespace(alignOffset: Int): String = { - state.split.modExt.mod match { - case Space => - val previousAlign = - if (lastModification != NoSplit) 0 - else getAlign(previous.formatToken) - val currentAlign = getAlign(tok, alignOffset) - getIndentation(1 + currentAlign + previousAlign) + private def appendWhitespace(alignOffset: Int, delayedAlign: Int)(implicit + sb: StringBuilder + ): Int = { + val mod = state.split.modExt.mod + def currentAlign = tokenAligns.get(i).fold(0)(_ + alignOffset) + val ws = mod match { case nl: NewlineT => val extraBlanks = if (i == locations.length - 1) 0 @@ -461,11 +457,18 @@ class FormatWriter(formatOps: FormatOps) { case p: Provided => p.betweenText - case NoSplit => "" + case NoSplit => + return delayedAlign + currentAlign // RETURNING! + + case _ => getIndentation(mod.length + currentAlign + delayedAlign) } + sb.append(ws) + 0 } - def formatWhitespace(implicit sb: StringBuilder): Unit = { + def formatWhitespace(delayedAlign: Int)(implicit + sb: StringBuilder + ): Int = { import org.scalafmt.config.TrailingCommas @@ -505,7 +508,7 @@ class FormatWriter(formatOps: FormatOps) { iter(curr, false) } - @inline def ws(offset: Int): Unit = sb.append(getWhitespace(offset)) + @inline def ws(offset: Int) = appendWhitespace(offset, delayedAlign) val noExtraOffset = !dialect.allowTrailingCommas || @@ -521,7 +524,9 @@ class FormatWriter(formatOps: FormatOps) { if tok.left.is[T.Comma] && isClosedDelimWithNewline(false) => sb.setLength(sb.length - 1) if (!tok.right.is[T.RightParen]) ws(1) - else if (style.spaces.inParentheses) sb.append(' ') + else if (style.spaces.inParentheses) { + sb.append(getIndentation(1 + delayedAlign)); 0 + } else delayedAlign // append comma if newline case TrailingCommas.always if !tok.left.is[T.Comma] && isClosedDelimWithNewline(true) => @@ -699,7 +704,7 @@ class FormatWriter(formatOps: FormatOps) { extends FormatCommentBase(style.maxColumn) { def format(): Unit = { val trimmed = removeTrailingWhiteSpace(text) - val isCommentedOut = lastModification match { + val isCommentedOut = prevState.split.modExt.mod match { case m: NewlineT if m.noIndent => true case _ => indent == 0 } diff --git a/scalafmt-tests/src/test/resources/align/DefaultWithAlign.stat b/scalafmt-tests/src/test/resources/align/DefaultWithAlign.stat index 267af7a741..bfa7c16da4 100644 --- a/scalafmt-tests/src/test/resources/align/DefaultWithAlign.stat +++ b/scalafmt-tests/src/test/resources/align/DefaultWithAlign.stat @@ -2067,7 +2067,7 @@ object a { >>> object a { def meeethod1(pram1: AnyRef): Any = ??? - def methd2(paaaaaram2: Any): Any = ??? - def meth3(param333333: Any): Any = ??? - def md4(param4: Any): Any = ??? + def methd2(paaaaaram2: Any): Any = ??? + def meth3(param333333: Any): Any = ??? + def md4(param4: Any): Any = ??? } diff --git a/scalafmt-tests/src/test/scala/org/scalafmt/util/HasTests.scala b/scalafmt-tests/src/test/scala/org/scalafmt/util/HasTests.scala index dcf148bebc..581572cd08 100644 --- a/scalafmt-tests/src/test/scala/org/scalafmt/util/HasTests.scala +++ b/scalafmt-tests/src/test/scala/org/scalafmt/util/HasTests.scala @@ -201,8 +201,11 @@ trait HasTests extends FormatAssertions { val builder = mutable.ArrayBuilder.make[FormatOutput] debug.locations.foreach { entry => val token = entry.curr.formatToken + implicit val sb = new StringBuilder() + sb.append(token.left.syntax) + entry.formatWhitespace(0) builder += FormatOutput( - token.left.syntax + entry.getWhitespace(0), + sb.result(), Option(debug.formatTokenExplored).fold(-1)(_(token.meta.idx)) ) }