Skip to content

feature: Add inlay hints for named parameters #7400

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
Apr 24, 2025

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Apr 13, 2025

Copy link
Member

@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.

I feel like it would be cool to have inlay hints for only "useful" named params. Not necessarily instead of this but maybe additionally. We could avoid showing named params:

  1. for single letter param names (or xs, ys), since those are usually meaningless
  2. when param name and passed argument have the same name

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 17, 2025

I feel like it would be cool to have inlay hints for only "useful" named params. Not necessarily instead of this but maybe additionally. We could avoid showing named params:

  1. for single letter param names (or xs, ys), since those are usually meaningless

This really depends on a use case. Most of the times I guess yeah, but sometimes it might be useful for some symbolic names.

  1. when param name and passed argument have the same name

I want to keep it consistent, if we don't show named params in some cases, users might actually think they are broken.

@tgodzik tgodzik force-pushed the add-named-params branch 3 times, most recently from 200eff4 to 516b6ae Compare April 17, 2025 11:52
@tgodzik tgodzik requested a review from kasiaMarek April 17, 2025 15:00
|""".stripMargin,
"""|object Main{
| def foo(x: => Int, y: Int, z: => Int)(w: Int, v: => Int): Unit = ()
| foo(/*x = => */1, /*y = */2, /*z = => */3)(/*w = */4, /*v = => */5)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harpocrates I noticed that our PR interact with each other, what do you think about the mix of named parameters and by name ones? Alternatively, we could drop '=' and have:

Suggested change
| foo(/*x = => */1, /*y = */2, /*z = => */3)(/*w = */4, /*v = => */5)
| foo(/*x => */1, /*y = */2, /*z => */3)(/*w = */4, /*v => */5)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think seeing = => is probably the most reasonable thing - unfortunately in scala x => 1 starts looking like a lambda w/ arg x. I don't have a strong opinion though...

@harpocrates
Copy link
Contributor

On dropping some parameter inlay-hints when they'd probably be noise: I don't really have an opinion to add, but wanted to mention this is the approach rust-analyzer took. Here's the function they used to bake in all of their heuristics:

https://github.com/rust-lang/rust-analyzer/blob/16745db3f763b6a1a315a91e8f4191b6a7a9da63/crates/ide/src/inlay_hints/param_name.rs#L111

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 18, 2025

On dropping some parameter inlay-hints when they'd probably be noise: I don't really have an opinion to add, but wanted to mention this is the approach rust-analyzer took. Here's the function they used to bake in all of their heuristics:

https://github.com/rust-lang/rust-analyzer/blob/16745db3f763b6a1a315a91e8f4191b6a7a9da63/crates/ide/src/inlay_hints/param_name.rs#L111

Thanks! Though I think we can add the heuristics later when testing the feature and user have some specific ideas. For now I will leave it as straightforward as possible.

| /*name = */"semanticdb-javac",
| /*version = */"BuildInfo.javaSemanticdbVersion",
| ),
| None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This becomes much more complex now since all the parameters are now synthetic if we reorder the params in any way (though the flag is not set in reality). I might work on it later, but I will just do a follow up issue before mergeing. TODO

@tgodzik tgodzik merged commit 2d9d29a into scalameta:main Apr 24, 2025
25 checks passed
@tgodzik tgodzik deleted the add-named-params branch April 24, 2025 18:18
tgodzik added a commit to scala/scala3 that referenced this pull request Jun 25, 2025
Add inlay hints for name parameters.

Porting scalameta/metals#7400
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.

4 participants