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: