Add partialeq_to_none lint #9288

Conversation

Contributor

lukaslueg commented 16 days ago

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

All reactions

Collaborator

rust-highfive commented 16 days ago

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.

Contributor

Jarcho commented 14 days ago

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.

Contributor

Author

lukaslueg commented 14 days ago •

edited

          // 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?!

Contributor

Author

lukaslueg commented 14 days ago

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?

Contributor

Jarcho commented 14 days ago

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.

Contributor

Author

lukaslueg commented 14 days ago •

edited

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

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.

Contributor