Merge pull request #272 from birkenfeld/conventions

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

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!

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,

src/methods.rs

@@ -23,11 +23,14 @@ declare_lint!(pub STRING_TO_STRING, Warn,
               "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 @@ impl LintPass for MethodsPass {
         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 @@ impl LintPass for MethodsPass {
     }
 }
 
+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 @@ impl SelfKind {
             (&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)]

tests/compile-fail/methods.rs

@@ -15,6 +15,9 @@ impl T {
     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 {