From 47145dec36fbe99960f45ee7065261e2dcfed152 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 5 Mar 2021 13:01:13 -0500 Subject: [PATCH] `len_without_is_empty` will now consider multiple impl blocks `len_without_is_empty` will now consider `#[allow]` on both the `len` method, and the type definition --- clippy_lints/src/len_zero.rs | 166 ++++++++++++++++++++------- clippy_utils/src/lib.rs | 21 +++- tests/ui/len_without_is_empty.rs | 45 ++++++++ tests/ui/len_without_is_empty.stderr | 76 ++++++------ 4 files changed, 231 insertions(+), 77 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index dab3e0565caf..1e1023b27435 100644 --- 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 index daeced6bad48..6582ad717071 100644 --- 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 index b5211318a150..6b3636a482e9 100644 --- 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 index d79c300c0744..f106506faf49 100644 --- 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