unnecessary_lazy_evaluations incorrectly suggest to eagerly evaluate unsafe methods

[AngelicosPhosphoros]

Summary

Sometimes, we want to lazily evaluate some unsafe methods because eager evaluation is undefined behaviour (e.g. if we eagerly dereference a pointer)

Lint Name

unnecessary_lazy_evaluations

Reproducer

I tried this code:

use std::ptr::NonNull;
pub fn get_ref(is_valid: bool, ptr: &NonNull<i32>)->Option<&i32> {
    unsafe {
        is_valid.then(||ptr.as_ref())
    }
}

I saw this happen:

warning: unnecessary closure used with `bool::then`
 --> src/lib.rs:4:9
  |
4 |         is_valid.then(||ptr.as_ref())
  |         ^^^^^^^^^--------------------
  |                  |
  |                  help: use `then_some(..)` instead: `then_some(ptr.as_ref())`
  |
  = 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

I expected to see this happen:

No warning because our code safe only because we call unsafe method NonNull::as_ref only if we have true in is_valid variable. Eagerly evaluating as_ref would be undefined behaviour because calling it only safe if there are no other living references and we are sure that it points to valid object.

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-pc-windows-msvc
release: 1.81.0
LLVM version: 18.1.7

Additional Labels

No response

[lukaslueg]

Same as #13226 ?

[AngelicosPhosphoros]

Yes, it is a duplicate.

[y21]

#13226 has a similar title, but based on the discussion there it really sounds more like it's specifically about the lint always warning field accesses without considering that it might be unions, whereas this is about const method calls being warned without considering that they might be marked unsafe.

So I wouldn't say that this is a duplicate. Or at least, a fix for that other issue wouldn't necessarily fix this one.

if we eagerly dereference a pointer

Note that this case is already checked for by the lint.

[AngelicosPhosphoros]

Note that this case is already checked for by the lint.

I meant, NonNull<T> is a pointer too but lint is not aware of that.

[y21]

Fair, but I still don't really think this is a duplicate or in any way related to the linked issue. They're both about how the lint interacts with unsafe code, but these two issues are about very different operations that are flagged (union access vs unsafe method calls).

Checking if the lazy evaluation is in an unsafe block leads to unnecessary false negatives (e.g. if you're in an unsafe fn you're in an unsafe context so we would presumably have to avoid running this lint in there even if there's no unsafe operation performed in the closure at all and where it would be perfectly valid to emit a warning).