unnecessary_lazy_evaluations does not take cost of expression into account

[jhpratt]

Summary

See reproducer.

Lint Name

unnecessary_lazy_evaluations

Reproducer

I tried this code [playground]:

#![allow(unused)]

struct ParsedItem<'a, T>(&'a [u8], T);

fn first_match<'a, T>(
    options: impl IntoIterator<Item = (&'a [u8], T)>,
    case_sensitive: bool,
) -> impl FnMut(&'a [u8]) -> Option<ParsedItem<'a, T>> {
    move |input| None
}

fn parse(input: &[u8]) {
    let zone_literal = first_match(
        [
            (&b"UT"[..], 0),
            (&b"GMT"[..], 0),
            (&b"EST"[..], -5),
            (&b"EDT"[..], -4),
            (&b"CST"[..], -6),
            (&b"CDT"[..], -5),
            (&b"MST"[..], -7),
            (&b"MDT"[..], -6),
            (&b"PST"[..], -8),
            (&b"PDT"[..], -7),
        ],
        false,
    )(input)
    .or_else(|| match input {
        [
            b'a'..=b'i' | b'k'..=b'z' | b'A'..=b'I' | b'K'..=b'Z',
            rest @ ..,
        ] => Some(ParsedItem(rest, 0)),
        _ => None,
    });
}

NB: This is actual code from the time crate. first_match is only present to ensure compilation succeeds.

I saw this happen:

Checking playground v0.0.1 (/playground)
warning: unnecessary closure used to substitute value for `Option::None`
  --> src/lib.rs:13:24
13 |       let zone_literal = first_match(
   |  ________________________^
14 | |         [
15 | |             (&b"UT"[..], 0),
16 | |             (&b"GMT"[..], 0),
...  |
33 | |         _ => None,
34 | |     });
   | |______^
   |
   = 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
help: use `or` instead
   |
13 ~     let zone_literal = first_match(
14 +         [
15 +             (&b"UT"[..], 0),
16 +             (&b"GMT"[..], 0),
17 +             (&b"EST"[..], -5),
18 +             (&b"EDT"[..], -4),
 ...

warning: `playground` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 5.90s

I expected to see this happen:

unnecessary_lazy_evaluations should not be triggered. The expression inside the or_else closure is arbitrary and non-trivial. By requesting the user change or_else to or, whatever the closure contains is now eagerly evaluated at an unknown cost. As such I believe the lint should only trigger when the closure contains a literal or value that is statically known (setting aside const eval). This would ensure that only trivial cases are linted against.

Version

rustc 1.59.0 (9d1b2106e 2022-02-23)
binary: rustc
commit-hash: 9d1b2106e23b1abd32fce1f17267604a5102f57a
commit-date: 2022-02-23
host: x86_64-unknown-linux-gnu
release: 1.59.0
LLVM version: 13.0.0

Additional Labels

No response

[ebobrow]

Some checking is already done in eager_or_lazy, but match expressions are not considered an expression type that requires lazy evaluation. Should that be changed?

[jhpratt]

Given that match expressions aren't lazy by themselves, I would think so. I'm on mobile now but can create a demo showing where the behavior is different later.

[jhpratt]

I just tried to create a demo with side effects, but the trivial example of printing a value doesn't trigger the lint. Regardless, the closure can have an arbitrary cost — this should be quite apparent. As such I think match should be included.

[ebobrow]

Looks like the behavior changed in PR #7639 and there's another issue linked. I don't think I'm qualified to make a decision, but given that multiple people have had issues maybe something should be changed.

[jeffs]

clippy::unnecessary_lazy_evaluations also doesn't recognize expressions that might cause underflow, as in (i > 0).then(|| i - 1) where i: usize. This has bitten me a couple of times recently.

[y21]

clippy::unnecessary_lazy_evaluations also doesn't recognize expressions that might cause underflow, as in (i > 0).then(|| i - 1) where i: usize. This has bitten me a couple of times recently.

That bug was fixed in #11002 and the fix for it should be on nightly

[SichangHe]

Seems related:

warning: unnecessary closure used with `bool::then`
   --> src/main.rs:323:34
    |
323 |           let import_percentages = (import_total > 0.0).then(|| Percentages {
    |  __________________________________^
324 | |             ok: import_ok * 100.0 / import_total,
325 | |             skip: import_skip * 100.0 / import_total,
326 | |             unrec: import_unrec * 100.0 / import_total,
327 | |             meh: import_meh * 100.0 / import_total,
328 | |             err: import_err * 100.0 / import_total,
329 | |         });
    | |__________^
    |
    = 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
help: use `then_some(..)` instead
    |