Merge pull request #730 from mcarton/unused-labels

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

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 131 lints included in this crate:
+There are 133 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
@@ -131,6 +132,7 @@ name
 [unstable_as_mut_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_mut_slice)                       | warn    | as_mut_slice is not stable and can be replaced by &mut v[..]see https://github.com/rust-lang/rust/issues/27729
 [unstable_as_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_slice)                               | warn    | as_slice is not stable and can be replaced by & v[..]see https://github.com/rust-lang/rust/issues/27729
 [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
+[unused_label](https://github.com/Manishearth/rust-clippy/wiki#unused_label)                                         | warn    | unused label
 [unused_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#unused_lifetimes)                                 | warn    | unused lifetimes in function definitions
 [use_debug](https://github.com/Manishearth/rust-clippy/wiki#use_debug)                                               | allow   | use `Debug`-based formatting
 [used_underscore_binding](https://github.com/Manishearth/rust-clippy/wiki#used_underscore_binding)                   | warn    | using a binding which is prefixed with an underscore

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 overflow_check_conditional;
@@ -94,6 +95,7 @@ pub mod temporary_assignment;
 pub mod transmute;
 pub mod types;
 pub mod unicode;
+pub mod unused_label;
 pub mod vec;
 pub mod zero_div_zero;
 // end lints modules, do not remove this comment, it’s used in `update_lints`
@@ -175,6 +177,8 @@ 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 overflow_check_conditional::OverflowCheckConditional);
+    reg.register_late_lint_pass(box unused_label::UnusedLabel);
+    reg.register_late_lint_pass(box new_without_default::NewWithoutDefault);
 
     reg.register_lint_group("clippy_pedantic", vec![
         enum_glob_use::ENUM_GLOB_USE,
@@ -283,6 +287,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,
         overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
@@ -309,6 +314,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         types::TYPE_COMPLEXITY,
         types::UNIT_CMP,
         unicode::ZERO_WIDTH_SPACE,
+        unused_label::UNUSED_LABEL,
         vec::USELESS_VEC,
         zero_div_zero::ZERO_DIVIDED_BY_ZERO,
     ]);

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,
+            match_type, method_chain_args, return_ty, same_tys, snippet, snippet_opt, span_lint,
-            walk_ptrs_ty, walk_ptrs_ty_depth};
+            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,12 @@ impl LateLintPass for MethodsPass {
                         }
                     }
 
-                    if &name.as_str() == &"new" {
+                    let ret_ty = return_ty(cx.tcx.node_id_to_type(implitem.id));
-                        let returns_self = if let FunctionRetTy::Return(ref ret_ty) = sig.decl.output {
+                    if &name.as_str() == &"new" && !ret_ty.map_or(false, |ret_ty| ret_ty.walk().any(|t| same_tys(cx, t, ty))) {
-                            let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
+                        span_lint(cx,
-                            let ret_ty = ast_ty_to_ty_cache.get(&ret_ty.id);
+                                  NEW_RET_NO_SELF,
-
+                                  sig.explicit_self.span,
-                            if let Some(&ret_ty) = ret_ty {
+                                  "methods called `new` usually return `Self`");
-                                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`");
-                        }
                     }
                 }
             }
@@ -485,7 +471,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 +855,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,65 @@
+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, return_ty, same_tys, 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 self_ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(cx.tcx.map.get_parent(id))).ty;
+
+                if_let_chain!{[
+                    let Some(ret_ty) = return_ty(cx.tcx.node_id_to_type(id)),
+                    same_tys(cx, self_ty, ret_ty),
+                    let Some(default_trait_id) = get_trait_def_id(cx, &DEFAULT_TRAIT_PATH),
+                    !implements_trait(cx, self_ty, default_trait_id, Vec::new())
+                ], {
+                    span_lint(cx, NEW_WITHOUT_DEFAULT, span,
+                              &format!("you should consider adding a `Default` implementation for `{}`", self_ty));
+                }}
+            }
+        }
+    }
+}

src/ptr_arg.rs

@@ -6,6 +6,7 @@ use rustc::front::map::NodeItem;
 use rustc::lint::*;
 use rustc::middle::ty;
 use rustc_front::hir::*;
+use syntax::ast::NodeId;
 use utils::{STRING_PATH, VEC_PATH};
 use utils::{span_lint, match_type};
 
@@ -35,7 +36,7 @@ impl LintPass for PtrArg {
 impl LateLintPass for PtrArg {
     fn check_item(&mut self, cx: &LateContext, item: &Item) {
         if let ItemFn(ref decl, _, _, _, _, _) = item.node {
-            check_fn(cx, decl);
+            check_fn(cx, decl, item.id);
         }
     }
 
@@ -46,34 +47,34 @@ impl LateLintPass for PtrArg {
                     return; // ignore trait impls
                 }
             }
-            check_fn(cx, &sig.decl);
+            check_fn(cx, &sig.decl, item.id);
         }
     }
 
     fn check_trait_item(&mut self, cx: &LateContext, item: &TraitItem) {
         if let MethodTraitItem(ref sig, _) = item.node {
-            check_fn(cx, &sig.decl);
+            check_fn(cx, &sig.decl, item.id);
         }
     }
 }
 
-fn check_fn(cx: &LateContext, decl: &FnDecl) {
+fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
-    for arg in &decl.inputs {
+    let fn_ty = cx.tcx.node_id_to_type(fn_id).fn_sig().skip_binder();
-        if let Some(ty) = cx.tcx.ast_ty_to_ty_cache.borrow().get(&arg.ty.id) {
+
-            if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = ty.sty {
+    for (arg, ty) in decl.inputs.iter().zip(&fn_ty.inputs) {
-                if match_type(cx, ty, &VEC_PATH) {
+        if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = ty.sty {
-                    span_lint(cx,
+            if match_type(cx, ty, &VEC_PATH) {
-                              PTR_ARG,
+                span_lint(cx,
-                              arg.ty.span,
+                          PTR_ARG,
-                              "writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \
+                          arg.ty.span,
-                               with non-Vec-based slices. Consider changing the type to `&[...]`");
+                          "writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \
-                } else if match_type(cx, ty, &STRING_PATH) {
+                           with non-Vec-based slices. Consider changing the type to `&[...]`");
-                    span_lint(cx,
+            } else if match_type(cx, ty, &STRING_PATH) {
-                              PTR_ARG,
+                span_lint(cx,
-                              arg.ty.span,
+                          PTR_ARG,
-                              "writing `&String` instead of `&str` involves a new object where a slice will do. \
+                          arg.ty.span,
-                               Consider changing the type to `&str`");
+                          "writing `&String` instead of `&str` involves a new object where a slice will do. \
-                }
+                           Consider changing the type to `&str`");
             }
         }
     }

src/unused_label.rs

@@ -0,0 +1,78 @@
+use rustc::lint::*;
+use rustc_front::hir;
+use rustc_front::intravisit::{FnKind, Visitor, walk_expr, walk_fn};
+use std::collections::HashMap;
+use syntax::ast;
+use syntax::codemap::Span;
+use syntax::parse::token::InternedString;
+use utils::{in_macro, span_lint};
+
+/// **What it does:** This lint checks for unused labels.
+///
+/// **Why is this bad?** Maybe the label should be used in which case there is an error in the
+/// code or it should be removed.
+///
+/// **Known problems:** Hopefully none.
+///
+/// **Example:**
+/// ```rust,ignore
+/// fn unused_label() {
+///     'label: for i in 1..2 {
+///         if i > 4 { continue }
+///     }
+/// ```
+declare_lint! {
+    pub UNUSED_LABEL,
+    Warn,
+    "unused label"
+}
+
+pub struct UnusedLabel;
+
+#[derive(Default)]
+struct UnusedLabelVisitor {
+    labels: HashMap<InternedString, Span>,
+}
+
+impl UnusedLabelVisitor {
+    pub fn new() -> UnusedLabelVisitor {
+        ::std::default::Default::default()
+    }
+}
+
+impl LintPass for UnusedLabel {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(UNUSED_LABEL)
+    }
+}
+
+impl LateLintPass for UnusedLabel {
+    fn check_fn(&mut self, cx: &LateContext, kind: FnKind, decl: &hir::FnDecl, body: &hir::Block, span: Span, _: ast::NodeId) {
+        if in_macro(cx, span) {
+            return;
+        }
+
+        let mut v = UnusedLabelVisitor::new();
+        walk_fn(&mut v, kind, decl, body, span);
+
+        for (label, span) in v.labels {
+            span_lint(cx, UNUSED_LABEL, span, &format!("unused label `{}`", label));
+        }
+    }
+}
+
+impl<'v> Visitor<'v> for UnusedLabelVisitor {
+    fn visit_expr(&mut self, expr: &hir::Expr) {
+        match expr.node {
+            hir::ExprBreak(Some(label)) | hir::ExprAgain(Some(label)) => {
+                self.labels.remove(&label.node.name.as_str());
+            }
+            hir::ExprLoop(_, Some(label)) | hir::ExprWhile(_, _, Some(label)) => {
+                self.labels.insert(label.name.as_str(), expr.span);
+            }
+            _ => (),
+        }
+
+        walk_expr(self, expr);
+    }
+}

src/utils/mod.rs

@@ -264,7 +264,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);
 
@@ -274,7 +274,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)
 }
@@ -731,3 +731,20 @@ pub fn unsugar_range(expr: &Expr) -> Option<UnsugaredRange> {
         None
     }
 }
+
+/// Convenience function to get the return type of a function or `None` if the function diverges.
+pub fn return_ty(fun: ty::Ty) -> Option<ty::Ty> {
+    if let ty::FnConverging(ret_ty) = fun.fn_sig().skip_binder().output {
+        Some(ret_ty)
+    } else {
+        None
+    }
+}
+
+/// Check if two types are the same.
+// FIXME: this works correctly for lifetimes bounds (`for <'a> Foo<'a>` == `for <'b> Foo<'b>` but
+// not for type parameters.
+pub fn same_tys<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, a: ty::Ty<'tcx>, b: ty::Ty<'tcx>) -> bool {
+    let infcx = infer::new_infer_ctxt(cx.tcx, &cx.tcx.tables, None);
+    infcx.can_equate(&cx.tcx.erase_regions(&a), &cx.tcx.erase_regions(&b)).is_ok()
+}

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;
@@ -28,6 +28,25 @@ impl T {
     //~| ERROR methods called `new` usually return `Self`
 }
 
+struct Lt<'a> {
+    foo: &'a u32,
+}
+
+impl<'a> Lt<'a> {
+    // The lifetime is different, but that’s irrelevant, see #734
+    #[allow(needless_lifetimes)]
+    pub fn new<'b>(s: &'b str) -> Lt<'b> { unimplemented!() }
+}
+
+struct Lt2<'a> {
+    foo: &'a u32,
+}
+
+impl<'a> Lt2<'a> {
+    // The lifetime is different, but that’s irrelevant, see #734
+    pub fn new(s: &str) -> Lt2 { unimplemented!() }
+}
+
 #[derive(Clone,Copy)]
 struct U;
 

tests/compile-fail/new_without_default.rs

@@ -0,0 +1,44 @@
+#![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 }
+}
+
+struct Generics<'a, T> {
+    foo: &'a bool,
+    bar: T,
+}
+
+impl<'c, V> Generics<'c, V> {
+    fn new<'b>() -> Generics<'b, V> { unimplemented!() } //~ERROR: you should consider adding a `Default` implementation for
+}
+
+fn main() {}

tests/compile-fail/unused_labels.rs

@@ -0,0 +1,35 @@
+#![plugin(clippy)]
+#![feature(plugin)]
+
+#![allow(dead_code, items_after_statements)]
+#![deny(unused_label)]
+
+fn unused_label() {
+    'label: for i in 1..2 { //~ERROR: unused label `'label`
+        if i > 4 { continue }
+    }
+}
+
+fn foo() {
+    'same_label_in_two_fns: loop {
+        break 'same_label_in_two_fns;
+    }
+}
+
+
+fn bla() {
+    'a: loop { break } //~ERROR: unused label `'a`
+    fn blub() {}
+}
+
+fn main() {
+    'a: for _ in 0..10 {
+        while let Some(42) = None {
+            continue 'a;
+        }
+    }
+
+    'same_label_in_two_fns: loop { //~ERROR: unused label `'same_label_in_two_fns`
+        let _ = 1;
+    }
+}