Skip to content

Lint non-expressive variable names #644

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.

Sign up for GitHub

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

Jump to bottom
Open
1 of 3 tasks
killercup opened this issue Feb 9, 2016 · 8 comments · May be fixed by #14818
Open
1 of 3 tasks

Lint non-expressive variable names #644

killercup opened this issue Feb 9, 2016 · 8 comments · May be fixed by #14818
Assignees
@JurrianFahner
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group T-AST Type: Requires working with the AST T-middle Type: Probably requires verifiying types

Comments

@killercup
Copy link
Member

killercup commented Feb 9, 2016 •
edited by rustbot
Loading

So, I've recently had to deal with code written in ▊▊▊▊▊ and for some reason the original authors of the software tended to use either one-letter variables or weirdly long names (that were not even very expressive). Now, don't get me wrong: I don't want to tell anyone how to name their bindings in Rust, I just want to suggest a few pointers.

I'm proposing a lint that suggests (very nicely) to the author to rethink the naming of bindings whose names fall in at least one of the following categories. This lint can, of course, only be a very subjective heuristic (which is basically the worst kind of lint there is), so I'll try to stay conservative.

  • The name contains the name of the type

e.g. let people_vec = vec![]

This is helpful in dynamically typed languages, but less so in a Rust which is statically typed. One difficulty might be the usage of abbreviations for types (or the opposite, like people_vector which is a Vec.

I think some prefixes (like is_ for bools) are okay, though.

  • There are more than 3 one-letter names in the same scope

x, y, z I can understand, but what do a, s, f, and l stand for?
Done in #727.

  • The name is more than 25 letters long

This is 1/4 of the usual line width. I've seen many such names that seem to be written that way only to allow the developer to not add documentation/comments in good conscience, since "the code is so obvious". I don't think that is very helpful (variable names describe the implementation, but probably don't help you grasp the bigger picture).

This should probably be Allow by default. I can also imagine this being split up into multiple sub-lints (or as a separate lint group). I don't want to call this a lint for "bad naming". We should probably call it "checking for non-expressive names".

What do you think of this? Is this something you want to do? Is this too opinionated? Do you have other suggestions on how to detect non-expressive names?

Please note that I'm focussing on the naming of variables/bindings, not methods or functions. Conventions for method names (e.g. in builders with/without set and get prefixes) should be discussed in another issue 😄
@killercup killercup changed the title Lint bad variable names Lint non-expressive variable names Feb 9, 2016
@oli-obk
Copy link
Contributor

oli-obk commented Feb 9, 2016

Also bad is when in a single scope there are names that are longer than 3-5 chars but only have a levensthein distance of 1 to another name

@Manishearth
Copy link
Member

The name contains the name of the type

I dunno, I find myself doing this often when there is an conceptual thing in layers of objects of different types. E.g.

let checked_expr = ....
if let ExprBlock(checked_block) = checked_expr.node {
  ...
}

@Manishearth
Copy link
Member

Perhaps only lint about foo_ty when foo and foo_* do not exist in scope

@killercup
Copy link
Member Author

Good point, @oli-obk, I hate it when this results in a typo that just happens to be valid name… (Also the first scientific suggestion, so we can check this with much more confidence.)

@Manishearth, that's a good example, since your name is actually 'checked', which is not very expressive IMHO but nevertheless fine if you know the context in which it's used. I was thinking of suffixes for 'general container types', to be honest. Expr(ession) and Block are type names that tell you what you are dealing with (and check_expr reads nicely), but checked_btreemap is probably less useful (I imagine your code might still work if checked_btreemap was actually a HashMap and no one renamed the binding).

@Manishearth
Copy link
Member

So how do we distinguish between useful names (like expr, block), and nonuseful ones (btreemap)?

It's also a common pattern to name things by exactly their type since often that variable is transient -- used only for one thing. You'll find this in clippy code often with variables named expr, ty, stmt, etc.

@mcarton mcarton mentioned this issue Feb 21, 2016
Closed
3 tasks
@ticki
Copy link

ticki commented Feb 26, 2016

Great ideas you got here 👍

@oli-obk oli-obk mentioned this issue Feb 29, 2016
Merged
@mcarton mcarton added good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST T-middle Type: Probably requires verifiying types A-lint Area: New lints L-style Lint: Belongs in the style lint group labels Jun 12, 2016
@killercup
Copy link
Member Author

I recently thought about this again and wanted to add: A lint that explicitly warns when using very long binding names for very short lived bindings that are used multiple times.

(Why the "used multiple times"? Because I want to allow code like let is_a_specific_thing = complex && boolean && expression; if is_a_specific_thing { … }!)

@JurrianFahner
Copy link

@rustbot claim

@JurrianFahner JurrianFahner linked a pull request May 15, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JurrianFahner JurrianFahner

Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group T-AST Type: Requires working with the AST T-middle Type: Probably requires verifiying types
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add lint long_variable_names JurrianFahner/rust-clippy
7 participants
@killercup @oli-obk @Manishearth @mcarton @flip1995 @JurrianFahner @ticki