Rust-clippy: Tracking Issue: Fixing broken lint suggestions

[phansch]

Now that we have rustfix integrated in our testsuite, we can use it to make sure that the suggestions we provide are valid rust code. This will also be useful once cargo fix --clippy is available to users.

This tracking issue is meant to give an overview of how many lints currently provide incorrect suggestions.

Below is the list of tests that include MachineApplicable but incorrect suggestions. For this issue to be closed, we need to add // run-rustfix to all of them and have the tests execute successfully.

If you want to help out with this, feel free to select one file that is not fixed and leave a comment below. Smaller files are most likely easier to fix.

1. Once you added // run-rustfix to the .rs file run cargo test && tests/ui/update-all-references.sh && cargo test. The tests for your file should fail and it should have generated a *.fixed file
2. Run rustc tests/ui/<your_test_file>.fixed to see the exact compilation error
3. Fix the issue (Follow CONTRIBUTING.md and feel free to ask questions)

Files that test lints which need to be fixed

• [x] tests/ui/assign_ops.rs
• [x] tests/ui/assign_ops2.rs
• [x] tests/ui/cmp_owned.rs
• [x] tests/ui/deref_addrof_double_trigger.rs
• [ ] tests/ui/entry.rs
• [x] tests/ui/eq_op.rs
• [x] tests/ui/float_cmp.rs
• [x] tests/ui/float_cmp_const.rs
• [x] tests/ui/for_loop.rs
• [x] tests/ui/identity_conversion.rs
• [x] tests/ui/implicit_return.rs
• [x] tests/ui/inline_fn_without_body.rs
• [x] tests/ui/int_plus_one.rs
• [x] tests/ui/literals.rs
• [x] tests/ui/map_flatten.rs
• [x] tests/ui/mem_discriminant.rs
• [x] tests/ui/needless_borrow.rs
• [x] tests/ui/needless_borrowed_ref.rs
• [x] tests/ui/needless_collect.rs
• [x] tests/ui/needless_return.rs
• [x] tests/ui/non_copy_const.rs
• [x] tests/ui/op_ref.rs (https://github.com/rust-lang/rust-clippy/issues/2597)
• [x] tests/ui/option_map_unit_fn.rs
• [x] tests/ui/or_fun_call.rs
• [x] tests/ui/outer_expn_data.rs
• [x] tests/ui/range_plus_minus_one.rs
• [x] tests/ui/redundant_closure_call.rs
• [x] tests/ui/redundant_pattern_matching.rs
• [x] tests/ui/rename.rs
• [x] tests/ui/renamed_builtin_attr.rs
• [x] tests/ui/redundant_static_lifetimes.rs
• [x] tests/ui/result_map_unit_fn.rs #4571
• [x] tests/ui/short_circuit_statement.rs
• [x] tests/ui/strings.rs
• [x] tests/ui/toplevel_ref_arg.rs #4567
• [x] tests/ui/unnecessary_clone.rs
• [x] tests/ui/unnecessary_operation.rs
• [x] tests/ui/wildcard_enum_match_arm.rs #4562
• [x] tests/ui/useless_attribute.rs

[detrumi]

I'm currently going through the list, writing down why each test doesn't work with run-rustfix yet, and fixing the easy ones.

[flip1995]

Could you share what you write down here, for lints which you don't get to fix, so that future contributors have an easier start?

[detrumi]

@flip1995 Was already doing that, but I guess I can just edit my comment while I'm working through the list.

Fixing this myself (PR: #3658)

• cfg_attr_rustfmt: Custom inner attributes are unstable. Let's disable the lint for inner attributes until #54726 stabilizes
• collapsible_if: unrelated cyclomatic_complexity warning that can be ignored
• duration_subsec: Simply needed #![allow(dead_code)]
• excessive_precision: Fixed by #!allow(dead_code,unused_variables)
• explicit_write: Fixed by #![allow(unused_imports)]
• inconsistent_digit_grouping: Triggered unrelated clippy::excessive_precision lint
• infallible_destructuring_match: Fixed by #![allow(dead_code, unreachable_code, unused_variables)]
• into_iter_on_ref: Triggered unrelated clippy::useless_vec lint
• large_digit_groups: Triggered clippy::excessive_precision lint
• map_clone: Fixed by #![allow(clippy::iter_cloned_collect)]
• mem_replace: Suggestion causes import to be unused, fixed by #![allow(unused_imports)]
• precedence: Allow some unrelated lints, and change out-of-range 0b1111_1111i8 literal
• redundant_field_names: Allow dead code, and remove stabilized feature toggles
• replace_consts: Fixed by #![allow(unused_variables)]
• starts_ends_with: Fixed by #![allow(unused_must_use)]
• types: Fixed by #![allow(dead_code, unused_variables)]
• unit_arg: Fixed by #[allow(unused_must_use)]
• unnecessary_fold: Fixed by adding type annotations and adding #![allow(dead_code)]

Not yet fixed

• bytecount: Suggestions use bytecount crate which probably isn't imported
• cast: Only cast-lossless is machine-applicable, others like cast_precision_loss aren't. Split the tests?
• decimal_literal_representation: 4_042_322_160 is out of range for i32, but using 4_042_322_160_u32 leads to a suggestion of 0xF0F0_F0F0, without the _u32
• expect_fun_call: Excess ) (this suggestion), but fixing that leads to a borrowed value does not live long enough error here
• fn_to_numeric_cast: The suggestion changes the type (to usize in a function that returns i32)
• for_loop: Suggests invalid .... Also several other invalid suggestions that result in syntax errors or iterating over ()
• len_zero: The len_without_is_empty lint triggers because the test defines its own len() function. len_without_is_empty should probably not trigger here.
• literals: rustfix panics with error Cannot replace slice of data that was already replaced.
• matches: The suggestions don't seem to be applied (or update-references.sh isn't working)
• methods: This test does #![warn(clippy::all)] and then whitelists some, but several other lints are triggered that are not in the whitelist. This test needs to be rewritten to trigger less lints, or it should use a blacklist instead
• needless_bool: Needs #![allow(clippy::no_effect, path_statements, unused_must_use)], and some lint warnings don't have suggestions, such as this if-then-else expression will always return true
• reference: The test is designed to trigger new lints after applying the suggestions, so it can't use the current run-rustfix (unless you can specify that a line will trigger a lint after applying the suggestion)
• strings: clippy::string_add currently doesn't give suggestions in all cases yet
• unnecessary_operation: [42, 55][get_number() as usize]; gets replaced by [42, 55];get_number() as usize;, but that triggers the lint again to replace get_number() as usize; with get_number();
• use_self: See #3567
• useless_asref: It replaces foo_rrrrmr((&&&&MoreRef).as_ref()) with foo_rrrrmr((&&&&MoreRef)), which triggers the unused_parens lint. The lint should probably remove those parens
• while_loop: This lint makes suggestions like while let Some(_x) = y { .. }, which aren't meant to be machine-applicable (run-rustfix literally places .. there, instead of the code block)

Notes on the process

That's the whole list! Some of these are tricky cases:

• Suggestions that trigger another lint afterwards. Would it be better to avoid doing that in tests, or should we be able to annotate that it's intended somehow? Maybe common cases like removing extra braces should still be included.
• Maybe a special case of the above, but what if a lint needs a new import, or removes the last usage of the import? Should the lint be able to add/remove imports?

[flip1995]

Suggestions that trigger another lint afterwards

Sometimes this is unavoidable. If it is somehow possible we should maybe set _all_ Clippy lints to warn/allow in the second pass, where the *.rustfix files are checked.

but what if a lint needs a new import, or removes the last usage of the import? Should the lint be able to add/remove imports

If that's the case, the lint shouldn't be MachineApplicable but MaybeIncorrect.

[mikerite]

Just a note. If the .fixed file fails to compile due a Clippy warning, you can run

CLIPPY_TESTS=true cargo run --bin clippy-driver tests/ui/<test>.fixed

to see.

[phansch]

I have replaced the list at the top with one generated with the rustfix-coverage flag from compiletest-rs. There's quite a bit to do.

[Manishearth]

The problem with tests/ui/deref_addrof_double_trigger.rs is that it needs two passes of the test-and-fix cycle. I feel like we should just exclude it.

[Manishearth]

I'm working through these from the top-down. Currently on eq_op.

[Manishearth]

Ticking off some boxes early:

• eq_op doesn't actually have suggestions
  
  
  
  • float_cmp_const is HasPlaceholders already
  
  

[Manishearth]

So _many_ of these issues seem to be "fixable tests and unfixable tests are mixed in the same file". For a rustfix test to work, the fixed test must compile with no clippy warnings, which means the test cannot _also_ contain tests which do not get fixed.

(Given that, this may be a much smaller problem than we imagined)

[Manishearth]

Posted https://github.com/rust-lang/rust-clippy/pull/4558, will make more progress tomorrow on the plane.

[Manishearth]

More in https://github.com/rust-lang/rust-clippy/pull/4575

With this and #4571 we have nine left to go!

[Manishearth]

4575 contains everything except op_ref.

[Manishearth]

I updated #4575 to just mark op_ref as MaybeIncorrect for now. We're done!

[phansch]

Awesome! thanks @Manishearth for finishing this up :tada:

[phansch]

Oh, as a next step, we could probably use the compiletest-rs rustfix coverage tracking to make sure all new MachineApplicable diagnostics have a test file. I'll file a new issue later today.

[Manishearth]

It would be nice if compiletest had a mode that didn't force you to split files into rustfixable and non-rustfixable, where it tries to fix the code and then rechecks the file, only looking for errors which aren't fixable. We can also check in the .fixed-stderr file in that case. This way we could run-rustfix by default.