Fix --explain lint lookup being case-mismatched - #17632
Conversation
rustbot
added
S-waiting-on-community-reviews
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. Please see the contribution instructions for more information. |
| // `explain` receives the lint name already lowercased by `src/main.rs`, while | ||
| // `Lint::name` is uppercase, so the two have to be reconciled before the lookup. | ||
|
|
||
| #[test] | ||
| fn explain_finds_declared_lints() { | ||
| if option_env!("RUSTC_TEST_SUITE").is_some() { | ||
| return; | ||
| } | ||
|
|
||
| for info in clippy_lints::declared_lints::LINTS { | ||
| let name = info.lint.name.strip_prefix("clippy::").unwrap().to_lowercase(); | ||
| assert_eq!( | ||
| clippy_lints::explain(&name), | ||
| 0, | ||
| "`--explain {name}` did not find a lint that exists" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn explain_accepts_names_as_typed() { | ||
| if option_env!("RUSTC_TEST_SUITE").is_some() { | ||
| return; | ||
| } | ||
|
|
||
| // Spelled out rather than derived from `Lint::name`, so that the lookup is | ||
| // checked against the form a user actually passes to `--explain`. | ||
| for name in ["allow_attributes", "absurd_extreme_comparisons", "toplevel_ref_arg"] { | ||
| assert_eq!(clippy_lints::explain(name), 0, "`--explain {name}` did not find a lint"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn explain_rejects_unknown_lint() { | ||
| if option_env!("RUSTC_TEST_SUITE").is_some() { | ||
| return; | ||
| } | ||
|
|
||
| assert_eq!(clippy_lints::explain("this_lint_does_not_exist"), 1); |
There was a problem hiding this comment.
These don't actually test what you want to test. You can get the built cargo-clippy path via env!("CARGO_BIN_EXE_cargo-clippy") and run it with --explain.
There was a problem hiding this comment.
Makes sense, thanks. Going through the binary also covers the lowercasing, the clippy:: stripping and the - to _ replacement in main.rs, none of which the direct call touches.
One thing before I write it: how many lints should it run? One process per lint puts all 832 at roughly a minute of wall clock, which feels like a lot to add to CI when they all go down the same path. I was going to spawn it for a couple of names plus the input forms main.rs actually rewrites:
allow_attributesALLOW_ATTRIBUTESclippy::allow_attributesallow-attributes- something unknown, expecting exit code 1
Would you rather it just iterate over declared_lints::LINTS and eat the runtime?
There was a problem hiding this comment.
I would just do lint-name, clippy::lint_name, LINT_NAME2, CLIPPY::LINT_NAME2 and not_a_lint. To test I would spawn all of them and then wait on the results; running them sequentially is pointlessly slow.
There was a problem hiding this comment.
Done, thanks. tests/explain.rs now spawns all five and waits afterwards, I gave it its own file rather than appending to config-consistency.rs, since that one is about config metadata and doesn't spawn anything.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
Jarcho
added
the
beta-nominated
`src/main.rs` lowercases the `--explain` argument, but `Lint::name` is uppercase (`clippy::ALLOW_ATTRIBUTES`), so the lookup in `explain` never matched and every lint reported `unknown lint`. The `to_ascii_uppercase` that reconciled the two was dropped in c97f7eb. Add a regression test that runs the built `cargo-clippy` binary, so the argument conversion in `src/main.rs` is covered along with the lookup. This code path had no tests at all.
25a394a to
52bb2d9
Compare
There was a problem hiding this comment.
Thank you. Going to merge this quickly since it needs to be backported.
@rustbot merge
rustbot
removed
the
S-waiting-on-community-reviews
3 participants
cargo clippy --explain <lint>printsunknown lintfor every lint on beta and nightly.src/main.rslowercases the argument, butLint::nameis uppercase (clippy::ALLOW_ATTRIBUTES), so the lookup inexplainnever matches. Theto_ascii_uppercasethat reconciled the two was dropped in c97f7eb ("Rewrite of the config parsing code"), and this restores that line verbatim.betaandmastercarry the commit, stable 0.1.98 does not, which is why--explainstill works there.The lowercase rebinding below the lookup is deliberately left alone.
mdconf.retain(...)keys on that form, so removing it would silently drop every configuration section.The tests are new because
--explainhad no coverage at all, which is how this went unreported for three weeks.cargo test --features internalpasses here apart from the 16tests/ui-internalcases, which fail the same way on an unmodified master checkout, so they look local to my Windows setup.#17573 moves this code and inherits the same mismatch, so whichever lands second needs a small rebase.
fixes #17620
changelog: Fix
cargo clippy --explain <lint>reportingunknown lintfor every lint