From ec799707718bfeff98b874b748cda07e8f3ff704 Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Mon, 28 Aug 2017 23:13:56 +0200 Subject: [PATCH] len_without_is_empty false positive #1740 --- clippy_lints/src/len_zero.rs | 55 +++++++++++++++++++++++++++++------- tests/ui/len_zero.rs | 10 +++++++ tests/ui/len_zero.stderr | 30 ++++++++++---------- 3 files changed, 70 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 4e40348facf..fceffc4c665 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -2,6 +2,7 @@ use rustc::lint::*; 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 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LenZero { } } -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 check_trait_items(cx: &LateContext, item: &Item, trait_items: &[TraitItemRef] } } - 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 c90e1193db1..e0e735a934b 100644 --- a/tests/ui/len_zero.rs +++ b/tests/ui/len_zero.rs @@ -125,6 +125,16 @@ impl HasWrongIsEmpty { } } +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 2c9edfe9b0e..5e3961808b8 100644 --- 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