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.

Sign up for GitHub

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

Jump to bottom

improvement: show args completions for all matching overloaded methods #5287

Merged
merged 5 commits into from
Aug 1, 2023
Merged

improvement: show args completions for all matching overloaded methods #5287

merged 5 commits into from
Aug 1, 2023

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented May 29, 2023 •
edited
Loading

  1. in Scala 3 added fallback for arg completions for overloaded methods (previously no arg completion would be visible):
  • collects methods w/ matching name
  • uses a heuristic to determine if the provided arguments match the method's signature
  1. in Scala 2:
  • handle OverloadedType
  • in fallback collect args from all matching overloaded methods. We use a heuristic do check if the method matches, it is less accurate than the one used for Scala 3.

@kasiaMarek kasiaMarek force-pushed the args-completions-for-overloaded branch 6 times, most recently from 6702f91 to b097f47 Compare June 2, 2023 15:05
@kasiaMarek kasiaMarek marked this pull request as ready for review June 5, 2023 08:17
jkciesluk
jkciesluk requested changes Jun 9, 2023
View reviewed changes
Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

This is really great! I found a few nitpicks and there is one part that I think might use a comment, but otherwise great job!

mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Show resolved Hide resolved
@kasiaMarek kasiaMarek force-pushed the args-completions-for-overloaded branch from b097f47 to dcf4ba4 Compare June 9, 2023 13:42
@kasiaMarek
Copy link
Contributor Author

I'm open to suggestions on how to fix the OutOfMemoryError that occurs in semanticdb. I've refactored quite substantially the Scala 2 file but it didn't help :(. @tgodzik ?

@tgodzik
Copy link
Contributor

tgodzik commented Jun 15, 2023

I don't see anything obvious, I will try to reproduce!

@kasiaMarek kasiaMarek force-pushed the args-completions-for-overloaded branch from 7a16f1f to f966f86 Compare June 26, 2023 09:55
jkciesluk
jkciesluk approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

LGTM!

mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala
@@ -15,55 +15,72 @@ trait ArgCompletions { this: MetalsGlobal =>
pos: Position,
text: String,
completions: CompletionResult
) extends CompletionPosition {
) extends CompletionPosition { self =>
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan on self type aliases, but I see it's useful here.

@kasiaMarek kasiaMarek force-pushed the args-completions-for-overloaded branch from f966f86 to 50cddda Compare June 27, 2023 14:54
tgodzik
tgodzik requested changes Jul 4, 2023
View reviewed changes
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.

This is great work! All the improvements will be super useful, we can work on more later on

mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala Show resolved Hide resolved
mtags/src/main/scala-2/scala/meta/internal/pc/completions/ArgCompletions.scala Outdated Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/CompletionArgSuite.scala Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/CompletionArgSuite.scala Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/CompletionArgSuite.scala Show resolved Hide resolved
tests/cross/src/test/scala/tests/pc/CompletionArgSuite.scala Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Outdated Show resolved Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/NamedArgCompletions.scala Show resolved Hide resolved
tgodzik
tgodzik reviewed Jul 5, 2023
View reviewed changes
tests/cross/src/test/scala/tests/pc/CompletionArgSuite.scala Outdated
@@ -507,7 +501,18 @@ class CompletionArgSuite extends BaseCompletionSuite {
|}
|""".stripMargin,
"""|x: Int
|x = : Byte
Copy link
Contributor

Choose a reason for hiding this comment

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

For the future, this actually shouldn't be allowed for infix arguments, could you create an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it

@kasiaMarek kasiaMarek requested a review from tgodzik July 20, 2023 08:57
jkciesluk
jkciesluk approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

LGTM

@kasiaMarek
improvement: show args completions for all matching overloaded methods
e516b09 
1. added fallback for arg completions for overloaded methods in Scala 3 (previously no arg completion would be visible)
2. in Scala 2, choose the matching ones from overloaded methods and collect matching arg completions from all of those
@kasiaMarek
ArgCompletions refactor
8ed55ee
@kasiaMarek
FuzzyArgMatcher.substituteTypeParams refactor
4cefa4b
@kasiaMarek
review refactor
fb7e86f
@kasiaMarek
bugfix: don't show arg completions for infix
033517b
@kasiaMarek kasiaMarek force-pushed the args-completions-for-overloaded branch from a073513 to 033517b Compare July 26, 2023 10:54
kasiaMarek added a commit to kasiaMarek/dotty that referenced this pull request Jul 27, 2023
@kasiaMarek
backport changes to presentation compiler from metals pr scalameta/me…
945bcdd 
…tals#5287
@kasiaMarek kasiaMarek mentioned this pull request Jul 27, 2023
Merged
kasiaMarek added a commit to kasiaMarek/dotty that referenced this pull request Jul 28, 2023
@kasiaMarek
backport changes to presentation compiler from metals pr scalameta/me…
6488bc3 
…tals#5287
tgodzik
tgodzik approved these changes Jul 31, 2023
View reviewed changes
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!

@kasiaMarek kasiaMarek merged commit 4690573 into scalameta:main Aug 1, 2023
23 checks passed
rochala pushed a commit to scala/scala3 that referenced this pull request Aug 1, 2023
@kasiaMarek
update presentation compiler with changes from PR 5287 (#18301)
fa38cb8 
update scala3 presentation compiler with changes from
scalameta/metals#5287
@jkciesluk jkciesluk modified the milestone: Metals v1.0.1 Aug 24, 2023
Kordyjan pushed a commit to scala/scala3 that referenced this pull request Dec 1, 2023
@kasiaMarek @Kordyjan
update presentation compiler with changes from PR 5287 (#18301)
f087659 
update scala3 presentation compiler with changes from
scalameta/metals#5287
[Cherry-picked fa38cb8]
Kordyjan pushed a commit to scala/scala3 that referenced this pull request Dec 7, 2023
@kasiaMarek @Kordyjan
update presentation compiler with changes from PR 5287 (#18301)
f3e984d 
update scala3 presentation compiler with changes from
scalameta/metals#5287
[Cherry-picked fa38cb8]
Kordyjan pushed a commit to scala/scala3 that referenced this pull request Dec 7, 2023
@kasiaMarek @Kordyjan
update presentation compiler with changes from PR 5287 (#18301)
f007727 
update scala3 presentation compiler with changes from
scalameta/metals#5287
[Cherry-picked fa38cb8]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik approved these changes

@jkciesluk jkciesluk jkciesluk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kasiaMarek @tgodzik @jkciesluk