new lint: self conventions for certain method names (fixes #267)

diff --git a/README.md b/README.md
index 27be09c612783e3e1518a48b6a2ac92ae95c9b28..1232f5268754395bbf5666a747c87c7760c9efb0 100644 (file)
--- a/README.md
+++ b/README.md
@@ -4,7 +4,7 @@
 A collection of lints that give helpful tips to newbies and catch oversights.
 
 ##Lints
-There are 52 lints included in this crate:
+There are 53 lints included in this crate:
 
 name                                                                                                 | default | meaning
 -----------------------------------------------------------------------------------------------------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -59,6 +59,7 @@ name
 [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp)                                 | warn    | comparing unit values (which is always `true` or `false`, respectively)
 [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect)                     | warn    | `collect()`ing an iterator without using the result; this is usually better written as a for loop
 [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop)                     | warn    | `loop { if let { ... } else break }` can be written as a `while let` loop
+[wrong_self_convention](https://github.com/Manishearth/rust-clippy/wiki#wrong_self_convention)       | warn    | defining a method named with an established prefix (like "into_") that takes `self` with the wrong convention
 [zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space)                 | deny    | using a zero-width space in a string literal, which is confusing
 
 More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas!

diff --git a/src/lib.rs b/src/lib.rs
index f4da8b03bc288f5c470eb53fa578365b111fc8f4..e51732818bae79ccdd95e5e4e780ac0f94d49484 100755 (executable)
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -114,6 +114,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         methods::SHOULD_IMPLEMENT_TRAIT,
         methods::STR_TO_STRING,
         methods::STRING_TO_STRING,
+        methods::WRONG_SELF_CONVENTION,
         misc::CMP_NAN,
         misc::CMP_OWNED,
         misc::FLOAT_CMP,

diff --git a/src/methods.rs b/src/methods.rs
index 50f3512b4f00347702c3d856c7360cd38001110b..5a20ba0683135820af528cff64f4a5ceb00e68e5 100644 (file)
--- a/src/methods.rs
+++ b/src/methods.rs
@@ -23,11 +23,14 @@
               "calling `String.to_string()` which is a no-op");
 declare_lint!(pub SHOULD_IMPLEMENT_TRAIT, Warn,
               "defining a method that should be implementing a std trait");
+declare_lint!(pub WRONG_SELF_CONVENTION, Warn,
+              "defining a method named with an established prefix (like \"into_\") that takes \
+               `self` with the wrong convention");
 
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
         lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING,
-                    SHOULD_IMPLEMENT_TRAIT)
+                    SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION)
     }
 
     fn check_expr(&mut self, cx: &Context, expr: &Expr) {
@@ -68,18 +71,28 @@ fn check_item(&mut self, cx: &Context, item: &Item) {
         if let ItemImpl(_, _, _, None, _, ref items) = item.node {
             for item in items {
                 let name = item.ident.name;
-                for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
-                    if_let_chain! {
-                        [
-                            name == method_name,
-                            let MethodImplItem(ref sig, _) = item.node,
-                            sig.decl.inputs.len() == n_args,
-                            out_type.matches(&sig.decl.output),
-                            self_kind.matches(&sig.explicit_self.node)
-                        ], {
-                            span_lint(cx, SHOULD_IMPLEMENT_TRAIT, item.span, &format!(
-                                "defining a method called `{}` on this type; consider implementing \
-                                 the `{}` trait or choosing a less ambiguous name", name, trait_name));
+                if let MethodImplItem(ref sig, _) = item.node {
+                    // check missing trait implementations
+                    for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
+                        if_let_chain! {
+                            [
+                                name == method_name,
+                                sig.decl.inputs.len() == n_args,
+                                out_type.matches(&sig.decl.output),
+                                self_kind.matches(&sig.explicit_self.node)
+                            ], {
+                                span_lint(cx, SHOULD_IMPLEMENT_TRAIT, item.span, &format!(
+                                    "defining a method called `{}` on this type; consider implementing \
+                                     the `{}` trait or choosing a less ambiguous name", name, trait_name));
+                            }
+                        }
+                    }
+                    // check conventions w.r.t. conversion method names and predicates
+                    for &(prefix, self_kind) in &CONVENTIONS {
+                        if name.as_str().starts_with(prefix) && !self_kind.matches(&sig.explicit_self.node) {
+                            span_lint(cx, WRONG_SELF_CONVENTION, sig.explicit_self.span, &format!(
+                                "methods called `{}*` usually take {}; consider choosing a less \
+                                 ambiguous name", prefix, self_kind.description()));
                         }
                     }
                 }
@@ -88,6 +101,14 @@ fn check_item(&mut self, cx: &Context, item: &Item) {
     }
 }
 
+const CONVENTIONS: [(&'static str, SelfKind); 5] = [
+    ("into_", ValueSelf),
+    ("to_", RefSelf),
+    ("as_", RefSelf),
+    ("is_", RefSelf),
+    ("from_", NoSelf),
+];
+
 const TRAIT_METHODS: [(&'static str, usize, SelfKind, OutType, &'static str); 30] = [
     ("add",        2, ValueSelf,  AnyType,  "std::ops::Add`"),
     ("sub",        2, ValueSelf,  AnyType,  "std::ops::Sub"),
@@ -126,7 +147,7 @@ enum SelfKind {
     ValueSelf,
     RefSelf,
     RefMutSelf,
-    NoSelf
+    NoSelf,
 }
 
 impl SelfKind {
@@ -136,9 +157,28 @@ fn matches(&self, slf: &ExplicitSelf_) -> bool {
             (&RefSelf, &SelfRegion(_, Mutability::MutImmutable, _)) => true,
             (&RefMutSelf, &SelfRegion(_, Mutability::MutMutable, _)) => true,
             (&NoSelf, &SelfStatic) => true,
+            (_, &SelfExplicit(ref ty, _)) => self.matches_explicit_type(ty),
             _ => false
         }
     }
+
+    fn matches_explicit_type(&self, ty: &Ty) -> bool {
+        match (self, &ty.node) {
+            (&ValueSelf, &TyPath(..)) => true,
+            (&RefSelf, &TyRptr(_, MutTy { mutbl: Mutability::MutImmutable, .. })) => true,
+            (&RefMutSelf, &TyRptr(_, MutTy { mutbl: Mutability::MutMutable, .. })) => true,
+            _ => false
+        }
+    }
+
+    fn description(&self) -> &'static str {
+        match *self {
+            ValueSelf => "self by value",
+            RefSelf => "self by reference",
+            RefMutSelf => "self by mutable reference",
+            NoSelf => "no self",
+        }
+    }
 }
 
 #[derive(Clone, Copy)]

diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs
index 1c81fefc4e51cf6271835079febe81022c6b6d79..560f36a9d5dfcdffee28dc6c3b5758e31cc00690 100755 (executable)
--- a/tests/compile-fail/methods.rs
+++ b/tests/compile-fail/methods.rs
@@ -15,6 +15,9 @@ fn drop(&mut self) { } //~ERROR defining a method called `drop`
     fn sub(&self, other: T) -> &T { self } // no error, self is a ref
     fn div(self) -> T { self } // no error, different #arguments
     fn rem(self, other: T) { } // no error, wrong return type
+
+    fn into_u32(self) -> u32 { 0 } // fine
+    fn into_u16(&self) -> u16 { 0 } //~ERROR methods called `into_*` usually take self by value
 }
 
 impl Mul<T> for T {