Skip to content

unnecessary_lazy_evaluations: Shouldn't warn about values with significant Drop impl #9427

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

Closed
sdroege opened this issue Sep 5, 2022 · 6 comments · Fixed by #9551
Closed
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 I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@sdroege
Copy link

sdroege commented Sep 5, 2022

Summary

The case in question that was wrongly reported is the following

warning: unnecessary closure used with `bool::then`
   --> glib/src/convert.rs:122:9
    |
122 |         (iconv as isize != -1).then(|| Self(iconv))
    |         ^^^^^^^^^^^^^^^^^^^^^^^--------------------
    |                                |
    |                                help: use `then_some(..)` instead: `then_some(Self(iconv))`
    |
    = note: `#[warn(clippy::unnecessary_lazy_evaluations)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations

Self here has a significant Drop impl, specifically it will close the isize there (it's basically like a fd and closing -1 is UB).

Lint Name

unnecessary_lazy_evaluations

Reproducer

I tried this code:

struct Foo(i32);

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

fn main() {
    (0 == 1).then(|| Foo(0));
}

I saw this happen:

warning: unnecessary closure used with `bool::then`
  --> src/main.rs:10:5
   |
10 |     (0 == 1).then(|| Foo(0));
   |     ^^^^^^^^^---------------
   |              |
   |              help: use `then_some(..)` instead: `then_some(Foo(0))`
   |
   = note: `#[warn(clippy::unnecessary_lazy_evaluations)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations

I expected to see this happen: nothing because the suggestion gives different behaviour

Version

rustc 1.65.0-nightly (289279de1 2022-09-04)
binary: rustc
commit-hash: 289279de116707f28cf9c18e4bbb8c6ec84ad75b
commit-date: 2022-09-04
host: x86_64-unknown-linux-gnu
release: 1.65.0-nightly
LLVM version: 15.0.0

Additional Labels

@rustbot label +I-suggestion-causes-error

@sdroege sdroege 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 Sep 5, 2022
@rustbot rustbot added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Sep 5, 2022
@kraktus
Copy link
Contributor

kraktus commented Sep 28, 2022

@rustbot claim

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Oct 29, 2022

This issue is not fixed, and should be reopened.

struct NoisyDrop;

impl Drop for NoisyDrop {
    fn drop(&mut self) {
        panic!();
    }
}

fn a() {
    let _ = false.then(|| NoisyDrop);
}

warns on clippy 0.1.66 (9565dfe 2022-10-28)

@kraktus
Copy link
Contributor

kraktus commented Oct 29, 2022

can reproduce on master, will look at

@flip1995 flip1995 reopened this Nov 4, 2022
bors added a commit that referenced this issue Nov 22, 2022
Fix [`unnecessary_lazy_eval`] when type has significant drop

fix for #9427 (comment)

However current implementation gives too many false positive, rending the lint almost useless.

I don't know what's the best way to check if a type has a "significant" drop (in the common meaning, not the internal rustc one, for example Option<(u8, u8)> should not be considered significant)

changelog: Fix [`unnecessary_lazy_eval`] when type has significant drop
@xFrednet
Copy link
Member

#9750 should have fixed this issue :).

@ju1ius
Copy link

ju1ius commented Jan 5, 2024

@xFrednet @kraktus I believe this issue is not fixed and should be reopened.

The following code:

extern crate ffi;
use core::ffi::c_char;

struct Buffer {
    ptr: *mut c_char,
    len: usize,
}
impl Buffer {
    unsafe fn from_owned_ptr(ptr: *mut c_char, len: usize) -> Option<Self> {
        (!ptr.is_null()).then(|| Self { ptr, len })
    }
}
impl Drop for Buffer {
    fn drop(&mut self) {
        unsafe { ffi::free(self.ptr) };
    }
}

Triggers this warning (using clippy 0.1.76):

warning: unnecessary closure used with `bool::then`
  --> buffer.rs:10:9
   |
48 |         (!ptr.is_null()).then(|| Self { ptr, len })
   |         ^^^^^^^^^^^^^^^^^--------------------------
   |                          |
   |                          help: use `then_some(..)` instead: `then_some(Self { ptr, len })`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
   = note: `#[warn(clippy::unnecessary_lazy_evaluations)]` on by default

@y21
Copy link
Member

y21 commented Jan 5, 2024

Opened #12097 for that one

bors added a commit that referenced this issue Jan 5, 2024
don't change eagerness for struct literal syntax with significant drop

Fixes the bug reported by `@ju1ius` in #9427 (comment).

`eager_or_lazy` already understands to suppress eagerness changes when the expression type has a significant drop impl, but only for initialization of tuple structs or unit structs. This changes it to also avoid changing it for `Self { .. }` and `TypeWithDrop { .. }`

changelog: [`unnecessary_lazy_eval`]: don't suggest changing eagerness for struct literal syntax when type has a significant drop impl
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
8 participants