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

Support completions for extension definition parameter #18331

Merged
merged 11 commits into from
Oct 3, 2023

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Aug 2, 2023

Extension methods are extended into normal definitions.
Because of that typed trees don't include any information about the extension method definition parameter:

extension (x: In@@)

In order to add completions, we check if there is an exact path to the untyped tree, and if not, we fall back to it. There may also be more possible cases like that, but I can't think of any at the moment.

@rochala rochala force-pushed the extension-definition-completions branch from d64894b to 281576d Compare August 24, 2023 16:09
@rochala
Copy link
Contributor Author

rochala commented Aug 25, 2023

This PR also contains refactor in Presentation Compiler completions, to reuse completion mode from the compiler instead of providing own logic for that. It caused a lot of issues already which tend to be fixed by this PR such as Term completions for case _: @@ or missing completion mode for extension method definitions, the main part of this PR

@rochala rochala requested a review from smarter August 25, 2023 07:24
@rochala rochala force-pushed the extension-definition-completions branch from b1d1944 to 7c8ebd9 Compare September 19, 2023 11:09
@rochala rochala added backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools labels Sep 19, 2023
Copy link
Contributor

@kasiaMarek kasiaMarek left a comment

Choose a reason for hiding this comment

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

Generally that looks reasonable, but I'm wondering about one thing, if you're using untyped trees only to establish the mode and get the prefix, how will that work for e.g.

extension (x: Bar.Fo@@)

will the scope for the completion search be correct?

@@ -256,8 +192,7 @@ class Completions(
private def findSuffix(symbol: Symbol): CompletionSuffix =
CompletionSuffix.empty
.chain { suffix => // for [] suffix
if shouldAddSnippet &&
cursorPos.allowBracketSuffix && symbol.info.typeParams.nonEmpty
if shouldAddSnippet && symbol.info.typeParams.nonEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you can just get rid of cursorPos.allowBracketSuffix. I think one of its responsibilities is getting rid of square brackets for methods, for which we want to have () as suffix. Can you add a test to make sure I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test, but overall I believe it was fixed #15750, and now we don't have to calculate it manually.

compiler/src/dotty/tools/dotc/interactive/Completion.scala Outdated Show resolved Hide resolved
@rochala
Copy link
Contributor Author

rochala commented Sep 21, 2023

Generally that looks reasonable, but I'm wondering about one thing, if you're using untyped trees only to establish the mode and get the prefix, how will that work for e.g.

extension (x: Bar.Fo@@)

will the scope for the completion search be correct?

Valid point. It won't work for any Select. I'll update the PR with the fix.

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! Just one comment

check(
"""|trait Foo
|object T:
| extension [A](using Foo)(x: Fo@@)(using Fo@@)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| extension [A](using Foo)(x: Fo@@)(using Fo@@)
| extension [A](using Foo)(x: Fo@@)(using Foo)

Copy link
Contributor

@kasiaMarek kasiaMarek 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 :)

@rochala rochala force-pushed the extension-definition-completions branch from 4df84e4 to 7300205 Compare October 3, 2023 12:50
@rochala rochala merged commit de4ad2b into scala:main Oct 3, 2023
18 checks passed
@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur pushed a commit that referenced this pull request Jun 19, 2024
Extension methods are extended into normal definitions.
Because of that typed trees don't include any information about the
extension method definition parameter:
```scala
extension (x: In@@)
```
In order to add completions, we check if there is an exact path to the
untyped tree, and if not, we fall back to it. There may also be more
possible cases like that, but I can't think of any at the moment.
[Cherry-picked de4ad2b]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
…LTS (#20635)

Backports #18331 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur pushed a commit that referenced this pull request Jun 20, 2024
Extension methods are extended into normal definitions.
Because of that typed trees don't include any information about the
extension method definition parameter:
```scala
extension (x: In@@)
```
In order to add completions, we check if there is an exact path to the
untyped tree, and if not, we fall back to it. There may also be more
possible cases like that, but I can't think of any at the moment.
[Cherry-picked de4ad2b]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
…LTS (#20688)

Backports #18331 to the LTS branch.

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
area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants