len_without_is_empty false positive #1740

diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs
index 4e40348facf2467372d8a1ff1d58b64ced3a7faa..fceffc4c665d153f6fd00c56f04e979f8aac75f4 100644 (file)
--- a/clippy_lints/src/len_zero.rs
+++ b/clippy_lints/src/len_zero.rs
@@ -2,6 +2,7 @@
 use rustc::hir::def_id::DefId;
 use rustc::ty;
 use rustc::hir::*;
+use std::collections::HashSet;
 use syntax::ast::{Lit, LitKind, Name};
 use syntax::codemap::{Span, Spanned};
 use utils::{get_item_name, in_macro, snippet, span_lint, span_lint_and_sugg, walk_ptrs_ty};
@@ -88,7 +89,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
     }
 }
 
-fn check_trait_items(cx: &LateContext, item: &Item, trait_items: &[TraitItemRef]) {
+fn check_trait_items(cx: &LateContext, visited_trait: &Item, trait_items: &[TraitItemRef]) {
     fn is_named_self(cx: &LateContext, item: &TraitItemRef, name: &str) -> bool {
         item.name == name &&
             if let AssociatedItemKind::Method { has_self } = item.kind {
@@ -102,18 +103,52 @@ fn is_named_self(cx: &LateContext, item: &TraitItemRef, name: &str) -> bool {
             }
     }
 
-    if !trait_items.iter().any(|i| is_named_self(cx, i, "is_empty")) {
-        if let Some(i) = trait_items.iter().find(|i| is_named_self(cx, i, "len")) {
-            if cx.access_levels.is_exported(i.id.node_id) {
-                span_lint(
-                    cx,
-                    LEN_WITHOUT_IS_EMPTY,
-                    item.span,
-                    &format!("trait `{}` has a `len` method but no `is_empty` method", item.name),
-                );
+    // fill the set with current and super traits
+    fn fill_trait_set<'a, 'b: 'a>(traitt: &'b Item, set: &'a mut HashSet<&'b Item>, cx: &'b LateContext) {
+        if set.insert(traitt) {
+            if let ItemTrait(.., ref ty_param_bounds, _) = traitt.node {
+                for ty_param_bound in ty_param_bounds {
+                    if let TraitTyParamBound(ref poly_trait_ref, _) = *ty_param_bound {
+                        let super_trait_node_id = cx.tcx
+                            .hir
+                            .as_local_node_id(poly_trait_ref.trait_ref.path.def.def_id())
+                            .expect("the DefId is local, the NodeId should be available");
+                        let super_trait = cx.tcx.hir.expect_item(super_trait_node_id);
+                        fill_trait_set(super_trait, set, cx);
+                    }
+                }
             }
         }
     }
+
+    if cx.access_levels.is_exported(visited_trait.id) &&
+        trait_items
+            .iter()
+            .any(|i| is_named_self(cx, i, "len"))
+    {
+        let mut current_and_super_traits = HashSet::new();
+        fill_trait_set(visited_trait, &mut current_and_super_traits, cx);
+
+        let is_empty_method_found = current_and_super_traits
+            .iter()
+            .flat_map(|i| match i.node {
+                ItemTrait(.., ref trait_items) => trait_items.iter(),
+                _ => bug!("should only handle traits"),
+            })
+            .any(|i| is_named_self(cx, i, "is_empty"));
+
+        if !is_empty_method_found {
+            span_lint(
+                cx,
+                LEN_WITHOUT_IS_EMPTY,
+                visited_trait.span,
+                &format!(
+                    "trait `{}` has a `len` method but no (possibly inherited) `is_empty` method",
+                    visited_trait.name
+                ),
+            );
+        }
+    }
 }
 
 fn check_impl_items(cx: &LateContext, item: &Item, impl_items: &[ImplItemRef]) {

diff --git a/tests/ui/len_zero.rs b/tests/ui/len_zero.rs
index c90e1193db15d6c62a433b102b1d1d32f7d29732..e0e735a934b2b4a3f06bc13bfbede3efa3cc41ef 100644 (file)
--- a/tests/ui/len_zero.rs
+++ b/tests/ui/len_zero.rs
@@ -125,6 +125,16 @@ pub fn is_empty(self: &Self, x : u32) -> bool {
     }
 }
 
+pub trait Empty {
+    fn is_empty(&self) -> bool;
+}
+
+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 {

diff --git a/tests/ui/len_zero.stderr b/tests/ui/len_zero.stderr
index 2c9edfe9b0eb0c1ce90b50688bf6602a08633ff0..5e3961808b87ed1865dce7b15f61f27fafcccbda 100644 (file)
--- a/tests/ui/len_zero.stderr
+++ b/tests/ui/len_zero.stderr
@@ -10,7 +10,7 @@ error: item `PubOne` has a public `len` method but no corresponding `is_empty` m
    |
    = note: `-D len-without-is-empty` implied by `-D warnings`
 
-error: trait `PubTraitsToo` has a `len` method but no `is_empty` method
+error: trait `PubTraitsToo` has a `len` method but no (possibly inherited) `is_empty` method
   --> $DIR/len_zero.rs:55:1
    |
 55 | / pub trait PubTraitsToo {
@@ -43,47 +43,47 @@ error: item `HasWrongIsEmpty` has a public `len` method but no corresponding `is
     | |_^
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:130:8
+   --> $DIR/len_zero.rs:140:8
     |
-130 |     if x.len() == 0 {
+140 |     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:134:8
+   --> $DIR/len_zero.rs:144:8
     |
-134 |     if "".len() == 0 {
+144 |     if "".len() == 0 {
     |        ^^^^^^^^^^^^^ help: using `is_empty` is more concise: `"".is_empty()`
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:148:8
+   --> $DIR/len_zero.rs:158:8
     |
-148 |     if has_is_empty.len() == 0 {
+158 |     if has_is_empty.len() == 0 {
     |        ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()`
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:151:8
+   --> $DIR/len_zero.rs:161:8
     |
-151 |     if has_is_empty.len() != 0 {
+161 |     if has_is_empty.len() != 0 {
     |        ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:154:8
+   --> $DIR/len_zero.rs:164:8
     |
-154 |     if has_is_empty.len() > 0 {
+164 |     if has_is_empty.len() > 0 {
     |        ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()`
 
 error: length comparison to zero
-   --> $DIR/len_zero.rs:160:8
+   --> $DIR/len_zero.rs:170:8
     |
-160 |     if with_is_empty.len() == 0 {
+170 |     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:172:8
+   --> $DIR/len_zero.rs:182:8
     |
-172 |     if b.len() != 0 {
+182 |     if b.len() != 0 {
     |        ^^^^^^^^^^^^ help: using `is_empty` is more concise: `!b.is_empty()`
 
 error: aborting due to 11 previous errors