Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Substitute logical folds to any() or all() #6827

Closed
totikom opened this issue Mar 2, 2021 · 7 comments · Fixed by #7133
Closed

Substitute logical folds to any() or all() #6827

totikom opened this issue Mar 2, 2021 · 7 comments · Fixed by #7133
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group

Comments

@totikom
Copy link

totikom commented Mar 2, 2021

What it does

Some uses of .fold() can be substituted to existing methods on iterators like .all() and .any().

Categories (optional)

  • Kind: clippy::complexity

What is the advantage of the recommended code over the original code

  • Dramatically improves code readability
  • In average work faster, as .any() and .all() are short-circuiting (see the docs)

Drawbacks

None.

Examples

all:

some_iterator.fold(true, | acc, x| {
    acc & x
});

Could be written as:

some_iterator.all(|x| x);

any:

some_iterator.fold(false, | acc, x| {
    acc | x
});

Could be written as:

some_iterator.any(|x| x);

More over, rustfmt will remove curly brackets in the original code, so closure in the .any() case will be formatted to completely unreadable and confusing |acc, x| acc | x

@totikom totikom added the A-lint Area: New lints label Mar 2, 2021
@totikom
Copy link
Author

totikom commented Mar 2, 2021

Actually, the things are a bit more complicated.
.fold() will always consume the whole iterator, while .all() (and .any()) exits after the first false (or true).
If the iterator has side-effects, the behavior of the original and the linted code may differ.

Maybe it should be moved to clippy::perf with warning about side-effects.

@totikom
Copy link
Author

totikom commented Mar 2, 2021

And one more note:
It is highly possible, that the iterator over bools was obtained via .map().
In that case:

some_iterator.map(| item | {
// a closure which returns bool
})
.fold(false, | acc, x| {
    acc | x
});

Could be written as:

some_iterator.any(| item | {
// a closure which returns bool
});

Or:

some_iterator.map(| item | {
// a closure which returns bool
})
.fold(true, | acc, x| {
    acc & x
});

Could be written as:

some_iterator.all(| item | {
// a closure which returns bool
});

@camsteffen
Copy link
Contributor

Isn't this unnecessary_fold?

@totikom
Copy link
Author

totikom commented Mar 3, 2021

Actually, it is. Didn't checked that.
I don't understand, if this lint is currently active?
I've created this issue, because clippy didn't show any warnings even with pedantic flag.

@camsteffen
Copy link
Contributor

camsteffen commented Mar 4, 2021

You are using bitwise operators & and | instead of logical operators && and ||. I'll keep this open since we probably should lint that. We could either lint any unnecessary bitwise operators, or just enhance unnecessary_fold, or both! But I think I'd prefer just adding a new lint bitwise_bool.

@totikom
Copy link
Author

totikom commented Mar 4, 2021

Yes, bitwise_bool sounds like an important lint.

@camsteffen camsteffen added good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group labels Mar 4, 2021
@arya-k
Copy link
Contributor

arya-k commented Apr 6, 2021

@rustbot claim

bors added a commit that referenced this issue May 17, 2021
Add `needless_bitwise_bool` lint

fixes #6827
fixes #1594

changelog: Add ``[`needless_bitwise_bool`]`` lint

Creates a new `bitwise_bool` lint to convert `x & y` to `x && y` when both `x` and `y` are booleans. I also had to adjust thh `needless_bool` lint slightly, and fix a couple failing dogfood tests. I made it a correctness lint as per flip1995's comment [here](#3385 (comment)), from a previous WIP attempt at this lint.
bors added a commit that referenced this issue May 17, 2021
Add `needless_bitwise_bool` lint

fixes #6827
fixes #1594

changelog: Add ``[`needless_bitwise_bool`]`` lint

Creates a new `bitwise_bool` lint to convert `x & y` to `x && y` when both `x` and `y` are booleans. I also had to adjust thh `needless_bool` lint slightly, and fix a couple failing dogfood tests. I made it a correctness lint as per flip1995's comment [here](#3385 (comment)), from a previous WIP attempt at this lint.
@bors bors closed this as completed in a3223af May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants