From 6a7e59b0657639811fc59f6d5bcdc63cb4937469 Mon Sep 17 00:00:00 2001 From: Albert Meltzer <7529386+kitbellew@users.noreply.github.com> Date: Mon, 28 Oct 2024 09:31:33 -0700 Subject: [PATCH] FormatTokens: fix token span computation Previous computation didn't include the right token, however, it was in fact applied to closed ranges. In most cases, it would have been applied to a closed delimiter and thus simply off by 1; however, with optional braces, the last token might be a lot more substantial and span multiple lines. Also, correct the implementation of `minSpan` check, since distance has now been adjusted anyway. --- docs/configuration.md | 11 +++++++---- .../org/scalafmt/internal/BestFirstSearch.scala | 2 +- .../scala/org/scalafmt/internal/FormatOps.scala | 2 +- .../scala/org/scalafmt/internal/FormatTokens.scala | 14 ++++++++------ .../intellij/CommunityIntellijScalaSuite.scala | 4 ++-- .../community/scala2/CommunityScala2Suite.scala | 4 ++-- .../community/scala3/CommunityScala3Suite.scala | 4 ++-- .../community/spark/CommunitySparkSuite.scala | 4 ++-- .../test/resources/newlines/source_classic.stat | 5 ++++- .../src/test/resources/newlines/source_fold.stat | 11 ++++++++--- .../src/test/resources/newlines/source_keep.stat | 5 ++++- .../src/test/resources/newlines/source_unfold.stat | 10 ++++++++-- .../src/test/scala/org/scalafmt/FormatTests.scala | 2 +- 13 files changed, 50 insertions(+), 28 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 0331c91ac2..b0eb677fbc 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -5671,11 +5671,14 @@ runner.optimizer.defnSite This optimization is enabled when all these criteria are satisfied: - `xxxSite.minSpan`: - must be non-negative, and the character distance between the matching - parentheses, excluding any whitespace, must exceed this value - (prior to v3.8.1, this parameter was called `forceConfigStyleOnOffset`) + must be non-negative and not exceed the character span covered by the entire + clause, excluding any whitespace + - prior to v3.8.1, this parameter was called `forceConfigStyleOnOffset` + - prior to v3.8.4, the parameter had to be strictly less than the span, and + the span calculation excluded the last token of the clause (a closing delim + spanning a single-character, unless optional braces had been used) - `xxxSite.minCount`: - must be positive and may not exceed the number of arguments + must be positive and not exceed the number of arguments (prior to v3.8.2, this parameter was called `forceConfigStyleMinCount`) > These parameters cannot be [overridden within a file](#for-code-block) diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala index ea495c49f7..c13a4403db 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/BestFirstSearch.scala @@ -41,7 +41,7 @@ private class BestFirstSearch private (range: Set[Range])(implicit ): Option[Token] = getEndOfBlock(ft, parensToo = true).collect { case close if close.left != stop && { // Block must span at least 3 lines to be worth recursing. - tokens.distance(ft, close) > style.maxColumn * 3 + tokens.span(ft, close) > style.maxColumn * 3 } => close.left } 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 index c115a13ea7..b2e67b36b9 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala @@ -1044,7 +1044,7 @@ class FormatOps( val values = clause.values if ( values.lengthCompare(cfg.minCount) >= 0 && - (cfg.minSpan == 0 || cfg.minSpan < distance(ftOpen, close)) + (cfg.minSpan == 0 || cfg.minSpan <= span(ftOpen, close)) ) { forces += ftOpen.meta.idx values.foreach(x => clearQueues += getHead(x).meta.idx) diff --git a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatTokens.scala b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatTokens.scala index 23fd083f87..18e794f48a 100644 --- a/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatTokens.scala +++ b/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatTokens.scala @@ -404,13 +404,15 @@ class FormatTokens(leftTok2tok: Map[TokenHash, Int])(val arr: Array[FormatToken] result } - def distance(left: FormatToken, right: FormatToken): Int = - nonWhitespaceOffset(right.idx) - nonWhitespaceOffset(left.idx) - def distance(tokens: Tokens): Int = - if (tokens.isEmpty) 0 - else distance(getHeadImpl(tokens), getLastImpl(tokens)) + def span(left: FormatToken, right: FormatToken): Int = { + val lastIdx = nonWhitespaceOffset.length - 1 + val rightIdx = if (right.idx >= lastIdx) lastIdx else right.idx + 1 + nonWhitespaceOffset(rightIdx) - nonWhitespaceOffset(left.idx) + } + def span(tokens: Tokens): Int = + if (tokens.isEmpty) 0 else span(getHeadImpl(tokens), getLastImpl(tokens)) @inline - def distance(tree: Tree): Int = distance(tree.tokens) + def span(tree: Tree): Int = span(tree.tokens) } diff --git a/scalafmt-tests-community/intellij/src/test/scala/org/scalafmt/community/intellij/CommunityIntellijScalaSuite.scala b/scalafmt-tests-community/intellij/src/test/scala/org/scalafmt/community/intellij/CommunityIntellijScalaSuite.scala index 4137ab7832..e96806393a 100644 --- a/scalafmt-tests-community/intellij/src/test/scala/org/scalafmt/community/intellij/CommunityIntellijScalaSuite.scala +++ b/scalafmt-tests-community/intellij/src/test/scala/org/scalafmt/community/intellij/CommunityIntellijScalaSuite.scala @@ -13,7 +13,7 @@ abstract class CommunityIntellijScalaSuite(name: String) class CommunityIntellijScala_2024_2_Suite extends CommunityIntellijScalaSuite("intellij-scala-2024.2") { - override protected def totalStatesVisited: Option[Int] = Some(47842896) + override protected def totalStatesVisited: Option[Int] = Some(47833716) override protected def builds = Seq(getBuild( "2024.2.28", @@ -51,7 +51,7 @@ class CommunityIntellijScala_2024_2_Suite class CommunityIntellijScala_2024_3_Suite extends CommunityIntellijScalaSuite("intellij-scala-2024.3") { - override protected def totalStatesVisited: Option[Int] = Some(48022013) + override protected def totalStatesVisited: Option[Int] = Some(48012302) override protected def builds = Seq(getBuild( "2024.3.4", diff --git a/scalafmt-tests-community/scala2/src/test/scala/org/scalafmt/community/scala2/CommunityScala2Suite.scala b/scalafmt-tests-community/scala2/src/test/scala/org/scalafmt/community/scala2/CommunityScala2Suite.scala index 807fcff06f..984a41918d 100644 --- a/scalafmt-tests-community/scala2/src/test/scala/org/scalafmt/community/scala2/CommunityScala2Suite.scala +++ b/scalafmt-tests-community/scala2/src/test/scala/org/scalafmt/community/scala2/CommunityScala2Suite.scala @@ -9,7 +9,7 @@ abstract class CommunityScala2Suite(name: String) class CommunityScala2_12Suite extends CommunityScala2Suite("scala-2.12") { - override protected def totalStatesVisited: Option[Int] = Some(35262413) + override protected def totalStatesVisited: Option[Int] = Some(35259833) override protected def builds = Seq(getBuild("v2.12.20", dialects.Scala212, 1277)) @@ -18,7 +18,7 @@ class CommunityScala2_12Suite extends CommunityScala2Suite("scala-2.12") { class CommunityScala2_13Suite extends CommunityScala2Suite("scala-2.13") { - override protected def totalStatesVisited: Option[Int] = Some(43936512) + override protected def totalStatesVisited: Option[Int] = Some(43931406) override protected def builds = Seq(getBuild("v2.13.14", dialects.Scala213, 1287)) diff --git a/scalafmt-tests-community/scala3/src/test/scala/org/scalafmt/community/scala3/CommunityScala3Suite.scala b/scalafmt-tests-community/scala3/src/test/scala/org/scalafmt/community/scala3/CommunityScala3Suite.scala index 05d363ce5e..01db494b00 100644 --- a/scalafmt-tests-community/scala3/src/test/scala/org/scalafmt/community/scala3/CommunityScala3Suite.scala +++ b/scalafmt-tests-community/scala3/src/test/scala/org/scalafmt/community/scala3/CommunityScala3Suite.scala @@ -9,7 +9,7 @@ abstract class CommunityScala3Suite(name: String) class CommunityScala3_2Suite extends CommunityScala3Suite("scala-3.2") { - override protected def totalStatesVisited: Option[Int] = Some(32923088) + override protected def totalStatesVisited: Option[Int] = Some(32916614) override protected def builds = Seq(getBuild("3.2.2", dialects.Scala32, 791)) @@ -17,7 +17,7 @@ class CommunityScala3_2Suite extends CommunityScala3Suite("scala-3.2") { class CommunityScala3_3Suite extends CommunityScala3Suite("scala-3.3") { - override protected def totalStatesVisited: Option[Int] = Some(35522867) + override protected def totalStatesVisited: Option[Int] = Some(35515588) override protected def builds = Seq(getBuild("3.3.3", dialects.Scala33, 861)) diff --git a/scalafmt-tests-community/spark/src/test/scala/org/scalafmt/community/spark/CommunitySparkSuite.scala b/scalafmt-tests-community/spark/src/test/scala/org/scalafmt/community/spark/CommunitySparkSuite.scala index 12130d6cd8..2413852a6b 100644 --- a/scalafmt-tests-community/spark/src/test/scala/org/scalafmt/community/spark/CommunitySparkSuite.scala +++ b/scalafmt-tests-community/spark/src/test/scala/org/scalafmt/community/spark/CommunitySparkSuite.scala @@ -9,7 +9,7 @@ abstract class CommunitySparkSuite(name: String) class CommunitySpark3_4Suite extends CommunitySparkSuite("spark-3.4") { - override protected def totalStatesVisited: Option[Int] = Some(71558119) + override protected def totalStatesVisited: Option[Int] = Some(71541585) override protected def builds = Seq(getBuild("v3.4.1", dialects.Scala213, 2585)) @@ -18,7 +18,7 @@ class CommunitySpark3_4Suite extends CommunitySparkSuite("spark-3.4") { class CommunitySpark3_5Suite extends CommunitySparkSuite("spark-3.5") { - override protected def totalStatesVisited: Option[Int] = Some(75699407) + override protected def totalStatesVisited: Option[Int] = Some(75679293) override protected def builds = Seq(getBuild("v3.5.3", dialects.Scala213, 2756)) diff --git a/scalafmt-tests/shared/src/test/resources/newlines/source_classic.stat b/scalafmt-tests/shared/src/test/resources/newlines/source_classic.stat index 771aa40641..e24e4e4a3c 100644 --- a/scalafmt-tests/shared/src/test/resources/newlines/source_classic.stat +++ b/scalafmt-tests/shared/src/test/resources/newlines/source_classic.stat @@ -2094,7 +2094,10 @@ object a { val fields = JField("engineVariant", JString(i.engineVariant)) :: JField("engineFactory", JString(i.engineFactory)) :: JField("batch", JString(i.batch)) :: - JField("env", Extraction.decompose(i.env)(DefaultFormats)) + JField( + "env", + Extraction.decompose(i.env)(DefaultFormats) + ) } <<< 8.10 maxColumn = 80 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 bac4ba656a..7f1e0e1c1b 100644 --- a/scalafmt-tests/shared/src/test/resources/newlines/source_fold.stat +++ b/scalafmt-tests/shared/src/test/resources/newlines/source_fold.stat @@ -2016,8 +2016,10 @@ val fields = JField("engineVariant", JString(i.engineVariant)) :: object a { val fields = JField("engineVariant", JString(i.engineVariant)) :: JField("engineFactory", JString(i.engineFactory)) :: - JField("batch", JString(i.batch)) :: - JField("env", Extraction.decompose(i.env)(DefaultFormats)) + JField("batch", JString(i.batch)) :: JField( + "env", + Extraction.decompose(i.env)(DefaultFormats) + ) } <<< 8.10 maxColumn = 80 @@ -2117,7 +2119,10 @@ val a = b match { JField("engineVariant", JString(i.engineVariant)) :: JField("engineFactory", JString(i.engineFactory)) :: JField("batch", JString(i.batch)) :: - JField("env", Extraction.decompose(i.env)(DefaultFormats)) :: + JField( + "env", + Extraction.decompose(i.env)(DefaultFormats) + ) :: JField( "sparkConf", Extraction.decompose(i.sparkConf)(DefaultFormats) diff --git a/scalafmt-tests/shared/src/test/resources/newlines/source_keep.stat b/scalafmt-tests/shared/src/test/resources/newlines/source_keep.stat index 384d136887..6ce8154bd3 100644 --- a/scalafmt-tests/shared/src/test/resources/newlines/source_keep.stat +++ b/scalafmt-tests/shared/src/test/resources/newlines/source_keep.stat @@ -2114,7 +2114,10 @@ object a { val fields = JField("engineVariant", JString(i.engineVariant)) :: JField("engineFactory", JString(i.engineFactory)) :: JField("batch", JString(i.batch)) :: - JField("env", Extraction.decompose(i.env)(DefaultFormats)) + JField( + "env", + Extraction.decompose(i.env)(DefaultFormats) + ) } <<< 8.10 maxColumn = 80 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 42f36a5a8f..831ed676d8 100644 --- a/scalafmt-tests/shared/src/test/resources/newlines/source_unfold.stat +++ b/scalafmt-tests/shared/src/test/resources/newlines/source_unfold.stat @@ -2283,7 +2283,10 @@ object a { JField("engineVariant", JString(i.engineVariant)) :: JField("engineFactory", JString(i.engineFactory)) :: JField("batch", JString(i.batch)) :: - JField("env", Extraction.decompose(i.env)(DefaultFormats)) + JField( + "env", + Extraction.decompose(i.env)(DefaultFormats) + ) } <<< 8.10 maxColumn = 80 @@ -2390,7 +2393,10 @@ val a = JField("engineVariant", JString(i.engineVariant)) :: JField("engineFactory", JString(i.engineFactory)) :: JField("batch", JString(i.batch1)) :: - JField("env", Extraction.decompose(i.env)(DefaultFormats)) :: + JField( + "env", + Extraction.decompose(i.env)(DefaultFormats) + ) :: JField( "sparkConf", Extraction.decompose(i.sparkConf)(DefaultFormats) 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 2cb1b12b5c..a616ca5240 100644 --- a/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala +++ b/scalafmt-tests/shared/src/test/scala/org/scalafmt/FormatTests.scala @@ -137,7 +137,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, 1488077, "total explored") + assertEquals(explored, 1487968, "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 :'(