similar_names: No longer suggest inserting or appending an underscore

[th1000s]

changelog: [similar_names] lint no longer suggests to insert or add an underscore to "fix" too similar names

[rust-highfive]

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

[giraffate]

Why do only specific keywords fixed here? It looks like that it's not limited to specific keywords. For example, book__ is suggested in case below.

let books = ["a", "b", "c"];
let book_ = "a";

[flip1995]

In addition to @giraffate question, my question is why this is a fix for this lint? The lint documentation states:

It's hard to distinguish between names that differ only by a single character.

types and type_ only differ by a single character. As do types and type. So the lint triggering here is correct. Since this lint is pedantic and triggering here is correct by the lint definition, I wouldn't change this. I don't see a linked issue about this, was there discussions of this change anywhere?

If you want a keyword as a variable name, you should add r# in front of the keyword instead of adding a _. So types and r#type should be fine. Does this lint trigger on those?

[flip1995]

This could be

Suggested change

	let rust_keywords = [
	"as", "break", "const", "continue", "crate", "else", "enum", "extern", "false", "fn", "for", "if", "impl",
	"in", "let", "loop", "match", "mod", "move", "mut", "pub", "ref", "return", "self", "Self", "static", "struct",
	"super", "trait", "true", "type", "unsafe", "use", "where", "while", "async", "await", "dyn", "abstract",
	"become", "box", "do", "final", "macro", "override", "priv", "typeof", "unsized", "virtual", "yield", "union",
	];
	rust_keywords.iter().any(|kw| *kw == maybe_keyword)
	Ident::with_dummy_span(Symbol::intern(maybe_keyword)).is_used_keyword()

[th1000s]

Thanks, much better than hardcoding the list. To also catch yield etc. I'd even use is_reserved().

[th1000s]

In the case of books / book_ the name book is available: One trailing character is not counted, maybe to allow for plurals such as this one.

The mentioned issue #6479 notes that an underscore is nicer to read that r#type which may look a bit out of place. I also agree that an underscore is visually distinct enough and would even suggest to not count trailing underscores at all, keyword or not.

[flip1995]

r#<keyword> exists in Rust, so that you can use keywords as idents. So

an underscore is nicer to read that r#type

But r#type is the Rust way to do it, so this isn't a valid argument for a change in Clippy. Also I disagree personally.

I also agree that an underscore is visually distinct enough and would even suggest to not count trailing underscores at all, keyword or not.

I'm not convinced that we should encourage adding an _ as the only distinguishing thing between variable names.

---

That being said, the suggestion with a trailing underscore is quite bad. I would just remove the help message if the distinguishing character is _.

[th1000s]

suggest[ing] a trailing underscore is quite bad.

Indeed, that just technically fixes the lint. Will fix that in any case.

What do @phansch and @camsteffen think? Should an exception be made when evading known keywords as the issue suggests? Maybe even go further and never count an underscore-character match, as abc and a_c are distinct enough?

[camsteffen]

The keyword thing doesn't make sense to me. The fact that type is a keyword doesn't make type_ and types any less similar.

I would just remove the help message if the distinguishing character is _.

...never count an underscore-character match, as abc and a_c are distinct enough?

These statements are saying the same thing. I think this makes sense. Then I would add another caveat that foo_ and foo__ should still lint.

[flip1995]

I would just remove the help message if the distinguishing character is _.

...never count an underscore-character match, as abc and a_c are distinct enough?

These statements are saying the same thing. I think this makes sense.

Not really, I think? I'm saying, that we should remove (or improve) the help message telling the user to change the name to a double underscore. I'm not suggesting to stop linting in those cases.

[camsteffen]

Oh I misunderstood. That makes sense too.

I think we could stop linting those cases since one underscore vs no underscores is a more visually distinct than different letters IMO.

[flip1995]

It's still pretty similar though. And this lint is a pedantic lint. So I wouldn't change it and let the lint be pedantic.

I would like to see a test if r#type vs types gets linted by this lint.

[giraffate]

ping from triage @th1000s. Does it still need to be discussed?

[th1000s]

Ok, then I'll just reword the current suggestion to "use a different name". Just adding a trailing _ "fixes" the lint but not the intent behind it.

[giraffate]

ping from triage @th1000s. Can you have any update on this?

[th1000s]

The help text now always reads:

help: use a more distinct name for either `bpple`, `apple`, or both

[camsteffen]

I don't find that very helpful. The two variables are already pointed out. How bout we just remove the help message?

[th1000s]

Very well, done.

[giraffate]

I think it looks good.

[giraffate]

@bors r+

Thanks!

[bors]

📌 Commit e8f57c3 has been approved by giraffate

[bors]

⌛ Testing commit e8f57c3 with merge 6103814...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 6103814 to master...