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

Clippy doesn't report error/warnings after cargo check #4612

Closed
b1zzu opened this issue Oct 2, 2019 · 41 comments · Fixed by #6687 or godwokenrises/godwoken#319
Closed

Clippy doesn't report error/warnings after cargo check #4612

b1zzu opened this issue Oct 2, 2019 · 41 comments · Fixed by #6687 or godwokenrises/godwoken#319
Labels
T-cargo Type: cargo related

Comments

@b1zzu
Copy link

b1zzu commented Oct 2, 2019

Environment

Project: https://github.com/b1zzu/wifiscanner/tree/clippy-bug (use the clippy-bug branch`)
Cargo: cargo 1.38.0 (23ef9a4ef 2019-08-20)
Clippy: clippy 0.0.212 (3aea860 2019-09-03)

Steps to reproduce

Run cargo clippy after cargo check

cargo check
cargo clippy

Clippy doesn't fail or display any errors.

Expected result

cargo clippy should fails with this errors:

warning: Constants have by default a `'static` lifetime
  --> src/lib.rs:87:22
   |
87 |     const PATH_ENV: &'static str = "PATH";
   |                     -^^^^^^^---- help: consider removing `'static`: `&str`
   |
   = note: `#[warn(clippy::redundant_static_lifetimes)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes

error: approximate value of `f{32, 64}::consts::PI` found. Consider using it directly
  --> src/lib.rs:85:14
   |
85 |     let _x = 3.14;
   |              ^^^^
   |
   = note: `#[deny(clippy::approx_constant)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant

warning: single-character string constant used as pattern
   --> src/lib.rs:157:16
    |
157 |         .split("\n")
    |                ^^^^ help: try using a char instead: `'\n'`
    |
    = note: `#[warn(clippy::single_char_pattern)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern

warning: single-character string constant used as pattern
   --> src/lib.rs:168:36
    |
168 |     for line in network_list.split("\n") {
    |                                    ^^^^ help: try using a char instead: `'\n'`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern

error: aborting due to previous error

error: Could not compile `wifiscanner`.

To learn more, run the command again with --verbose.

Workaround

cargo check
cargo clean
cargo clippy
@mati865
Copy link
Contributor

mati865 commented Oct 2, 2019

The only reasonable way to fix it is #3837

With nightly toolchain you can use cargo clippy-preview -Z unstable-options as workaround.

@b1zzu
Copy link
Author

b1zzu commented Oct 2, 2019

@mati865 for now I will use cargo clean before clippy

@matthiaskrgr
Copy link
Member

It works better if you touch all files of your root crate, this way you don't have to rebuild all the dependencies every time:

find . | grep "\.rs$" | xargs touch ; cargo clippy

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy A-documentation Area: Adding or improving documentation labels Oct 3, 2019
@flip1995
Copy link
Member

flip1995 commented Oct 3, 2019

I think we should document this (find . | grep "\.rs$" | xargs touch ; cargo clippy) until cargo clippy (in cargo) is stable.

@b1zzu
Copy link
Author

b1zzu commented Oct 3, 2019

@flip1995 where would you document it? In a new separate Troubleshooting section or as a comment/tips to the step-3-run-clippy

@flip1995
Copy link
Member

flip1995 commented Oct 3, 2019

I think at step-3-run-clippy is enough, since it will be removed anyway in the near future.

@maxbla
Copy link

maxbla commented Dec 9, 2019

This issue can also be triggered by running clippy multiple times with different options.
main.rs

fn main() {
    let _1 = 1_usize;
    println!("{}", _1);
}
$ cargo clean
$ cargo clippy -- --deny clippy::just_underscores_and_digits
    Compiling ...
error: consider choosing a more descriptive name
...
$ cargo clippy -- --allow clippy::all
$ cargo clippy -- --deny clippy::just_underscores_and_digits

cargo clean fixes this manifestation of the problem also

@sourcefrog
Copy link
Contributor

It looks like this is now fixed?

cargo check
cargo clippy

shows the right errors every time I run it.

clippy 0.0.212 (69f99e7 2019-12-14)
cargo 1.41.0 (626f0f40e 2019-12-03)

@flip1995
Copy link
Member

flip1995 commented Feb 3, 2020

That would surprise me, because we didn't change anything.

I just tested #4612 (comment) and the behavior didn't change there.

@humb1t
Copy link

humb1t commented Mar 27, 2020

Clippy still depends on check: clippy 0.0.212 (4ee1206 2020-02-01)

@najamelan
Copy link

The only reasonable way to fix it is #3837

What about having cargo clippy run the touch **.rs workaround itself? It would depend on platform support for the touch command and globbing, but I feel this issue is important enough to try this.

@mati865
Copy link
Contributor

mati865 commented Apr 17, 2020

What about having cargo clippy run the touch **.rs workaround itself?

You can just make a script that will call touch and Clippy.

@humb1t
Copy link

humb1t commented Apr 17, 2020

@mati865 don't you think that this is not a solution?
Imagine that cargo test will not run tests if cargo build was already executed.

@mati865
Copy link
Contributor

mati865 commented Apr 17, 2020

@humb1t neither modifying files without permission is the solution.

@flip1995
Copy link
Member

This is now fixed when running Clippy with

cargo clippy -Zunstable-options

on nightly.

@humb1t
Copy link

humb1t commented Apr 17, 2020

@mati865 totally agree with you

@matthiaskrgr
Copy link
Member

The feature should be available as soon as nightly 2020-04-18 rolls out.

@saleemrashid
Copy link

saleemrashid commented Apr 23, 2020

If I run the following commands (in this order) in my workspace, which has some dead_code, all the commands work as expected

# dead_code errors
cargo clippy -Z unstable-options -- -D warnings
# dead_code + clippy::restriction errors
cargo clippy -Z unstable-options -- -D warnings -D clippy::restriction
# dead_code errors
cargo clippy -Z unstable-options -- -D warnings

However, if I use warnings instead of errors, the first invocation's output is cached.

# dead_code warnings
cargo clippy -Z unstable-options
# dead_code warnings
cargo clippy -Z unstable-options -- -W clippy::restriction
find -name "*.rs" -exec touch {} \;
# dead_code + clippy::restriction warnings
cargo clippy -Z unstable-options -- -W clippy::restriction
# dead_code + clippy::restriction warnings
cargo clippy -Z unstable-options

Not sure if I'm using an outdated Clippy version, none of the comments seem to specify which is the "fixed" nightly, or even link to the relevant pull request.


rustc 1.44.0-nightly (b2e36e6c2 2020-04-22)
clippy 0.0.212 (891e1a8 2020-04-18)

@flip1995
Copy link
Member

You should be using the nightly where this is already included.

@yaahc any ideas?

@saleemrashid
Copy link

saleemrashid commented Apr 23, 2020

Here's a shorter reproduction case

echo "fn unused() {}" > src/lib.rs
cargo clippy -Z unstable-options
cargo clippy -Z unstable-options -- --allow dead_code
touch src/lib.rs
cargo clippy -Z unstable-options -- --allow dead_code
cargo clippy -Z unstable-options

Screenshot
Screenshot


Note that it works fine with errors, but not warnings.

echo "pub fn bad() -> bool { 0 <= 0 }" > src/lib.rs
cargo clippy -Z unstable-options
cargo clippy -Z unstable-options -- --allow clippy::eq_op
touch src/lib.rs
cargo clippy -Z unstable-options -- --warn clippy::all
cargo clippy -Z unstable-options -- --allow clippy::eq_op

Screenshot from 2020-04-24 00-37-16
Screenshot from 2020-04-24 00-37-48


rustc 1.44.0-nightly (b2e36e6c2 2020-04-22)
cargo 1.44.0-nightly (8751eb301 2020-04-21)
clippy 0.0.212 (891e1a8 2020-04-18)

@yaahc
Copy link
Member

yaahc commented Apr 24, 2020

This is probably an issue with cargo, I'll check it out tomorrow.

cc @ehuss

@ehuss
Copy link
Contributor

ehuss commented Apr 24, 2020

The caching with different flags is rust-lang/cargo#7862. The issue is twofold. One is that clippy hides the flags from cargo (via __CLIPPY_HACKERY__). The other is that Cargo doesn't provide a way to hash flags.

EDIT: I haven't looked into this, but I'm uncertain why clippy uses the flag hack. If it used RUSTFLAGS instead, it might just work? Something worth investigating.

@phansch phansch added the T-cargo Type: cargo related label Apr 26, 2020
rrbutani added a commit to rrbutani/xterm-js-sys that referenced this issue May 18, 2020
We'll just run `cargo clean` first (until this gets resolved: rust-lang/rust-clippy/issues/4612).
@flip1995
Copy link
Member

This is now fixed in Clippy and will land in some nightly over the weekend. This means the fix will hit stable (if nothing goes wrong) in 1.52.0.

@tesuji
Copy link
Contributor

tesuji commented Feb 10, 2021

Hi, I'm happy this issue fixed.
From the changelog it seems that

cargo clippy no longer shares the same build cache as cargo check.

One issue for me is that clippy run slower than cargo-check command.
Before this change, I run cargo check and touch main.rs or lib.rs to redo the lints by clippy.

There are some reasons why clippy is slower than cargo-check:

  1. clippy has many more lints to run compare to just official lints supported by rust in cargo-check.
  2. even though warnings/errors by lints for transitive dependencies are silent by default,
    the lints actually run for all the code in dependencies. Don't run allowed lints rust#74704

Should I opened an issue to track this on clippy side ?

Those said, my personal issue should not be a reason to revert the fix.
Thank you for working on this.

@flip1995
Copy link
Member

One issue for me is that clippy run slower than cargo-check command.

Yes this is true for your crate, not for the dependencies. This is because Clippy runs more lints and analyzes your code more than cargo check. It shouldn't be that noticeable though.

Clippy lints are not run for dependencies (except local path dependencies, but you can disable this behavior with cargo clippy -- --no-deps). The issue you linked isn't really about linting dependencies - that's just an effect of this issue. There's nothing from the Clippy side that could be done.

Before this change, I run cargo check and touch main.rs or lib.rs to redo the lints by clippy.

This change didn't even land yet. The effect of this change is solely that you don't have to touch those files anymore to run cargo clippy after cargo check.

wchargin added a commit to tensorflow/tensorboard that referenced this issue May 8, 2021
Summary:
Rust 1.52 is out! The [release notes] have the rundown, but I’m excited
in particular for [`str::split_once`] and a [caching fix for Clippy].

[release notes]: https://blog.rust-lang.org/2021/05/06/Rust-1.52.0.html
[`str::split_once`]: https://doc.rust-lang.org/std/primitive.str.html#method.split_once
[caching fix for Clippy]: rust-lang/rust-clippy#4612

wchargin-branch: rust-v1.52.0
wchargin-source: b4be6db4e642e3c2bbf34d476556901cf12c0c60
wchargin added a commit to tensorflow/tensorboard that referenced this issue May 8, 2021
Summary:
Rust 1.52 is out! The [release notes] have the rundown, but I’m excited
in particular for [`str::split_once`] and a [caching fix for Clippy].

[release notes]: https://blog.rust-lang.org/2021/05/06/Rust-1.52.0.html
[`str::split_once`]: https://doc.rust-lang.org/std/primitive.str.html#method.split_once
[caching fix for Clippy]: rust-lang/rust-clippy#4612

wchargin-branch: rust-v1.52.0
@hellow554
Copy link
Contributor

This issue is still pinned. Is this on purpose or just a leftover? I could understand it if you want people who aren't using the latest stable version to see this in the first place. But as said: is it on purpose?

@flip1995
Copy link
Member

Since last Thursday it isn't on purpose anymore, since this hit stable 🎉 Thanks again @everyone who worked on this! I removed the pin, thanks for reminder.

@flip1995 flip1995 unpinned this issue May 12, 2021
connec added a commit to connec/cloudformatious that referenced this issue May 17, 2021
This was being triggered by the derive for EnumSetType, which generates
an item that triggers the lint. It wasn't seen until now due to
[rust-lang/rust-clippy#4612][1], which was only fixed in Rust 1.52.0.

[1]: rust-lang/rust-clippy#4612
connec added a commit to connec/cloudformatious that referenced this issue May 17, 2021
This was being triggered by the derive for EnumSetType, which generates
an item that triggers the lint. It wasn't seen until now due to
[rust-lang/rust-clippy#4612][1], which was only fixed in Rust 1.52.0.

[1]: rust-lang/rust-clippy#4612
zeroqn added a commit to zeroqn/godwoken that referenced this issue Aug 30, 2021
cbrune added a commit to cbrune/cargo.el that referenced this issue Mar 10, 2022
This patch removes the `-Zunstable-options` from the default args for
the clippy command.  According to rust-lang/rust-clippy#4612, this
work around is no longer needed.

Fixes: kwrooijen#125

Signed-off-by: Curt Brune <curt@brune.net>
cbrune added a commit to cbrune/cargo.el that referenced this issue Mar 10, 2022
This patch removes the `-Zunstable-options` from the default args for
the clippy command.  According to rust-lang/rust-clippy#4612, this
work around is no longer needed.

Fixes: kwrooijen#125

Signed-off-by: Curt Brune <curt@brune.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-cargo Type: cargo related
Projects
None yet