From 71a2f860eec42afa15b204e28b2817f3c79160ab Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Fri, 6 Dec 2024 13:59:05 +0100 Subject: [PATCH] improvement: thresholds for folding regions - don't merge region with parent when if parent too small without it - allow to configure folding region threshold using server properties - make threshold work consistently --- .../internal/metals/MetalsLspService.scala | 1 + .../internal/metals/MetalsServerConfig.scala | 6 ++++ .../parsing/FoldingRangeExtractor.scala | 27 ++++++++++-------- .../parsing/FoldingRangeProvider.scala | 24 ++++++---------- .../parsing/JavaFoldingRangeExtractor.scala | 2 +- .../expect/Main.java | 9 +++--- .../expect/i6938.scala | 28 +++++++++++++++++++ .../input/Main.java | 1 + .../input/i6938.scala | 28 +++++++++++++++++++ .../expect/BlockWithNoBraces.scala | 4 +-- .../foldingRange/expect/Blocks.scala | 8 +++--- .../foldingRange/expect/ForLoop.scala | 6 ++-- .../foldingRange/expect/ForYield.scala | 4 +-- .../foldingRange/expect/Literals.scala | 4 +-- .../resources/foldingRange/expect/Main.java | 9 +++--- .../expect/PartialFunctionCases.scala | 4 +-- .../resources/foldingRange/expect/Try.scala | 4 +-- .../resources/foldingRange/input/Main.java | 1 + .../test/scala/tests/FoldingRangeSuite.scala | 2 +- 19 files changed, 118 insertions(+), 54 deletions(-) create mode 100644 tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/i6938.scala create mode 100644 tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/input/i6938.scala diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index 2c2502fdabe..a3c615f4443 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -255,6 +255,7 @@ abstract class MetalsLspService( trees, buffers, foldOnlyLines = initializeParams.foldOnlyLines, + clientConfig.initialConfig.foldingRageMinimumSpan, ) protected val diagnostics: Diagnostics = new Diagnostics( diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala index ad8b2e2396a..483a0dbc78f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala @@ -121,6 +121,11 @@ final case class MetalsServerConfig( "metals.enable-best-effort", default = false, ), + foldingRageMinimumSpan: Int = + Option(System.getProperty("metals.folding-range-minimum-span")) + .filter(_.forall(Character.isDigit(_))) + .map(_.toInt) + .getOrElse(3), ) { override def toString: String = List[String]( @@ -144,6 +149,7 @@ final case class MetalsServerConfig( s"worksheet-timeout=$worksheetTimeout", s"debug-server-start-timeout=$debugServerStartTimeout", s"enable-best-effort=$enableBestEffort", + s"folding-range-minimum-span=$foldingRageMinimumSpan", ).mkString("MetalsServerConfig(\n ", ",\n ", "\n)") } object MetalsServerConfig { diff --git a/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeExtractor.scala b/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeExtractor.scala index f393754c6d1..5434ab4bd35 100644 --- a/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeExtractor.scala +++ b/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeExtractor.scala @@ -18,10 +18,8 @@ import org.eclipse.{lsp4j => l} final class FoldingRangeExtractor( distance: TokenEditDistance, foldOnlyLines: Boolean, + spanThreshold: Int, ) { - private val spanThreshold = 2 - private val distanceToEnclosingThreshold = 3 - private val ranges = new FoldingRanges(foldOnlyLines) def extract(tree: Tree): util.List[FoldingRange] = { @@ -31,19 +29,19 @@ final class FoldingRangeExtractor( } def extractFrom(tree: Tree, enclosing: Position): Unit = { - // All Defn statements except one-liners must fold - if (span(tree.pos) > spanThreshold) { + if (span(tree.pos) > 0) { val newEnclosing = tree match { + case _ if tree.pos.startLine == enclosing.startLine => + enclosing case Foldable((pos, adjust)) - if tree.is[Defn] || span(enclosing) - span( - pos - ) > distanceToEnclosingThreshold => + if pos.startLine != enclosing.startLine && spanAboveThreshold( + pos, + adjust, + ) => distance.toRevised(pos.toLsp) match { case Some(revisedPos) => - if (pos.startLine != enclosing.startLine) { - val range = createRange(revisedPos) - ranges.add(Region, range, adjust) - } + val range = createRange(revisedPos) + ranges.add(Region, range, adjust) pos case None => enclosing } @@ -63,6 +61,11 @@ final class FoldingRangeExtractor( } } + private def spanAboveThreshold(pos: Position, adjust: Boolean) = { + val adjustValue = if (adjust && foldOnlyLines) 1 else 0 + span(pos) - adjustValue >= spanThreshold + } + private def extractImports(trees: List[Tree]) = { val importGroups = mutable.ListBuffer(mutable.ListBuffer.empty[Import]) val nonImport = mutable.ListBuffer[Tree]() diff --git a/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeProvider.scala b/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeProvider.scala index 762e11cb607..0bc15abdd63 100644 --- a/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/parsing/FoldingRangeProvider.scala @@ -4,7 +4,6 @@ import java.util import java.util.Collections import scala.meta.inputs.Input -import scala.meta.inputs.Position import scala.meta.internal.metals.Buffers import scala.meta.internal.metals.MetalsEnrichments._ import scala.meta.internal.parsing.FoldingRangeProvider._ @@ -16,6 +15,7 @@ final class FoldingRangeProvider( val trees: Trees, buffers: Buffers, foldOnlyLines: Boolean, + foldingRageMinimumSpan: Int, ) { def getRangedForScala( @@ -32,7 +32,11 @@ final class FoldingRangeProvider( val distance = TokenEditDistance(fromTree, revised, trees).getOrElse( TokenEditDistance.NoMatch ) - val extractor = new FoldingRangeExtractor(distance, foldOnlyLines) + val extractor = new FoldingRangeExtractor( + distance, + foldOnlyLines, + foldingRageMinimumSpan, + ) extractor.extract(tree) } result.getOrElse(util.Collections.emptyList()) @@ -45,7 +49,9 @@ final class FoldingRangeProvider( code <- buffers.get(filePath) if filePath.isJava } yield { - JavaFoldingRangeExtractor.extract(code, filePath, foldOnlyLines).asJava + JavaFoldingRangeExtractor + .extract(code, filePath, foldOnlyLines, foldingRageMinimumSpan) + .asJava } result.getOrElse(util.Collections.emptyList()) @@ -61,11 +67,6 @@ final class FoldingRanges(foldOnlyLines: Boolean) { def get: util.List[FoldingRange] = Collections.unmodifiableList(allRanges) - def add(kind: String, pos: Position): Unit = { - val range = createRange(pos) - add(kind, range) - } - def add(kind: String, range: FoldingRange): Unit = { range.setKind(kind) add(range, adjust = true) @@ -93,13 +94,6 @@ final class FoldingRanges(foldOnlyLines: Boolean) { } } - private def createRange(pos: Position): FoldingRange = { - val range = new FoldingRange(pos.startLine, pos.endLine) - range.setStartCharacter(pos.startColumn) - range.setEndCharacter(pos.endColumn) - range - } - // examples of collapsed: "class A {}" or "def foo = {}" private def isNotCollapsed(range: FoldingRange): Boolean = spansMultipleLines(range) || foldsMoreThanThreshold(range) diff --git a/metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala b/metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala index b4d8ee7c722..40f67a929d8 100644 --- a/metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala +++ b/metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala @@ -20,7 +20,6 @@ import org.eclipse.lsp4j.FoldingRange import org.eclipse.lsp4j.FoldingRangeKind final object JavaFoldingRangeExtractor { - private val spanThreshold = 2 private case class Range( startPos: Long, @@ -165,6 +164,7 @@ final object JavaFoldingRangeExtractor { text: String, path: AbsolutePath, foldOnlyLines: Boolean, + spanThreshold: Int, ): List[FoldingRange] = { getTrees(text, path) match { case Some((trees, root)) => diff --git a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/Main.java b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/Main.java index 1f4e63b643f..a6ac62d7c6a 100644 --- a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/Main.java +++ b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/Main.java @@ -23,6 +23,7 @@ public class Main >>region>>{ abstract class A >>region>>{ abstract void hello(); + abstract void hello2(); }<>region>>{ @@ -41,13 +42,13 @@ String hello(String str) >>region>>{ return "asssd"; }<>region>>{ + void openingCommentInStr() { System.out.println("/* ignore"); - }<>region>>{ + void closingCommentInStr() { System.out.println("*/ ignore"); - }<>region>>{ if (true) >>region>>{ diff --git a/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/i6938.scala b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/i6938.scala new file mode 100644 index 00000000000..10e81e853bf --- /dev/null +++ b/tests/unit/src/test/resources/foldingRange-scala3-foldLineOnly/expect/i6938.scala @@ -0,0 +1,28 @@ +def xd =>>region>> + val t = + 1 + 2 + + t match + case 3 => println("ok") + case _ => println("ko")<>region>> + val t = 1 + 2 + + t match>>region>> + case 3 => + println("ok") + case _ => println("ko")<>region>> + val t = + 1 + 2 + + t match>>region>> + case 3 => + println("ok") + case _ => println("ko")< println("ok") + case _ => println("ko") +end xd + + +def xd2 = + val t = 1 + 2 + + t match + case 3 => + println("ok") + case _ => println("ko") +end xd2 + +def xd3 = + val t = + 1 + 2 + + t match + case 3 => + println("ok") + case _ => println("ko") +end xd3 diff --git a/tests/unit/src/test/resources/foldingRange-scala3/expect/BlockWithNoBraces.scala b/tests/unit/src/test/resources/foldingRange-scala3/expect/BlockWithNoBraces.scala index 7a84a442e80..015e986f5e9 100644 --- a/tests/unit/src/test/resources/foldingRange-scala3/expect/BlockWithNoBraces.scala +++ b/tests/unit/src/test/resources/foldingRange-scala3/expect/BlockWithNoBraces.scala @@ -43,7 +43,7 @@ def fooNested(): Unit =>>region>> def fooWithMatch(): Unit =>>region>> def bar(): Unit =>>region>> - ??? match + ??? match>>region>> case 1 =>>>region>> ??? ??? @@ -56,7 +56,7 @@ def fooWithMatch(): Unit =>>region>> case 6 => ???<>region>>{ def chain =>>region>> Seq(1).map{ x => - x + 1 - + 1 + >>region>>x + 1 + 1 + 1 + 1 + 1 + 1 + + 1<>region>>{ - _ + 1 - + 1 + >>region>>_ + 1 + 1 + 1 + 1 + 1 + + 1<>region>> Seq(1).map( diff --git a/tests/unit/src/test/resources/foldingRange/expect/ForLoop.scala b/tests/unit/src/test/resources/foldingRange/expect/ForLoop.scala index 5cf4a0a8e68..f7b9deaa5d2 100644 --- a/tests/unit/src/test/resources/foldingRange/expect/ForLoop.scala +++ b/tests/unit/src/test/resources/foldingRange/expect/ForLoop.scala @@ -7,14 +7,14 @@ class A >>region>>{ }<>region>> - for>>region>>{ + for{ x <- ??? - }<>region>>{ ??? ??? ??? ??? - }<>region>> for diff --git a/tests/unit/src/test/resources/foldingRange/expect/ForYield.scala b/tests/unit/src/test/resources/foldingRange/expect/ForYield.scala index 48babdbe1e0..c44a8fa0184 100644 --- a/tests/unit/src/test/resources/foldingRange/expect/ForYield.scala +++ b/tests/unit/src/test/resources/foldingRange/expect/ForYield.scala @@ -9,11 +9,11 @@ class A >>region>>{ def noSpacing =>>region>> for{ x <- ??? - }yield{ + }yield>>region>>{ ??? ??? ??? - }<>region>> for diff --git a/tests/unit/src/test/resources/foldingRange/expect/Literals.scala b/tests/unit/src/test/resources/foldingRange/expect/Literals.scala index 64ffbc8e4fa..cabe725d101 100644 --- a/tests/unit/src/test/resources/foldingRange/expect/Literals.scala +++ b/tests/unit/src/test/resources/foldingRange/expect/Literals.scala @@ -1,13 +1,13 @@ class A>>region>>{ val multilineString =>>region>> - """ + """>>region>> | | | | | | - """.stripMargin<>region>>{ abstract class A >>region>>{ abstract void hello(); + abstract void aaa(); }<>region>>{ @@ -41,13 +42,13 @@ String hello(String str) >>region>>{ return "asssd"; }<>region>>{ + void openingCommentInStr() { System.out.println("/* ignore"); - }<>region>>{ + void closingCommentInStr() { System.out.println("*/ ignore"); - }<>region>>{ if (true) >>region>>{ diff --git a/tests/unit/src/test/resources/foldingRange/expect/PartialFunctionCases.scala b/tests/unit/src/test/resources/foldingRange/expect/PartialFunctionCases.scala index 5e2ce10dded..3c25266a536 100644 --- a/tests/unit/src/test/resources/foldingRange/expect/PartialFunctionCases.scala +++ b/tests/unit/src/test/resources/foldingRange/expect/PartialFunctionCases.scala @@ -1,7 +1,7 @@ class A >>region>>{ val tryCatch =>>region>> try ??? - catch { + catch >>region>>{ case 0 => case 1 => println() case 2 =>>>region>> @@ -12,7 +12,7 @@ class A >>region>>{ case 3 => println() case _ => println() - }<>region>> ??? match { case 0 => diff --git a/tests/unit/src/test/resources/foldingRange/expect/Try.scala b/tests/unit/src/test/resources/foldingRange/expect/Try.scala index 6da985ff66a..7607f038a23 100644 --- a/tests/unit/src/test/resources/foldingRange/expect/Try.scala +++ b/tests/unit/src/test/resources/foldingRange/expect/Try.scala @@ -1,7 +1,7 @@ class A>>region>>{ val shortExpr =>>region>> try ??? - catch { + catch >>region>>{ case 0 => case 1 => println() case 2 =>>>region>> @@ -12,7 +12,7 @@ class A>>region>>{ case 3 => println() case _ => println() - }<>region>> try >>region>>{ diff --git a/tests/unit/src/test/resources/foldingRange/input/Main.java b/tests/unit/src/test/resources/foldingRange/input/Main.java index ea802b386e7..d2cedc92595 100644 --- a/tests/unit/src/test/resources/foldingRange/input/Main.java +++ b/tests/unit/src/test/resources/foldingRange/input/Main.java @@ -23,6 +23,7 @@ public class Main { abstract class A { abstract void hello(); + abstract void aaa(); } abstract class B extends A { diff --git a/tests/unit/src/test/scala/tests/FoldingRangeSuite.scala b/tests/unit/src/test/scala/tests/FoldingRangeSuite.scala index 01eb9d84407..12b755e26dd 100644 --- a/tests/unit/src/test/scala/tests/FoldingRangeSuite.scala +++ b/tests/unit/src/test/scala/tests/FoldingRangeSuite.scala @@ -19,7 +19,7 @@ abstract class FoldingRangeSuite( ) extends DirectoryExpectSuite(s"$directory/expect") { private val (buffers, trees) = TreeUtils.getTrees(scalaVersion) private val foldingRangeProvider = - new FoldingRangeProvider(trees, buffers, lineFoldingOnly) + new FoldingRangeProvider(trees, buffers, lineFoldingOnly, 3) override def testCases(): List[ExpectTestCase] = { val inputDirectory = AbsolutePath(testResourceDirectory)