rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.46k stars 1.55k forks source link

Wrongly suggested expression simplification inside match-like expression where drop behavior is different #10922

Open xilexio opened 1 year ago

xilexio commented 1 year ago

Summary

Values X in matched expressions in match X { ... }, while let ... = X { ... }, if let ... = X { ... } have their lifetime extended until the end of match/while-let/if-let. In this context, simplification of some expressions such as (|| code)() into code produces semantically different results - values in the first being dropped at the end of the function and not dropped until the end of match/while-let/if-let in the second. Therefore, clippy should be extra careful about proposing simplifications that involve reducing the number of scopes in this context.

Lint Name

redundant_closure_call

Reproducer

I tried this code in clippy 0.1.72 (e6d4725 2023-06-05):

struct NoisyDrop(u8);
impl Drop for NoisyDrop {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

fn noise() -> NoisyDrop {
    NoisyDrop(0)
}

fn main() {
    println!("before match 1");
    match noise().0 {
        0 => {
            println!("inside match 1");
        }
        _ => {
            unreachable!()
        }
    };

    println!("before match 2");
    match (|| noise().0)() {
        0 => {
            println!("inside match 2");
        }
        _ => {
            unreachable!()
        }
    };
}

I saw this happen:

warning: try not to call a closure in the expression where it is declared
  --> src/main.rs:21:11
   |
21 |     match (|| noise().0)() {
   |           ^^^^^^^^^^^^^^^^ help: try doing something like: `noise().0`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_call
   = note: `#[warn(clippy::redundant_closure_call)]` on by default

I expected to see this happen: No suggestion at all.

Version

rustc 1.72.0-nightly (e6d4725c7 2023-06-05)
binary: rustc
commit-hash: e6d4725c76f3b526c74454bc51afdf6daf133506
commit-date: 2023-06-05
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.4

Additional Labels

I-suggestion-causes-error

asquared31415 commented 1 year ago

@rustbot label +I-suggestion-causes-error

asquared31415 commented 1 year ago

Not sure if "error" is the right wording here, but I didn't see an "incorrect suggestion" label

CinusMinus commented 1 week ago

I found a related example where this suggestion does actually cause a compile error because of the lifetime extension, which is arguably still less bad than subtly changing the runtime behavior

trait Blub {
    type Iterator<'a>: Iterator<Item=Self::Key> where Self: 'a;
    type Key: Copy;
    fn keys(&self) -> Self::Iterator<'_>;
    fn remove(&mut self, k: Self::Key);
}

impl<T> Blub for Vec<T> {
    type Iterator<'a> = std::ops::Range<usize> where Self: 'a;
    type Key = usize;
    fn keys(&self) -> Self::Iterator<'_> {
        0..self.len()
    }
    fn remove(&mut self, i: usize) {
        Vec::remove(self, i);
    }
}

fn bla<T: Blub>(blub: &mut T) {
    while let Some(i) = (|| blub.keys().next())() {
        blub.remove(i);
    }
}

fn main() {
    let mut blub = vec![1, 2, 3, 4, 5, 6, 7, 8, 9];
    bla(&mut blub);
    println!("blub: {:?}", blub);
}