New lint [manual_try_fold]

[Centri3]

Closes #10208

---

changelog: New lint [manual_try_fold]
#11012

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[xFrednet]

@blyxyas would you mind reviewing this PR? :)

[blyxyas]

Yeah, will review it 🐉

[blyxyas]

Just too small nits, I couldn't see anything else wrong. Thanks! ❤️

[blyxyas]

Just a minor documentation change and I think this is done!

[xFrednet]

TIL about try_fold ^^

I have a few tiny NITs and a question. Then it should be ready for master :)

[xFrednet]

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

[Centri3]

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

[blyxyas]

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

[xFrednet]

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 :)

[dralley]

@Centri3

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.

[bors]

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

[bors]

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

[xFrednet]

LGTM, thank you for the new lint

And review @blyxyas

@bors r=blyxyas,xFrednet

[bors]

📌 Commit cb5d7e3 has been approved by blyxyas,xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit cb5d7e3 with merge 37f4c17...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas,xFrednet
Pushing 37f4c17 to master...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas,xFrednet
Pushing 37f4c17 to master...