Auto merge of #6226 - Urcra:master, r=flip1995

Add lint for comparing to empty slices instead of using .is_empty()

Hey first time making a clippy lint

I added the implementation of the lint the `len_zero` since it shared a lot of the code, I would otherwise have to rewrite. Just tell me if the lint should use it's own file instead

changelog: Add lint for comparing to empty slices

Fixes #6217

CHANGELOG.md

@@ -1665,6 +1665,7 @@ Released 2018-09-13
 [`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
 [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
 [`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
+[`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
 [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator
 [`create_dir`]: https://rust-lang.github.io/rust-clippy/master/index.html#create_dir
 [`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute

clippy_lints/src/len_zero.rs

@@ -68,7 +68,44 @@
     "traits or impls with a public `len` method but no corresponding `is_empty` method"
 }
 
-declare_lint_pass!(LenZero => [LEN_ZERO, LEN_WITHOUT_IS_EMPTY]);
+declare_clippy_lint! {
+    /// **What it does:** Checks for comparing to an empty slice such as "" or [],`
+    /// and suggests using `.is_empty()` where applicable.
+    ///
+    /// **Why is this bad?** Some structures can answer `.is_empty()` much faster
+    /// than checking for equality. So it is good to get into the habit of using
+    /// `.is_empty()`, and having it is cheap.
+    /// Besides, it makes the intent clearer than a manual comparison in some contexts.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```ignore
+    /// if s == "" {
+    ///     ..
+    /// }
+    ///
+    /// if arr == [] {
+    ///     ..
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```ignore
+    /// if s.is_empty() {
+    ///     ..
+    /// }
+    ///
+    /// if arr.is_empty() {
+    ///     ..
+    /// }
+    /// ```
+    pub COMPARISON_TO_EMPTY,
+    style,
+    "checking `x == \"\"` or `x == []` (or similar) when `.is_empty()` could be used instead"
+}
+
+declare_lint_pass!(LenZero => [LEN_ZERO, LEN_WITHOUT_IS_EMPTY, COMPARISON_TO_EMPTY]);
 
 impl<'tcx> LateLintPass<'tcx> for LenZero {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
@@ -221,6 +258,8 @@ fn check_cmp(cx: &LateContext<'_>, span: Span, method: &Expr<'_>, lit: &Expr<'_>
         }
 
         check_len(cx, span, method_path.ident.name, args, &lit.node, op, compare_to)
+    } else {
+        check_empty_expr(cx, span, method, lit, op)
     }
 }
 
@@ -258,6 +297,42 @@ fn check_len(
     }
 }
 
+fn check_empty_expr(cx: &LateContext<'_>, span: Span, lit1: &Expr<'_>, lit2: &Expr<'_>, op: &str) {
+    if (is_empty_array(lit2) || is_empty_string(lit2)) && has_is_empty(cx, lit1) {
+        let mut applicability = Applicability::MachineApplicable;
+        span_lint_and_sugg(
+            cx,
+            COMPARISON_TO_EMPTY,
+            span,
+            "comparison to empty slice",
+            &format!("using `{}is_empty` is clearer and more explicit", op),
+            format!(
+                "{}{}.is_empty()",
+                op,
+                snippet_with_applicability(cx, lit1.span, "_", &mut applicability)
+            ),
+            applicability,
+        );
+    }
+}
+
+fn is_empty_string(expr: &Expr<'_>) -> bool {
+    if let ExprKind::Lit(ref lit) = expr.kind {
+        if let LitKind::Str(lit, _) = lit.node {
+            let lit = lit.as_str();
+            return lit == "";
+        }
+    }
+    false
+}
+
+fn is_empty_array(expr: &Expr<'_>) -> bool {
+    if let ExprKind::Array(ref arr) = expr.kind {
+        return arr.is_empty();
+    }
+    false
+}
+
 /// Checks if this type has an `is_empty` method.
 fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     /// Gets an `AssocItem` and return true if it matches `is_empty(self)`.

clippy_lints/src/lib.rs

@@ -614,6 +614,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &large_const_arrays::LARGE_CONST_ARRAYS,
         &large_enum_variant::LARGE_ENUM_VARIANT,
         &large_stack_arrays::LARGE_STACK_ARRAYS,
+        &len_zero::COMPARISON_TO_EMPTY,
         &len_zero::LEN_WITHOUT_IS_EMPTY,
         &len_zero::LEN_ZERO,
         &let_if_seq::USELESS_LET_IF_SEQ,
@@ -1365,6 +1366,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&int_plus_one::INT_PLUS_ONE),
         LintId::of(&large_const_arrays::LARGE_CONST_ARRAYS),
         LintId::of(&large_enum_variant::LARGE_ENUM_VARIANT),
+        LintId::of(&len_zero::COMPARISON_TO_EMPTY),
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),
         LintId::of(&len_zero::LEN_ZERO),
         LintId::of(&let_underscore::LET_UNDERSCORE_LOCK),
@@ -1591,6 +1593,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&functions::RESULT_UNIT_ERR),
         LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
         LintId::of(&inherent_to_string::INHERENT_TO_STRING),
+        LintId::of(&len_zero::COMPARISON_TO_EMPTY),
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),
         LintId::of(&len_zero::LEN_ZERO),
         LintId::of(&literal_representation::INCONSISTENT_DIGIT_GROUPING),

src/lintlist/mod.rs

@@ -298,6 +298,13 @@
         deprecation: None,
         module: "comparison_chain",
     },
+    Lint {
+        name: "comparison_to_empty",
+        group: "style",
+        desc: "checking `x == \"\"` or `x == []` (or similar) when `.is_empty()` could be used instead",
+        deprecation: None,
+        module: "len_zero",
+    },
     Lint {
         name: "copy_iterator",
         group: "pedantic",

tests/ui/comparison_to_empty.fixed

@@ -0,0 +1,23 @@
+// run-rustfix
+
+#![warn(clippy::comparison_to_empty)]
+
+fn main() {
+    // Disallow comparisons to empty
+    let s = String::new();
+    let _ = s.is_empty();
+    let _ = !s.is_empty();
+
+    let v = vec![0];
+    let _ = v.is_empty();
+    let _ = !v.is_empty();
+
+    // Allow comparisons to non-empty
+    let s = String::new();
+    let _ = s == " ";
+    let _ = s != " ";
+
+    let v = vec![0];
+    let _ = v == [0];
+    let _ = v != [0];
+}

tests/ui/comparison_to_empty.rs

@@ -0,0 +1,23 @@
+// run-rustfix
+
+#![warn(clippy::comparison_to_empty)]
+
+fn main() {
+    // Disallow comparisons to empty
+    let s = String::new();
+    let _ = s == "";
+    let _ = s != "";
+
+    let v = vec![0];
+    let _ = v == [];
+    let _ = v != [];
+
+    // Allow comparisons to non-empty
+    let s = String::new();
+    let _ = s == " ";
+    let _ = s != " ";
+
+    let v = vec![0];
+    let _ = v == [0];
+    let _ = v != [0];
+}

tests/ui/comparison_to_empty.stderr

@@ -0,0 +1,28 @@
+error: comparison to empty slice
+  --> $DIR/comparison_to_empty.rs:8:13
+   |
+LL |     let _ = s == "";
+   |             ^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()`
+   |
+   = note: `-D clippy::comparison-to-empty` implied by `-D warnings`
+
+error: comparison to empty slice
+  --> $DIR/comparison_to_empty.rs:9:13
+   |
+LL |     let _ = s != "";
+   |             ^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!s.is_empty()`
+
+error: comparison to empty slice
+  --> $DIR/comparison_to_empty.rs:12:13
+   |
+LL |     let _ = v == [];
+   |             ^^^^^^^ help: using `is_empty` is clearer and more explicit: `v.is_empty()`
+
+error: comparison to empty slice
+  --> $DIR/comparison_to_empty.rs:13:13
+   |
+LL |     let _ = v != [];
+   |             ^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!v.is_empty()`
+
+error: aborting due to 4 previous errors
+