Merge pull request #654 from mcarton/new

llogiq · llogiq · commit 5521a757077d · 2016-02-13T22:19:36.000+01:00
Lints about `new` methods
diff --git a/README.md b/README.md
@@ -8,7 +8,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 120 lints included in this crate:
+There are 121 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -78,6 +78,7 @@ name
 [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop)                     | warn    | for-looping over a range of indices where an iterator over items would do
 [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return)                             | warn    | using a return statement like `return expr;` where an expression would suffice
 [needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update)                             | warn    | using `{ ..base }` when there are no missing fields
+[new_ret_no_self](https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self)                             | warn    | not returning `Self` in a `new` method
 [no_effect](https://github.com/Manishearth/rust-clippy/wiki#no_effect)                                         | warn    | statements with no effect
 [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal)                         | allow   | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead
 [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options)           | warn    | nonsensical combination of options for opening a file
diff --git a/src/lib.rs b/src/lib.rs
@@ -234,6 +234,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         methods::CLONE_ON_COPY,
         methods::EXTEND_FROM_SLICE,
         methods::FILTER_NEXT,
+        methods::NEW_RET_NO_SELF,
         methods::OK_EXPECT,
         methods::OPTION_MAP_UNWRAP_OR,
         methods::OPTION_MAP_UNWRAP_OR_ELSE,
diff --git a/src/methods.rs b/src/methods.rs
@@ -1,11 +1,11 @@
-use rustc_front::hir::*;
 use rustc::lint::*;
-use rustc::middle::ty;
 use rustc::middle::subst::{Subst, TypeSpace};
-use std::iter;
+use rustc::middle::ty;
+use rustc_front::hir::*;
 use std::borrow::Cow;
-use syntax::ptr::P;
+use std::{fmt, iter};
 use syntax::codemap::Span;
+use syntax::ptr::P;
 
 use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, match_path, match_trait_method,
             match_type, method_chain_args, snippet, snippet_opt, span_lint, span_lint_and_then, span_note_and_lint,
@@ -274,11 +274,27 @@ declare_lint! {
 ///    println!("{:p} {:p}",*y, z); // prints out the same pointer
 /// }
 /// ```
-/// 
 declare_lint! {
     pub CLONE_DOUBLE_REF, Warn, "using `clone` on `&&T`"
 }
 
+/// **What it does:** This lint warns about `new` not returning `Self`.
+///
+/// **Why is this bad?** As a convention, `new` methods are used to make a new instance of a type.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// impl Foo {
+///     fn new(..) -> NotAFoo {
+///     }
+/// }
+/// ```
+declare_lint! {
+    pub NEW_RET_NO_SELF, Warn, "not returning `Self` in a `new` method"
+}
+
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
         lint_array!(EXTEND_FROM_SLICE,
@@ -295,7 +311,8 @@ impl LintPass for MethodsPass {
                     OR_FUN_CALL,
                     CHARS_NEXT_CMP,
                     CLONE_ON_COPY,
-                    CLONE_DOUBLE_REF)
+                    CLONE_DOUBLE_REF,
+                    NEW_RET_NO_SELF)
     }
 }
 
@@ -368,10 +385,11 @@ impl LateLintPass for MethodsPass {
                             }
                         }
                     }
+
                     // check conventions w.r.t. conversion method names and predicates
                     let is_copy = is_copy(cx, &ty, &item);
-                    for &(prefix, self_kinds) in &CONVENTIONS {
-                        if name.as_str().starts_with(prefix) &&
+                    for &(ref conv, self_kinds) in &CONVENTIONS {
+                        if conv.check(&name.as_str()) &&
                            !self_kinds.iter().any(|k| k.matches(&sig.explicit_self.node, is_copy)) {
                             let lint = if item.vis == Visibility::Public {
                                 WRONG_PUB_SELF_CONVENTION
@@ -381,15 +399,38 @@ impl LateLintPass for MethodsPass {
                             span_lint(cx,
                                       lint,
                                       sig.explicit_self.span,
-                                      &format!("methods called `{}*` usually take {}; consider choosing a less \
+                                      &format!("methods called `{}` usually take {}; consider choosing a less \
                                                 ambiguous name",
-                                               prefix,
+                                               conv,
                                                &self_kinds.iter()
                                                           .map(|k| k.description())
                                                           .collect::<Vec<_>>()
                                                           .join(" or ")));
                         }
                     }
+
+                    if &name.as_str() == &"new" {
+                        let returns_self = if let FunctionRetTy::Return(ref ret_ty) = sig.decl.output {
+                            let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
+                            let ty = ast_ty_to_ty_cache.get(&ty.id);
+                            let ret_ty = ast_ty_to_ty_cache.get(&ret_ty.id);
+
+                            match (ty, ret_ty) {
+                                (Some(&ty), Some(&ret_ty)) => ret_ty.walk().any(|t| t == ty),
+                                _ => false,
+                            }
+                        }
+                        else {
+                            false
+                        };
+
+                        if !returns_self {
+                            span_lint(cx,
+                                      NEW_RET_NO_SELF,
+                                      sig.explicit_self.span,
+                                      "methods called `new` usually return `Self`");
+                        }
+                    }
                 }
             }
         }
@@ -789,69 +830,61 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
     None
 }
 
-/// This checks whether a given type is known to implement Debug. It's
-/// conservative, i.e. it should not return false positives, but will return
-/// false negatives.
+/// This checks whether a given type is known to implement Debug.
 fn has_debug_impl<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool {
-    let no_ref_ty = walk_ptrs_ty(ty);
-    let debug = match cx.tcx.lang_items.debug_trait() {
-        Some(debug) => debug,
-        None => return false,
-    };
-    let debug_def = cx.tcx.lookup_trait_def(debug);
-    let mut debug_impl_exists = false;
-    debug_def.for_each_relevant_impl(cx.tcx, no_ref_ty, |d| {
-        let self_ty = &cx.tcx.impl_trait_ref(d).and_then(|im| im.substs.self_ty());
-        if let Some(self_ty) = *self_ty {
-            if !self_ty.flags.get().contains(ty::TypeFlags::HAS_PARAMS) {
-                debug_impl_exists = true;
-            }
-        }
-    });
-    debug_impl_exists
+    match cx.tcx.lang_items.debug_trait() {
+        Some(debug) => implements_trait(cx, ty, debug, Some(vec![])),
+        None => false,
+    }
+}
+
+enum Convention {
+    Eq(&'static str),
+    StartsWith(&'static str),
 }
 
 #[cfg_attr(rustfmt, rustfmt_skip)]
-const CONVENTIONS: [(&'static str, &'static [SelfKind]); 5] = [
-    ("into_", &[SelfKind::Value]),
-    ("to_", &[SelfKind::Ref]),
-    ("as_", &[SelfKind::Ref, SelfKind::RefMut]),
-    ("is_", &[SelfKind::Ref, SelfKind::No]),
-    ("from_", &[SelfKind::No]),
+const CONVENTIONS: [(Convention, &'static [SelfKind]); 6] = [
+    (Convention::Eq("new"), &[SelfKind::No]),
+    (Convention::StartsWith("as_"), &[SelfKind::Ref, SelfKind::RefMut]),
+    (Convention::StartsWith("from_"), &[SelfKind::No]),
+    (Convention::StartsWith("into_"), &[SelfKind::Value]),
+    (Convention::StartsWith("is_"), &[SelfKind::Ref, SelfKind::No]),
+    (Convention::StartsWith("to_"), &[SelfKind::Ref]),
 ];
 
 #[cfg_attr(rustfmt, rustfmt_skip)]
 const TRAIT_METHODS: [(&'static str, usize, SelfKind, OutType, &'static str); 30] = [
     ("add", 2, SelfKind::Value, OutType::Any, "std::ops::Add"),
-    ("sub", 2, SelfKind::Value, OutType::Any, "std::ops::Sub"),
-    ("mul", 2, SelfKind::Value, OutType::Any, "std::ops::Mul"),
-    ("div", 2, SelfKind::Value, OutType::Any, "std::ops::Div"),
-    ("rem", 2, SelfKind::Value, OutType::Any, "std::ops::Rem"),
-    ("shl", 2, SelfKind::Value, OutType::Any, "std::ops::Shl"),
-    ("shr", 2, SelfKind::Value, OutType::Any, "std::ops::Shr"),
+    ("as_mut", 1, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
+    ("as_ref", 1, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
     ("bitand", 2, SelfKind::Value, OutType::Any, "std::ops::BitAnd"),
     ("bitor", 2, SelfKind::Value, OutType::Any, "std::ops::BitOr"),
     ("bitxor", 2, SelfKind::Value, OutType::Any, "std::ops::BitXor"),
-    ("neg", 1, SelfKind::Value, OutType::Any, "std::ops::Neg"),
-    ("not", 1, SelfKind::Value, OutType::Any, "std::ops::Not"),
-    ("drop", 1, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
-    ("index", 2, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
-    ("index_mut", 2, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
-    ("deref", 1, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
-    ("deref_mut", 1, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
-    ("clone", 1, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
     ("borrow", 1, SelfKind::Ref, OutType::Ref, "std::borrow::Borrow"),
     ("borrow_mut", 1, SelfKind::RefMut, OutType::Ref, "std::borrow::BorrowMut"),
-    ("as_ref", 1, SelfKind::Ref, OutType::Ref, "std::convert::AsRef"),
-    ("as_mut", 1, SelfKind::RefMut, OutType::Ref, "std::convert::AsMut"),
-    ("eq", 2, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
+    ("clone", 1, SelfKind::Ref, OutType::Any, "std::clone::Clone"),
     ("cmp", 2, SelfKind::Ref, OutType::Any, "std::cmp::Ord"),
     ("default", 0, SelfKind::No, OutType::Any, "std::default::Default"),
-    ("hash", 2, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
-    ("next", 1, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
-    ("into_iter", 1, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
+    ("deref", 1, SelfKind::Ref, OutType::Ref, "std::ops::Deref"),
+    ("deref_mut", 1, SelfKind::RefMut, OutType::Ref, "std::ops::DerefMut"),
+    ("div", 2, SelfKind::Value, OutType::Any, "std::ops::Div"),
+    ("drop", 1, SelfKind::RefMut, OutType::Unit, "std::ops::Drop"),
+    ("eq", 2, SelfKind::Ref, OutType::Bool, "std::cmp::PartialEq"),
     ("from_iter", 1, SelfKind::No, OutType::Any, "std::iter::FromIterator"),
     ("from_str", 1, SelfKind::No, OutType::Any, "std::str::FromStr"),
+    ("hash", 2, SelfKind::Ref, OutType::Unit, "std::hash::Hash"),
+    ("index", 2, SelfKind::Ref, OutType::Ref, "std::ops::Index"),
+    ("index_mut", 2, SelfKind::RefMut, OutType::Ref, "std::ops::IndexMut"),
+    ("into_iter", 1, SelfKind::Value, OutType::Any, "std::iter::IntoIterator"),
+    ("mul", 2, SelfKind::Value, OutType::Any, "std::ops::Mul"),
+    ("neg", 1, SelfKind::Value, OutType::Any, "std::ops::Neg"),
+    ("next", 1, SelfKind::RefMut, OutType::Any, "std::iter::Iterator"),
+    ("not", 1, SelfKind::Value, OutType::Any, "std::ops::Not"),
+    ("rem", 2, SelfKind::Value, OutType::Any, "std::ops::Rem"),
+    ("shl", 2, SelfKind::Value, OutType::Any, "std::ops::Shl"),
+    ("shr", 2, SelfKind::Value, OutType::Any, "std::ops::Shr"),
+    ("sub", 2, SelfKind::Value, OutType::Any, "std::ops::Sub"),
 ];
 
 #[derive(Clone, Copy)]
@@ -896,6 +929,24 @@ impl SelfKind {
     }
 }
 
+impl Convention {
+    fn check(&self, other: &str) -> bool {
+        match *self {
+            Convention::Eq(this) => this == other,
+            Convention::StartsWith(this) => other.starts_with(this),
+        }
+    }
+}
+
+impl fmt::Display for Convention {
+    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+        match *self {
+            Convention::Eq(this) => this.fmt(f),
+            Convention::StartsWith(this) => this.fmt(f).and_then(|_| '*'.fmt(f)),
+        }
+    }
+}
+
 #[derive(Clone, Copy)]
 enum OutType {
     Unit,
diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs
@@ -22,15 +22,28 @@ impl T {
     fn into_u16(&self) -> u16 { 0 } //~ERROR methods called `into_*` usually take self by value
 
     fn to_something(self) -> u32 { 0 } //~ERROR methods called `to_*` usually take self by reference
+
+    fn new(self) {}
+    //~^ ERROR methods called `new` usually take no self
+    //~| ERROR methods called `new` usually return `Self`
 }
 
 #[derive(Clone,Copy)]
 struct U;
 
 impl U {
+    fn new() -> Self { U }
     fn to_something(self) -> u32 { 0 } // ok because U is Copy
 }
 
+struct V<T> {
+    _dummy: T
+}
+
+impl<T> V<T> {
+    fn new() -> Option<V<T>> { None }
+}
+
 impl Mul<T> for T {
     type Output = T;
     fn mul(self, other: T) -> T { self } // no error, obviously
@@ -274,10 +287,8 @@ fn main() {
     // the error type implements `Debug`
     let res2: Result<i32, MyError> = Ok(0);
     res2.ok().expect("oh noes!");
-    // we currently don't warn if the error type has a type parameter
-    // (but it would be nice if we did)
     let res3: Result<u32, MyErrorWithParam<u8>>= Ok(0);
-    res3.ok().expect("whoof");
+    res3.ok().expect("whoof"); //~ERROR called `ok().expect()`
     let res4: Result<u32, io::Error> = Ok(0);
     res4.ok().expect("argh"); //~ERROR called `ok().expect()`
     let res5: io::Result<u32> = Ok(0);
