Unify completion pos usage, fix presentation compiler crash in interpolation #19614

rochala · 2024-02-05T09:14:56Z

Fixes crash for added tests in CompletionInterpolatorSuite along with unification and better naming for completion pos, source pos, and span usage.

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

jkciesluk · 2024-02-05T11:00:28Z

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

      offsetParams: OffsetParams,
      adjustedPath: List[Tree]
  )(using Context): CompletionPos =
-    infer(cursorPos, offsetParams.uri().nn, offsetParams.text().nn, adjustedPath)
+    infer(sourcePosition, offsetParams.uri().nn, String(sourcePosition.source.content()), adjustedPath)


Why this change?

sourcePosition contains artificial "CURSOR", offsetparams.text() is plain sourcecode

Actually looks like text is unused and we should remove it

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

kasiaMarek · 2024-02-09T13:58:37Z

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

-    infer(cursorPos, offsetParams.uri().nn, offsetParams.text().nn, adjustedPath)
+    val identEnd = adjustedPath match
+      case (ident: Ident) :: _ if ident.toString.contains(Cursor.value) =>
+        ident.span.end - Cursor.value.length


What about e.g. Select? Wouldn't you have to consider all possible trees for this to work this way?

RefTree.name should handle all the cases

including import, export?

Actually yes. I think that it is impossible for pathTo to return ImportSelector or ExportSelector as head as there are only 2 cases:

we do normal import, and this Identifier has the same span as this ImportSelector

we do a rename, which has another Identifier that will be matched in pathTo if we are inside it.

Unless it is empty ? Maybe a test like this will ensure its correctness.

kasiaMarek · 2024-02-09T14:17:20Z

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

@@ -107,7 +106,7 @@ class Completions(
  end includeSymbol

  def completions(): (List[CompletionValue], SymbolSearch.Result) =
-    val (advanced, exclusive) = advancedCompletions(path, pos, completionPos)
+    val (advanced, exclusive) = advancedCompletions(path, completionPos.originalCursorPosition, completionPos)


Suggested change

val (advanced, exclusive) = advancedCompletions(path, completionPos.originalCursorPosition, completionPos)

val (advanced, exclusive) = advancedCompletions(path, completionPos)

And to avoid too many changes in advancedCompletions just do:

val pos = completionPos.originalCursorPosition

kasiaMarek · 2024-02-13T15:13:12Z

compiler/src/dotty/tools/dotc/interactive/Completion.scala

-        case untpd.Ident(_) :: untpd.Import(_, _) :: _ => Mode.ImportOrExport
-        case untpd.Ident(_) :: (_: untpd.ImportSelector) :: _ => Mode.ImportOrExport
-        case untpd.Literal(Constants.Constant(_: String)) :: _ => Mode.Term // literal completions
+        case untpd.Ident(_) :: (_: untpd.ImportSelector) :: _ => Mode.ImportOrExport // import scala.@@


possibly also should be member?

As far as I know member completions are fetched from symbol search. They are also source of extension methods outside the scope. ain this scenario we can only complete with direct members

kasiaMarek · 2024-02-13T15:21:55Z

compiler/src/dotty/tools/dotc/interactive/Completion.scala

@@ -211,7 +194,6 @@ object Completion:
      case tpd.Select(qual, _) :: _               if qual.typeOpt.hasSimpleKind => completer.selectionCompletions(qual)
      case tpd.Select(qual, _) :: _                                             => Map.empty
      case (tree: tpd.ImportOrExport) :: _                                      => completer.directMemberCompletions(tree.expr)


Is this case needed?

Yes, those are typed Trees, and this is the head of the path. We need it to find package members

kasiaMarek · 2024-02-13T15:23:53Z

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

-         |unshared - scala.annotation.internal
-         |unspecialized - scala.annotation""".stripMargin,
-      topLines = Some(5)
+      "",


why did this change?

these are illegal completions at this position, it expects the name. On the other hand you can add annotations in this position. I'll add tests that ensure that they are present if we start a query with @

rochala · 2024-02-15T17:07:42Z

I also removed 2 files: Custom completions and its Tests. They were added to the compiler for ammonite, but after all they were also never used. They failed the tests, and the reason behind it that the file hasn't been updated for 3 years, and getting it up to date is too much costs / not worth.

jkciesluk

They were added to the compiler for ammonite, but after all they were also never used. They failed the tests, and the reason behind it that the file hasn't been updated for 3 years, and getting it up to date is too much costs / not worth.

We also have AmmoniteIvyCompletions now, which should do the same thing.

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

jkciesluk · 2024-02-19T16:21:47Z

compiler/src/dotty/tools/dotc/interactive/Completion.scala


-        case (sel: untpd.ImportSelector) :: _ =>
-          if sel.imported.span.contains(pos.span) then Mode.ImportOrExport


Why remove this check?

Actually I have to revert it. I was tricked by our tests, as they don't enter completions in this position for some reason. We should probably also add check to not compute any completions when mode is None...

compiler/src/dotty/tools/dotc/interactive/Completion.scala

rochala · 2024-02-19T18:03:42Z

They were added to the compiler for ammonite, but after all they were also never used. They failed the tests, and the reason behind it that the file hasn't been updated for 3 years, and getting it up to date is too much costs / not worth.

We also have AmmoniteIvyCompletions now, which should do the same thing.

This is actually a bit different. They planned to use this in repl completions.

kasiaMarek

lgtm

…olator

…olation (#19614) Fixes crash for added tests in `CompletionInterpolatorSuite` along with unification and better naming for completion pos, source pos, and span usage. [Cherry-picked 6b52789]

… in interpolation" to LTS (#20964) Backports #19614 to the LTS branch. PR submitted by the release tooling. [skip ci]

rochala requested review from jkciesluk and kasiaMarek February 5, 2024 10:04

jkciesluk reviewed Feb 5, 2024

View reviewed changes

kasiaMarek reviewed Feb 9, 2024

View reviewed changes

rochala force-pushed the unify-completion-pos branch from 79325bf to 86bf70d Compare February 13, 2024 13:40

kasiaMarek reviewed Feb 13, 2024

View reviewed changes

jkciesluk reviewed Feb 19, 2024

View reviewed changes

kasiaMarek approved these changes Feb 21, 2024

View reviewed changes

jkciesluk approved these changes Feb 28, 2024

View reviewed changes

rochala added 9 commits February 28, 2024 14:15

Unify completion pos usage, fix presentation compiler crash in interp…

7d1d325

…olator

Remove leftovers

12e7d4a

Remove leftovers

2b009c5

Apply review suggestions

a6be0cf

apply review comments, remove unnecessary param

ebe397c

Remove cases which are impossible to match

f899864

Remove unnecessary cases, fix CI, remove never used files

9f630b7

Revert changes to completion mode for renamed imports

d9c6efb

Add completions for given types

27acfd9

rochala force-pushed the unify-completion-pos branch from 90e950f to 27acfd9 Compare February 28, 2024 13:17

rochala merged commit 6b52789 into scala:main Feb 29, 2024
19 checks passed

rochala mentioned this pull request Feb 29, 2024

Fix CI to make up for latest changes in dotty scalameta/metals#6181

Merged

Kordyjan added this to the 3.4.2 milestone Mar 28, 2024

tgodzik added the area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools label May 8, 2024

WojciechMazur mentioned this pull request Jul 2, 2024

Backport "Unify completion pos usage, fix presentation compiler crash in interpolation" to LTS #20964

Merged

WojciechMazur added a commit that referenced this pull request Jul 3, 2024

Backport "Unify completion pos usage, fix presentation compiler crash…

a4e3a85

… in interpolation" to LTS (#20964) Backports #19614 to the LTS branch. PR submitted by the release tooling. [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify completion pos usage, fix presentation compiler crash in interpolation #19614

Unify completion pos usage, fix presentation compiler crash in interpolation #19614

rochala commented Feb 5, 2024

jkciesluk Feb 5, 2024

rochala Feb 5, 2024

jkciesluk Feb 5, 2024

kasiaMarek Feb 9, 2024

rochala Feb 10, 2024

kasiaMarek Feb 12, 2024

rochala Feb 12, 2024

rochala Feb 12, 2024

kasiaMarek Feb 9, 2024

kasiaMarek Feb 13, 2024

rochala Feb 15, 2024

kasiaMarek Feb 13, 2024

rochala Feb 15, 2024

kasiaMarek Feb 13, 2024

rochala Feb 15, 2024

rochala commented Feb 15, 2024

jkciesluk left a comment

jkciesluk Feb 19, 2024

rochala Feb 19, 2024

rochala commented Feb 19, 2024

kasiaMarek left a comment

	val (advanced, exclusive) = advancedCompletions(path, completionPos.originalCursorPosition, completionPos)
	val (advanced, exclusive) = advancedCompletions(path, completionPos)


		case (sel: untpd.ImportSelector) :: _ =>
		if sel.imported.span.contains(pos.span) then Mode.ImportOrExport

Unify completion pos usage, fix presentation compiler crash in interpolation #19614

Unify completion pos usage, fix presentation compiler crash in interpolation #19614

Conversation

rochala commented Feb 5, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochala commented Feb 15, 2024

jkciesluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochala commented Feb 19, 2024

kasiaMarek left a comment

Choose a reason for hiding this comment