Merge pull request #2730 from NiekGr/len_zero_use_of_one

Update len_zero to handle comparisions with one

clippy_lints/src/len_zero.rs

@@ -1,7 +1,7 @@
-use rustc::lint::*;
 use rustc::hir::def_id::DefId;
-use rustc::ty;
 use rustc::hir::*;
+use rustc::lint::*;
+use rustc::ty;
 use std::collections::HashSet;
 use syntax::ast::{Lit, LitKind, Name};
 use syntax::codemap::{Span, Spanned};
@@ -81,8 +81,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LenZero {
 
         if let ExprBinary(Spanned { node: cmp, .. }, ref left, ref right) = expr.node {
             match cmp {
-                BiEq => check_cmp(cx, expr.span, left, right, ""),
+                BiEq => {
-                BiGt | BiNe => check_cmp(cx, expr.span, left, right, "!"),
+                    check_cmp(cx, expr.span, left, right, "", 0); // len == 0
+                    check_cmp(cx, expr.span, right, left, "", 0); // 0 == len
+                },
+                BiNe => {
+                    check_cmp(cx, expr.span, left, right, "!", 0); // len != 0
+                    check_cmp(cx, expr.span, right, left, "!", 0); // 0 != len
+                },
+                BiGt => {
+                    check_cmp(cx, expr.span, left, right, "!", 0); // len > 0
+                    check_cmp(cx, expr.span, right, left, "", 1); // 1 > len
+                },
+                BiLt => {
+                    check_cmp(cx, expr.span, left, right, "", 1); // len < 1
+                    check_cmp(cx, expr.span, right, left, "!", 0); // 0 < len
+                },
+                BiGe => check_cmp(cx, expr.span, left, right, "!", 1), // len <= 1
+                BiLe => check_cmp(cx, expr.span, right, left, "!", 1), // 1 >= len
                 _ => (),
             }
         }
@@ -168,40 +184,45 @@ fn check_impl_items(cx: &LateContext, item: &Item, impl_items: &[ImplItemRef]) {
                 cx,
                 LEN_WITHOUT_IS_EMPTY,
                 item.span,
-                &format!("item `{}` has a public `len` method but {} `is_empty` method", ty, is_empty),
+                &format!(
+                    "item `{}` has a public `len` method but {} `is_empty` method",
+                    ty, is_empty
+                ),
             );
         }
     }
 }
 
-fn check_cmp(cx: &LateContext, span: Span, left: &Expr, right: &Expr, op: &str) {
+fn check_cmp(cx: &LateContext, span: Span, method: &Expr, lit: &Expr, op: &str, compare_to: u32) {
-    // check if we are in an is_empty() method
+    if let (&ExprMethodCall(ref method_path, _, ref args), &ExprLit(ref lit)) = (&method.node, &lit.node) {
-    if let Some(name) = get_item_name(cx, left) {
+        // check if we are in an is_empty() method
-        if name == "is_empty" {
+        if let Some(name) = get_item_name(cx, method) {
-            return;
+            if name == "is_empty" {
+                return;
+            }
         }
-    }
+
-    match (&left.node, &right.node) {
+        check_len(cx, span, method_path.name, args, lit, op, compare_to)
-        (&ExprLit(ref lit), &ExprMethodCall(ref method_path, _, ref args)) |
-        (&ExprMethodCall(ref method_path, _, ref args), &ExprLit(ref lit)) => {
-            check_len_zero(cx, span, method_path.name, args, lit, op)
-        },
-        _ => (),
     }
 }
 
-fn check_len_zero(cx: &LateContext, span: Span, name: Name, args: &[Expr], lit: &Lit, op: &str) {
+fn check_len(cx: &LateContext, span: Span, method_name: Name, args: &[Expr], lit: &Lit, op: &str, compare_to: u32) {
     if let Spanned {
-        node: LitKind::Int(0, _),
+        node: LitKind::Int(lit, _),
         ..
     } = *lit
     {
-        if name == "len" && args.len() == 1 && has_is_empty(cx, &args[0]) {
+        // check if length is compared to the specified number
+        if lit != u128::from(compare_to) {
+            return;
+        }
+
+        if method_name == "len" && args.len() == 1 && has_is_empty(cx, &args[0]) {
             span_lint_and_sugg(
                 cx,
                 LEN_ZERO,
                 span,
-                "length comparison to zero",
+                &format!("length comparison to {}", if compare_to == 0 { "zero" } else { "one" }),
                 "using `is_empty` is more concise",
                 format!("{}{}.is_empty()", op, snippet(cx, args[0].span, "_")),
             );

tests/ui/len_zero.rs

@@ -1,6 +1,3 @@
-
-
-
 #![warn(len_without_is_empty, len_zero)]
 #![allow(dead_code, unused)]
 
@@ -12,7 +9,8 @@ impl PubOne {
     }
 }
 
-impl PubOne { // A second impl for this struct - the error span shouldn't mention this
+impl PubOne {
+    // A second impl for this struct - the error span shouldn't mention this
     pub fn irrelevant(self: &Self) -> bool {
         false
     }
@@ -39,7 +37,8 @@ impl PubAllowed {
 struct NotPubOne;
 
 impl NotPubOne {
-    pub fn len(self: &Self) -> isize { // no error, len is pub but `NotPubOne` is not exported anyway
+    pub fn len(self: &Self) -> isize {
+        // no error, len is pub but `NotPubOne` is not exported anyway
         1
     }
 }
@@ -47,7 +46,8 @@ impl NotPubOne {
 struct One;
 
 impl One {
-    fn len(self: &Self) -> isize { // no error, len is private, see #1085
+    fn len(self: &Self) -> isize {
+        // no error, len is private, see #1085
         1
     }
 }
@@ -120,7 +120,7 @@ impl HasWrongIsEmpty {
         1
     }
 
-    pub fn is_empty(self: &Self, x : u32) -> bool {
+    pub fn is_empty(self: &Self, x: u32) -> bool {
         false
     }
 }
@@ -129,28 +129,28 @@ pub trait Empty {
     fn is_empty(&self) -> bool;
 }
 
-pub trait InheritingEmpty: Empty { //must not trigger LEN_WITHOUT_IS_EMPTY
+pub trait InheritingEmpty: Empty {
+    //must not trigger LEN_WITHOUT_IS_EMPTY
     fn len(&self) -> isize;
 }
 
-
-
 fn main() {
     let x = [1, 2];
     if x.len() == 0 {
         println!("This should not happen!");
     }
 
-    if "".len() == 0 {
+    if "".len() == 0 {}
-    }
 
     let y = One;
-    if y.len()  == 0 { //no error because One does not have .is_empty()
+    if y.len() == 0 {
+        //no error because One does not have .is_empty()
         println!("This should not happen either!");
     }
 
-    let z : &TraitsToo = &y;
+    let z: &TraitsToo = &y;
-    if z.len() > 0 { //no error, because TraitsToo has no .is_empty() method
+    if z.len() > 0 {
+        //no error, because TraitsToo has no .is_empty() method
         println!("Nor should this!");
     }
 
@@ -164,6 +164,43 @@ fn main() {
     if has_is_empty.len() > 0 {
         println!("Or this!");
     }
+    if has_is_empty.len() < 1 {
+        println!("Or this!");
+    }
+    if has_is_empty.len() >= 1 {
+        println!("Or this!");
+    }
+    if has_is_empty.len() > 1 {
+        // no error
+        println!("This can happen.");
+    }
+    if has_is_empty.len() <= 1 {
+        // no error
+        println!("This can happen.");
+    }
+    if 0 == has_is_empty.len() {
+        println!("Or this!");
+    }
+    if 0 != has_is_empty.len() {
+        println!("Or this!");
+    }
+    if 0 < has_is_empty.len() {
+        println!("Or this!");
+    }
+    if 1 <= has_is_empty.len() {
+        println!("Or this!");
+    }
+    if 1 > has_is_empty.len() {
+        println!("Or this!");
+    }
+    if 1 < has_is_empty.len() {
+        // no error
+        println!("This can happen.");
+    }
+    if 1 >= has_is_empty.len() {
+        // no error
+        println!("This can happen.");
+    }
     assert!(!has_is_empty.is_empty());
 
     let with_is_empty: &WithIsEmpty = &Wither;
@@ -173,14 +210,14 @@ fn main() {
     assert!(!with_is_empty.is_empty());
 
     let has_wrong_is_empty = HasWrongIsEmpty;
-    if has_wrong_is_empty.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty()
+    if has_wrong_is_empty.len() == 0 {
+        //no error as HasWrongIsEmpty does not have .is_empty()
         println!("Or this!");
     }
 }
 
 fn test_slice(b: &[u8]) {
-    if b.len() != 0 {
+    if b.len() != 0 {}
-    }
 }
 
 // this used to ICE

tests/ui/len_zero.stderr

@@ -1,11 +1,11 @@
 error: item `PubOne` has a public `len` method but no corresponding `is_empty` method
-  --> $DIR/len_zero.rs:9:1
+  --> $DIR/len_zero.rs:6:1
    |
-9  | / impl PubOne {
+6  | / impl PubOne {
-10 | |     pub fn len(self: &Self) -> isize {
+7  | |     pub fn len(self: &Self) -> isize {
-11 | |         1
+8  | |         1
-12 | |     }
+9  | |     }
-13 | | }
+10 | | }
    | |_^
    |
    = note: `-D len-without-is-empty` implied by `-D warnings`
@@ -43,17 +43,17 @@ error: item `HasWrongIsEmpty` has a public `len` method but no corresponding `is
     | |_^
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:140:8
+   --> $DIR/len_zero.rs:139:8
     |
-140 |     if x.len() == 0 {
+139 |     if x.len() == 0 {
     |        ^^^^^^^^^^^^ help: using `is_empty` is more concise: `x.is_empty()`
     |
     = note: `-D len-zero` implied by `-D warnings`
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:144:8
+   --> $DIR/len_zero.rs:143:8
     |
-144 |     if "".len() == 0 {
+143 |     if "".len() == 0 {}
     |        ^^^^^^^^^^^^^ help: using `is_empty` is more concise: `"".is_empty()`
 
 error: length comparison to zero
@@ -74,25 +74,67 @@ error: length comparison to zero
 164 |     if has_is_empty.len() > 0 {
     |        ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
 
-error: length comparison to zero
+error: length comparison to one
+   --> $DIR/len_zero.rs:167:8
+    |
+167 |     if has_is_empty.len() < 1 {
+    |        ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()`
+
+error: length comparison to one
    --> $DIR/len_zero.rs:170:8
     |
-170 |     if with_is_empty.len() == 0 {
+170 |     if has_is_empty.len() >= 1 {
+    |        ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
+
+error: length comparison to zero
+   --> $DIR/len_zero.rs:181:8
+    |
+181 |     if 0 == has_is_empty.len() {
+    |        ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()`
+
+error: length comparison to zero
+   --> $DIR/len_zero.rs:184:8
+    |
+184 |     if 0 != has_is_empty.len() {
+    |        ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
+
+error: length comparison to zero
+   --> $DIR/len_zero.rs:187:8
+    |
+187 |     if 0 < has_is_empty.len() {
+    |        ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
+
+error: length comparison to one
+   --> $DIR/len_zero.rs:190:8
+    |
+190 |     if 1 <= has_is_empty.len() {
+    |        ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
+
+error: length comparison to one
+   --> $DIR/len_zero.rs:193:8
+    |
+193 |     if 1 > has_is_empty.len() {
+    |        ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()`
+
+error: length comparison to zero
+   --> $DIR/len_zero.rs:207:8
+    |
+207 |     if with_is_empty.len() == 0 {
     |        ^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `with_is_empty.is_empty()`
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:182:8
+   --> $DIR/len_zero.rs:220:8
     |
-182 |     if b.len() != 0 {
+220 |     if b.len() != 0 {}
     |        ^^^^^^^^^^^^ help: using `is_empty` is more concise: `!b.is_empty()`
 
 error: trait `DependsOnFoo` has a `len` method but no (possibly inherited) `is_empty` method
-   --> $DIR/len_zero.rs:189:1
+   --> $DIR/len_zero.rs:226:1
     |
-189 | / pub trait DependsOnFoo: Foo {
+226 | / pub trait DependsOnFoo: Foo {
-190 | |     fn len(&mut self) -> usize;
+227 | |     fn len(&mut self) -> usize;
-191 | | }
+228 | | }
     | |_^
 
-error: aborting due to 12 previous errors
+error: aborting due to 19 previous errors