-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add partialeq_to_none lint #9288
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
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) soon. Please see the contribution instructions for more information. |
dca49be
to
0d95efe
Compare
Implementation looks fine. Two things before merging:
|
// We are only interested in comparisons between `Option` and a literal `Option::None`
let scrutinee = match (
is_none_ctor(left_side) && is_ty_option(right_side),
is_none_ctor(right_side) && is_ty_option(right_side),
) {
(true, false) => right_side,
(false, true) => left_side,
_ => return,
}; Do we even need the |
Also: |
You do need to check for an option type as You can use |
Moving from rust-clippy/tests/ui/manual_assert.rs Lines 19 to 21 in 5721ca9
Should I update the triggering code in the ui-tests, bless the results, or I don't feel strongly about the name, yet I think
which is correct. |
Normally adding For the lint name, use whichever one you prefer. |
I've had to make some changes to get the suggestions right (see the |
Looks good. You can squash the changes. |
2e66c86
to
657b0da
Compare
Squashed |
Thank you. @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
I'm hitting this lint for code of the form |
Perhaps we should add a macro check for the |
@trevyn Thanks for the comment. The lint is certainly wrong as it would hardcode whatever |
Don't lint literal `None` from expansion This addresses #9288 (comment): If the literal `None` is from expansion, we never lint. This is correct because e.g. replacing the call to `option_env!` with whatever that macro expanded to at the time of linting is certainly wrong. changelog: Don't lint [`partialeq_to_none`] for macro-expansions
Initial implementation of #9275, adding lint
partialeq_to_none
. This is my first time working onclippy
, so please review carefully.I'm unsure especially about the
Sugg
, as it covers the entireBinOp
, instead of just covering one of the sides and the operator (see the multi-line example). I was unsure if pinpointing the suggestion wouldn't be brittle...changelog: [
PARTIALEQ_TO_NONE
]: Initial commit