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.
scalameta · Oct 28, 2024 · 6a7e59b · 6a7e59b
1 parent adbe24a
commit 6a7e59b
Show file tree

Hide file tree

Showing 13 changed files with 50 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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/...intellij/src/test/scala/org/scalafmt/community/intellij/CommunityIntellijScalaSuite.scala b/...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/...-community/scala2/src/test/scala/org/scalafmt/community/scala2/CommunityScala2Suite.scala b/...-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/...-community/scala3/src/test/scala/org/scalafmt/community/scala3/CommunityScala3Suite.scala b/...-community/scala3/src/test/scala/org/scalafmt/community/scala3/CommunityScala3Suite.scala
@@ -9,15 +9,15 @@ 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))
 
 }
 
 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/...sts-community/spark/src/test/scala/org/scalafmt/community/spark/CommunitySparkSuite.scala b/...sts-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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 :'(