Incorrect clippy::derivable_impls in the presence of conditional compilation

[VorpalBlade]

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

[estebank]

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.

[TimNN]

As a similar case, I have code that looks similar to bool_field: cfg!(feature = "hello") in a Default impl, which also triggers a false-positive from this lint.

[estebank]

Took a quick look at this again, and the issue is that what you have in the lint is the HIR, but I don't think the placeholder attribute makes it past the AST. We might have to change that (on the compiler side, maybe with another side-table? or on clippy's side by collecting these during the run of early-lints?) for this lint to know about cfgs.

[ada4a]

@rustbot label S-blocked