From 9cfc42275d400d5b192e10e005d4f7ef772156b7 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Tue, 17 May 2016 08:33:57 +0200 Subject: [PATCH] Split `new_without_default` and `new_without_default_derive`. This is still very slow, because we do a trait lookup for each field. Perhaps storing the visited types in a set to reuse types would improve performance somewhat. Also we may want to pre-decide some known types (e.g. `Vec`, `Option`). --- CHANGELOG.md | 1 + README.md | 3 +- src/lib.rs | 1 + src/new_without_default.rs | 84 ++++++++++++++++++++--- tests/compile-fail/methods.rs | 2 +- tests/compile-fail/new_without_default.rs | 6 +- 6 files changed, 82 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59b03b301c6..9a1e7322e76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -174,6 +174,7 @@ All notable changes to this project will be documented in this file. [`neg_multiply`]: https://github.com/Manishearth/rust-clippy/wiki#neg_multiply [`new_ret_no_self`]: https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self [`new_without_default`]: https://github.com/Manishearth/rust-clippy/wiki#new_without_default +[`new_without_default_derive`]: https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive [`no_effect`]: https://github.com/Manishearth/rust-clippy/wiki#no_effect [`non_ascii_literal`]: https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal [`nonminimal_bool`]: https://github.com/Manishearth/rust-clippy/wiki#nonminimal_bool diff --git a/README.md b/README.md index 6843afb4ede..4c156dfb7a0 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 150 lints included in this crate: +There are 151 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -107,6 +107,7 @@ name [neg_multiply](https://github.com/Manishearth/rust-clippy/wiki#neg_multiply) | warn | Warns on multiplying integers with -1 [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 +[new_without_default_derive](https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive) | warn | `fn new() -> Self` without `#[derive]`able `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 [nonminimal_bool](https://github.com/Manishearth/rust-clippy/wiki#nonminimal_bool) | allow | checks for boolean expressions that can be written more concisely diff --git a/src/lib.rs b/src/lib.rs index bcf8f86b10a..41c26cf7109 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -527,6 +527,7 @@ pub fn plugin_registrar(reg: &mut Registry) { needless_update::NEEDLESS_UPDATE, neg_multiply::NEG_MULTIPLY, new_without_default::NEW_WITHOUT_DEFAULT, + new_without_default::NEW_WITHOUT_DEFAULT_DERIVE, no_effect::NO_EFFECT, no_effect::UNNECESSARY_OPERATION, non_expressive_names::MANY_SINGLE_CHAR_NAMES, diff --git a/src/new_without_default.rs b/src/new_without_default.rs index 46021ec8836..08d517014ee 100644 --- a/src/new_without_default.rs +++ b/src/new_without_default.rs @@ -1,6 +1,8 @@ use rustc::hir::intravisit::FnKind; +use rustc::hir::def_id::DefId; use rustc::hir; use rustc::lint::*; +use rustc::ty; use syntax::ast; use syntax::codemap::Span; use utils::paths; @@ -20,11 +22,11 @@ /// **Example:** /// /// ```rust,ignore -/// struct Foo; +/// struct Foo(Bar); /// /// impl Foo { /// fn new() -> Self { -/// Foo +/// Foo(Bar::new()) /// } /// } /// ``` @@ -32,29 +34,58 @@ /// Instead, use: /// /// ```rust -/// struct Foo; +/// struct Foo(Bar); /// /// impl Default for Foo { /// fn default() -> Self { -/// Foo +/// Foo(Bar::new()) /// } /// } /// ``` /// /// You can also have `new()` call `Default::default()` -/// declare_lint! { pub NEW_WITHOUT_DEFAULT, Warn, "`fn new() -> Self` method without `Default` implementation" } +/// **What it does:** This lints about type with a `fn new() -> Self` method +/// and no implementation of +/// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) +/// +/// **Why is this bad?** User might expect to be able to use +/// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) +/// as the type can be +/// constructed without arguments. +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** +/// +/// ```rust,ignore +/// struct Foo; +/// +/// impl Foo { +/// fn new() -> Self { +/// Foo +/// } +/// } +/// ``` +/// +/// Just prepend `#[derive(Default)]` before the `struct` definition +declare_lint! { + pub NEW_WITHOUT_DEFAULT_DERIVE, + Warn, + "`fn new() -> Self` without `#[derive]`able `Default` implementation" +} + #[derive(Copy,Clone)] pub struct NewWithoutDefault; impl LintPass for NewWithoutDefault { fn get_lints(&self) -> LintArray { - lint_array!(NEW_WITHOUT_DEFAULT) + lint_array!(NEW_WITHOUT_DEFAULT, NEW_WITHOUT_DEFAULT_DERIVE) } } @@ -66,8 +97,8 @@ fn check_fn(&mut self, cx: &LateContext, kind: FnKind, decl: &hir::FnDecl, _: &h 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; - + let self_ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id( + cx.tcx.map.get_parent(id))).ty; if_let_chain!{[ self_ty.walk_shallow().next().is_none(), // implements_trait does not work with generics let Some(ret_ty) = return_ty(cx, id), @@ -75,10 +106,43 @@ fn check_fn(&mut self, cx: &LateContext, kind: FnKind, decl: &hir::FnDecl, _: &h let Some(default_trait_id) = get_trait_def_id(cx, &paths::DEFAULT_TRAIT), !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)); + if can_derive_default(self_ty, cx, default_trait_id) { + span_lint(cx, + NEW_WITHOUT_DEFAULT_DERIVE, span, + &format!("you should consider deriving a \ + `Default` implementation for `{}`", + self_ty)). + span_suggestion(span, + "try this", + "#[derive(Default)]".into()); + } else { + span_lint(cx, + NEW_WITHOUT_DEFAULT, span, + &format!("you should consider adding a \ + `Default` implementation for `{}`", + self_ty)). + span_suggestion(span, + "try this", + format!("impl Default for {} {{ fn default() -> \ + Self {{ {}::new() }} }}", self_ty, self_ty)); + } }} } } } } + +fn can_derive_default<'t, 'c>(ty: ty::Ty<'t>, cx: &LateContext<'c, 't>, default_trait_id: DefId) -> bool { + match ty.sty { + ty::TyStruct(ref adt_def, ref substs) => { + for field in adt_def.all_fields() { + let f_ty = field.ty(cx.tcx, substs); + if !implements_trait(cx, f_ty, default_trait_id, Vec::new()) { + return false + } + } + true + }, + _ => false + } +} diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index f6e4a9a31e0..78ffbb1a58a 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -3,7 +3,7 @@ #![plugin(clippy)] #![deny(clippy, clippy_pedantic)] -#![allow(blacklisted_name, unused, print_stdout, non_ascii_literal, new_without_default)] +#![allow(blacklisted_name, unused, print_stdout, non_ascii_literal, new_without_default, new_without_default_derive)] 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 index 30015f6c9e8..17f2a8d7b41 100644 --- a/tests/compile-fail/new_without_default.rs +++ b/tests/compile-fail/new_without_default.rs @@ -2,18 +2,18 @@ #![plugin(clippy)] #![allow(dead_code)] -#![deny(new_without_default)] +#![deny(new_without_default, new_without_default_derive)] struct Foo; impl Foo { - fn new() -> Foo { Foo } //~ERROR: you should consider adding a `Default` implementation for `Foo` + fn new() -> Foo { Foo } //~ERROR: you should consider deriving a `Default` implementation for `Foo` } struct Bar; impl Bar { - fn new() -> Self { Bar } //~ERROR: you should consider adding a `Default` implementation for `Bar` + fn new() -> Self { Bar } //~ERROR: you should consider deriving a `Default` implementation for `Bar` } struct Ok; -- 2.44.0