Lint types with fn new() -> Self and no Default impl

Author: mcarton

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 130 lints included in this crate:
+There are 131 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -83,6 +83,7 @@ name
 [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
+[new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default)                           | warn    | `fn new() -> Self` method without `Default` implementation
 [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

@@ -77,6 +77,7 @@ pub mod mutex_atomic;
 pub mod needless_bool;
 pub mod needless_features;
 pub mod needless_update;
+pub mod new_without_default;
 pub mod no_effect;
 pub mod open_options;
 pub mod panic;
@@ -175,6 +176,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box swap::Swap);
     reg.register_early_lint_pass(box if_not_else::IfNotElse);
     reg.register_late_lint_pass(box unused_label::UnusedLabel::new());
+    reg.register_late_lint_pass(box new_without_default::NewWithoutDefault);
 
     reg.register_lint_group("clippy_pedantic", vec![
         enum_glob_use::ENUM_GLOB_USE,
@@ -282,6 +284,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         needless_features::UNSTABLE_AS_MUT_SLICE,
         needless_features::UNSTABLE_AS_SLICE,
         needless_update::NEEDLESS_UPDATE,
+        new_without_default::NEW_WITHOUT_DEFAULT,
         no_effect::NO_EFFECT,
         open_options::NONSENSICAL_OPEN_OPTIONS,
         panic::PANIC_PARAMS,

src/methods.rs

@@ -10,8 +10,8 @@ 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,
-            walk_ptrs_ty, walk_ptrs_ty_depth};
+            match_type, method_chain_args, returns_self, snippet, snippet_opt, span_lint,
+            span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth};
 use utils::{BTREEMAP_ENTRY_PATH, DEFAULT_TRAIT_PATH, HASHMAP_ENTRY_PATH, OPTION_PATH, RESULT_PATH, STRING_PATH,
             VEC_PATH};
 use utils::MethodArgs;
@@ -431,26 +431,11 @@ impl LateLintPass for MethodsPass {
                         }
                     }
 
-                    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 ret_ty = ast_ty_to_ty_cache.get(&ret_ty.id);
-
-                            if let Some(&ret_ty) = ret_ty {
-                                ret_ty.walk().any(|t| t == ty)
-                            } else {
-                                false
-                            }
-                        } else {
-                            false
-                        };
-
-                        if !returns_self {
-                            span_lint(cx,
-                                      NEW_RET_NO_SELF,
-                                      sig.explicit_self.span,
-                                      "methods called `new` usually return `Self`");
-                        }
+                    if &name.as_str() == &"new" && !returns_self(cx, &sig.decl.output, ty)  {
+                        span_lint(cx,
+                                  NEW_RET_NO_SELF,
+                                  sig.explicit_self.span,
+                                  "methods called `new` usually return `Self`");
                     }
                 }
             }
@@ -485,7 +470,7 @@ fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P<Expr>])
                         return false;
                     };
 
-                    if implements_trait(cx, arg_ty, default_trait_id, None) {
+                    if implements_trait(cx, arg_ty, default_trait_id, Vec::new()) {
                         span_lint(cx,
                                   OR_FUN_CALL,
                                   span,
@@ -869,7 +854,7 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
 /// 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 {
     match cx.tcx.lang_items.debug_trait() {
-        Some(debug) => implements_trait(cx, ty, debug, Some(vec![])),
+        Some(debug) => implements_trait(cx, ty, debug, Vec::new()),
         None => false,
     }
 }

src/misc.rs

@@ -253,7 +253,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
         None => return,
     };
 
-    if !implements_trait(cx, arg_ty, partial_eq_trait_id, Some(vec![other_ty])) {
+    if !implements_trait(cx, arg_ty, partial_eq_trait_id, vec![other_ty]) {
         return;
     }
 

src/new_without_default.rs

@@ -0,0 +1,63 @@
+use rustc::lint::*;
+use rustc_front::hir;
+use rustc_front::intravisit::FnKind;
+use syntax::ast;
+use syntax::codemap::Span;
+use utils::{get_trait_def_id, implements_trait, in_external_macro, returns_self, span_lint, DEFAULT_TRAIT_PATH};
+
+/// **What it does:** This lints about type with a `fn new() -> Self` method and no `Default`
+/// implementation.
+///
+/// **Why is this bad?** User might expect to be able to use `Default` is the type can be
+/// constructed without arguments.
+///
+/// **Known problems:** Hopefully none.
+///
+/// **Example:**
+///
+/// ```rust,ignore
+/// struct Foo;
+///
+/// impl Foo {
+///     fn new() -> Self {
+///         Foo
+///     }
+/// }
+/// ```
+declare_lint! {
+    pub NEW_WITHOUT_DEFAULT,
+    Warn,
+    "`fn new() -> Self` method without `Default` implementation"
+}
+
+#[derive(Copy,Clone)]
+pub struct NewWithoutDefault;
+
+impl LintPass for NewWithoutDefault {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(NEW_WITHOUT_DEFAULT)
+    }
+}
+
+impl LateLintPass for NewWithoutDefault {
+    fn check_fn(&mut self, cx: &LateContext, kind: FnKind, decl: &hir::FnDecl, _: &hir::Block, span: Span, id: ast::NodeId) {
+        if in_external_macro(cx, span) {
+            return;
+        }
+
+        if let FnKind::Method(name, _, _) = kind {
+            if decl.inputs.is_empty() && name.as_str() == "new" {
+                let ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(cx.tcx.map.get_parent(id))).ty;
+
+                if  returns_self(cx, &decl.output, ty) {
+                    if let Some(default_trait_id) = get_trait_def_id(cx, &DEFAULT_TRAIT_PATH) {
+                        if !implements_trait(cx, ty, default_trait_id, Vec::new()) {
+                            span_lint(cx, NEW_WITHOUT_DEFAULT, span,
+                                      &format!("you should consider adding a `Default` implementation for `{}`", ty));
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

src/unused_label.rs

@@ -27,17 +27,15 @@ declare_lint! {
     "unused label"
 }
 
+#[derive(Default)]
 pub struct UnusedLabel {
     declared_labels: HashMap<InternedString, Span>,
     used_labels: HashSet<InternedString>,
 }
 
 impl UnusedLabel {
     pub fn new() -> UnusedLabel {
-        UnusedLabel {
-            declared_labels: HashMap::new(),
-            used_labels: HashSet::new(),
-        }
+        ::std::default::Default::default()
     }
 }
 

src/utils/mod.rs

@@ -258,7 +258,7 @@ pub fn get_trait_def_id(cx: &LateContext, path: &[&str]) -> Option<DefId> {
 /// Check whether a type implements a trait.
 /// See also `get_trait_def_id`.
 pub fn implements_trait<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>, trait_id: DefId,
-                                  ty_params: Option<Vec<ty::Ty<'tcx>>>)
+                                  ty_params: Vec<ty::Ty<'tcx>>)
                                   -> bool {
     cx.tcx.populate_implementations_for_trait_if_necessary(trait_id);
 
@@ -268,7 +268,7 @@ pub fn implements_trait<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>,
                                                      trait_id,
                                                      0,
                                                      ty,
-                                                     ty_params.unwrap_or_default());
+                                                     ty_params);
 
     traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)
 }
@@ -673,3 +673,19 @@ pub fn camel_case_from(s: &str) -> usize {
     }
     last_i
 }
+
+/// Return whether a method returns `Self`.
+pub fn returns_self(cx: &LateContext, ret: &FunctionRetTy, ty: ty::Ty) -> bool {
+    if let FunctionRetTy::Return(ref ret_ty) = *ret {
+        let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
+        let ret_ty = ast_ty_to_ty_cache.get(&ret_ty.id);
+
+        if let Some(&ret_ty) = ret_ty {
+            ret_ty.walk().any(|t| t == ty)
+        } else {
+            false
+        }
+    } else {
+        false
+    }
+}

tests/compile-fail/methods.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(clippy, clippy_pedantic)]
-#![allow(unused, print_stdout, non_ascii_literal)]
+#![allow(unused, print_stdout, non_ascii_literal, new_without_default)]
 
 use std::collections::BTreeMap;
 use std::collections::HashMap;

tests/compile-fail/new_without_default.rs

@@ -0,0 +1,35 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![allow(dead_code)]
+#![deny(new_without_default)]
+
+struct Foo;
+
+impl Foo {
+    fn new() -> Foo { Foo } //~ERROR: you should consider adding a `Default` implementation for `Foo`
+}
+
+struct Bar;
+
+impl Bar {
+    fn new() -> Self { Bar } //~ERROR: you should consider adding a `Default` implementation for `Bar`
+}
+
+struct Ok;
+
+impl Ok {
+    fn new() -> Self { Ok }
+}
+
+impl Default for Ok {
+    fn default() -> Self { Ok }
+}
+
+struct Params;
+
+impl Params {
+    fn new(_: u32) -> Self { Params }
+}
+
+fn main() {}