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

unnecessary_fold: .any and .all may not always be safe replacements of .fold #3351

Open
ghost opened this issue Oct 23, 2018 · 5 comments
Open
Labels
C-bug Category: Clippy is not doing the correct thing L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@ghost
Copy link

ghost commented Oct 23, 2018

Clippy version: 0.0.212
https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#unnecessary_fold

Both .any and .all are short-circuiting while .fold evaluates every element in the iterator, if the iterator incurs side effects these two methods will have different behavior than .fold.

For example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=893a485dc791c89d95d6ce6f744af9bf

@ghost ghost changed the title .any and .all may not always be safe replacements of .fold unnecessary_fold: .any and .all may not always be safe replacements of .fold Oct 27, 2018
@phansch phansch added C-bug Category: Clippy is not doing the correct thing L-suggestion Lint: Improving, adding or fixing lint suggestions labels Oct 27, 2018
@camsteffen
Copy link
Contributor

I just ran into this.

I think the short-circuiting methods (any and all) should be removed from this lint. If they are still wanted, they should be moved to a pedantic lint with known false positives.

At a minimum, the warning message should have a note like "this may change semantics" or "this may change side-effects".

@camsteffen
Copy link
Contributor

I suppose it may be possible to detect cases where there are side-effects?

@maartendeprez
Copy link

Hi, I also ran into this. Clippy suggests replacing .fold(false, |a,b| a || b) with .any(...), but applying the suggested fix would lead to subtle bugs in my code because of side effects in the iterator. I think this lint (in its current version) should at least be MaybeIncorrect and explain the differences.

@falsetru
Copy link

@dbeckwith
Copy link

Also just noticed this. I think it's good to mention any as a possible alternative to fold but it should be very clear about the difference in short-circuiting behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

5 participants