Scala 3 type completion #4174

vzmerr · 2022-07-20T14:19:07Z

it partially resolves #3954
it partially resolves #3956

filtering out invalid trait, object, class, import, or apply method completions according to position for scala 3.
adding square bracket suffix where relevant for type, new, and instantiation completions, for scala 3.
adding curly brace suffix in new completions of trait, abstract, java interface types for scala 3.
applying filtering without enriching with symbols on the interpolate completions

Not covered in the PR:

member completion for Scala 3 as in:

object A {
  new scala.Iterable@@
}

Covering it requires covering the case for CompletionKind.Members in enrichWithSymbolSearch and filtering out the non-members, in a similar way to the postProcess method in CompletionApplication.

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

vzmerr · 2022-07-26T07:44:56Z

help needed on detecting the existing type parameter in the following code:
def foo ArrayBuffer@@[Int] = ???
I have tried printing different members of the symbol, including its paramSymss; as well as printing the Tree path. But I could not get to detecting the existing type parameter.

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

tgodzik

Thanks for working on this! I left a lot of comments, but almost all of them are minor. I think the PR improves things a lot.

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

tests/cross/src/test/scala/tests/pc/CompletionSnippetSuite.scala

tests/cross/src/test/scala/tests/pc/CompletionSuite.scala

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

tanishiking · 2022-07-27T03:20:30Z

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

+ end match
+ else ""
+
+ val bracketSuffix =


I think it's still needed. Currently we never put multiple suffixes with $0 at the same time.

However, it's extremely difficult for me to understand there won't be multiple $0 because each of them is not explicitly separated by if/else if/else. We (Metals developers in the future) have to read all of the code around here to check all those conditions won't be all true in any cases, to prevent Metals from putting multiple braces. That leads to the code being error-prone.

tanishiking · 2022-07-28T04:40:51Z

Is this ready for review? Please let me know when it's ready by re-request review :)

tanishiking

I left some comments, but most of them are about refactoring.

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

tanishiking · 2022-07-28T16:28:49Z

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

+ && (symbol.toString.startsWith(
+ "trait"
+ ) // trait A{ def doSomething: Int}


Suggested change

&& (symbol.toString.startsWith(

"trait"

) // trait A{ def doSomething: Int}

&& (symbol.is(Trait)) // trait A{ def doSomething: Int}

can you explain which test will fail? I think it shouldn't be flaky (if it is, it's a compiler bug and we should report it)

(sorry for duplicated comment, but I want to discuss this in another thread)

oh, I would mention the test case. Generally the flags are flaky. I have it on my todo list to put the issue on dotty and add the link in here. As an example, checking the is(Trait) flag on some cases was retuning false, while calling it on .info.typeSymbol. was returning true. We discussed it with Vadim on Metals team Slack, but certainly it has to get formally documented somewhere.

this was the type5 test in CompletionSnippetSuite that used to fail before but now passes. So I reverted to using flags 👍

Update: the reason the demo was failing was the use of Flags.Trait instead of toString.startsWith("trait ")
Now that I reverted to using toString, the demo works correctly. I am now scratching my head.

I was thinking about that. But it is actually coming from scala.collection.mutable

I wonder how I can add a test that fails with Flags.Trait and why type5 is now passing regardless.

It turned out that it was Intellisense that was messing up with things.

I created this Issue in Dotty to explain the problem scala/scala3#15790

Thank you for taking a look!
Let's go with the current implementation, and we can refactor later 👍

tanishiking · 2022-07-28T16:30:34Z

I'm still wondering why we can't use Trait flag https://github.com/scalameta/metals/pull/4174/files#r932450969, and I left some comments about refactoring but otherwise it looks good to me, this is great improvement! ❤️

tgodzik

LGTM from my side! Just one comment.

tests/cross/src/test/scala/tests/pc/CompletionSnippetSuite.scala

kpodsiad

I wonder, can code from Completions.scala be extracted to object TypeCompletions.scala?

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

vzmerr · 2022-07-29T16:33:54Z

Update

There seems to be a Dotty bug which causes the requiredPackage in toSymbols method of CompilerSearchVisitor to drop java from java.io and turn it into just object io
But types such as java.io.StringBufferInputStream must be specified with their package, or they won't be recognised by the compiler.
I am not familiar with the exact purpose of the toSymbols method though. Help would be appreciated.

This bug is causing the StringBufferInputStream to be suggested as a completion to StringBuffe@@ without any clue for the user about the package it can be found in.

Also, technically speaking, I do not understand the exact purpose of the toSymbols method there. We are feeding it java.io which is the correct package. and also StringBufferInputStream. Then the method just messes it up and erases the package information in producing the symbol. What's the purpose of that?

I can fix it in this PR or I can put it in a followup?

@dos65 Hey Vadim, can you please a take a look here?

Update

Oh, sorry, adding a test in CompletionSuite showed that the package information was not removed from the symbol.

Currently, due to this exception, the completions also crash and do not show up properly in demo.

tanishiking

Looks like unit tests are failing, but it's LGTM:+1: thank you so much for working on this!

Also, technically speaking, I do not understand the exact purpose of the toSymbols method there. We are feeding it java.io which is the correct package. and also StringBufferInputStream. Then the method just messes it up and erases the package information in producing the symbol. What's the purpose of that?
I can fix it in this PR or I can put it in a followup?

Thank you for finding the issue! You can create an issue and work on that in a follow-up 👍

For what it's worth, in my opinion, if I were you, I will make a PR as small as possible and try to do one thing in one PR.
For example

You're doing 4 things in this PR (as you explained in the PR description)
- Instead, you might be able to create a PR for each thing.
- (Of course, you can do more than one thing if it's comfortable for you, or if the changes are an atomic changes).
- If you do multiple things in one PR
  - PR will be bigger, if PR is bigger, it takes more time to merge the PR in general 😭
  - it makes it harder for reviews to review the diff because reviews can't tell which parts of diff are for which purpose.
    - If you want to do more than one thing, it would be good if you can explicitly separate the commits, Add better Scala CLI support #3790 is a good example. This PR is doing two things, but he did those things in a different commit. Therefore we could (relatively) easily review the change (even though it's gigantic).
toSymbols issue is somehow related to this change, but is it necessary to fix in this PR?
- if it's trivial, we can fix it immediately, but if it may take some efforts (or requires some investigations), I would recommend you to do it in a separate follow-up PR 👍
- I mentioned something around Flags.Trait, but I didn't mean "you have to fix the problem right now, otherwise you can't merge it" (sorry if you felt pressured 🙇). You can open an issue (as you did 👍 ) and follow-up it later :)

vzmerr · 2022-08-01T08:50:11Z

Looks like unit tests are failing, but it's LGTM👍 thank you so much for working on this!

Also, technically speaking, I do not understand the exact purpose of the toSymbols method there. We are feeding it java.io which is the correct package. and also StringBufferInputStream. Then the method just messes it up and erases the package information in producing the symbol. What's the purpose of that?
I can fix it in this PR or I can put it in a followup?

Thank you for finding the issue! You can create an issue and work on that in a follow-up 👍

For what it's worth, in my opinion, if I were you, I will make a PR as small as possible and try to do one thing in one PR. For example

You're doing 4 things in this PR (as you explained in the PR description)

Instead, you might be able to create a PR for each thing.

(Of course, you can do more than one thing if it's comfortable for you, or if the changes are an atomic changes).

If you do multiple things in one PR

PR will be bigger, if PR is bigger, it takes more time to merge the PR in general 😭

it makes it harder for reviews to review the diff because reviews can't tell which parts of diff are for which purpose.

If you want to do more than one thing, it would be good if you can explicitly separate the commits, Add better Scala CLI support #3790 is a good example. This PR is doing two things, but he did those things in a different commit. Therefore we could (relatively) easily review the change (even though it's gigantic).

toSymbols issue is somehow related to this change, but is it necessary to fix in this PR?

if it's trivial, we can fix it immediately, but if it may take some efforts (or requires some investigations), I would recommend you to do it in a separate follow-up PR 👍

I mentioned something around Flags.Trait, but I didn't mean "you have to fix the problem right now, otherwise you can't merge it" (sorry if you felt pressured 🙇). You can open an issue (as you did 👍 ) and follow-up it later :)

Thank you so much for taking the time to write this detailed feedback 👍
I would just be fixing the tests and pushing.

…ompletions according to position for scala 3. adding square bracket suffix where relevant for type, new, and instantiation completions, for scala 3. adding curly brace suffix in new completions of trait, abstract, java interface types for scala 3.

adjust CompletionInterpolatorSuite and CompletionWorkspaceSuite

…ests.

Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

…letions results

…asNoSquareBracket calculation

Co-authored-by: Kamil Podsiadło <37124721+kpodsiad@users.noreply.github.com>

correct detection of traits

This reverts commit 36b68ec.

* Revert "Add more test cases for package completions and fix issues" This reverts commit c328f09. * Revert "bugfix: Show package completions" This reverts commit 417eebc. * Revert "Scala 3 type completion (#4174)" This reverts commit 36b68ec.

vzmerr marked this pull request as draft July 20, 2022 14:19

tgodzik reviewed Jul 20, 2022

View reviewed changes

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala Outdated Show resolved Hide resolved

vzmerr force-pushed the scala-3-type-completion branch 2 times, most recently from a104745 to 8afd9b9 Compare July 21, 2022 17:13

tgodzik reviewed Jul 26, 2022

View reviewed changes

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala Outdated Show resolved Hide resolved

vzmerr force-pushed the scala-3-type-completion branch 3 times, most recently from 0dc2199 to bf7da78 Compare July 26, 2022 11:58

vzmerr marked this pull request as ready for review July 26, 2022 11:59

vzmerr requested review from tgodzik, dos65 and tanishiking July 26, 2022 11:59

vzmerr force-pushed the scala-3-type-completion branch 2 times, most recently from f762df5 to 2804d6b Compare July 26, 2022 13:33

tgodzik requested changes Jul 26, 2022

View reviewed changes

tanishiking requested changes Jul 27, 2022

View reviewed changes

vzmerr mentioned this pull request Jul 27, 2022

Trees after typer losing information scala/scala3#15750

Closed

vzmerr requested review from tgodzik and tanishiking July 28, 2022 10:43

tanishiking requested changes Jul 28, 2022

View reviewed changes

tanishiking reviewed Jul 28, 2022

View reviewed changes

vzmerr requested a review from tanishiking July 29, 2022 09:26

tgodzik approved these changes Jul 29, 2022

View reviewed changes

tests/cross/src/test/scala/tests/pc/CompletionSnippetSuite.scala Show resolved Hide resolved

tests/cross/src/test/scala/tests/pc/CompletionSnippetSuite.scala Show resolved Hide resolved

kpodsiad reviewed Jul 29, 2022

View reviewed changes

vzmerr force-pushed the scala-3-type-completion branch from d21022b to 1489b5b Compare July 31, 2022 14:27

vzmerr requested a review from kpodsiad July 31, 2022 15:43

vzmerr mentioned this pull request Jul 31, 2022

Incorrect flags for some completion items scala/scala3#15790

Closed

vzmerr force-pushed the scala-3-type-completion branch from b1fb592 to f5abae1 Compare July 31, 2022 19:14

tanishiking approved these changes Aug 1, 2022

View reviewed changes

vzmerr and others added 16 commits August 1, 2022 10:59

preventing duplicate bracket insertion

c78538f

adjust CompletionInterpolatorSuite and CompletionWorkspaceSuite

adjusting the CompletionWorkspaceSuite and CompletionSuite for Scala 2

91a31e0

ignore "3.2.0-RC1" up to "3.2.1" for the failing interpolator suite t…

c6846e9

…ests.

Apply suggestions from code review

d073ff1

Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

renamings from review

a72befb

turn type completion variables into immutable values

82e2651

handling the insertion of cursor position

967f7bd

usage of Flags instead of toString for object and methods

f93fe81

removed unnecessary compat maps for Scala3

87b6970

Applying filtering without enrichWithSymbolSearch on interpolate comp…

198bfa3

…letions results

refactoring the suffix and filtering logic in CursorPositionCondition

3af9166

test empty parameter list with an implicit param for braces insertion

12a127a

linking the dotty issue of trees after typer losing information for h…

81d989e

…asNoSquareBracket calculation

Apply suggestions from code review

82482b1

Co-authored-by: Kamil Podsiadło <37124721+kpodsiad@users.noreply.github.com>

excluding abstract methods for {} insertion

5de87b2

correct detection of traits

vzmerr force-pushed the scala-3-type-completion branch from 0516b47 to 913aa09 Compare August 1, 2022 09:17

allowing method traits in method call pos

d0bc467

vzmerr force-pushed the scala-3-type-completion branch from 913aa09 to d0bc467 Compare August 1, 2022 09:42

tanishiking merged commit 36b68ec into scalameta:main Aug 1, 2022

tanishiking mentioned this pull request Aug 9, 2022

fix: type completions additonal fixes #4234

Merged

tanishiking added a commit to tanishiking/metals that referenced this pull request Aug 9, 2022

Revert "Scala 3 type completion (scalameta#4174)"

33f4e60

This reverts commit 36b68ec.

tanishiking mentioned this pull request Aug 9, 2022

[Scala 3] Revert type completions feature #4236

Merged

tanishiking mentioned this pull request Aug 9, 2022

Revert "[Scala 3] Revert type completions feature (#4236)" #4237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala 3 type completion #4174

Scala 3 type completion #4174

vzmerr commented Jul 20, 2022 •

edited

Loading

vzmerr commented Jul 26, 2022

tgodzik left a comment

tanishiking Jul 27, 2022 •

edited

Loading

tanishiking commented Jul 28, 2022

tanishiking left a comment

tanishiking Jul 28, 2022

tanishiking Jul 28, 2022

vzmerr Jul 28, 2022 •

edited

Loading

vzmerr Jul 29, 2022 •

edited

Loading

vzmerr Jul 29, 2022

vzmerr Jul 29, 2022

vzmerr Jul 29, 2022

vzmerr Jul 29, 2022

vzmerr Jul 31, 2022

tanishiking Aug 1, 2022

tanishiking commented Jul 28, 2022

tgodzik left a comment

kpodsiad left a comment •

edited

Loading

vzmerr commented Jul 29, 2022 •

edited

Loading

tanishiking left a comment •

edited

Loading

vzmerr commented Aug 1, 2022

Scala 3 type completion #4174

Scala 3 type completion #4174

Conversation

vzmerr commented Jul 20, 2022 • edited Loading

vzmerr commented Jul 26, 2022

tgodzik left a comment

Choose a reason for hiding this comment

tanishiking Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

tanishiking commented Jul 28, 2022

tanishiking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzmerr Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

vzmerr Jul 29, 2022 • 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

Choose a reason for hiding this comment

tanishiking commented Jul 28, 2022

tgodzik left a comment

Choose a reason for hiding this comment

kpodsiad left a comment • edited Loading

Choose a reason for hiding this comment

vzmerr commented Jul 29, 2022 • edited Loading

tanishiking left a comment • edited Loading

Choose a reason for hiding this comment

vzmerr commented Aug 1, 2022

vzmerr commented Jul 20, 2022 •

edited

Loading

tanishiking Jul 27, 2022 •

edited

Loading

vzmerr Jul 28, 2022 •

edited

Loading

vzmerr Jul 29, 2022 •

edited

Loading

kpodsiad left a comment •

edited

Loading

vzmerr commented Jul 29, 2022 •

edited

Loading

tanishiking left a comment •

edited

Loading