From 8122d3e8cbf21a430254fb1024f75a0fb431bf88 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Tue, 6 Jun 2017 19:26:50 +0200 Subject: [PATCH 1/2] Check for AsRef/AsMut arguments in wrong_self_convention This fixes #451 --- clippy_lints/src/methods.rs | 54 +++++++++++++++++-- clippy_lints/src/utils/paths.rs | 1 + .../examples/wrong_self_convention.rs | 2 + .../examples/wrong_self_convention.stderr | 40 +++++++------- 4 files changed, 72 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 738350a4af98..92e3d5d3b681 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -10,7 +10,7 @@ use syntax::codemap::Span; use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, is_copy, match_path, match_trait_method, match_type, method_chain_args, return_ty, same_tys, snippet, span_lint, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, last_path_segment, single_segment_path, - match_def_path, is_self, is_self_ty, iter_input_pats}; + match_def_path, is_self, is_self_ty, iter_input_pats, match_path_old}; use utils::paths; use utils::sugg; @@ -649,7 +649,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if name == method_name && sig.decl.inputs.len() == n_args && out_type.matches(&sig.decl.output) && - self_kind.matches(first_arg_ty, first_arg, self_ty, false) { + self_kind.matches(first_arg_ty, first_arg, self_ty, false, &sig.generics) { span_lint(cx, SHOULD_IMPLEMENT_TRAIT, implitem.span, &format!( "defining a method called `{}` on this type; consider implementing \ the `{}` trait or choosing a less ambiguous name", name, trait_name)); @@ -663,7 +663,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { for &(ref conv, self_kinds) in &CONVENTIONS { if_let_chain! {[ conv.check(&name.as_str()), - !self_kinds.iter().any(|k| k.matches(first_arg_ty, first_arg, self_ty, is_copy)), + !self_kinds.iter().any(|k| k.matches(first_arg_ty, first_arg, self_ty, is_copy, &sig.generics)), ], { let lint = if item.vis == hir::Visibility::Public { WRONG_PUB_SELF_CONVENTION @@ -1353,7 +1353,7 @@ enum SelfKind { } impl SelfKind { - fn matches(self, ty: &hir::Ty, arg: &hir::Arg, self_ty: &hir::Ty, allow_value_for_ref: bool) -> bool { + fn matches(self, ty: &hir::Ty, arg: &hir::Arg, self_ty: &hir::Ty, allow_value_for_ref: bool, generics: &hir::Generics) -> bool { // Self types in the HIR are desugared to explicit self types. So it will always be `self: // SomeType`, // where SomeType can be `Self` or an explicit impl self type (e.g. `Foo` if the impl is on `Foo`) @@ -1384,7 +1384,12 @@ impl SelfKind { _ => false, } } else { - self == SelfKind::No + match self { + SelfKind::Value => false, + SelfKind::Ref => is_astrait(ty, self_ty, generics, &paths::ASREF_TRAIT), + SelfKind::RefMut => is_astrait(ty, self_ty, generics, &paths::ASMUT_TRAIT), + SelfKind::No => true + } } } @@ -1398,6 +1403,45 @@ impl SelfKind { } } +fn is_astrait(ty: &hir::Ty, self_ty: &hir::Ty, generics: &hir::Generics, name: &[&str]) -> bool { + single_segment_ty(ty).map_or(false, |seg| { + generics.ty_params.iter().any(|param| { + param.name == seg.name && param.bounds.iter().any(|bound| { + if let hir::TyParamBound::TraitTyParamBound(ref ptr, ..) = *bound { + let path = &ptr.trait_ref.path; + match_path_old(path, name) && path.segments.last().map_or(false, |s| { + if let hir::PathParameters::AngleBracketedParameters(ref data) = s.parameters { + data.types.len() == 1 && (is_self_ty(&data.types[0]) || is_ty(&*data.types[0], self_ty)) + } else { + false + } + }) + } else { + false + } + }) + }) + }) +} + +fn is_ty(ty: &hir::Ty, self_ty: &hir::Ty) -> bool { + match (&ty.node, &self_ty.node) { + (&hir::TyPath(hir::QPath::Resolved(_, ref ty_path)), &hir::TyPath(hir::QPath::Resolved(_, ref self_ty_path))) => { + ty_path.segments.iter().rev().map(|seg| seg.name).zip( + self_ty_path.segments.iter().rev().map(|seg| seg.name)).all(|(l, r)| l == r) + } + _ => false + } +} + +fn single_segment_ty(ty: &hir::Ty) -> Option<&hir::PathSegment> { + if let hir::TyPath(ref path) = ty.node { + single_segment_path(path) + } else { + None + } +} + impl Convention { fn check(&self, other: &str) -> bool { match *self { diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 738a497c6ab6..eb9979343948 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -1,5 +1,6 @@ //! This module contains paths to types and functions Clippy needs to know about. +pub const ASMUT_TRAIT: [&'static str; 3] = ["core", "convert", "AsMut"]; pub const ASREF_TRAIT: [&'static str; 3] = ["core", "convert", "AsRef"]; pub const BEGIN_PANIC: [&'static str; 3] = ["std", "panicking", "begin_panic"]; pub const BINARY_HEAP: [&'static str; 3] = ["collections", "binary_heap", "BinaryHeap"]; diff --git a/clippy_tests/examples/wrong_self_convention.rs b/clippy_tests/examples/wrong_self_convention.rs index 0fbe4eff6d6c..91b60c8faaa2 100644 --- a/clippy_tests/examples/wrong_self_convention.rs +++ b/clippy_tests/examples/wrong_self_convention.rs @@ -29,6 +29,8 @@ impl Foo { #[allow(wrong_self_convention)] pub fn from_cake(self) {} + fn as_x>(_: F) { } + fn as_y>(_: F) { } } struct Bar; diff --git a/clippy_tests/examples/wrong_self_convention.stderr b/clippy_tests/examples/wrong_self_convention.stderr index 7a96ed71024a..91ed88dd4afa 100644 --- a/clippy_tests/examples/wrong_self_convention.stderr +++ b/clippy_tests/examples/wrong_self_convention.stderr @@ -15,81 +15,81 @@ error: methods called `from_*` usually take no self; consider choosing a less am = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name - --> wrong_self_convention.rs:38:15 + --> wrong_self_convention.rs:40:15 | -38 | fn as_i32(self) {} +40 | fn as_i32(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name - --> wrong_self_convention.rs:40:17 + --> wrong_self_convention.rs:42:17 | -40 | fn into_i32(&self) {} +42 | fn into_i32(&self) {} | ^^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name - --> wrong_self_convention.rs:42:15 + --> wrong_self_convention.rs:44:15 | -42 | fn is_i32(self) {} +44 | fn is_i32(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name - --> wrong_self_convention.rs:44:15 + --> wrong_self_convention.rs:46:15 | -44 | fn to_i32(self) {} +46 | fn to_i32(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `from_*` usually take no self; consider choosing a less ambiguous name - --> wrong_self_convention.rs:46:17 + --> wrong_self_convention.rs:48:17 | -46 | fn from_i32(self) {} +48 | fn from_i32(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `as_*` usually take self by reference or self by mutable reference; consider choosing a less ambiguous name - --> wrong_self_convention.rs:48:19 + --> wrong_self_convention.rs:50:19 | -48 | pub fn as_i64(self) {} +50 | pub fn as_i64(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `into_*` usually take self by value; consider choosing a less ambiguous name - --> wrong_self_convention.rs:49:21 + --> wrong_self_convention.rs:51:21 | -49 | pub fn into_i64(&self) {} +51 | pub fn into_i64(&self) {} | ^^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name - --> wrong_self_convention.rs:50:19 + --> wrong_self_convention.rs:52:19 | -50 | pub fn is_i64(self) {} +52 | pub fn is_i64(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `to_*` usually take self by reference; consider choosing a less ambiguous name - --> wrong_self_convention.rs:51:19 + --> wrong_self_convention.rs:53:19 | -51 | pub fn to_i64(self) {} +53 | pub fn to_i64(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` error: methods called `from_*` usually take no self; consider choosing a less ambiguous name - --> wrong_self_convention.rs:52:21 + --> wrong_self_convention.rs:54:21 | -52 | pub fn from_i64(self) {} +54 | pub fn from_i64(self) {} | ^^^^ | = note: `-D wrong-self-convention` implied by `-D warnings` From a648cfeae1a042069a727236d0d3aa97a47165f6 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 7 Jun 2017 05:40:57 +0200 Subject: [PATCH 2/2] better naming, Iterator::eq --- clippy_lints/src/methods.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 92e3d5d3b681..f8fae40121e3 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1386,8 +1386,8 @@ impl SelfKind { } else { match self { SelfKind::Value => false, - SelfKind::Ref => is_astrait(ty, self_ty, generics, &paths::ASREF_TRAIT), - SelfKind::RefMut => is_astrait(ty, self_ty, generics, &paths::ASMUT_TRAIT), + SelfKind::Ref => is_as_ref_or_mut_trait(ty, self_ty, generics, &paths::ASREF_TRAIT), + SelfKind::RefMut => is_as_ref_or_mut_trait(ty, self_ty, generics, &paths::ASMUT_TRAIT), SelfKind::No => true } } @@ -1403,7 +1403,7 @@ impl SelfKind { } } -fn is_astrait(ty: &hir::Ty, self_ty: &hir::Ty, generics: &hir::Generics, name: &[&str]) -> bool { +fn is_as_ref_or_mut_trait(ty: &hir::Ty, self_ty: &hir::Ty, generics: &hir::Generics, name: &[&str]) -> bool { single_segment_ty(ty).map_or(false, |seg| { generics.ty_params.iter().any(|param| { param.name == seg.name && param.bounds.iter().any(|bound| { @@ -1426,9 +1426,10 @@ fn is_astrait(ty: &hir::Ty, self_ty: &hir::Ty, generics: &hir::Generics, name: & fn is_ty(ty: &hir::Ty, self_ty: &hir::Ty) -> bool { match (&ty.node, &self_ty.node) { - (&hir::TyPath(hir::QPath::Resolved(_, ref ty_path)), &hir::TyPath(hir::QPath::Resolved(_, ref self_ty_path))) => { - ty_path.segments.iter().rev().map(|seg| seg.name).zip( - self_ty_path.segments.iter().rev().map(|seg| seg.name)).all(|(l, r)| l == r) + (&hir::TyPath(hir::QPath::Resolved(_, ref ty_path)), + &hir::TyPath(hir::QPath::Resolved(_, ref self_ty_path))) => { + ty_path.segments.iter().map(|seg| seg.name).eq( + self_ty_path.segments.iter().map(|seg| seg.name)) } _ => false }