len_without_is_empty will now consider multiple impl blocks

Jarcho · Jarcho · commit 47145dec36fb · 2021-03-07T09:40:18.000-05:00
`len_without_is_empty` will now consider `#[allow]` on both the `len` method, and the type definition
diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs
@@ -1,11 +1,17 @@
-use crate::utils::{get_item_name, snippet_with_applicability, span_lint, span_lint_and_sugg};
+use crate::utils::{
+    get_item_name, get_parent_as_impl, is_allowed, snippet_with_applicability, span_lint, span_lint_and_sugg,
+    span_lint_and_then,
+};
+use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
-use rustc_hir::def_id::DefId;
-use rustc_hir::{AssocItemKind, BinOpKind, Expr, ExprKind, Impl, ImplItemRef, Item, ItemKind, TraitItemRef};
+use rustc_hir::{
+    def_id::DefId, AssocItemKind, BinOpKind, Expr, ExprKind, FnRetTy, ImplItem, ImplItemKind, ImplicitSelfKind, Item,
+    ItemKind, Mutability, Node, TraitItemRef, TyKind,
+};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty;
+use rustc_middle::ty::{self, AssocKind, FnSig};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::{Span, Spanned, Symbol};
 
@@ -113,14 +119,38 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
             return;
         }
 
-        match item.kind {
-            ItemKind::Trait(_, _, _, _, ref trait_items) => check_trait_items(cx, item, trait_items),
-            ItemKind::Impl(Impl {
-                of_trait: None,
-                items: ref impl_items,
-                ..
-            }) => check_impl_items(cx, item, impl_items),
-            _ => (),
+        if let ItemKind::Trait(_, _, _, _, ref trait_items) = item.kind {
+            check_trait_items(cx, item, trait_items);
+        }
+    }
+
+    fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {
+        if_chain! {
+            if item.ident.as_str() == "len";
+            if let ImplItemKind::Fn(sig, _) = &item.kind;
+            if sig.decl.implicit_self.has_implicit_self();
+            if cx.access_levels.is_exported(item.hir_id());
+            if matches!(sig.decl.output, FnRetTy::Return(_));
+            if let Some(imp) = get_parent_as_impl(cx.tcx, item.hir_id());
+            if imp.of_trait.is_none();
+            if let TyKind::Path(ty_path) = &imp.self_ty.kind;
+            if let Some(ty_id) = cx.qpath_res(ty_path, imp.self_ty.hir_id).opt_def_id();
+            if let Some(local_id) = ty_id.as_local();
+            let ty_hir_id = cx.tcx.hir().local_def_id_to_hir_id(local_id);
+            if !is_allowed(cx, LEN_WITHOUT_IS_EMPTY, ty_hir_id);
+            then {
+                let (name, kind) = match cx.tcx.hir().find(ty_hir_id) {
+                    Some(Node::ForeignItem(x)) => (x.ident.name, "extern type"),
+                    Some(Node::Item(x)) => match x.kind {
+                        ItemKind::Struct(..) => (x.ident.name, "struct"),
+                        ItemKind::Enum(..) => (x.ident.name, "enum"),
+                        ItemKind::Union(..) => (x.ident.name, "union"),
+                        _ => (x.ident.name, "type"),
+                    }
+                    _ => return,
+                };
+                check_for_is_empty(cx, sig.span, sig.decl.implicit_self, ty_id, name, kind)
+            }
         }
     }
 
@@ -202,40 +232,94 @@ fn check_trait_items(cx: &LateContext<'_>, visited_trait: &Item<'_>, trait_items
     }
 }
 
-fn check_impl_items(cx: &LateContext<'_>, item: &Item<'_>, impl_items: &[ImplItemRef<'_>]) {
-    fn is_named_self(cx: &LateContext<'_>, item: &ImplItemRef<'_>, name: &str) -> bool {
-        item.ident.name.as_str() == name
-            && if let AssocItemKind::Fn { has_self } = item.kind {
-                has_self && cx.tcx.fn_sig(item.id.def_id).inputs().skip_binder().len() == 1
-            } else {
-                false
-            }
+/// Checks if the given signature matches the expectations for `is_empty`
+fn check_is_empty_sig(cx: &LateContext<'_>, sig: FnSig<'_>, self_kind: ImplicitSelfKind) -> bool {
+    match &**sig.inputs_and_output {
+        [arg, res] if *res == cx.tcx.types.bool => {
+            matches!(
+                (arg.kind(), self_kind),
+                (ty::Ref(_, _, Mutability::Not), ImplicitSelfKind::ImmRef)
+                    | (ty::Ref(_, _, Mutability::Mut), ImplicitSelfKind::MutRef)
+            ) || (!arg.is_ref() && matches!(self_kind, ImplicitSelfKind::Imm | ImplicitSelfKind::Mut))
+        },
+        _ => false,
     }
+}
 
-    let is_empty = if let Some(is_empty) = impl_items.iter().find(|i| is_named_self(cx, i, "is_empty")) {
-        if cx.access_levels.is_exported(is_empty.id.hir_id()) {
-            return;
-        }
-        "a private"
-    } else {
-        "no corresponding"
-    };
-
-    if let Some(i) = impl_items.iter().find(|i| is_named_self(cx, i, "len")) {
-        if cx.access_levels.is_exported(i.id.hir_id()) {
-            let ty = cx.tcx.type_of(item.def_id);
+/// Checks if the given type has an `is_empty` method with the appropriate signature.
+fn check_for_is_empty(
+    cx: &LateContext<'_>,
+    span: Span,
+    self_kind: ImplicitSelfKind,
+    impl_ty: DefId,
+    item_name: Symbol,
+    item_kind: &str,
+) {
+    let is_empty = Symbol::intern("is_empty");
+    let is_empty = cx
+        .tcx
+        .inherent_impls(impl_ty)
+        .iter()
+        .flat_map(|&id| cx.tcx.associated_items(id).filter_by_name_unhygienic(is_empty))
+        .find(|item| item.kind == AssocKind::Fn);
 
-            span_lint(
-                cx,
-                LEN_WITHOUT_IS_EMPTY,
-                item.span,
-                &format!(
-                    "item `{}` has a public `len` method but {} `is_empty` method",
-                    ty, is_empty
+    let (msg, is_empty_span, self_kind) = match is_empty {
+        None => (
+            format!(
+                "{} `{}` has a public `len` method, but no `is_empty` method",
+                item_kind,
+                item_name.as_str(),
+            ),
+            None,
+            None,
+        ),
+        Some(is_empty)
+            if !cx
+                .access_levels
+                .is_exported(cx.tcx.hir().local_def_id_to_hir_id(is_empty.def_id.expect_local())) =>
+        {
+            (
+                format!(
+                    "{} `{}` has a public `len` method, but a private `is_empty` method",
+                    item_kind,
+                    item_name.as_str(),
                 ),
-            );
+                Some(cx.tcx.def_span(is_empty.def_id)),
+                None,
+            )
+        },
+        Some(is_empty)
+            if !(is_empty.fn_has_self_parameter
+                && check_is_empty_sig(cx, cx.tcx.fn_sig(is_empty.def_id).skip_binder(), self_kind)) =>
+        {
+            (
+                format!(
+                    "{} `{}` has a public `len` method, but the `is_empty` method has an unexpected signature",
+                    item_kind,
+                    item_name.as_str(),
+                ),
+                Some(cx.tcx.def_span(is_empty.def_id)),
+                Some(self_kind),
+            )
+        },
+        Some(_) => return,
+    };
+
+    span_lint_and_then(cx, LEN_WITHOUT_IS_EMPTY, span, &msg, |db| {
+        if let Some(span) = is_empty_span {
+            db.span_note(span, "`is_empty` defined here");
         }
-    }
+        if let Some(self_kind) = self_kind {
+            db.note(&format!(
+                "expected signature: `({}self) -> bool`",
+                match self_kind {
+                    ImplicitSelfKind::ImmRef => "&",
+                    ImplicitSelfKind::MutRef => "&mut ",
+                    _ => "",
+                }
+            ));
+        }
+    });
 }
 
 fn check_cmp(cx: &LateContext<'_>, span: Span, method: &Expr<'_>, lit: &Expr<'_>, op: &str, compare_to: u32) {
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -63,9 +63,9 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_hir::Node;
 use rustc_hir::{
-    def, Arm, Block, Body, Constness, Crate, Expr, ExprKind, FnDecl, GenericArgs, HirId, ImplItem, ImplItemKind, Item,
-    ItemKind, MatchSource, Param, Pat, PatKind, Path, PathSegment, QPath, TraitItem, TraitItemKind, TraitRef, TyKind,
-    Unsafety,
+    def, Arm, Block, Body, Constness, Crate, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, ImplItem, ImplItemKind,
+    Item, ItemKind, MatchSource, Param, Pat, PatKind, Path, PathSegment, QPath, TraitItem, TraitItemKind, TraitRef,
+    TyKind, Unsafety,
 };
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, Level, Lint, LintContext};
@@ -1004,6 +1004,21 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
     })
 }
 
+/// Gets the parent node if it's an impl block.
+pub fn get_parent_as_impl(tcx: TyCtxt<'_>, id: HirId) -> Option<&Impl<'_>> {
+    let map = tcx.hir();
+    match map.parent_iter(id).next() {
+        Some((
+            _,
+            Node::Item(Item {
+                kind: ItemKind::Impl(imp),
+                ..
+            }),
+        )) => Some(imp),
+        _ => None,
+    }
+}
+
 /// Returns the base type for HIR references and pointers.
 pub fn walk_ptrs_hir_ty<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> &'tcx hir::Ty<'tcx> {
     match ty.kind {
diff --git a/tests/ui/len_without_is_empty.rs b/tests/ui/len_without_is_empty.rs
@@ -34,6 +34,24 @@ impl PubAllowed {
     }
 }
 
+pub struct PubAllowedFn;
+
+impl PubAllowedFn {
+    #[allow(clippy::len_without_is_empty)]
+    pub fn len(&self) -> isize {
+        1
+    }
+}
+
+#[allow(clippy::len_without_is_empty)]
+pub struct PubAllowedStruct;
+
+impl PubAllowedStruct {
+    pub fn len(&self) -> isize {
+        1
+    }
+}
+
 pub trait PubTraitsToo {
     fn len(&self) -> isize;
 }
@@ -68,6 +86,18 @@ impl HasWrongIsEmpty {
     }
 }
 
+pub struct MismatchedSelf;
+
+impl MismatchedSelf {
+    pub fn len(self) -> isize {
+        1
+    }
+
+    pub fn is_empty(&self) -> bool {
+        false
+    }
+}
+
 struct NotPubOne;
 
 impl NotPubOne {
@@ -142,4 +172,19 @@ pub trait DependsOnFoo: Foo {
     fn len(&mut self) -> usize;
 }
 
+pub struct MultipleImpls;
+
+// issue #1562
+impl MultipleImpls {
+    pub fn len(&self) -> usize {
+        1
+    }
+}
+
+impl MultipleImpls {
+    pub fn is_empty(&self) -> bool {
+        false
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/len_without_is_empty.stderr b/tests/ui/len_without_is_empty.stderr
@@ -1,54 +1,64 @@
-error: item `PubOne` has a public `len` method but no corresponding `is_empty` method
-  --> $DIR/len_without_is_empty.rs:6:1
+error: struct `PubOne` has a public `len` method, but no `is_empty` method
+  --> $DIR/len_without_is_empty.rs:7:5
    |
-LL | / impl PubOne {
-LL | |     pub fn len(&self) -> isize {
-LL | |         1
-LL | |     }
-LL | | }
-   | |_^
+LL |     pub fn len(&self) -> isize {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::len-without-is-empty` implied by `-D warnings`
 
 error: trait `PubTraitsToo` has a `len` method but no (possibly inherited) `is_empty` method
-  --> $DIR/len_without_is_empty.rs:37:1
+  --> $DIR/len_without_is_empty.rs:55:1
    |
 LL | / pub trait PubTraitsToo {
 LL | |     fn len(&self) -> isize;
 LL | | }
    | |_^
 
-error: item `HasIsEmpty` has a public `len` method but a private `is_empty` method
-  --> $DIR/len_without_is_empty.rs:49:1
-   |
-LL | / impl HasIsEmpty {
-LL | |     pub fn len(&self) -> isize {
-LL | |         1
-LL | |     }
-...  |
-LL | |     }
-LL | | }
-   | |_^
+error: struct `HasIsEmpty` has a public `len` method, but a private `is_empty` method
+  --> $DIR/len_without_is_empty.rs:68:5
+   |
+LL |     pub fn len(&self) -> isize {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: `is_empty` defined here
+  --> $DIR/len_without_is_empty.rs:72:5
+   |
+LL |     fn is_empty(&self) -> bool {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: item `HasWrongIsEmpty` has a public `len` method but no corresponding `is_empty` method
-  --> $DIR/len_without_is_empty.rs:61:1
-   |
-LL | / impl HasWrongIsEmpty {
-LL | |     pub fn len(&self) -> isize {
-LL | |         1
-LL | |     }
-...  |
-LL | |     }
-LL | | }
-   | |_^
+error: struct `HasWrongIsEmpty` has a public `len` method, but the `is_empty` method has an unexpected signature
+  --> $DIR/len_without_is_empty.rs:80:5
+   |
+LL |     pub fn len(&self) -> isize {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: `is_empty` defined here
+  --> $DIR/len_without_is_empty.rs:84:5
+   |
+LL |     pub fn is_empty(&self, x: u32) -> bool {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: expected signature: `(&self) -> bool`
+
+error: struct `MismatchedSelf` has a public `len` method, but the `is_empty` method has an unexpected signature
+  --> $DIR/len_without_is_empty.rs:92:5
+   |
+LL |     pub fn len(self) -> isize {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: `is_empty` defined here
+  --> $DIR/len_without_is_empty.rs:96:5
+   |
+LL |     pub fn is_empty(&self) -> bool {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: expected signature: `(self) -> bool`
 
 error: trait `DependsOnFoo` has a `len` method but no (possibly inherited) `is_empty` method
-  --> $DIR/len_without_is_empty.rs:141:1
+  --> $DIR/len_without_is_empty.rs:171:1
    |
 LL | / pub trait DependsOnFoo: Foo {
 LL | |     fn len(&mut self) -> usize;
 LL | | }
    | |_^
 
-error: aborting due to 5 previous errors
+error: aborting due to 6 previous errors
 
