unnecessary_lazy_evaluations warns about str::len

[jplatte]

Summary

str::len within an unwrap_or_else seems to cause unnecessary_lazy_evaluations to trigger even though it's a method call, not a constant which I assumed to be the only case where unnecessary_lazy_evaluations triggers.

Reproducer

I tried this code (Playground):

fn main() {
    let x = "str:with_separator";
    let first_part_pos = x.find(':').unwrap_or_else(|| x.len());
    dbg!(first_part_pos);
}

I expected to see this happen: No warnings

Instead, this happened:

warning: unnecessary closure used to substitute value for `Option::None`
 --> src/main.rs:3:26
  |
3 |     let first_part_pos = x.find(':').unwrap_or_else(|| x.len());
  |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `unwrap_or` instead: `x.find(':').unwrap_or(x.len())`
  |
  = 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

Version

rustc 1.59.0-nightly (e6b883c74 2021-12-08)
binary: rustc
commit-hash: e6b883c74f49f32cb5d1cbad3457f2b8805a4a38
commit-date: 2021-12-08
host: x86_64-unknown-linux-gnu
release: 1.59.0-nightly
LLVM version: 13.0.0

Additional Labels

No response

[Jarcho]

This is working as intended. str::len is effectively the same as reading a field, which has no reason to be lazily evaluated here.

[jplatte]

Is str::len special-cased? Which other method calls are deemed "effectively the same as reading a field"?

[Qwaz]

The logic is determined by expr_eagerness() function. Method names with len(), is_empty(), and as_*() are recognized.

[hellow554]

The same is true for Vec.len:

pub fn fo(x: Vec<usize>) -> usize {
    x.get(3).cloned().unwrap_or_else(|| x.len())
}

This is somewhat confusing, because function calls used to be wrapped in a closure.

I think at least (!) the error message should be extended to reflect the new behavoir or else people will get really confused (including me).

[camsteffen]

The logic is determined by expr_eagerness() function. Method names with len(), is_empty(), and as_*() are recognized.

Yeah considering that this is a warn-by-default lint, I would consider changing that case to NoChange (allow the coder's preference). I personally like making function calls lazy as a simple rule.

[mlodato517]

Is this eagerness check also present for "simple" match statements like in this? I imagine so because it handles function calls in the match arms but just making sure!

Edit: Oh maybe the block for match is here which doesn't return self.eagerness = Lazy so maybe that makes sense? Super guessing just from the names of things 😅

[hellow554]

What about determining if that function is constand only linting them?

[TethysSvensson]

I love the idea of having this heuristic, even if it does make it a bit harder to write clippy-compliant code "by hand".

However I also agree that the introduction of the heuristic was handled somewhat sub-optimal and has at least introduced quite a bit of unnecessary churn in our code-base. This was from a combination of the confusing error message and the problem outlined in this comment on the relevant pull request:

[lopopolo]

If we are to de-closure/de-combinator clippy's suggestion, I think the code would end up looking something like this:

fn main() {
    let x = "str:with_separator";
    let default = x.len();
    let first_part_pos = if let Some(first_part_pos) = x.find(':') {
        first_part_pos
    } else {
        default
    };
    dbg!(first_part_pos);
}

If I were writing the if let ... else version by hand, I'd probably make it look like this:

fn main() {
    let x = "str:with_separator";
    let first_part_pos = if let Some(first_part_pos) = x.find(':') {
        first_part_pos
    } else {
        x.len()
    };
    dbg!(first_part_pos);
}

I think the original sample with the closure given in this issue looks closer to how I'd write this same code with if let ... else and would love if this on-by-default lint would accept the code which has the closure.