mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-10 23:24:24 +00:00
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
This commit is contained in:
Jason Newcomb
parent
5945e85f34
commit
47145dec36
4 changed files with 229 additions and 75 deletions
|
|
@ -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"
|
||||
/// 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);
|
||||
|
||||
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,
|
||||
};
|
||||
|
||||
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);
|
||||
|
||||
span_lint(
|
||||
cx,
|
||||
LEN_WITHOUT_IS_EMPTY,
|
||||
item.span,
|
||||
&format!(
|
||||
"item `{}` has a public `len` method but {} `is_empty` method",
|
||||
ty, is_empty
|
||||
),
|
||||
);
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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() {}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
error: struct `HasIsEmpty` has a public `len` method, but a private `is_empty` method
|
||||
--> $DIR/len_without_is_empty.rs:68:5
|
||||
|
|
||||
LL | / impl HasIsEmpty {
|
||||
LL | | pub fn len(&self) -> isize {
|
||||
LL | | 1
|
||||
LL | | }
|
||||
... |
|
||||
LL | | }
|
||||
LL | | }
|
||||
| |_^
|
||||
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
|
||||
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 | / impl HasWrongIsEmpty {
|
||||
LL | | pub fn len(&self) -> isize {
|
||||
LL | | 1
|
||||
LL | | }
|
||||
... |
|
||||
LL | | }
|
||||
LL | | }
|
||||
| |_^
|
||||
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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue