Skip to content

min_ident_chars triggers on imported identifiers, even from the standard library #12232

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

Closed
8573 opened this issue Feb 6, 2024 · 2 comments · Fixed by #12285
Closed

min_ident_chars triggers on imported identifiers, even from the standard library #12232

8573 opened this issue Feb 6, 2024 · 2 comments · Fixed by #12285
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@8573
Copy link

8573 commented Feb 6, 2024

Summary

min_ident_chars triggers on imported identifiers, even from the standard library, even though the user may be unable to do anything about it, especially in the case of the standard library.

The lint's documentation says:

What it does

Checks for idents which comprise of a single letter.

Note: This lint can be very noisy when enabled; it may be desirable to only enable it temporarily.

Why is this bad?

In many cases it’s not, but at times it can severely hinder readability. Some codebases may wish to disallow this to improve readability.

If the reason for the lint is for an individual codebase to improve its readability, I don't think it makes enough sense for it to complain about identifiers from outside the codebase.

(Also, "comprise of" should be either "comprise" or "consist of".)

Lint Name

min_ident_chars

Reproducer

I tried this code:

use core::f64::consts::E;

I saw this happen:

warning: this ident consists of a single char
    --> src/[...]
     |
[...]|     use core::f64::consts::E;
     |                            ^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars

I expected to see this happen:

No warning about an identifier in the standard library, which I don't control

Version

rustc 1.73.0 (cc66ad468 2023-10-03) (built from a source tarball)
binary: rustc
commit-hash: cc66ad468955717ab92600c770da8c1601a4ff33
commit-date: 2023-10-03
host: x86_64-unknown-linux-gnu
release: 1.73.0
LLVM version: 16.0.6

Additional Labels

No response

@8573 8573 added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 6, 2024
@Centri3
Copy link
Member

Centri3 commented Feb 7, 2024

You can (nvm we don't even handle aliases in use)

use core::f64::consts::E as EEEEEEEE;

Should definitely ignore non local def-ids tho

@Ethiraric
Copy link
Contributor

@rustbot claim

Ethiraric added a commit to Ethiraric/rust-clippy that referenced this issue Feb 12, 2024
Suppress the `min_ident_chars` warning for items whose name we cannot
control. Do not warn for `use a::b`, but warn for `use a::b as c`, since
`c` is a local identifier.

Fixes rust-lang#12232
Ethiraric added a commit to Ethiraric/rust-clippy that referenced this issue Feb 13, 2024
Suppress the `min_ident_chars` warning for items whose name we cannot
control. Do not warn for `use a::b`, but warn for `use a::b as c`, since
`c` is a local identifier.

Fixes rust-lang#12232
bors added a commit that referenced this issue Feb 14, 2024
Ignore imported items in `min_ident_chars`

Suppress the `min_ident_chars` warning for items whose name we cannot control. Do not warn for `use a::b`, but warn for `use a::b as c`, since `c` is a local identifier.

Fixes #12232

---

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`min_ident_chars`]: Do not warn on non-local identifiers
@bors bors closed this as completed in c1c2c3e Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants