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

almost_swapped false positive a=a; a=a (in clap::Parser derive) #10421

Closed
3 tasks done
MingweiSamuel opened this issue Feb 28, 2023 · 6 comments · Fixed by #10502
Closed
3 tasks done

almost_swapped false positive a=a; a=a (in clap::Parser derive) #10421

MingweiSamuel opened this issue Feb 28, 2023 · 6 comments · Fixed by #10502
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@MingweiSamuel
Copy link
Contributor

MingweiSamuel commented Feb 28, 2023

Summary

#10177

  • clippy::almost_swapped's error message is wrong when triggered in a derive macro. Not sure about other macros.

error: this looks like you are trying to swap __clap_subcommand and Parser

There is no variable called Parser, that is the name of the derive macro. Should be:

error: this looks like you are trying to swap __clap_subcommand and __clap_subcommand

  • clippy::almost_swapped triggers when you would be "swapping" the same variable with itself. I.e. the following triggers it:
let a = a;
let a = a;

First impression wouldn't be that we're trying to swap a with a since they are the same name. And the generated suggestion std::mem::swap(&mut a, &mut a) is silly; breaks aliasing rules.

  • clippy::almost_swapped triggers within macros in the first place. Should it?

Obviously that code above is a little silly, but can be generated in macros, e.g. in clap::Parser's derive. Not sure if clippy is supposed to trigger within macros in general--I.e. #10318 ?

Lint Name

almost_swapped

Reproducer

I tried this code:

fn main() {
    let a = 0;
    let a = a;
    let a = a;
}

I saw this happen:

error: this looks like you are trying to swap `a` and `a`
 --> src/main.rs:3:5
  |
3 | /     let a = a;
4 | |     let a = a;
  | |_____________^ help: try: `std::mem::swap(&mut a, &mut a)`
  |
  = 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: `#[deny(clippy::almost_swapped)]` on by default

I expected to see this happen:

No error


Clap macro

I tried this code:

use clap::Parser;

#[derive(Parser)]
enum MyEnum {
    MyVariant
}

Which expands to:

// ...
#[allow(dead_code, unreachable_code, unused_variables, unused_braces)]
#[allow(clippy :: style, clippy :: complexity, clippy :: pedantic, clippy ::
restriction, clippy :: perf, clippy :: deprecated, clippy :: nursery, clippy
:: cargo, clippy :: suspicious_else_formatting,)]
#[deny(clippy :: correctness)]
impl clap::Subcommand for MyEnum {
    fn augment_subcommands<'b>(__clap_app: clap::Command) -> clap::Command {
        ;
        let __clap_app = __clap_app;
        let __clap_app =
            __clap_app.subcommand({
                    let __clap_subcommand = clap::Command::new("my-variant");
                    let __clap_subcommand = __clap_subcommand;
                    let __clap_subcommand = __clap_subcommand;
                    __clap_subcommand
                });
        ;
        __clap_app
    }
    fn augment_subcommands_for_update<'b>(__clap_app: clap::Command)
        -> clap::Command {
        ;
        let __clap_app = __clap_app;
        let __clap_app =
            __clap_app.subcommand({
                    let __clap_subcommand = clap::Command::new("my-variant");
                    let __clap_subcommand = __clap_subcommand;
                    let __clap_subcommand = __clap_subcommand;
                    __clap_subcommand
                });
        ;
        __clap_app
    }
    fn has_subcommand(__clap_name: &str) -> bool {
        if "my-variant" == __clap_name { return true }
        false
    }
}

I saw this happen:

error: this looks like you are trying to swap `__clap_subcommand` and `Parser`
 --> src/lib.rs:3:10
  |
3 | #[derive(Parser)]
  |          ^^^^^^
  |
  = 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/lib.rs:3:10
  |
3 | #[derive(Parser)]
  |          ^^^^^^
  = note: `#[deny(clippy::almost_swapped)]` implied by `#[deny(clippy::correctness)]`
  = note: this error originates in the derive macro `Parser` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `playground` due to previous error

I expected to see this happen:

No error

Or

error: this looks like you are trying to swap `__clap_subcommand` and `__clap_subcommand`

Version

Playground:
Build using the Nightly version: 1.69.0-nightly

(2023-02-27 7281249a19a9755e9d88)

Local windows:
$ rustc +nightly -Vv
rustc 1.69.0-nightly (d962ea578 2023-02-26)
binary: rustc
commit-hash: d962ea57899d64dc8a57040142c6b498a57c8064
commit-date: 2023-02-26
host: x86_64-pc-windows-msvc
release: 1.69.0-nightly
LLVM version: 15.0.7

Additional Labels

No response

@MingweiSamuel MingweiSamuel added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 28, 2023
@MingweiSamuel
Copy link
Contributor Author

MingweiSamuel commented Feb 28, 2023

clap-rs/clap#4733
clap-rs/clap#4735
(fix for the duplicate lets: clap-rs/clap@c1caf46 )

@glandium
Copy link

Also, neither #![allow(clippy::correctness)] nor #![allow(clippy::almost_swapped)] silence the warning.

@MingweiSamuel
Copy link
Contributor Author

@glandium

Also, neither #![allow(clippy::correctness)] nor #![allow(clippy::almost_swapped)] silence the warning.

Only in the clap derive since it had its own #[deny(clippy::correctness)] attribute. Removed in clap-rs/clap#4739

jplatte added a commit to matrix-org/matrix-rust-sdk that referenced this issue Mar 10, 2023
jplatte added a commit to matrix-org/matrix-rust-sdk that referenced this issue Mar 11, 2023
jplatte added a commit to matrix-org/matrix-rust-sdk that referenced this issue Mar 11, 2023
@blyxyas
Copy link
Member

blyxyas commented Mar 13, 2023

@rustbot claim

bors added a commit that referenced this issue Mar 14, 2023
Fix `almost_swapped` false positive (`let mut a = b; a = a`)

Fixes `2` in #10421
changelog: [`almost_swapped`]: Fix false positive when a variable is changed to itself. (`a = a`)
@bors bors closed this as completed in 5c4040e Mar 17, 2023
@blyxyas
Copy link
Member

blyxyas commented Mar 17, 2023

Task 1 and 3 should be marked as completed to indicate that they are also completed.

@MingweiSamuel
Copy link
Contributor Author

Marked as done! 🎉

micolous added a commit to micolous/webauthn-rs that referenced this issue Apr 22, 2023
micolous added a commit to micolous/webauthn-rs that referenced this issue Apr 22, 2023
micolous added a commit to kanidm/webauthn-rs that referenced this issue Apr 22, 2023
flihp added a commit to flihp/dice-util that referenced this issue Apr 26, 2023
This fixes a known issue introduced in rust 1.69 clippy that required
additional changes to clap. See:
rust-lang/rust-clippy#10421
clap-rs/clap#4733
flihp added a commit to oxidecomputer/dice-util that referenced this issue Apr 26, 2023
This fixes a known issue introduced in rust 1.69 clippy that required
additional changes to clap. See:
rust-lang/rust-clippy#10421
clap-rs/clap#4733
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants