Auto merge of #9288 - lukaslueg:partialeqnone, r=Jarcho

Add partialeq_to_none lint

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

CHANGELOG.md

@@ -3824,6 +3824,7 @@ Released 2018-09-13
 [`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params
 [`panicking_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_unwrap
 [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl
+[`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none
 [`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite
 [`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch
 [`possible_missing_comma`]: https://rust-lang.github.io/rust-clippy/master/index.html#possible_missing_comma

clippy_lints/src/lib.register_all.rs

@@ -266,6 +266,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP),
     LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
     LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL),
+    LintId::of(partialeq_to_none::PARTIALEQ_TO_NONE),
     LintId::of(precedence::PRECEDENCE),
     LintId::of(ptr::CMP_NULL),
     LintId::of(ptr::INVALID_NULL_PTR_USAGE),

clippy_lints/src/lib.register_lints.rs

@@ -454,6 +454,7 @@ store.register_lints(&[
     panic_unimplemented::UNIMPLEMENTED,
     panic_unimplemented::UNREACHABLE,
     partialeq_ne_impl::PARTIALEQ_NE_IMPL,
+    partialeq_to_none::PARTIALEQ_TO_NONE,
     pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE,
     pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF,
     path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE,

clippy_lints/src/lib.register_style.rs

@@ -100,6 +100,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
     LintId::of(operators::ASSIGN_OP_PATTERN),
     LintId::of(operators::OP_REF),
     LintId::of(operators::PTR_EQ),
+    LintId::of(partialeq_to_none::PARTIALEQ_TO_NONE),
     LintId::of(ptr::CMP_NULL),
     LintId::of(ptr::PTR_ARG),
     LintId::of(question_mark::QUESTION_MARK),

clippy_lints/src/lib.rs

@@ -332,6 +332,7 @@ mod overflow_check_conditional;
 mod panic_in_result_fn;
 mod panic_unimplemented;
 mod partialeq_ne_impl;
+mod partialeq_to_none;
 mod pass_by_ref_or_value;
 mod path_buf_push_overwrite;
 mod pattern_type_mismatch;
@@ -931,6 +932,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(invalid_utf8_in_unchecked::InvalidUtf8InUnchecked));
     store.register_late_pass(|| Box::new(std_instead_of_core::StdReexports::default()));
     store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed));
+    store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/partialeq_to_none.rs

@@ -0,0 +1,104 @@
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg, is_lang_ctor, peel_hir_expr_refs, peel_ref_operators, sugg,
+    ty::is_type_diagnostic_item,
+};
+use rustc_errors::Applicability;
+use rustc_hir::{BinOpKind, Expr, ExprKind, LangItem};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for binary comparisons to a literal `Option::None`.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// A programmer checking if some `foo` is `None` via a comparison `foo == None`
+    /// is usually inspired from other programming languages (e.g. `foo is None`
+    /// in Python).
+    /// Checking if a value of type `Option<T>` is (not) equal to `None` in that
+    /// way relies on `T: PartialEq` to do the comparison, which is unneeded.
+    ///
+    /// ### Example
+    /// ```rust
+    /// fn foo(f: Option<u32>) -> &'static str {
+    ///     if f != None { "yay" } else { "nay" }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn foo(f: Option<u32>) -> &'static str {
+    ///     if f.is_some() { "yay" } else { "nay" }
+    /// }
+    /// ```
+    #[clippy::version = "1.64.0"]
+    pub PARTIALEQ_TO_NONE,
+    style,
+    "Binary comparison to `Option<T>::None` relies on `T: PartialEq`, which is unneeded"
+}
+declare_lint_pass!(PartialeqToNone => [PARTIALEQ_TO_NONE]);
+
+impl<'tcx> LateLintPass<'tcx> for PartialeqToNone {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
+        // Skip expanded code, as we have no control over it anyway...
+        if e.span.from_expansion() {
+            return;
+        }
+
+        // If the expression is of type `Option`
+        let is_ty_option =
+            |expr: &Expr<'_>| is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr).peel_refs(), sym::Option);
+
+        // If the expression is a literal `Option::None`
+        let is_none_ctor = |expr: &Expr<'_>| {
+            matches!(&peel_hir_expr_refs(expr).0.kind,
+            ExprKind::Path(p) if is_lang_ctor(cx, p, LangItem::OptionNone))
+        };
+
+        let mut applicability = Applicability::MachineApplicable;
+
+        if let ExprKind::Binary(op, left_side, right_side) = e.kind {
+            // All other comparisons (e.g. `>= None`) have special meaning wrt T
+            let is_eq = match op.node {
+                BinOpKind::Eq => true,
+                BinOpKind::Ne => false,
+                _ => return,
+            };
+
+            // 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(left_side),
+            ) {
+                (true, false) => right_side,
+                (false, true) => left_side,
+                _ => return,
+            };
+
+            // Peel away refs/derefs (as long as we don't cross manual deref impls), as
+            // autoref/autoderef will take care of those
+            let sugg = format!(
+                "{}.{}",
+                sugg::Sugg::hir_with_applicability(cx, peel_ref_operators(cx, scrutinee), "..", &mut applicability)
+                    .maybe_par(),
+                if is_eq { "is_none()" } else { "is_some()" }
+            );
+
+            span_lint_and_sugg(
+                cx,
+                PARTIALEQ_TO_NONE,
+                e.span,
+                "binary comparison to literal `Option::None`",
+                if is_eq {
+                    "use `Option::is_none()` instead"
+                } else {
+                    "use `Option::is_some()` instead"
+                },
+                sugg,
+                applicability,
+            );
+        }
+    }
+}

tests/ui/ifs_same_cond.rs

@@ -32,9 +32,9 @@ fn ifs_same_cond() {
     };
 
     let mut v = vec![1];
-    if v.pop() == None {
+    if v.pop().is_none() {
         // ok, functions
-    } else if v.pop() == None {
+    } else if v.pop().is_none() {
     }
 
     if v.len() == 42 {

tests/ui/manual_assert.edition2018.fixed

@@ -17,7 +17,7 @@ fn main() {
     let c = Some(2);
     if !a.is_empty()
         && a.len() == 3
-        && c != None
+        && c.is_some()
         && !a.is_empty()
         && a.len() == 3
         && !a.is_empty()

tests/ui/manual_assert.edition2021.fixed

@@ -17,7 +17,7 @@ fn main() {
     let c = Some(2);
     if !a.is_empty()
         && a.len() == 3
-        && c != None
+        && c.is_some()
         && !a.is_empty()
         && a.len() == 3
         && !a.is_empty()

tests/ui/manual_assert.fixed

@@ -11,7 +11,7 @@ fn main() {
     let c = Some(2);
     if !a.is_empty()
         && a.len() == 3
-        && c != None
+        && c.is_some()
         && !a.is_empty()
         && a.len() == 3
         && !a.is_empty()

tests/ui/manual_assert.rs

@@ -17,7 +17,7 @@ fn main() {
     let c = Some(2);
     if !a.is_empty()
         && a.len() == 3
-        && c != None
+        && c.is_some()
         && !a.is_empty()
         && a.len() == 3
         && !a.is_empty()

tests/ui/partialeq_to_none.fixed

@@ -0,0 +1,62 @@
+// run-rustfix
+#![warn(clippy::partialeq_to_none)]
+
+struct Foobar;
+
+impl PartialEq<Option<()>> for Foobar {
+    fn eq(&self, _: &Option<()>) -> bool {
+        false
+    }
+}
+
+#[allow(dead_code)]
+fn foo(f: Option<u32>) -> &'static str {
+    if f.is_some() { "yay" } else { "nay" }
+}
+
+fn foobar() -> Option<()> {
+    None
+}
+
+fn bar() -> Result<(), ()> {
+    Ok(())
+}
+
+fn optref() -> &'static &'static Option<()> {
+    &&None
+}
+
+fn main() {
+    let x = Some(0);
+
+    let _ = x.is_none();
+    let _ = x.is_some();
+    let _ = x.is_none();
+    let _ = x.is_some();
+
+    if foobar().is_none() {}
+
+    if bar().ok().is_some() {}
+
+    let _ = Some(1 + 2).is_some();
+
+    let _ = { Some(0) }.is_none();
+
+    let _ = {
+        /*
+          This comment runs long
+        */
+        Some(1)
+    }.is_some();
+
+    // Should not trigger, as `Foobar` is not an `Option` and has no `is_none`
+    let _ = Foobar == None;
+
+    let _ = optref().is_none();
+    let _ = optref().is_some();
+    let _ = optref().is_none();
+    let _ = optref().is_some();
+
+    let x = Box::new(Option::<()>::None);
+    let _ = (*x).is_some();
+}

tests/ui/partialeq_to_none.rs

@@ -0,0 +1,62 @@
+// run-rustfix
+#![warn(clippy::partialeq_to_none)]
+
+struct Foobar;
+
+impl PartialEq<Option<()>> for Foobar {
+    fn eq(&self, _: &Option<()>) -> bool {
+        false
+    }
+}
+
+#[allow(dead_code)]
+fn foo(f: Option<u32>) -> &'static str {
+    if f != None { "yay" } else { "nay" }
+}
+
+fn foobar() -> Option<()> {
+    None
+}
+
+fn bar() -> Result<(), ()> {
+    Ok(())
+}
+
+fn optref() -> &'static &'static Option<()> {
+    &&None
+}
+
+fn main() {
+    let x = Some(0);
+
+    let _ = x == None;
+    let _ = x != None;
+    let _ = None == x;
+    let _ = None != x;
+
+    if foobar() == None {}
+
+    if bar().ok() != None {}
+
+    let _ = Some(1 + 2) != None;
+
+    let _ = { Some(0) } == None;
+
+    let _ = {
+        /*
+          This comment runs long
+        */
+        Some(1)
+    } != None;
+
+    // Should not trigger, as `Foobar` is not an `Option` and has no `is_none`
+    let _ = Foobar == None;
+
+    let _ = optref() == &&None;
+    let _ = &&None != optref();
+    let _ = **optref() == None;
+    let _ = &None != *optref();
+
+    let x = Box::new(Option::<()>::None);
+    let _ = None != *x;
+}

tests/ui/partialeq_to_none.stderr

@@ -0,0 +1,110 @@
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:14:8
+   |
+LL |     if f != None { "yay" } else { "nay" }
+   |        ^^^^^^^^^ help: use `Option::is_some()` instead: `f.is_some()`
+   |
+   = note: `-D clippy::partialeq-to-none` implied by `-D warnings`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:32:13
+   |
+LL |     let _ = x == None;
+   |             ^^^^^^^^^ help: use `Option::is_none()` instead: `x.is_none()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:33:13
+   |
+LL |     let _ = x != None;
+   |             ^^^^^^^^^ help: use `Option::is_some()` instead: `x.is_some()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:34:13
+   |
+LL |     let _ = None == x;
+   |             ^^^^^^^^^ help: use `Option::is_none()` instead: `x.is_none()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:35:13
+   |
+LL |     let _ = None != x;
+   |             ^^^^^^^^^ help: use `Option::is_some()` instead: `x.is_some()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:37:8
+   |
+LL |     if foobar() == None {}
+   |        ^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `foobar().is_none()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:39:8
+   |
+LL |     if bar().ok() != None {}
+   |        ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `bar().ok().is_some()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:41:13
+   |
+LL |     let _ = Some(1 + 2) != None;
+   |             ^^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `Some(1 + 2).is_some()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:43:13
+   |
+LL |     let _ = { Some(0) } == None;
+   |             ^^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `{ Some(0) }.is_none()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:45:13
+   |
+LL |       let _ = {
+   |  _____________^
+LL | |         /*
+LL | |           This comment runs long
+LL | |         */
+LL | |         Some(1)
+LL | |     } != None;
+   | |_____________^
+   |
+help: use `Option::is_some()` instead
+   |
+LL ~     let _ = {
+LL +         /*
+LL +           This comment runs long
+LL +         */
+LL +         Some(1)
+LL ~     }.is_some();
+   |
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:55:13
+   |
+LL |     let _ = optref() == &&None;
+   |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `optref().is_none()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:56:13
+   |
+LL |     let _ = &&None != optref();
+   |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `optref().is_some()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:57:13
+   |
+LL |     let _ = **optref() == None;
+   |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `optref().is_none()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:58:13
+   |
+LL |     let _ = &None != *optref();
+   |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `optref().is_some()`
+
+error: binary comparison to literal `Option::None`
+  --> $DIR/partialeq_to_none.rs:61:13
+   |
+LL |     let _ = None != *x;
+   |             ^^^^^^^^^^ help: use `Option::is_some()` instead: `(*x).is_some()`
+
+error: aborting due to 15 previous errors
+

tests/ui/same_functions_in_if_condition.rs

@@ -48,9 +48,9 @@ fn ifs_same_cond_fn() {
     }
 
     let mut v = vec![1];
-    if v.pop() == None {
+    if v.pop().is_none() {
         //~ ERROR ifs same condition
-    } else if v.pop() == None {
+    } else if v.pop().is_none() {
     }
 
     if v.len() == 42 {

tests/ui/same_functions_in_if_condition.stderr

@@ -50,14 +50,14 @@ LL |     if obj.method_arg(a) {
 error: this `if` has the same function call as a previous `if`
   --> $DIR/same_functions_in_if_condition.rs:53:15
    |
-LL |     } else if v.pop() == None {
-   |               ^^^^^^^^^^^^^^^
+LL |     } else if v.pop().is_none() {
+   |               ^^^^^^^^^^^^^^^^^
    |
 note: same as this
   --> $DIR/same_functions_in_if_condition.rs:51:8
    |
-LL |     if v.pop() == None {
-   |        ^^^^^^^^^^^^^^^
+LL |     if v.pop().is_none() {
+   |        ^^^^^^^^^^^^^^^^^
 
 error: this `if` has the same function call as a previous `if`
   --> $DIR/same_functions_in_if_condition.rs:58:15