Skip to content
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

Tracking Issue: Fixing broken lint suggestions #3630

Open
phansch opened this Issue Jan 4, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@phansch
Copy link
Collaborator

phansch commented Jan 4, 2019

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 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

  • tests/ui/bytecount.rs
  • tests/ui/cast.rs
  • tests/ui/cfg_attr_rustfmt.rs
  • tests/ui/collapsible_if.rs
  • tests/ui/decimal_literal_representation.rs
  • tests/ui/duration_subsec.rs
  • tests/ui/excessive_precision.rs
  • tests/ui/expect_fun_call.rs
  • tests/ui/explicit_write.rs
  • tests/ui/fn_to_numeric_cast.rs
  • tests/ui/for_loop.rs
  • tests/ui/get_unwrap.rs #3625
  • tests/ui/inconsistent_digit_grouping.rs
  • tests/ui/infallible_destructuring_match.rs
  • tests/ui/into_iter_on_ref.rs
  • tests/ui/large_digit_groups.rs
  • tests/ui/len_zero.rs
  • tests/ui/literals.rs #3887
  • tests/ui/manual_memcpy.rs
  • tests/ui/map_clone.rs
  • tests/ui/matches.rs
  • tests/ui/mem_replace.rs
  • tests/ui/methods.rs
  • tests/ui/needless_bool.rs
  • tests/ui/precedence.rs
  • tests/ui/redundant_field_names.rs
  • tests/ui/reference.rs
  • tests/ui/replace_consts.rs
  • tests/ui/starts_ends_with.rs
  • tests/ui/strings.rs
  • tests/ui/string_lit_as_bytes.rs #3879
  • tests/ui/types.rs
  • tests/ui/unit_arg.rs
  • tests/ui/unnecessary_fold.rs
  • tests/ui/unnecessary_operation.rs
  • tests/ui/use_self.rs
  • tests/ui/useless_asref.rs #3886
  • tests/ui/while_loop.rs
@detrumi

This comment has been minimized.

Copy link
Contributor

detrumi commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Collaborator

flip1995 commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Contributor

detrumi commented Jan 13, 2019

@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?

bors added a commit that referenced this issue Jan 14, 2019

Auto merge of #3658 - detrumi:add-several-run-rustfix-annotations, r=…
…phansch

Add several run rustfix annotations

Adds `run-rustfix` to 18 of the tests from the tracking issue #3630.
Each test has its own commit, to make reviewing easier (hopefully this is easier to review than 18 separate PRs).

## Changes
- `cfg_attr_rustfmt`: Custom inner attributes are unstable. Let's disable the lint for inner attributes until [#54726](rust-lang/rust#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`: Avoid triggering `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`: Avoid triggering `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)]`
@flip1995

This comment has been minimized.

Copy link
Collaborator

flip1995 commented Jan 14, 2019

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 added a commit to mikerite/rust-clippy that referenced this issue Jan 24, 2019

Fix `expect_fun_call` lint suggestions
This commit corrects some bad suggestions produced by the
`expect_fun_call` lint and enables `rust-fix` checking on the tests.

Addresses rust-lang#3630

bors added a commit that referenced this issue Jan 26, 2019

Auto merge of #3690 - mikerite:fix-3630-expect-fun-call, r=phansch
Fix `expect_fun_call` lint suggestions

This commit corrects some bad suggestions produced by the
`expect_fun_call` lint and enables `rust-fix` checking on the tests.

Addresses #3630

g-bartoszek added a commit to g-bartoszek/rust-clippy that referenced this issue Feb 5, 2019

Fix `expect_fun_call` lint suggestions
This commit corrects some bad suggestions produced by the
`expect_fun_call` lint and enables `rust-fix` checking on the tests.

Addresses rust-lang#3630

g-bartoszek added a commit to g-bartoszek/rust-clippy that referenced this issue Feb 7, 2019

Fix `expect_fun_call` lint suggestions
This commit corrects some bad suggestions produced by the
`expect_fun_call` lint and enables `rust-fix` checking on the tests.

Addresses rust-lang#3630
@mikerite

This comment has been minimized.

Copy link
Contributor

mikerite commented Mar 7, 2019

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.

bors added a commit that referenced this issue Mar 15, 2019

Auto merge of #3886 - phansch:rustfix_useless_asref, r=flip1995
Enable rustfix for `useless_asref` lint tests

cc #3630
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.