Add partialeq_to_none lint

[lukaslueg]

Initial implementation of #9275, adding lint partialeq_to_none. This is my first time working on clippy, so please review carefully.

I'm unsure especially about the Sugg, as it covers the entire BinOp, 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

[rust-highfive]

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.

[Jarcho]

Implementation looks fine. Two things before merging:

• Should this be a style lint or a pedantic lint. I'm leaning towards style.
• I don't particularly like PartialEq in the lint name. There's nothing really wrong with it and it's not misleading, but it seems like unnecessary detail. eq_none might be better.

[lukaslueg]

          // 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 is_ty_option() (don't mind the right_side bug 👀😅) to rule out comparisons like 1u32 == None? AFAICS, as a LateLintPass comes after type-resolution, non-applicable comparisons are already ruled out?!

[lukaslueg]

Also: x != &None does not trigger because the Expr &None is not is_lang_ctor(.., .., LangItem::OptionNone). How to peel off the references from a QPath akin to Ty::peel_refs?

[Jarcho]

You do need to check for an option type as PartialEq can be implemented across types.

You can use clippy_utils::peel_hir_expr_refs.

[lukaslueg]

Moving from nursery to style enables the lint by default, which causes it to get triggered in other ui-tests. For instance:

rust-clippy/tests/ui/manual_assert.rs

Lines 19 to 21 in 5721ca9

	&& a.len() == 3
	&& c != None
	&& !a.is_empty()

Should I update the triggering code in the ui-tests, bless the results, or allow(clippy::partialeq_to_none) in those ui-tests?

I don't feel strongly about the name, yet I think partialeq is more precise because the lint covers both eq and ne from the PartialEq trait (while recognizing that up to now we don't cover fully qualified syntax).

lintcheck has one extra warning wrt to

https://github.com/rust-lang/cargo/blob/0ced33b1656b0dbe3dece629a77768db74ed5bd7/src/cargo/ops/cargo_new.rs#L500

which is correct.

[Jarcho]

Normally adding allow in tests is the better approach. It's possible the change can cause the test to no longer function.

For the lint name, use whichever one you prefer.

[lukaslueg]

I've had to make some changes to get the suggestions right (see the Box-example). LGTM

[Jarcho]

Looks good. You can squash the changes.

[lukaslueg]

Squashed

[Jarcho]

Thank you. @bors r+

[bors]

📌 Commit 657b0da has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit 657b0da with merge 3af9072...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 3af9072 to master...

[trevyn]

I'm hitting this lint for code of the form foo == option_env!("BAR"), is that intended?

[llogiq]

Perhaps we should add a macro check for the None expr.

[lukaslueg]

@trevyn Thanks for the comment. The lint is certainly wrong as it would hardcode whatever option_env expanded to at that time.