Lint about new methods not returning Self

Author: mcarton

README.md

@@ -6,7 +6,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
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -76,6 +76,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

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,

src/methods.rs

@@ -278,6 +278,23 @@ 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,
@@ -294,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)
     }
 }
 
@@ -390,6 +408,29 @@ impl LateLintPass for MethodsPass {
                                                           .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)) => ty == ret_ty,
+                                _ => false,
+                            }
+                        }
+                        else {
+                            false
+                        };
+
+                        if !returns_self {
+                            span_lint(cx,
+                                      NEW_RET_NO_SELF,
+                                      sig.explicit_self.span,
+                                      "methods called `new` usually return `Self`");
+                        }
+                    }
                 }
             }
         }

tests/compile-fail/methods.rs

@@ -23,13 +23,16 @@ impl T {
 
     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
+    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
 }