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

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

Open
xilexio opened this issue Jun 11, 2023 · 3 comments
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-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@xilexio
Copy link

xilexio commented Jun 11, 2023

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

@xilexio xilexio 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 Jun 11, 2023
@asquared31415
Copy link
Contributor

@rustbot label +I-suggestion-causes-error

@rustbot rustbot added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Jun 11, 2023
@asquared31415
Copy link
Contributor

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

bors added a commit that referenced this issue Jun 19, 2023
[`redundant_closure_call`]: handle nested closures

Fixes #9956.

This ended up being a much larger change than I'd thought, and I ended up having to pretty much rewrite it as a late lint pass, because it needs access to certain things that I don't think are available in early lint passes (e.g. getting the parent expr). I think this'll be required to fi-x #10922 anyway, so this is probably fine.
(edit: had to write "fi-x" because "fix" makes github think that this PR fixes it, which it doesn't 😅 )

Previously, it would suggest changing `(|| || 42)()()` to `|| 42()`, which is a type error (it needs parens: `(|| 42)()`). In my opinion, though, the suggested fix should have really been `42`, so that's what this PR changes.

changelog: [`redundant_closure_call`]: handle nested closures and rewrite as a late lint pass
bors added a commit that referenced this issue Jun 19, 2023
[`redundant_closure_call`]: handle nested closures

Fixes #9956.

This ended up being a much larger change than I'd thought, and I ended up having to pretty much rewrite it as a late lint pass, because it needs access to certain things that I don't think are available in early lint passes (e.g. getting the parent expr). I think this'll be required to fi-x #10922 anyway, so this is probably fine.
(edit: had to write "fi-x" because "fix" makes github think that this PR fixes it, which it doesn't 😅 )

Previously, it would suggest changing `(|| || 42)()()` to `|| 42()`, which is a type error (it needs parens: `(|| 42)()`). In my opinion, though, the suggested fix should have really been `42`, so that's what this PR changes.

changelog: [`redundant_closure_call`]: handle nested closures and rewrite as a late lint pass
@CinusMinus
Copy link

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);
}

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-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

No branches or pull requests

4 participants