Merge pull request rust-lang#1816 from Manishearth/fix-451

Check for AsRef/AsMut arguments in wrong_self_convention

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_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
+            }
         }
     }
 
@@ -1398,6 +1403,46 @@ impl SelfKind {
     }
 }
 
+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| {
+                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().map(|seg| seg.name).eq(
+                self_ty_path.segments.iter().map(|seg| seg.name))
+        }
+        _ => 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 {

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"];

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: AsRef<Self>>(_: F) { }
+    fn as_y<F: AsRef<Foo>>(_: F) { }
 }
 
 struct Bar;

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`