New lint [manual_try_fold]
#11012
New lint [manual_try_fold]
#11012
Conversation
|
r? @xFrednet (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
@blyxyas would you mind reviewing this PR? :) |
|
Yeah, will review it 🐉 |
Centri3
force-pushed
the
manual_try_fold
branch
2 times, most recently
from
44a2d61 to
80447ad
Compare
| /// ``` | ||
| #[clippy::version = "1.72.0"] | ||
| pub MANUAL_TRY_FOLD, | ||
| perf, |
There was a problem hiding this comment.
IDK, if perf is too strict, if the lint already has a know problem. Or do you mean with doesn't take into account whether a function the implementation of Try?
If not, I think we should do some tests on more creates or start in pedantic first
There was a problem hiding this comment.
The "known problem" isn't really an issue, imo. The known problem is referring to something like
x.iter().fold(Some(1), |mut acc, i| if let Some(acc) = acc { acc.checked_add(i) } else { println!("it's none!"); Some(1) });(pseudocode)
This code is not really possible with try_fold.
But something like
x.iter().fold(Some(1), |mut acc, i| acc?.checked_add(i));(pseudocode again)
Can be changed to try_fold just fine, as it does nothing in the None case.
There are very few cases where that's the desired behavior, as it doesn't change the outcome at all. You can short-circuit there and it changes nothing. Unless the programmer explicitly handles the None case, doing something with it, it's just a performance gain
There was a problem hiding this comment.
I also agree that, if this behaviour is well-documented (as I think it is, as it appears in the std documentation and in the lint's documentation) it doesn't really represent an issue.
Also, I don't think I've ever seen a fold that handles a None case, but I'll make a Github regex search to confirm how many times does this happen
There was a problem hiding this comment.
Yeah, I'm just a more cautious when it comes to warn-by-default lints. I would really like nightly lints at some point, but first I finally want to stabilize lint reasons.
I believe it should be fine :)
There was a problem hiding this comment.
Can be changed to try_fold just fine, as it does nothing in the None case.
There are very few cases where that's the desired behavior, as it doesn't change the outcome at all. You can short-circuit there and it changes nothing. Unless the programmer explicitly handles the None case, doing something with it, it's just a performance gain
In that specific case, sure. But sometimes fold is used for things that are fallible, but you don't want it to short-circuit.
Example:
self.public_key
.public_subkeys
.iter()
.filter(|sub_key| {
if sub_key.key_id().as_ref() == key_id.as_ref() {
log::trace!(
"Found a matching key id {:?} == {:?}",
sub_key.key_id(),
key_id
);
true
} else {
log::trace!("Not the one we want: {:?}", sub_key);
false
}
})
.fold(
Err(Error::KeyNotFoundError {
key_ref: format!("{:?}", key_id),
}),
|previous_res, sub_key| {
if previous_res.is_err() {
log::trace!("Test next candidate subkey");
signature.verify(sub_key, &mut data).map_err(|e| {
Error::VerificationError {
source: Box::new(e),
key_ref: format!("{:?}", sub_key.key_id()),
}
})
} else {
log::trace!("Signature already verified, nop");
Ok(())
}
},
)This code initializes with an error of "KeyNotFoundError", and replaces it with an error of "VerificationError" after a subkey has been tried unsuccessfully, but still tries all of the subkeys before returning an error. If there's a successful result it short-circuits, but in the Ok case rather than the error case. So the suggestion to use try_fold would never work.
This might be rare enough to not be worth worrying about - I'm not suggesting that the lint should be reverted necessarily. But the try_fold suggestion was misleading, and I'm going to need to mark the lint ignored or rewrite the code.
|
☔ The latest upstream changes (presumably #11030) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #11020) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
1 similar comment
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Closes #10208
changelog: New lint [
manual_try_fold]#11012