chore: satisfy clippy on the current toolchain - #1518
Open
Eljees wants to merge 3 commits into
Open
Conversation
The Lint job fails on master with the clippy CI now installs, so it is red on every pull request opened since that bump - 13 of the 25 currently open, including 11 dependabot bumps. None of the findings sit in code those branches touch. - cobertura.rs: iter_kv_map. `into_iter().filter_map(|(_, l)| ...)` over a map whose keys are discarded is `into_values()`. - path_rewriting.rs: collapsible_match. The two `Some(..)` arms fold into match guards. - producer.rs: byte_char_slices. `[b'T', b'N', b':']` is `*b"TN:"`. - producer.rs: collapsible_match on `handle_file`. Those three content checks cannot become guards - `file` is an `Option<&mut impl Read>`, so moving it in a guard makes it moved for the whole match and the arms fail with E0382. Allowed, with that reason recorded next to it. `cargo fmt --check` is clean and `cargo test --lib` is unchanged.
Only reachable once the library compiles again: clippy's redundant_ref lint on
`panic!("Failed to copy {:?}", &gcda_zip_path)` in tests/test.rs. Inlined the
binding into the format string while there.
The crate is edition 2018, where implicit format-args capture does not exist,
so "{gcda_zip_path:?}" is taken as a literal message with an unused
placeholder. Dropping just the redundant reference is enough for the lint.
Author
|
Green now - 10/10, Two follow-up commits were needed, both from lints that only appear once the previous error stops the build early:
Worth flagging the reason for the churn: the clippy CI installs is newer than any toolchain I can reach, so I cannot reproduce its exact lint set locally and had to find those two through CI. If you have a preference for pinning the toolchain - a Once this lands, #1515 and #1516 only need |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lintfails onmasteritself with the clippy CI now installs, so it is red on every pull request opened since that bump - 13 of the 25 open ones when I checked, including 11 dependabot bumps, #1394, and my own #1515 and #1516. None of the findings are in code those branches touch, so nobody can turn their own PR green.The five findings CI reports, and what this does with each:
iter_kv_mapcobertura.rs:67into_iter().filter_map(|(_, l)| ...)discards the key, so it isinto_values()collapsible_matchpath_rewriting.rs:426Some(..)arms fold into match guardsbyte_char_slicesx2producer.rs:148[b'T', b'N', b':']is*b"TN:"collapsible_matchproducer.rs:112The last one is the only judgement call. The natural rewrite does not compile:
fileis anOption<&mut impl Read>and so notCopy, and a value moved in a match guard counts as moved for the whole match, so the three content checks inhandle_filehave to stay in the arm bodies. I put an#[allow(clippy::collapsible_match)]on the function with that reason written next to it rather than restructure the match. Happy to do it differently if you would rather.One thing I deliberately left out: an older clippy (1.95 here) additionally reports
collapsible_matchatsrc/parser.rs:742- the current CI clippy does not. It cannot become a guard either, because the inner condition uses?. I leftparser.rsuntouched so this does not conflict with #1515 and #1516, which both change that file. Say the word and I will fold it in.Checked locally:
cargo clippy --bins --tests --examples --all -- -D rust_2018_idioms -D warningsis clean apart from that oneparser.rsline,cargo fmt -- --checkpasses, andcargo test --libis unchanged at 133 passed (the fourllvm_toolsfailures here needllvm-profdataand fail identically onmaster).