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.

Sign up for GitHub

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

Jump to bottom

cargo clippy and cargo clippy --fix --allow-dirty have different reports #13527

Open
Rustin170506 opened this issue Mar 4, 2024 · 7 comments · May be fixed by #15192
Open

cargo clippy and cargo clippy --fix --allow-dirty have different reports #13527

Rustin170506 opened this issue Mar 4, 2024 · 7 comments · May be fixed by #15192
Assignees
@Rustin170506
Labels
A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) C-bug Category: bug Command-clippy Command-fix S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing.

Comments

@Rustin170506
Copy link
Member

Rustin170506 commented Mar 4, 2024 •
edited
Loading

Problem

I ran cargo clippy and cargo clippy --fix --allow-dirty in the same project but got different reports.

  1. cargo clippy
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.16s
  1. cargo clippy --fix --allow-dirty
    Checking cargo-information v0.4.2 (/Volumes/t7/code/cargo-information)
       Fixed tests/testsuite/cargo_information/git_dependency/mod.rs (1 fix)
warning: use of a disallowed/placeholder name `baz`
 --> tests/testsuite/cargo_information/git_dependency/mod.rs:9:9
  |
9 |     let baz = git::new("baz", |project| {
  |         ^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_names
  = note: `#[warn(clippy::disallowed_names)]` on by default

warning: use of a disallowed/placeholder name `foo`
  --> tests/testsuite/cargo_information/git_dependency/mod.rs:15:9
   |
15 |     let foo = project()
   |         ^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_names

warning: `cargo-information` (test "testsuite") generated 2 warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.78s

Steps

  1. Clone https://github.com/hi-rustin/cargo-information
  2. Rustup: rustup toolchain install nightly-2024-02-21
  3. cargo clippy and check the result
  4. cargo clippy --fix --allow-dirty and check the result

Possible Solution(s)

None

Notes

I am willing to investigate and fix this issue.

Version

Cargo:

➜  workspace git:(draft/mystifying-neco) ✗ cargo --version
cargo 1.78.0-nightly (7b7af3077 2024-02-17)

Toolchain:

active toolchain
----------------

nightly-2024-02-21-x86_64-unknown-linux-gnu (default)
rustc 1.78.0-nightly (bb594538f 2024-02-20)

Clippy:

➜  workspace git:(draft/mystifying-neco) ✗ cargo clippy --version
clippy 0.1.78 (bb59453 2024-02-20)
@Rustin170506 Rustin170506 added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Mar 4, 2024
@epage
Copy link
Contributor

epage commented Mar 4, 2024

I'm assuming this is more of a clippy issue but I do not have permission to transfer this issue to their repo.

@weihanglo weihanglo added S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-triage Status: This issue is waiting on initial triage. labels Mar 29, 2024
@xFrednet
Copy link
Member

xFrednet commented Jul 26, 2024 •
edited
Loading

@epage I believe this is a difference with the cargo check and cargo fix defaults.1 I'm doing some testing on a similar issue, and it seems like cargo fix includes test files, while a normal cargo check run doesn't. The issue also lists additional warnings from the test files.

Thus far I've sadly been unable to confirm this suspicion, as I'm not quite sure where to check for the difference in Cargo's code.

One thing I can say, is that in my testing, the two comments cargo fix/cargo check behaved differently until I specified the --lib flag to only build the library crate.

Edit: Looking at cargo fix command I'm guessing that it selects all members in the workspace by default.

Footnotes

  1. My understanding is, that cargo clippy --fix internally calls cargo fix with clippy set as the driver. cargo clippy is similar, that it internally calls cargo check with clippy as a driver ↩

@weihanglo
Copy link
Member

I believe this is a difference with the cargo check and cargo fix defaults.

That's a correct guess. cargo fix by default runs cargo check --all-targets. The cargo-fix help manual has mentioned that difference:

if you’d like to apply all fixes to the current package, you can run:

cargo fix

which behaves the same as cargo check --all-targets.

While it is expected, however still confusing. Unsure how we should address this.

@weihanglo weihanglo added Command-fix Command-clippy A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels Jul 26, 2024
@xFrednet
Copy link
Member

Is there a reason why it uses --all-targets by default? For my use case, I'm currently trying to make it behave like the normal cargo check without the flag, but the only solution I found was to use --libs respectively --bins, which is sadly not generic for all kind of crates

@epage
Copy link
Contributor

epage commented Jul 26, 2024

My best guess is that the UI was optimized for edition migrations which has been the primary focus of cargo fix until we started advertising it on warnings.

@epage
Copy link
Contributor

epage commented Feb 4, 2025

We discussed this in the Cargo team meeting today.

I propose that we change cargo fix from defaulting to all targets to

  • cargo fix defaults to the same targets as cargo check
  • If --edition or --edition-idioms is passed in, then you get the current cargo fix behavior

CC @rust-lang/clippy as this would impact cargo clippys behavior,

This issue is fundamentally a clippy issue as they control their CLI. This is a result of their current implementation (of the --fix flag changing them from wrapping cargo check to cargo fix). cargo fix was changed to support this behavior for edition migrations in #5739. As cargo clippy --fix is not related to edition migrations, the behavior doesn't make sense. The implementation could always change to give clippy more control over their default targets. I suspect it could be easier to fix with an idea I have for cargo fix to loop over cargo check. I've suggested making a prototype of that idea a GSoC project.

However, if we change cargo fixs behavior, then cargo clippy should automatically pick that up. The current behavior is motivated by edition migration but that is not the full extend of cargo fixs behavior but is exclusive to --edition and --edition-idioms. cargo fix --edition has a reasonable expectation for what its programmatic behavior is. cargo fix however is less clear as fixable code edits are less likely to be made programmatic and cargo fix may output broken code. Another potential reason for cargo fix --edition to maintain behavior is if we decide to deprecate <build-target>.edition fields in manifests, then edition migration will be on the package level so it makes sense to select the whole package.

@weihanglo weihanglo mentioned this issue Feb 5, 2025
Open
@Manishearth
Copy link
Member

I like the proposal. cargo fix and cargo clippy --fix should have the same set of affected files, they both run lints.

@Rustin170506 Rustin170506 self-assigned this Feb 16, 2025
@Rustin170506 Rustin170506 linked a pull request Feb 16, 2025 that will close this issue
Open
@ehuss ehuss added S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Rustin170506 Rustin170506

Labels
A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) C-bug Category: bug Command-clippy Command-fix S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing.
Projects
None yet
Milestone
No milestone
6 participants
@ehuss @epage @Manishearth @weihanglo @xFrednet @Rustin170506