diff --git a/README.md b/README.md index 77518df6b399..9f3c30d39b3f 100644 --- 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 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 diff --git a/src/lib.rs b/src/lib.rs index 51292bea8b2f..8c619a1eaf72 100644 --- a/src/lib.rs +++ b/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, ]); diff --git a/src/methods.rs b/src/methods.rs index 94548cf9672d..6d33f31d45c5 100644 --- a/src/methods.rs +++ b/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, return_ty, same_tys, 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,12 @@ 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`"); - } + let ret_ty = return_ty(cx.tcx.node_id_to_type(implitem.id)); + if &name.as_str() == &"new" && !ret_ty.map_or(false, |ret_ty| ret_ty.walk().any(|t| same_tys(cx, t, ty))) { + 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]) 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> { /// 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, } } diff --git a/src/misc.rs b/src/misc.rs index fae780e4ced0..c7aeb7f9a2f6 100644 --- a/src/misc.rs +++ b/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; } diff --git a/src/new_without_default.rs b/src/new_without_default.rs new file mode 100644 index 000000000000..d341afb4d921 --- /dev/null +++ b/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)); + }} + } + } + } +} diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index c02e5609b8c7..85baf3310c3d 100644 --- a/src/ptr_arg.rs +++ b/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) { - for arg in &decl.inputs { - 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 { - if match_type(cx, ty, &VEC_PATH) { - span_lint(cx, - PTR_ARG, - arg.ty.span, - "writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \ - with non-Vec-based slices. Consider changing the type to `&[...]`"); - } else if match_type(cx, ty, &STRING_PATH) { - span_lint(cx, - PTR_ARG, - arg.ty.span, - "writing `&String` instead of `&str` involves a new object where a slice will do. \ - Consider changing the type to `&str`"); - } +fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) { + let fn_ty = cx.tcx.node_id_to_type(fn_id).fn_sig().skip_binder(); + + for (arg, ty) in decl.inputs.iter().zip(&fn_ty.inputs) { + if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = ty.sty { + if match_type(cx, ty, &VEC_PATH) { + span_lint(cx, + PTR_ARG, + arg.ty.span, + "writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \ + with non-Vec-based slices. Consider changing the type to `&[...]`"); + } else if match_type(cx, ty, &STRING_PATH) { + span_lint(cx, + PTR_ARG, + arg.ty.span, + "writing `&String` instead of `&str` involves a new object where a slice will do. \ + Consider changing the type to `&str`"); } } } diff --git a/src/unused_label.rs b/src/unused_label.rs new file mode 100644 index 000000000000..f2ecad7cc821 --- /dev/null +++ b/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, +} + +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); + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index b001d9530edd..c626fcb8930c 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -264,7 +264,7 @@ pub fn get_trait_def_id(cx: &LateContext, path: &[&str]) -> Option { /// 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>>) + ty_params: Vec>) -> 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 { 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 { + 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() +} diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index c450c9532847..46f14d5d921c 100644 --- a/tests/compile-fail/methods.rs +++ b/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; diff --git a/tests/compile-fail/new_without_default.rs b/tests/compile-fail/new_without_default.rs new file mode 100755 index 000000000000..cc033043bc59 --- /dev/null +++ b/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() {} diff --git a/tests/compile-fail/unused_labels.rs b/tests/compile-fail/unused_labels.rs new file mode 100755 index 000000000000..26b4d4a2f3b2 --- /dev/null +++ b/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; + } +}