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

Some clap v3.x Subcommand derives cause clippy errors since Rust 1.69.0 #4857

Closed
2 tasks done
hds opened this issue Apr 25, 2023 · 8 comments
Closed
2 tasks done

Some clap v3.x Subcommand derives cause clippy errors since Rust 1.69.0 #4857

hds opened this issue Apr 25, 2023 · 8 comments
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much

Comments

@hds
Copy link

hds commented Apr 25, 2023

Please complete the following tasks

Rust Version

rustc 1.69.0 (84c898d65 2023-04-16)

Clap Version

clap 3.2.23, clap_derive 3.2.18

Minimal reproducible code

#[derive(clap::Subcommand)]
enum Command {
    One,
}

fn main() {}

Steps to reproduce the bug with the above code

Run clippy with defaults:

cargo clippy

Actual Behaviour

This results in the following error in clippy.

error: this looks like you are trying to swap `__clap_subcommand` and `clap::Subcommand`
 --> src/main.rs:1:10
  |
1 | #[derive(clap::Subcommand)]
  |          ^^^^^^^^^^^^^^^^
  |
  = note: or maybe you should use `std::mem::replace`?
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped
note: the lint level is defined here
 --> src/main.rs:1:10
  |
1 | #[derive(clap::Subcommand)]
  |          ^^^^^^^^^^^^^^^^
  = note: `#[deny(clippy::almost_swapped)]` implied by `#[deny(clippy::correctness)]`
  = note: this error originates in the derive macro `clap::Subcommand` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `clap-deny-correctness` due to previous error

Expected Behaviour

The clippy error is problematic because the attribute that specifies deny is inside the macro itself. That means that as a user of Clap, I'm not able to override the deny.

Specifically this attribute:

#[deny(clippy::correctness)]

Which is present in 7 specific places in clap_derive.

Additional Context

For Clap v4.x, these attributes were already removed in #4739.

Doing the same in the v3-master branch is probably sufficient

Debug Output

No response

@hds hds added the C-bug Category: Updating dependencies label Apr 25, 2023
@epage epage added E-easy Call for participation: Experience needed to fix: Easy / not much A-derive Area: #[derive]` macro API labels Apr 25, 2023
@epage
Copy link
Member

epage commented Apr 25, 2023

I am fine with someone fixing this in v3-master.

hds added a commit to hds/clap that referenced this issue Apr 25, 2023
The `#[deny(correctness)]` attribute is inside some generated code (such
as the `Subcommand` derive macro). As of Rust 1.69.0, this is triggering
a clippy error in some uses. Due to the placement of the `deny`, it is
not possible for a user of clap to override it.

This change removes all the instances of this `deny` attribute. The same
was done on the `master` branch in clap-rs#4739.

Fixes: clap-rs#4857
hds added a commit to hds/clap that referenced this issue Apr 25, 2023
The `#[deny(correctness)]` attribute is inside some generated code (such
as the `Subcommand` derive macro). As of Rust 1.69.0, this is triggering
a clippy error in some uses. Due to the placement of the `deny`, it is
not possible for a user of clap to override it.

This change removes all the instances of this `deny` attribute. The same
was done on the `master` branch in clap-rs#4739.

Fixes: clap-rs#4857
hds added a commit to hds/clap that referenced this issue Apr 25, 2023
The `#[deny(correctness)]` attribute is inside some generated code (such
as the `Subcommand` derive macro). As of Rust 1.69.0, this is triggering
a clippy error in some uses. Due to the placement of the `deny`, it is
not possible for a user of clap to override it.

This change removes all the instances of this `deny` attribute. The same
was done on the `master` branch in clap-rs#4739.

Fixes: clap-rs#4857
@guswynn
Copy link

guswynn commented Apr 26, 2023

@epage Can we #[allow(almost_swapped)] in v3 instead of just removing the deny clause? It's quite onerous to allow these warnings, as annotating the struct/enum with #[allow(clippy::almost_swapped)] doesn't work, you have to use a #![allow(clippy::almost_swapped)] at the module level. If one wants to limit the scope of this allow, they have to introduce a new submodule for all their clap-derived types.

I'm willing to make the pr to do this!

@epage
Copy link
Member

epage commented Apr 27, 2023

I will say that the idea is to match clap v4. We should treat anything in v3 as backports. I should have looked more closely to confirm that. I would be open to a PR if it makes things more aligned with v4. If someone wants something that is not in v4, we should put it in v4 first.

@hds
Copy link
Author

hds commented Apr 27, 2023

Adding #![allow(clippy::almost_swapped)] was already done in v4 (in #4735). I didn't include it in #4858 for the sake of keeping it as small as possible. If @epage is happy for this to also be backported, then I can create the PR.

@epage
Copy link
Member

epage commented Apr 27, 2023

I would be fine with that

@guswynn
Copy link

guswynn commented Apr 27, 2023

Thanks for following up here @hds !

hds added a commit to hds/clap that referenced this issue Apr 27, 2023
As of Rust 1.69, the `clippy::almost_swapped` lint was enhanced and now
triggers on certain derive macros inside clap. To avoid the need for the
user to allow these lints outside of the macro (which generally requires
a module level `allow`), the allow was added to the generated code for
the v4 branch in clap-rs#4735.

This change backports that fix the the `v3-master` branch.

Fixes: clap-rs#4857
@hds
Copy link
Author

hds commented Apr 27, 2023

I've backported the v4 change #4735 to v3-master, now available in #4866.

The same docs test is failing that failed in the previous PR on that branch.

@epage
Copy link
Member

epage commented Jun 14, 2023

I believe this is now resolved, so I'm going to close this. If there is something specific to this issue that still isn't resolved, let us know!

@epage epage closed this as completed Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies E-easy Call for participation: Experience needed to fix: Easy / not much
Projects
None yet
Development

No branches or pull requests

3 participants