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

Incorrect clippy::derivable_impls in the presence of conditional compilation #13160

Open
VorpalBlade opened this issue Jul 25, 2024 · 1 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 I-suggestion-causes-bug Issue: The suggestion compiles but changes the code to behave in an unintended way

Comments

@VorpalBlade
Copy link

Summary

Clippy will happily change the meaning of code using conditional compilation inside a custom Default when using --fix. The code compiles but is now incorrect.

Lint Name

clippy::derivable_impls

Reproducer

I tried this code:

/// Package manager to use 
#[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)]
pub enum ConcreteBackend {
    #[cfg(feature = "arch_linux")]
    Pacman,
    #[cfg(feature = "debian")]
    Apt,
    Flatpak,
}

// Clippy is wrong, this cannot be derived due to the cfg_if
impl Default for ConcreteBackend {
    fn default() -> Self {
        cfg_if::cfg_if! {
            if #[cfg(feature = "arch_linux")] {
                ConcreteBackend::Pacman
            } else if #[cfg(feature = "debian")] {
                ConcreteBackend::Apt
            } else {
                ConcreteBackend::Flatpak
            }
        }
    }
}

I saw this happen:

With --fix it creates the following code that is not equivalent (when no features are enabled, you will get different variants of this depending on what features are enabled)

/// Package manager to use 
#[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)]
#[derive(Default)]
pub enum ConcreteBackend {
    #[cfg(feature = "arch_linux")]
    Pacman,
    #[cfg(feature = "debian")]
    Apt,
    #[default]
    Flatpak,
}

The reported clippy message without --fix is:

    Checking repro v0.1.0 (/home/arvid/src/repro)
warning: this `impl` can be derived
  --> src/main.rs:12:1
   |
12 | / impl Default for ConcreteBackend {
13 | |     fn default() -> Self {
14 | |         cfg_if::cfg_if! {
15 | |             if #[cfg(feature = "arch_linux")] {
...  |
23 | |     }
24 | | }
   | |_^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
   = note: `#[warn(clippy::derivable_impls)]` on by default
   = help: remove the manual implementation...
help: ...and instead derive it...
   |
3  + #[derive(Default)]
4  | pub enum ConcreteBackend {
   |
help: ...and mark the default variant
   |
8  ~     #[default]
9  ~     Flatpak,
   |

warning: `repro` (bin "repro") generated 1 warning (run `cargo clippy --fix --bin "repro"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s

I expected to see this happen:

Either Clippy should use cfg_attr here on default, or if that is too complicated it should detect that it breaks the code and refuse to auto-fix it.

Version

rustc 1.80.0 (051478957 2024-07-21)
binary: rustc
commit-hash: 051478957371ee0084a7c0913941d2a8c4757bb9
commit-date: 2024-07-21
host: x86_64-unknown-linux-gnu
release: 1.80.0
LLVM version: 18.1.7

Additional Labels

@rustbot label +I-suggestion-causes-bug

@VorpalBlade VorpalBlade 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 Jul 25, 2024
@rustbot rustbot added the I-suggestion-causes-bug Issue: The suggestion compiles but changes the code to behave in an unintended way label Jul 25, 2024
@estebank
Copy link
Contributor

Because of the way [the macro expansion for cfg-if works], it is not possible to detect post-expansion today, but it will be with rust-lang/rust#133823, which adds a placeholder attribute to every item that has been left behind after a "positively evaluated" cfg. By looking for the presence of that attribute, clippy can then decide not to touch the impl and avoid linting that case.

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 I-suggestion-causes-bug Issue: The suggestion compiles but changes the code to behave in an unintended way
Projects
None yet
Development

No branches or pull requests

3 participants