Remove artificial `CURSOR` added to code in the completions #20899

rochala · 2024-06-30T22:19:37Z

This PR aims to remove the need for artificial CURSOR identifier which was appended after current cursor position:

Lis@@

Would result in code:

Lis@@CURSOR

This lead to super inefficient IDE experience in terms of number of compilations done after each change, but let's be a bit more specific. Let's imagine that we have 2 scenarios in which IDE events arrive to metals:

Scenario A: Completion (CURSOR compilation) -> Inlay Hints (No CURSOR compilation)
Scenario B: Semantic Highlight (No CURSOR compilation) -> Completion (CURSOR compilation) -> Inlay Hints (No CURSOR compilation)

On top of that, we've implemented a compilation caching, where code snippet and compiler configuration is a key. Now you should notice the issue, that adding a CURSOR into a code has different compilation result with cache invalidations.

In theory, we could handle CURSOR compilation as normal ones, but in reality it is a completely different run with different result (for example in diagnostics, as each one will contain CURSOR in the message). This is a no-go, especially if we would want to have diagnostics coming from presentation compiler in the future.

Because of that, each keypress results in at least 2 compilation and in the worst case scenario in 3. This also make metals way more battery heavy.

This PR is an attempt to drop CURSOR insertion for most cases.

A bit of history, how we ended up with CURSOR in a first place.
Most of the reasons are caused by parser and its recovery.
For example, finding a proper scope in this snippet:

def outer: Int =
  def inner: Int =
    val innerVal = 1
  @@ // completion triggered here

We have to find the correct scope in which we are (inner vs outer). We can achieve this in multiple ways, for example, count indents. This solution may not be so straightforward, as there can be different indentations etc. Inserting a synthetic CURSOR into this place:

def outer: Int =
  def inner: Int =
    val innerVal = 1
  @@CURSOR // completion triggered here

Will actually parse into an identifier with scope provided to us by Scala compiler. This is way easier and will always be correct.

Second example are keywords, let's say we have the following snippet:

var value = 0
val newValue = 1
value = new@@

This code will expect a type, as the parser found a new keyword. Adding a CURSOR here resolves the problem, as now we're dealing with newCURSOR, not new keyword (identifier vs keyword).

This PR is basically a change, which disables adding a CURSOR in all cases but 2 mentioned above. Those cases are very, very, very rare and is something that we can deal with. With this change, each compilation will now be cached and reused as intended resulting in way longer battery life, performance, response times and will enable us to access diagnostics for free without risking recompilation.

TODO:

- remove caching for snippets with CURSOR,
- add tests to verify it.

I'd also love to have this backported to LTS, as it is a significant performance tweak and will allow me to add diagnostics on the fly for the Scastie.

[test_windows_full]

kasiaMarek · 2024-07-08T11:43:45Z

compiler/src/dotty/tools/dotc/ast/NavigateAST.scala

+ 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


Suggested change

val bestSpan= currentBest.head.span

val bestSpan = currentBest.head.span

kasiaMarek · 2024-07-08T12:08:54Z

presentation-compiler/src/main/dotty/tools/pc/AutoImportsProvider.scala

@@ -67,7 +66,7 @@ final class AutoImportsProvider(
 val results = symbols.result.filter(isExactMatch(_, name))

 if results.nonEmpty then
- val correctedPos = CompletionPos.infer(pos, params, path).toSourcePosition
+ val correctedPos = CompletionPos.infer(pos, params, path, false).toSourcePosition


Suggested change

val correctedPos = CompletionPos.infer(pos, params, path, false).toSourcePosition

val correctedPos = CompletionPos.infer(pos, params, path, withCURSOR = false).toSourcePosition

presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionSuite.scala

kasiaMarek · 2024-07-08T12:13:30Z

presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionSuite.scala

- |until(end: Long): Exclusive[Long]
- |until(end: Long, step: Long): Exclusive[Long]


Why did this change?

This change probably was caused by fact, that ImplicitSearch now worked with a fully typed trees (with CURSOR it was an error), and this was the match. I'm curious tho what would be the output for 1.unt@@

kasiaMarek · 2024-07-08T12:14:35Z

presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionArgSuite.scala

@@ -614,8 +614,9 @@ class CompletionArgSuite extends BaseCompletionSuite:
 check(
 s"""|case class Context()
 |
- |def foo(arg1: (Context) ?=> Int, arg2: Int): String = ???
- |val m = foo(ar@@)
+ |object Main:


Why do you wrap those in an object?

We're reusing the presentation compiler, and we still have a problem with mutating context (I may have a fix for this in the future). We should always wrap entries into something, as this will override all previous state.

kasiaMarek · 2024-07-08T13:05:41Z

presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala

+ * 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


The cursor should be applied rarely, does it make sense to treat it specially here?

We have to treat it specially. Running a compilation mutates the context / changes diagnostics. There is no way to use a Driver without mutating a context. This means that to not change and reuse previous compilation, we have to create a new Driver

We may try saving a context and replacing it, but I'm not whether this is the best idea. As far as I know, this approach was tried when @tanishiking was adding compilation caching in a very first place. I also think that this is unnecessary overengineering.

kasiaMarek · 2024-07-08T13:08:01Z

presentation-compiler/src/main/dotty/tools/pc/completions/CompletionProvider.scala

+
+ val tpdPath = tpdPath0 match
+ case Select(qual, name) :: tail
+ // If for any reason we end up in param after lifting, we want to inline the synthetic val


Can you add an example here?

kasiaMarek · 2024-07-08T13:11:19Z

presentation-compiler/test/dotty/tools/pc/tests/CompilerCachingSuite.scala

+ private def checkCompilationCount(params: VirtualFileParams, expected: Int): Unit =
+ presentationCompiler match
+ case pc: ScalaPresentationCompiler =>
+ val compilations= pc.compilerAccess.withNonInterruptableCompiler(Some(params))(-1, EmptyCancelToken) { driver =>


Suggested change

val compilations= pc.compilerAccess.withNonInterruptableCompiler(Some(params))(-1, EmptyCancelToken) { driver =>

val compilations = pc.compilerAccess.withNonInterruptableCompiler(Some(params))(-1, EmptyCancelToken) { driver =>

kasiaMarek · 2024-07-08T13:12:51Z

presentation-compiler/test/dotty/tools/pc/tests/CompilerCachingSuite.scala

+ def beforeEach: Unit =
+ presentationCompiler.restart()
+
+ // We want to run art least one compilation, so runId points at 3.


Suggested change

// We want to run art least one compilation, so runId points at 3.

// We want to run at least one compilation, so runId points at 3.

kasiaMarek · 2024-07-08T13:16:48Z

presentation-compiler/test/dotty/tools/pc/tests/CompilerCachingSuite.scala

+ private def checkCompilationCount(params: VirtualFileParams, expected: Int): Unit =
+ presentationCompiler match
+ case pc: ScalaPresentationCompiler =>
+ val compilations= pc.compilerAccess.withNonInterruptableCompiler(Some(params))(-1, EmptyCancelToken) { driver =>


params in withNonInterruptableCompiler are just for reporting, it should be a bit cleaner if you don't pass it

tgodzik

Looks good! I think this would be a great improvement. I don't have anything to add aside from what @kasiaMarek already commented

rochala added 2 commits July 4, 2024 21:11

Remove artificial CURSOR added for the completions

562b66d

Add tests, don't cache when CURSOR is added

cddbc28

rochala force-pushed the cursor-removal branch from 941bc76 to cddbc28 Compare July 4, 2024 19:19

kasiaMarek reviewed Jul 8, 2024

View reviewed changes

tgodzik reviewed Jul 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove artificial `CURSOR` added to code in the completions #20899

Remove artificial `CURSOR` added to code in the completions #20899

rochala commented Jun 30, 2024 •

edited

Loading

kasiaMarek Jul 8, 2024

kasiaMarek Jul 8, 2024

kasiaMarek Jul 8, 2024

rochala Jul 8, 2024

kasiaMarek Jul 8, 2024

rochala Jul 8, 2024

kasiaMarek Jul 8, 2024 •

edited

Loading

rochala Jul 8, 2024 •

edited

Loading

rochala Jul 8, 2024

kasiaMarek Jul 8, 2024

kasiaMarek Jul 8, 2024

kasiaMarek Jul 8, 2024

kasiaMarek Jul 8, 2024

tgodzik left a comment

	val bestSpan= currentBest.head.span
	val bestSpan = currentBest.head.span

	val correctedPos = CompletionPos.infer(pos, params, path, false).toSourcePosition
	val correctedPos = CompletionPos.infer(pos, params, path, withCURSOR = false).toSourcePosition

		\|until(end: Long): Exclusive[Long]
		\|until(end: Long, step: Long): Exclusive[Long]

	val compilations= pc.compilerAccess.withNonInterruptableCompiler(Some(params))(-1, EmptyCancelToken) { driver =>
	val compilations = pc.compilerAccess.withNonInterruptableCompiler(Some(params))(-1, EmptyCancelToken) { driver =>

	// We want to run art least one compilation, so runId points at 3.
	// We want to run at least one compilation, so runId points at 3.

Remove artificial CURSOR added to code in the completions #20899

Are you sure you want to change the base?

Remove artificial CURSOR added to code in the completions #20899

Conversation

rochala commented Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiaMarek Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

rochala Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Remove artificial `CURSOR` added to code in the completions #20899

Remove artificial `CURSOR` added to code in the completions #20899

rochala commented Jun 30, 2024 •

edited

Loading

kasiaMarek Jul 8, 2024 •

edited

Loading

rochala Jul 8, 2024 •

edited

Loading