-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Make manual_is_variant_and to cover manual is_none_or
#16424
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
Conversation
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.
This needs an MSRV check. is_none_or is from 1.82 whereas is_some_and is from 1.70. You should also be checking for is_some_and || is_none as well.
|
All resolved. Thank you! |
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 can remove some redundant work by using something like:
let (none_recv, some_recv, some_arg) = match (&lhs.kind, &rhs.kind) {
(
ExprKind::MethodCall(sym::is_none, none_recv, [], ..),
ExprKind::MethodCall(sym::is_some, some_recv, [some_arg], ..),
) => (none_recv, some_recv, some_arg),
(
ExprKind::MethodCall(sym::is_some, some_recv, [some_arg], ..),
ExprKind::MethodCall(sym::is_none, none_recv, [], ..),
) => (none_recv, some_recv, some_arg),
_ => return,
};You also don't want to use method_call since it doesn't allow any of the arguments to be from an expansion. It really shouldn't ever be used.
184ff7c to
e4e5741
Compare
| ExprKind::Binary(op, lhs, rhs) if op.node == hir::BinOpKind::Or => { | ||
| manual_is_variant_and::check_or(cx, expr, lhs, rhs, self.msrv); | ||
| }, |
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.
This looks very awkward imo. Since this particular pattern (opt.is_none() || opt.is_some_and()) doesn't really fit into the methods group, maybe it would make sense to create a separate top-level file to put it into? What do you all think?
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.
I prefer to keep it here.
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.
When the lint crate is split either the whole lint will have to be in the methods pass or in a different pass. So this can stay as it is.
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.
Thank you.
Closes #16419
changelog: [
manual_is_variant_and] enhance to cover manualis_none_or