feat: new lint for and_then when returning Option or Result

[aaron-ang]

close #6436

changelog: add new return_and_then lint

Additional src files changed to reflect the lint.

[rustbot]

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Manishearth]

r? @samueltardieu

bit swamped rn

[rustbot]

Could not assign reviewer from: samueltardieu.
User(s) samueltardieu are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

[Manishearth]

r? clippy

[samueltardieu]

You need to check that the expression you split in two parts doesn't use references to temporaries between those parts.

For example, the following code will fail

        String::from("<BOOM>")
            .strip_prefix("<")
            .and_then(|s| s.strip_suffix(">").map(String::from))

as the suggestion to use

        let s = String::from("<BOOM>")
            .strip_prefix("<")?;
        s.strip_suffix(">").map(String::from)

will not pass the borrow checker.

Maybe you could, after you check that the receiver is Option or Result, use something like:

    if let ty::Adt(_, args) = recv_type.kind()
        && args.iter().next().and_then(GenericArg::as_type).is_some_and(Ty::is_ref)
        && for_each_unconsumed_temporary(cx, recv, |_| ControlFlow::Break(())).is_break()
    {
        return;
    }

I've not checked if it would cover all the cases, as the lambda might not be called on non droppable types according to the comment on for_each_unconsumed_temporary(), but in practice it seems to work well.

[Centri3]

Happy to merge when Samuel approves 👍

[aaron-ang]

You need to check that the expression you split in two parts doesn't use references to temporaries between those parts.

For example, the following code will fail

        String::from("<BOOM>")
            .strip_prefix("<")
            .and_then(|s| s.strip_suffix(">").map(String::from))

as the suggestion to use

        let s = String::from("<BOOM>")
            .strip_prefix("<")?;
        s.strip_suffix(">").map(String::from)

will not pass the borrow checker.

Maybe you could, after you check that the receiver is Option or Result, use something like:

    if let ty::Adt(_, args) = recv_type.kind()
        && args.iter().next().and_then(GenericArg::as_type).is_some_and(Ty::is_ref)
        && for_each_unconsumed_temporary(cx, recv, |_| ControlFlow::Break(())).is_break()
    {
        return;
    }

I've not checked if it would cover all the cases, as the lambda might not be called on non droppable types according to the comment on for_each_unconsumed_temporary(), but in practice it seems to work well.

thanks for the comments! I've incorporated the suggestions :)

[samueltardieu]

This is starting to look very good. Could you please squash this? The ideal would be two commits (so that other people can easily review the lint code and its effects separately):

• one commit which implements the lint.
• one commit which applies it to Clippy sources.

I've started a FCP thread on Zulip.

[samueltardieu]

Also, this work could probably extend to such expressions placed after an explicit return as well, but this can be done in another PR.

[aaron-ang]

Also, this work could probably extend to such expressions placed after an explicit return as well, but this can be done in another PR.

wouldn't https://rust-lang.github.io/rust-clippy/master/index.html#needless_return catch that?

[samueltardieu]

wouldn't https://rust-lang.github.io/rust-clippy/master/index.html#needless_return catch that?

I'm specifically talking about a non-needless (early) return, i.e. a return which is not the last expression of a function.

[aaron-ang]

This is starting to look very good. Could you please squash this? The ideal would be two commits (so that other people can easily review the lint code and its effects separately):

• one commit which implements the lint.
• one commit which applies it to Clippy sources.

I've started a FCP thread on Zulip.

done :)

[samueltardieu]

@aaron-ang As you may have seen, there seem to be a variety of opinions concerning this lint: are both forms equally acceptable, is one or the other the preferred one, etc.

Could you move the lint to the restriction category, so that people will be free to opt-in for this lint?

[aaron-ang]

@aaron-ang As you may have seen, there seem to be a variety of opinions concerning this lint: are both forms equally acceptable, is one or the other the preferred one, etc.

Could you move the lint to the restriction category, so that people will be free to opt-in for this lint?

Ok, should I revert the lint fix to src files too?

[samueltardieu]

Ok, should I revert the lint fix to src files too?

Yes, I guess it would make sense.

[aaron-ang]

@samueltardieu I have made the changes. Is this gd to merge?

[samueltardieu]

For me it's ok if @Centri3 agrees. I would just change the commit message, we don't use conventional commits in Clippy, look at other commit messages for inspiration.