new lint: inherent methods that should be trait impls (fixes rust-lang#218)

Author: birkenfeld

README.md

@@ -4,7 +4,7 @@
 A collection of lints that give helpful tips to newbies and catch oversights.
 
 ##Lints
-There are 45 lints included in this crate:
+There are 46 lints included in this crate:
 
 name                     | default | meaning
 -------------------------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -44,6 +44,7 @@ ptr_arg                  | allow   | fn arguments of the type `&Vec<...>` or `&S
 range_step_by_zero       | warn    | using Range::step_by(0), which produces an infinite iterator
 redundant_closure        | warn    | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`)
 result_unwrap_used       | allow   | using `Result.unwrap()`, which might be better handled
+should_implement_trait   | warn    | defining a method that should be implementing a std trait
 single_match             | warn    | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead
 str_to_string            | warn    | using `to_string()` on a str, which should be `to_owned()`
 string_add               | allow   | using `x + ..` where x is a `String`; suggests using `push_str()` instead

src/lib.rs

@@ -92,6 +92,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         matches::SINGLE_MATCH,
         methods::OPTION_UNWRAP_USED,
         methods::RESULT_UNWRAP_USED,
+        methods::SHOULD_IMPLEMENT_TRAIT,
         methods::STR_TO_STRING,
         methods::STRING_TO_STRING,
         misc::CMP_NAN,

src/methods.rs

@@ -2,9 +2,12 @@ use syntax::ast::*;
 use rustc::lint::*;
 use rustc::middle::ty;
 
-use utils::{span_lint, match_type, walk_ptrs_ty};
+use utils::{span_lint, match_path, match_type, walk_ptrs_ty};
 use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
 
+use self::SelfKind::*;
+use self::OutType::*;
+
 #[derive(Copy,Clone)]
 pub struct MethodsPass;
 
@@ -16,10 +19,13 @@ declare_lint!(pub STR_TO_STRING, Warn,
               "using `to_string()` on a str, which should be `to_owned()`");
 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");
 
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING)
+        lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING,
+                    SHOULD_IMPLEMENT_TRAIT)
     }
 
     fn check_expr(&mut self, cx: &Context, expr: &Expr) {
@@ -46,4 +52,112 @@ impl LintPass for MethodsPass {
             }
         }
     }
+
+    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));
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+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"),
+    ("mul",        2, ValueSelf,  AnyType,  "std::ops::Mul"),
+    ("div",        2, ValueSelf,  AnyType,  "std::ops::Div"),
+    ("rem",        2, ValueSelf,  AnyType,  "std::ops::Rem"),
+    ("shl",        2, ValueSelf,  AnyType,  "std::ops::Shl"),
+    ("shr",        2, ValueSelf,  AnyType,  "std::ops::Shr"),
+    ("bitand",     2, ValueSelf,  AnyType,  "std::ops::BitAnd"),
+    ("bitor",      2, ValueSelf,  AnyType,  "std::ops::BitOr"),
+    ("bitxor",     2, ValueSelf,  AnyType,  "std::ops::BitXor"),
+    ("neg",        1, ValueSelf,  AnyType,  "std::ops::Neg"),
+    ("not",        1, ValueSelf,  AnyType,  "std::ops::Not"),
+    ("drop",       1, RefMutSelf, UnitType, "std::ops::Drop"),
+    ("index",      2, RefSelf,    RefType,  "std::ops::Index"),
+    ("index_mut",  2, RefMutSelf, RefType,  "std::ops::IndexMut"),
+    ("deref",      1, RefSelf,    RefType,  "std::ops::Deref"),
+    ("deref_mut",  1, RefMutSelf, RefType,  "std::ops::DerefMut"),
+    ("clone",      1, RefSelf,    AnyType,  "std::clone::Clone"),
+    ("borrow",     1, RefSelf,    RefType,  "std::borrow::Borrow"),
+    ("borrow_mut", 1, RefMutSelf, RefType,  "std::borrow::BorrowMut"),
+    ("as_ref",     1, RefSelf,    RefType,  "std::convert::AsRef"),
+    ("as_mut",     1, RefMutSelf, RefType,  "std::convert::AsMut"),
+    ("eq",         2, RefSelf,    BoolType, "std::cmp::PartialEq"),
+    ("cmp",        2, RefSelf,    AnyType,  "std::cmp::Ord"),
+    ("default",    0, NoSelf,     AnyType,  "std::default::Default"),
+    ("hash",       2, RefSelf,    UnitType, "std::hash::Hash"),
+    ("next",       1, RefMutSelf, AnyType,  "std::iter::Iterator"),
+    ("into_iter",  1, ValueSelf,  AnyType,  "std::iter::IntoIterator"),
+    ("from_iter",  1, NoSelf,     AnyType,  "std::iter::FromIterator"),
+    ("from_str",   1, NoSelf,     AnyType,  "std::str::FromStr"),
+];
+
+#[derive(Clone, Copy)]
+enum SelfKind {
+    ValueSelf,
+    RefSelf,
+    RefMutSelf,
+    NoSelf
+}
+
+impl SelfKind {
+    fn matches(&self, slf: &ExplicitSelf_) -> bool {
+        match (self, slf) {
+            (&ValueSelf, &SelfValue(_)) => true,
+            (&RefSelf, &SelfRegion(_, Mutability::MutImmutable, _)) => true,
+            (&RefMutSelf, &SelfRegion(_, Mutability::MutMutable, _)) => true,
+            (&NoSelf, &SelfStatic) => true,
+            _ => false
+        }
+    }
+}
+
+#[derive(Clone, Copy)]
+enum OutType {
+    UnitType,
+    BoolType,
+    AnyType,
+    RefType,
+}
+
+impl OutType {
+    fn matches(&self, ty: &FunctionRetTy) -> bool {
+        match (self, ty) {
+            (&UnitType, &DefaultReturn(_)) => true,
+            (&UnitType, &Return(ref ty)) if ty.node == TyTup(vec![]) => true,
+            (&BoolType, &Return(ref ty)) if is_bool(ty) => true,
+            (&AnyType, &Return(ref ty)) if ty.node != TyTup(vec![])  => true,
+            (&RefType, &Return(ref ty)) => {
+                if let TyRptr(_, _) = ty.node { true } else { false }
+            }
+            _ => false
+        }
+    }
+}
+
+fn is_bool(ty: &Ty) -> bool {
+    if let TyPath(None, ref p) = ty.node {
+        if match_path(p, &["bool"]) {
+            return true;
+        }
+    }
+    false
 }

tests/compile-fail/methods.rs

@@ -1,8 +1,27 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
-#[deny(option_unwrap_used, result_unwrap_used)]
-#[deny(str_to_string, string_to_string)]
+#![allow(unused)]
+#![deny(clippy)]
+
+use std::ops::Mul;
+
+struct T;
+
+impl T {
+    fn add(self, other: T) -> T { self } //~ERROR defining a method called `add`
+    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
+}
+
+impl Mul<T> for T {
+    type Output = T;
+    fn mul(self, other: T) -> T { self } // no error, obviously
+}
+
 fn main() {
     let opt = Some(0);
     let _ = opt.unwrap();  //~ERROR used unwrap() on an Option