Improve heuristic for eagerness suggestion

[Jarcho]

Still to be done:

• a more complete list of cheap functions
• a way to limit the complexity of cheap expressions

changelog: Improve heuristics for or_fun_call and unnecessary_lazy_evaluations

[rust-highfive]

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

[camsteffen]

I don't really love the idea of teaching Clippy an exhaustive list of cheap functions in std.

[Manishearth]

Hmm, yeah, I'm unsure. @rust-lang/clippy ?

[xFrednet]

I think it would be better to have it consistent for all functions, regardless of how simple they are. It could be confusing for newcomers why some functions are accepted and some not.

Also, I'm not sure how such simple closures are handled in rustc, but they could be stored as function pointers, which would still make them cheaper than calling them beforehand. Function calls are still more expensive than passing a function pointer as an argument, as far as I know.

The PR description states:

Fix false positives on or_fun_call for cheap std functions.

Have we ever had FP reports like these? 🤔

[Jarcho]

Practically speaking it isn't a performance issue. Everything will be inlined anyways (don't know where debug builds stand on this).

Have we ever had FP reports like these? 🤔

No reported issues, but people have mentioned it being dumb. Changing foo.unwrap_or(bar.len()) to foo.unwrap_or_else(|| bar.len()) just isn't a good suggestion.

I don't really love the idea of teaching Clippy an exhaustive list of cheap functions in std.

I agree it's an unwieldy list. Would a shortlist of the worst offenders be ok? Off the top of my head would be len and is_empty and various conversions such as as_slice and as_ref.

Do we have access to the mir for external crates from clippy? Then we could just look at the function and check if it's trivial or not.

[Manishearth]

Then we could just look at the function and check if it's trivial or not.

This does get close to the kinds of analyses we'd like to avoid having to do.

But I'm not opposed to an allowlist of functions, I just want to be sure that's the path we want to go down.

[camsteffen]

How bout a blanket allow on names as_*, len and is_empty? All these names are cheap by convention, right?

[Manishearth]

That works

[Jarcho]

This does get close to the kinds of analyses we'd like to avoid having to do.

Is there a particular reason to avoid doing so in this case?

How bout a blanket allow on names as_*, len and is_empty? All these names are cheap by convention, right?

That would remove almost every case I've had it trigger on. Should it be limited to std functions just to be safe? I've seen len involve iterators before. (e.g. self.chunks.iter().map(|x| x.len()).sum())

[Manishearth]

Is there a particular reason to avoid doing so in this case?

Global analyses tend to be more expensive and brittle

[bors]

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

[Jarcho]

Removed the list of functions. Now there's only a few heuristics:

• const functions where all arguments are constant expressions will suggest eager evaluation
• Methods with the name len is_empty or as_* will suggest eager evaluation if they are from std, or not to change the order otherwise.
• If the type has no fields other than generic types and the only constraints on those types are marker traits, then methods taking self or &self and returning bool will not suggest changing the evaluation order.

[camsteffen]

const functions where all arguments are constant expressions will suggest eager evaluation

Does it work to just check that the entire function call is_const_expr?

If the type has no fields other than generic types and the only constraints on those types are marker traits, then methods taking self or &self and returning bool will not suggest changing the evaluation order.

Hmm could you explain how you came up with this?

[Jarcho]

Does it work to just check that the entire function call is_const_expr?

That would also work. The check should be moved into expr_eagerness.

Hmm could you explain how you came up with this?

The goal with this is to catch the helper functions for enums to check for specific variants. The various constraints on the fields are to limit the function to only checking the variants. Technically the function could do anything, it's just very unlikely given the constraints applied. The name could be limited to is_* as that's the convention used for such functions.

[Manishearth]

going to r? @camsteffen on this one ; I mostly think it's okay but want someone else to make the call 😄

[bors]

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

[Jarcho]

Anything else for the review?

[camsteffen]

Looks like everything from the last review is resolved. Needs a rebase.

[bors]

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

[camsteffen]

LGTM after another rebase, thanks!

[camsteffen]

Can you remove WIP from the PR title? I could do it but I just want to be sure.

[Jarcho]

I completely forgot that was still there.

[camsteffen]

@bors r+

[bors]

📌 Commit 2938ffd has been approved by camsteffen

[bors]

⌛ Testing commit 2938ffd with merge 5b1b65b...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 5b1b65b to master...

[cgwalters]

I think this was a good change, but it did create a situation where old clippy warns about new code, new clippy warns about old code; we hit this in ostreedev/ostree-rs-ext#256 (comment)

We pin to a specific version for linting in our CI, but not every developer uses the same rustc.

For future changes like this, I think it'd be nice to have the new clippy not warn on previously accepted code - at least for a decent period of time.