-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: include trailing underscores when hiding inlay hints #20858
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
fix: include trailing underscores when hiding inlay hints #20858
Conversation
Using suffix underscores to avoid keywords is one of the common skill, and inlayHints hiding should support it
Example
---
**Before this PR**:
```rust
fn far(loop_: u32) {}
fn faz(r#loop: u32) {}
let loop_level = 0;
far(loop_level);
//^^^^^^^^^^ loop_
faz(loop_level);
```
**After this PR**:
```rust
fn far(loop_: u32) {}
fn faz(r#loop: u32) {}
let loop_level = 0;
far(loop_level);
faz(loop_level);
```
95ee6af to
700748e
Compare
|
Personally, I'm opposed to this. This feels like adding confusing magic. If we'll have data that shows that this is common practice in the ecosystem (more common than alternative methods, e.g. choosing alternative words and using Something I do consider is removing prefix underscores, as this is codified in the language (to discard unused warning). |
Please run At least it is indeed a common pattern |
|
rust-analyzer is unique in that, by its nature, it has many names that are Rust keywords. Other projects will have less. And yet I'm not sure that's the most common way of mangling them - in fact I'm pretty sure it's not. We use |
|
I ran the following command in the Out of 1021 crates, this skill was used 6699 times |
|
Your regex is incorrect. I printed the first 100 results in my dir. Here's the summary:
|
|
I tried to use |
|
And if I become more strict, and uses the regex: [\(,][\s\n*](as_|async_|await_|break_|const_|continue_|crate_|dyn_|else_|enum_|extern_|false_|fn_|for_|if_|impl_|in_|let_|loop_|match_|mod_|move_|mut_|pub_|ref_|return_|Self_|static_|struct_|super_|trait_|true_|type_|unsafe_|use_|where_|while_|async_|await_|dyn_|abstract_|become_|box_|do_|final_|gen_|macro_|override_|priv_|try_|typeof_|unsized_|virtual_|yield_|try_|macro_rules_|raw_|safe_|union_|i8_|i16_|i32_|i64_|i128_|isize_|u8_|u16_|u32_|u64_|u128_|usize_|char_|bool_|f32_|f64_): Which:
I get only 188 matches. And remember, my criterion was not "widely used", it was "widely used more than alternatives, enough to justify special treatment. And that we have no real way to measure with regexes. However, now I see (and sorry for looking too quickly) that this is not about changing the name of the parameter in the hint, just about hiding it when there is a match. That I don't mind both prefix and suffix underscores, we already have a bunch of heuristics there anyway. I will prefer this PR to also strip leading underscores though. |
The new regex returned more accurate results
This seems insignificant, it's just inlayHints and hardly disrupts old behavior
For 'alternative words', proactive typo (such as 'krate') increases the burden of first reading,
The function parameters have nothing to do with this
Do I need to implement these? |
No, because:
|
Using EDIT: If I understand correctly, is this PR okay? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks!
Using suffix underscores to avoid keywords is one of the common skill, and inlayHints hiding should support it
Example
Before this PR:
After this PR: