-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: new lint for and_then
when returning Option or Result
#14051
base: master
Are you sure you want to change the base?
Conversation
r? @Manishearth rustbot has assigned @Manishearth. Use |
bit swamped rn |
Could not assign reviewer from: |
r? clippy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Happy to merge when Samuel approves 👍 |
thanks for the comments! I've incorporated the suggestions :) |
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):
I've started a FCP thread on Zulip. |
Also, this work could probably extend to such expressions placed after an explicit |
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) |
48e4fe8
to
a2bcb11
Compare
done :) |
@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 |
Ok, should I revert the lint fix to src files too? |
Yes, I guess it would make sense. |
a2bcb11
to
5850083
Compare
5850083
to
de1c90f
Compare
close #6436
changelog: add new return_and_then lint
Additional src files changed to reflect the lint.