diff --git a/README.md b/README.md index 3b0a4f882..9f3c30d39 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 132 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 diff --git a/src/lib.rs b/src/lib.rs index bb9d9b620..8c619a1ea 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; @@ -177,6 +178,7 @@ pub fn plugin_registrar(reg: &mut Registry) { 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, @@ -285,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, diff --git a/src/methods.rs b/src/methods.rs index 94548cf96..663c7dd1b 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, 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]) 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> { /// 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 fae780e4c..c7aeb7f9a 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 000000000..466633649 --- /dev/null +++ b/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)); + } + } + } + } + } + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index b001d9530..46feafb1d 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,19 @@ pub fn unsugar_range(expr: &Expr) -> Option { None } } + +/// 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 + } +} diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index c450c9532..0acab8be4 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; diff --git a/tests/compile-fail/new_without_default.rs b/tests/compile-fail/new_without_default.rs new file mode 100755 index 000000000..5f00179a9 --- /dev/null +++ b/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() {}