Skip to content

improvement: Suggest to add using as a code action #23079

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Apr 30, 2025

This should add the code action to both Metals and Intellij.

I will try to go over the existing rewrites to see if we can add more actions easily.

Fixes #23071

@tgodzik tgodzik requested a review from mbovel April 30, 2025 18:55
@som-snytt
Copy link
Contributor

I added patching to "unused import" but by now I'd forgotten the mechanism. I remember being confused by the difference between a patch and an action patch. I see that I added action patches to my Message.

Is this a matter of "every message should be a Message"? and every patch should be an action patch in a Message?

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 30, 2025

Is this a matter of "every message should be a Message"? and every patch should be an action patch in a Message?

I think so? Especially if we can add a rewrite, we should also add it as an action.

)
)
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could reduce parens by preferring colon syntax. I remember the impedance mismatch of supplying lists when there was always one patch and for action. Scala 2 had CodeAction.apply return a list of one action. If only Scala supported varargs...

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 up a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw a deeply nested stack of colons in the parser and that is also a challenge (for me) to read. It happens that the args on the preceding line is supposed to be the unused args1, so there was additional cognitive load.

https://github.com/scala/scala3/blob/main/compiler/src/dotty/tools/dotc/parsing/Parsers.scala#L1803

@@ -3,5 +3,5 @@
| ^
| Implicit parameters should be provided with a `using` clause.
| This code can be rewritten automatically under -rewrite -source 3.7-migration.
| To disable the warning, please use the following option:
| To disable the warning, please use the following option:
Copy link
Contributor

Choose a reason for hiding this comment

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

it's nice to find a like-minded dev. By coincidence, today I:
image

Copy link
Contributor Author

@tgodzik tgodzik May 1, 2025

Choose a reason for hiding this comment

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

My editor removes the space and I couldn't be bothered to find the setting responsible for it :S

This should add the code action to both Metals and Intellij.

I will try to go over the existing rewrites to see if we can add more actions easily.

Fixes scala#23071
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.

Add a compiler quick fix for "Implicit parameters should be provided with a using clause"
2 participants