Add tests, don't cache when CURSOR is added

scala · Jul 4, 2024 · cddbc28 · cddbc28
1 parent 562b66d
commit cddbc28
Show file tree

Hide file tree

Showing 27 changed files with 256 additions and 94 deletions.
diff --git a/compiler/src/dotty/tools/dotc/ast/NavigateAST.scala b/compiler/src/dotty/tools/dotc/ast/NavigateAST.scala
@@ -75,24 +75,41 @@ object NavigateAST {
   def pathTo(span: Span, from: List[Positioned], skipZeroExtent: Boolean = false)(using Context): List[Positioned] = {
     def childPath(it: Iterator[Any], path: List[Positioned]): List[Positioned] = {
       var bestFit: List[Positioned] = path
-      while (it.hasNext) {
-        val path1 = it.next() match {
-          // FIXME this has to be changed to deterministicaly find recoveed tree
-          case untpd.Select(qual, name) if name == StdNames.nme.??? => path
+      while (it.hasNext) do
+        val path1 = it.next() match
+          case sel: untpd.Select if isTreeFromRecovery(sel) => path
           case p: Positioned if !p.isInstanceOf[Closure[?]] => singlePath(p, path)
           case m: untpd.Modifiers => childPath(m.productIterator, path)
           case xs: List[?] => childPath(xs.iterator, path)
           case _ => path
-        }
-        if ((path1 ne path) &&
-            ((bestFit eq path) ||
-             bestFit.head.span != path1.head.span &&
-             envelops(bestFit.head.span, path1.head.span)))
+
+        if (path1 ne path) && ((bestFit eq path) || isBetterFit(bestFit, path1)) then
           bestFit = path1
-      }
+
       bestFit
     }
 
+    /**
+      * When choosing better fit we compare spans. If candidate span has starting or ending point inside (exclusive)
+      * current best fit it is selected as new best fit. This means that same spans are failing the first predicate.
+      *
+      * In case when spans start and end at same offsets we prefer non synthethic one.
+      */
+    def isBetterFit(currentBest: List[Positioned], candidate: List[Positioned]): Boolean =
+      if currentBest.isEmpty && candidate.nonEmpty then true
+      else if currentBest.nonEmpty && candidate.nonEmpty then
+        val bestSpan= currentBest.head.span
+        val candidateSpan = candidate.head.span
+
+        bestSpan != candidateSpan &&
+          envelops(bestSpan, candidateSpan) ||
+          bestSpan.contains(candidateSpan) && bestSpan.isSynthetic && !candidateSpan.isSynthetic
+      else false
+
+
+    def isTreeFromRecovery(p: untpd.Select): Boolean =
+      p.name == StdNames.nme.??? && p.qualifier.symbol.name == StdNames.nme.Predef && p.span.isSynthetic
+
     def envelops(a: Span, b: Span): Boolean =
       !b.exists || a.exists && (
         (a.start < b.start && a.end >= b.end ) || (a.start <= b.start && a.end > b.end)

diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala
@@ -402,7 +402,7 @@ object Parsers {
       false
     }
 
-    def errorTermTree(start: Offset): Tree = atSpan(start, in.offset, in.offset) { unimplementedExpr }
+    def errorTermTree(start: Offset): Tree = atSpan(Span(start, in.offset)) { unimplementedExpr }
 
     private var inFunReturnType = false
     private def fromWithinReturnType[T](body: => T): T = {

diff --git a/language-server/test/dotty/tools/languageserver/CompletionTest.scala b/language-server/test/dotty/tools/languageserver/CompletionTest.scala
@@ -1705,15 +1705,6 @@ class CompletionTest {
        ("getOrElse", Method, "[V1 >: String](key: Int, default: => V1): V1"),
      ))
 
-  @Test def testtest: Unit =
-    code"""|object M {
-           |  def sel$m1
-           |}
-           |"""
-     .completion(m1, Set(
-       ("getOrElse", Method, "[V1 >: String](key: Int, default: => V1): V1"),
-     ))
-
   @Test def noEnumCompletionInNewContext: Unit =
     code"""|enum TestEnum:
            |  case TestCase

diff --git a/language-server/test/dotty/tools/languageserver/HoverTest.scala b/language-server/test/dotty/tools/languageserver/HoverTest.scala
@@ -227,7 +227,7 @@ class HoverTest {
   @Test def enums: Unit = {
     code"""|package example
            |enum TestEnum3:
-           | case ${m1}A${m2} // no tooltip
+           |  case ${m1}A${m2} // no tooltip
            |
            |"""
       .hover(m1 to m2, hoverContent("example.TestEnum3"))

diff --git a/presentation-compiler/src/main/dotty/tools/pc/AutoImportsProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/AutoImportsProvider.scala
@@ -13,7 +13,6 @@ import dotty.tools.dotc.core.Symbols.*
 import dotty.tools.dotc.interactive.Interactive
 import dotty.tools.dotc.interactive.InteractiveDriver
 import dotty.tools.dotc.util.SourceFile
-import dotty.tools.pc.AutoImports.*
 import dotty.tools.pc.completions.CompletionPos
 import dotty.tools.pc.utils.InteractiveEnrichments.*
 

diff --git a/...rc/main/dotty/tools/pc/MetalsDriver.scala → ...c/main/dotty/tools/pc/CachingDriver.scala b/...rc/main/dotty/tools/pc/MetalsDriver.scala → ...c/main/dotty/tools/pc/CachingDriver.scala
@@ -10,8 +10,8 @@ import dotty.tools.dotc.util.SourceFile
 import scala.compiletime.uninitialized
 
 /**
- * MetalsDriver is a wrapper class that provides a compilation cache for InteractiveDriver.
- * MetalsDriver skips running compilation if
+ * CachingDriver is a wrapper class that provides a compilation cache for InteractiveDriver.
+ * CachingDriver skips running compilation if
  * - the target URI of `run` is the same as the previous target URI
  * - the content didn't change since the last compilation.
  *
@@ -27,9 +27,7 @@ import scala.compiletime.uninitialized
  * To avoid the complexity related to currentCtx,
  * we decided to cache only when the target URI only if the same as the previous run.
  */
-class MetalsDriver(
-    override val settings: List[String]
-) extends InteractiveDriver(settings):
+class CachingDriver(override val settings: List[String]) extends InteractiveDriver(settings):
 
   @volatile private var lastCompiledURI: URI = uninitialized
 
@@ -55,4 +53,4 @@ class MetalsDriver(
     lastCompiledURI = uri
     diags
 
-end MetalsDriver
+end CachingDriver
diff --git a/presentation-compiler/src/main/dotty/tools/pc/PcInlayHintsProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/PcInlayHintsProvider.scala
@@ -14,7 +14,6 @@ import scala.meta.internal.pc.LabelPart.*
 import scala.meta.pc.InlayHintsParams
 import scala.meta.pc.SymbolSearch
 
-import dotty.tools.dotc.ast.tpd
 import dotty.tools.dotc.ast.tpd.*
 import dotty.tools.dotc.core.Contexts.Context
 import dotty.tools.dotc.core.Flags

diff --git a/presentation-compiler/src/main/dotty/tools/pc/Scala3CompilerAccess.scala b/presentation-compiler/src/main/dotty/tools/pc/Scala3CompilerAccess.scala
@@ -8,13 +8,14 @@ import scala.meta.internal.pc.CompilerAccess
 import scala.meta.pc.PresentationCompilerConfig
 
 import dotty.tools.dotc.reporting.StoreReporter
+import dotty.tools.dotc.interactive.InteractiveDriver
 
 class Scala3CompilerAccess(
     config: PresentationCompilerConfig,
     sh: Option[ScheduledExecutorService],
     newCompiler: () => Scala3CompilerWrapper
 )(using ec: ExecutionContextExecutor, rc: ReportContext)
-    extends CompilerAccess[StoreReporter, MetalsDriver](
+    extends CompilerAccess[StoreReporter, InteractiveDriver](
       config,
       sh,
       newCompiler,

diff --git a/presentation-compiler/src/main/dotty/tools/pc/Scala3CompilerWrapper.scala b/presentation-compiler/src/main/dotty/tools/pc/Scala3CompilerWrapper.scala
@@ -4,11 +4,12 @@ import scala.meta.internal.pc.CompilerWrapper
 import scala.meta.internal.pc.ReporterAccess
 
 import dotty.tools.dotc.reporting.StoreReporter
+import dotty.tools.dotc.interactive.InteractiveDriver
 
-class Scala3CompilerWrapper(driver: MetalsDriver)
-    extends CompilerWrapper[StoreReporter, MetalsDriver]:
+class Scala3CompilerWrapper(driver: InteractiveDriver)
+    extends CompilerWrapper[StoreReporter, InteractiveDriver]:
 
-  override def compiler(): MetalsDriver = driver
+  override def compiler(): InteractiveDriver = driver
 
   override def resetReporter(): Unit =
     val ctx = driver.currentCtx

diff --git a/presentation-compiler/src/main/dotty/tools/pc/ScalaPresentationCompiler.scala b/presentation-compiler/src/main/dotty/tools/pc/ScalaPresentationCompiler.scala
@@ -32,11 +32,13 @@ import dotty.tools.dotc.reporting.StoreReporter
 import dotty.tools.pc.completions.CompletionProvider
 import dotty.tools.pc.completions.OverrideCompletions
 import dotty.tools.pc.buildinfo.BuildInfo
+import dotty.tools.pc.SymbolInformationProvider
+import dotty.tools.dotc.interactive.InteractiveDriver
 
 import org.eclipse.lsp4j.DocumentHighlight
 import org.eclipse.lsp4j.TextEdit
 import org.eclipse.lsp4j as l
-import dotty.tools.pc.SymbolInformationProvider
+
 
 case class ScalaPresentationCompiler(
     buildTargetIdentifier: String = "",
@@ -69,14 +71,20 @@ case class ScalaPresentationCompiler(
   override def withReportsLoggerLevel(level: String): PresentationCompiler =
     copy(reportsLevel = ReportLevel.fromString(level))
 
-  val compilerAccess: CompilerAccess[StoreReporter, MetalsDriver] =
+  val compilerAccess: CompilerAccess[StoreReporter, InteractiveDriver] =
     Scala3CompilerAccess(
       config,
       sh,
-      () => new Scala3CompilerWrapper(newDriver)
-    )(using
-      ec
-    )
+      () => new Scala3CompilerWrapper(CachingDriver(driverSettings))
+    )(using ec)
+
+  val driverSettings =
+    val implicitSuggestionTimeout = List("-Ximport-suggestion-timeout", "0")
+    val defaultFlags = List("-color:never")
+    val filteredOptions = removeDoubleOptions(options.filterNot(forbiddenOptions))
+
+    filteredOptions ::: defaultFlags ::: implicitSuggestionTimeout ::: "-classpath" :: classpath
+      .mkString(File.pathSeparator) :: Nil
 
   private def removeDoubleOptions(options: List[String]): List[String] =
     options match
@@ -85,19 +93,6 @@ case class ScalaPresentationCompiler(
       case head :: tail => head :: removeDoubleOptions(tail)
       case Nil => options
 
-  def newDriver: MetalsDriver =
-    val implicitSuggestionTimeout = List("-Ximport-suggestion-timeout", "0")
-    val defaultFlags = List("-color:never")
-    val filteredOptions = removeDoubleOptions(
-      options.filterNot(forbiddenOptions)
-    )
-    val settings =
-      filteredOptions ::: defaultFlags ::: implicitSuggestionTimeout ::: "-classpath" :: classpath
-        .mkString(
-          File.pathSeparator
-        ) :: Nil
-    new MetalsDriver(settings)
-
   override def semanticTokens(
       params: VirtualFileParams
   ): CompletableFuture[ju.List[Node]] =
@@ -139,6 +134,7 @@ case class ScalaPresentationCompiler(
       new CompletionProvider(
         search,
         driver,
+        () => InteractiveDriver(driverSettings),
         params,
         config,
         buildTargetIdentifier,

diff --git a/presentation-compiler/src/main/dotty/tools/pc/ScriptFirstImportPosition.scala b/presentation-compiler/src/main/dotty/tools/pc/ScriptFirstImportPosition.scala
@@ -1,6 +1,5 @@
 package dotty.tools.pc
 
-import dotty.tools.dotc.ast.tpd.*
 import dotty.tools.dotc.core.Comments.Comment
 
 object ScriptFirstImportPosition:

diff --git a/presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala
@@ -1,6 +1,5 @@
 package dotty.tools.pc
 
-import dotty.tools.dotc.ast.tpd.*
 import dotty.tools.dotc.core.Contexts.*
 import dotty.tools.dotc.core.Flags
 import dotty.tools.dotc.core.Symbols.*

diff --git a/presentation-compiler/src/main/dotty/tools/pc/SymbolInformationProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/SymbolInformationProvider.scala
@@ -6,8 +6,6 @@ import scala.meta.pc.PcSymbolKind
 import scala.meta.pc.PcSymbolProperty
 
 import dotty.tools.dotc.core.Contexts.Context
-import dotty.tools.dotc.core.Denotations.Denotation
-import dotty.tools.dotc.core.Denotations.MultiDenotation
 import dotty.tools.dotc.core.Flags
 import dotty.tools.dotc.core.Names.*
 import dotty.tools.dotc.core.StdNames.nme
@@ -18,6 +16,7 @@ import dotty.tools.pc.utils.InteractiveEnrichments.allSymbols
 import dotty.tools.pc.utils.InteractiveEnrichments.stripBackticks
 import scala.meta.internal.pc.PcSymbolInformation
 import scala.meta.internal.pc.SymbolInfo
+import dotty.tools.dotc.core.Denotations.{Denotation, MultiDenotation}
 
 class SymbolInformationProvider(using Context):
 

diff --git a/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala
@@ -16,6 +16,7 @@ import dotty.tools.dotc.core.Contexts.Context
 import dotty.tools.dotc.core.Phases
 import dotty.tools.dotc.core.StdNames.nme
 import dotty.tools.dotc.core.Flags
+import dotty.tools.dotc.core.Names.DerivedName
 import dotty.tools.dotc.interactive.Interactive
 import dotty.tools.dotc.interactive.Completion
 import dotty.tools.dotc.interactive.InteractiveDriver
@@ -38,7 +39,8 @@ import org.eclipse.lsp4j.TextEdit
 
 class CompletionProvider(
     search: SymbolSearch,
-    driver: InteractiveDriver,
+    cachingDriver: InteractiveDriver,
+    freshDriver: () => InteractiveDriver,
     params: OffsetParams,
     config: PresentationCompilerConfig,
     buildTargetIdentifier: String,
@@ -50,6 +52,16 @@ class CompletionProvider(
 
     val (wasCursorApplied, code) = applyCompletionCursor(params)
     val sourceFile = SourceFile.virtual(uri, code)
+
+    /** Creating a new fresh driver is way slower than reusing existing one,
+     *  but runnig a compilation has side effects that modifies the state of the driver.
+     *  We don't want to affect cachingDriver state with compilation including "CURSOR" suffix.
+     *
+     *  We could in theory save this fresh driver for reuse, but it is a choice between extra memory usage and speed.
+     *  The scenario in which "CURSOR" is applied (empty query or query equal to any keyword) has a slim chance of happening.
+     */
+
+    val driver = if wasCursorApplied then freshDriver() else cachingDriver
     driver.run(uri, sourceFile)
 
     given ctx: Context = driver.currentCtx
@@ -61,11 +73,12 @@ class CompletionProvider(
         val adjustedPath = Interactive.resolveTypedOrUntypedPath(tpdPath0, pos)(using newctx)
 
         val tpdPath = tpdPath0 match
-          // $1$ // FIXME add check for a $1$ name to make sure we only do the below in lifting case
-          case Select(qual, name) :: tail if qual.symbol.is(Flags.Synthetic) =>
-            qual.symbol.defTree match
-              case valdef: ValDef => Select(valdef.rhs, name) :: tail
-              case _ => tpdPath0
+          case Select(qual, name) :: tail
+            // If for any reason we end up in param after lifting, we want to inline the synthetic val
+            if qual.symbol.is(Flags.Synthetic) && qual.symbol.name.isInstanceOf[DerivedName] =>
+              qual.symbol.defTree match
+                case valdef: ValDef => Select(valdef.rhs, name) :: tail
+                case _ => tpdPath0
           case _ => tpdPath0
 
 

diff --git a/presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala b/presentation-compiler/src/main/dotty/tools/pc/completions/Completions.scala
@@ -5,7 +5,6 @@ import java.nio.file.Path
 import java.nio.file.Paths
 
 import scala.collection.mutable
-import scala.meta.internal.metals.Fuzzy
 import scala.meta.internal.metals.ReportContext
 import scala.meta.internal.mtags.CoursierComplete
 import scala.meta.internal.pc.{IdentifierComparator, MemberOrdering, CompletionFuzzy}
@@ -27,15 +26,12 @@ import dotty.tools.dotc.core.Symbols.*
 import dotty.tools.dotc.core.Types.*
 import dotty.tools.dotc.interactive.Completion
 import dotty.tools.dotc.interactive.Completion.Mode
-import dotty.tools.dotc.interactive.Interactive
 import dotty.tools.dotc.util.SourcePosition
 import dotty.tools.dotc.util.SrcPos
 import dotty.tools.pc.AutoImports.AutoImportsGenerator
 import dotty.tools.pc.buildinfo.BuildInfo
 import dotty.tools.pc.completions.OverrideCompletions.OverrideExtractor
 import dotty.tools.pc.utils.InteractiveEnrichments.*
-import dotty.tools.dotc.core.Denotations.SingleDenotation
-import dotty.tools.dotc.interactive.Interactive
 
 class Completions(
     text: String,
@@ -271,7 +267,6 @@ class Completions(
         val affix = if methodDenot.symbol.isConstructor && existsApply then
           adjustedPath match
             case (select @ Select(qual, _)) :: _ =>
-              val start = qual.span.start
               val insertRange = select.sourcePos.startPos.withEnd(completionPos.queryEnd).toLsp
 
               suffix
@@ -651,7 +646,7 @@ class Completions(
       .collect { case symbolic: CompletionValue.Symbolic => symbolic }
       .groupBy(_.symbol.fullName) // we somehow have to ignore proxy type
 
-    val filteredSymbolicCompletions = symbolicCompletionsMap.filter: (name, denots) =>
+    val filteredSymbolicCompletions = symbolicCompletionsMap.filter: (name, _) =>
       lazy val existsTypeWithoutSuffix: Boolean = !symbolicCompletionsMap
         .get(name.toTypeName)
         .forall(_.forall(sym => sym.snippetAffix.suffixes.nonEmpty))

diff --git a/presentation-compiler/src/main/dotty/tools/pc/completions/NamedArgCompletions.scala b/presentation-compiler/src/main/dotty/tools/pc/completions/NamedArgCompletions.scala
@@ -27,7 +27,6 @@ import dotty.tools.dotc.core.Types.TermRef
 import dotty.tools.dotc.core.Types.Type
 import dotty.tools.dotc.core.Types.TypeBounds
 import dotty.tools.dotc.core.Types.WildcardType
-import dotty.tools.dotc.util.SourcePosition
 import dotty.tools.pc.IndexedContext
 import dotty.tools.pc.utils.InteractiveEnrichments.*
 import scala.annotation.tailrec
@@ -295,7 +294,6 @@ object NamedArgCompletions:
       )
     }
 
-    // FIXME pass query here
     val prefix = ident
       .map(_.name.toString)
       .getOrElse("")

diff --git a/presentation-compiler/src/main/dotty/tools/pc/printer/ShortenedTypePrinter.scala b/presentation-compiler/src/main/dotty/tools/pc/printer/ShortenedTypePrinter.scala
@@ -24,8 +24,6 @@ import dotty.tools.dotc.printing.RefinedPrinter
 import dotty.tools.dotc.printing.Texts.Text
 import dotty.tools.pc.AutoImports.AutoImportsGenerator
 import dotty.tools.pc.AutoImports.ImportSel
-import dotty.tools.pc.AutoImports.ImportSel.Direct
-import dotty.tools.pc.AutoImports.ImportSel.Rename
 import dotty.tools.pc.IndexedContext
 import dotty.tools.pc.IndexedContext.Result
 import dotty.tools.pc.Params

diff --git a/presentation-compiler/test/dotty/tools/pc/base/BaseInlayHintsSuite.scala b/presentation-compiler/test/dotty/tools/pc/base/BaseInlayHintsSuite.scala
@@ -8,9 +8,7 @@ import scala.meta.internal.metals.CompilerRangeParams
 import scala.language.unsafeNulls
 
 import dotty.tools.pc.utils.TestInlayHints
-import dotty.tools.pc.utils.TextEdits
 
-import org.eclipse.lsp4j.TextEdit
 
 class BaseInlayHintsSuite extends BasePCSuite {
 
@@ -55,4 +53,4 @@ class BaseInlayHintsSuite extends BasePCSuite {
       obtained,
     )
 
-}
+}