Skip to content

Draft: additional completions for using clause #23647

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

Merged
merged 2 commits into from
Aug 14, 2025
Merged

Conversation

vder
Copy link
Contributor

@vder vder commented Aug 1, 2025

fix for #22939

@vder
Copy link
Contributor Author

vder commented Aug 1, 2025

I've added some additional tests that are failing currently:

  1. using2 - duplicates using
  2. using3 - adds using to the last argument ()

@vder
Copy link
Contributor Author

vder commented Aug 1, 2025

Scala CLA signed

|@main def main1(): Unit =
| val str = "hello"
| val int = 4
| hello(using str, int)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this check is actually failing because we check if arguments.size == 1

I would leave this test out or just document the current behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah the test was written this way as this was one of the proposed solutions to do nothing when we have 2 arguments. When I remove that guard the using keyword will be before int arg anyway. I'm not sure should i leave it that way or try to fix it.

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.

Thanks for working on this!

adjustedPath match
case (ident: Ident) :: (app@Apply(_,args)) :: _ if args.size == 1 =>
app.symbol.info match
case mt@MethodType(termNames) if app.symbol.paramSymss.last.exists(_.is(Given)) =>
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
case mt@MethodType(termNames) if app.symbol.paramSymss.last.exists(_.is(Given)) =>
case mt@MethodType(termNames) if app.symbol.paramSymss.last.exists(_.is(Given)) &&
!text.substring(app.fun.span.end, arg.span.start).contains("using") =>

I was thinking if we can make it simpler, but I can't see how.

CompletionAffix.empty
.chain { suffix => // for [] suffix
if shouldAddSuffix && symbol.info.typeParams.nonEmpty then
suffix.withNewSuffixSnippet(Affix(SuffixKind.Bracket))
else suffix
}
.chain{ suffix =>
adjustedPath match
case (ident: Ident) :: (app@Apply(_,args)) :: _ if args.size == 1 =>
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
case (ident: Ident) :: (app@Apply(_,args)) :: _ if args.size == 1 =>
case (ident: Ident) :: (app@Apply(_, List(arg))) :: _ =>

|@main def main1(): Unit =
| val str = "hello"
| val int = 4
| hello(using str, int)
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
| hello(using str, int)
| hello(str, int)

let's document and fix later

@vder vder force-pushed the issue-22939-fix branch from 721e6a5 to 78b6dbd Compare August 14, 2025 10:04
@vder vder marked this pull request as ready for review August 14, 2025 12:05
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! Thanks for working on this!

@tgodzik tgodzik merged commit 2b004ee into scala:main Aug 14, 2025
45 checks passed
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.

2 participants