Add unnecessary lazy evaluation lint

[bugadani]

changelog: Add [unnecessary_lazy_evaluations] lint that checks for usages of unwrap_or_else and similar functions that can be simplified.

Closes #5715

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[bugadani]

I can see at least one rename-fail still, so I'll do that soon, and I think I need to lint array indexing expressions as well, so this is still a work in progress.

[ghost]

Does this get linted?

some_option.unwrap_or_else(|| Some(very_expensive_func()))

[bugadani]

Thanks for noticing that. Currently it does get linted, but it certainly shouldn't. Also, a lot of tuples don't. However, that opens an other can of worms: how complex expressions should the lint catch? A 15-level nested tuple-tower should probably be left lazily evaluated, but I don't know where the limit should be.

For now, only member access, indexing, and a tower of Some/Ok/Err get linted. Or at least those should.

[Uriopass]

Since this is the dual of or_fun_call, shouldn't you lint what or_fun_call wouldn't ? It's the same logic but reversed

[bugadani]

@Uriopass what do you expect to get linted which doesn't currently?

The logic may be similar, but I can't have this lint unwrap things that are potentially expensive, while or_fun_call can re-wrap anything it wants. Maybe I need a closer look on that lint?

[Uriopass]

Ah ok I see, I thought that any constant value should be put with unwrap_or, but I agree that a [10000; u32] might be too big to be linted.

[bugadani]

but I agree that a [10000; u32] might be too big to be linted.

Hmm I should add a test for that.

[flip1995]

Ping from triage @bugadani. Is there anything we can help you with? If so, feel free to ping your reviewer anytime!

[bugadani]

This lint will not trigger for arrays but my only reasoning for that choice is that in the eager case (at least with some trivial test code, see https://godbolt.org/z/r3vPbv), the the generated machine code uses more memory (double the stack usage). I'm not sure if a test is necessary for this case, I'm leaning towards no right now.

@yaahc I think the PR is good to go.

[yaahc]

Looks good to go once the build failures are resolved @bugadani

[bugadani]

I've rebased to latest master, because I did not find any other way to trigger the build once more. The past failures were at the time when clippy itself wasn't building. Let's see how this goes

[yaahc]

Hmm, looks like it still failed @bugadani, you might need to run ./setup-toolchain.sh to repro the compiler errors locally. cc @flip1995

Edit: nvm I see you got it

[bugadani]

Thanks - hopefully this solves it. If not, figuring out local build again will take some time :)

Edit: Looks like some things changed since this was written. I'll try to sort things out tomorrow.

[bugadani]

Great, now this is conflicting with an other lint. Back to the drawing board I guess. Thank you both for your help :)

[bors]

☔ The latest upstream changes (presumably #5881) made this pull request unmergeable. Please resolve the merge conflicts.

[bugadani]

Hmm, sorry for being 3 weeks late. I took a look at the test errors, and I don't understand how the new lint can interfere with map_unwrap_or. Is the lazy_eval lint somehow enabled during testing of map_unwrap_or? Or is the failure unrelated?

[flip1995]

You can just allow this lint in the other test file. It doesn't interfere with the other lint, but lints some test cases in the other file, because they're accidentally written in a way that they trigger this new lint. This happens from time to time and allowing the new lint in an old test file is the easiest way to deal with this.

Also can you move the linting code to it's own module in the methods module, like some other lints, please?

[bugadani]

Thanks for explaining. So right now a piece of code can trigger two lints. Isn't it bad? I believe (I actually forgot) this one is written so that it can be automatically fixed, what does Clippy do in case the other one can be, as well?

I'm asking these because this lint is relatively low value so if this kind of two-for-one isn't desired, this PR can just be dropped.

[flip1995]

Since these are in the same module you can do something like

// If the other lint didn't trigger
if !lint_old_lint(...) {
    // trigger the new lint
    lint_new_lint()
}

[bugadani]

And what if these can, at least sometimes be sequentially applied?

[flip1995]

In that case the suggestion of the old lint triggering the new lint is not a problem (when applied the old lint doesn't trigger anymore and the new lint will get executed on the changed code). Only linting the same snippet of code twice at the same time is problematic.

[bugadani]

I actually can't believe CI passed. Thanks for the help and explanations!

[flip1995]

LGTM overall. Just some cleanup left to do.

Thanks for getting back to this and fixing everything so quickly!

[flip1995]

@bors r=flip1995,yaahc

Thanks!

[bors]

📌 Commit fc1e07e has been approved by flip1995,yaahc

[bors]

⌛ Testing commit fc1e07e with merge 8d0d89a...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995,yaahc
Pushing 8d0d89a to master...