Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove artificial CURSOR added to code in the completions #20899

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Jun 30, 2024

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]

compiler/src/dotty/tools/dotc/ast/NavigateAST.scala Outdated Show resolved Hide resolved
Comment on lines -532 to -534
|until(end: Long): Exclusive[Long]
|until(end: Long, step: Long): Exclusive[Long]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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@@

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this completion is run on 1.unt, it shows old results too. This is a limitation / bug of current completion members coming from extension methods.

The underlying issue is a fact that we're fetching all matching implicits instead of only the best ones that will be used in that exact context.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rochala rochala requested a review from kasiaMarek August 28, 2024 15:10
@rochala rochala marked this pull request as ready for review August 28, 2024 15:11
@rochala
Copy link
Contributor Author

rochala commented Aug 28, 2024

IMPORTANT: When backporting make sure that #21438 is backported before this PR.

@rochala rochala requested a review from tgodzik September 2, 2024 10:21
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tgodzik tgodzik merged commit f774497 into scala:main Sep 2, 2024
28 checks passed
@WojciechMazur WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024
WojciechMazur added a commit that referenced this pull request Dec 4, 2024
…" to LTS (#22114)

Backports #20899 to the 3.3.5.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants