From a578bed69ac2a9b33fcb871f9ad7dbf02355cb82 Mon Sep 17 00:00:00 2001 From: Andy Russell Date: Mon, 18 May 2020 18:35:49 -0400 Subject: [PATCH] new_without_default: do not suggest deriving --- clippy_lints/src/new_without_default.rs | 118 +++++------------------- tests/ui/new_without_default.rs | 11 +++ tests/ui/new_without_default.stderr | 35 ++++++- 3 files changed, 64 insertions(+), 100 deletions(-) diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index a599667b8d8..3b88e4c4cb1 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_without_default.rs @@ -1,27 +1,20 @@ use crate::utils::paths; use crate::utils::sugg::DiagnosticBuilderExt; -use crate::utils::{get_trait_def_id, implements_trait, return_ty, same_tys, span_lint_hir_and_then}; +use crate::utils::{get_trait_def_id, return_ty, same_tys, span_lint_hir_and_then}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::def_id::DefId; use rustc_hir::HirIdSet; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, Ty}; +use rustc_middle::ty::Ty; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::source_map::Span; declare_clippy_lint! { /// **What it does:** Checks for types with a `fn new() -> Self` method and no /// implementation of /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html). /// - /// It detects both the case when a manual - /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) - /// implementation is required and also when it can be created with - /// `#[derive(Default)]` - /// /// **Why is this bad?** The 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. @@ -40,46 +33,17 @@ /// } /// ``` /// - /// Instead, use: + /// To fix the lint, and a `Default` implementation that delegates to `new`: /// /// ```ignore /// struct Foo(Bar); /// /// impl Default for Foo { /// fn default() -> Self { - /// Foo(Bar::new()) + /// Foo::new() /// } /// } /// ``` - /// - /// Or, if - /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) - /// can be derived by `#[derive(Default)]`: - /// - /// ```rust - /// struct Foo; - /// - /// impl Foo { - /// fn new() -> Self { - /// Foo - /// } - /// } - /// ``` - /// - /// Instead, use: - /// - /// ```rust - /// #[derive(Default)] - /// struct Foo; - /// - /// impl Foo { - /// fn new() -> Self { - /// Foo - /// } - /// } - /// ``` - /// - /// You can also have `new()` call `Default::default()`. pub NEW_WITHOUT_DEFAULT, style, "`fn new() -> Self` method without `Default` implementation" @@ -158,46 +122,25 @@ fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item<'_>) } } - if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) { - span_lint_hir_and_then( - cx, - NEW_WITHOUT_DEFAULT, - id, - impl_item.span, - &format!( - "you should consider deriving a `Default` implementation for `{}`", - self_ty - ), - |diag| { - diag.suggest_item_with_attr( - cx, - sp, - "try this", - "#[derive(Default)]", - Applicability::MaybeIncorrect, - ); - }); - } else { - span_lint_hir_and_then( - cx, - NEW_WITHOUT_DEFAULT, - id, - impl_item.span, - &format!( - "you should consider adding a `Default` implementation for `{}`", - self_ty - ), - |diag| { - diag.suggest_prepend_item( - cx, - item.span, - "try this", - &create_new_without_default_suggest_msg(self_ty), - Applicability::MaybeIncorrect, - ); - }, - ); - } + span_lint_hir_and_then( + cx, + NEW_WITHOUT_DEFAULT, + id, + impl_item.span, + &format!( + "you should consider adding a `Default` implementation for `{}`", + self_ty + ), + |diag| { + diag.suggest_prepend_item( + cx, + item.span, + "try this", + &create_new_without_default_suggest_msg(self_ty), + Applicability::MaybeIncorrect, + ); + }, + ); } } } @@ -217,18 +160,3 @@ fn default() -> Self {{ }} }}", ty) } - -fn can_derive_default<'t, 'c>(ty: Ty<'t>, cx: &LateContext<'c, 't>, default_trait_id: DefId) -> Option { - match ty.kind { - ty::Adt(adt_def, substs) if adt_def.is_struct() => { - for field in adt_def.all_fields() { - let f_ty = field.ty(cx.tcx, substs); - if !implements_trait(cx, f_ty, default_trait_id, &[]) { - return None; - } - } - Some(cx.tcx.def_span(adt_def.did)) - }, - _ => None, - } -} diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs index 781ea7bb152..3b6041823d8 100644 --- a/tests/ui/new_without_default.rs +++ b/tests/ui/new_without_default.rs @@ -148,4 +148,15 @@ pub fn new() -> Self { } } +pub struct NewNotEqualToDerive { + foo: i32, +} + +impl NewNotEqualToDerive { + // This `new` implementation is not equal to a derived `Default`, so do not suggest deriving. + pub fn new() -> Self { + NewNotEqualToDerive { foo: 1 } + } +} + fn main() {} diff --git a/tests/ui/new_without_default.stderr b/tests/ui/new_without_default.stderr index 5e485d40663..e529e441eb7 100644 --- a/tests/ui/new_without_default.stderr +++ b/tests/ui/new_without_default.stderr @@ -1,4 +1,4 @@ -error: you should consider deriving a `Default` implementation for `Foo` +error: you should consider adding a `Default` implementation for `Foo` --> $DIR/new_without_default.rs:8:5 | LL | / pub fn new() -> Foo { @@ -9,10 +9,14 @@ LL | | } = note: `-D clippy::new-without-default` implied by `-D warnings` help: try this | -LL | #[derive(Default)] +LL | impl Default for Foo { +LL | fn default() -> Self { +LL | Self::new() +LL | } +LL | } | -error: you should consider deriving a `Default` implementation for `Bar` +error: you should consider adding a `Default` implementation for `Bar` --> $DIR/new_without_default.rs:16:5 | LL | / pub fn new() -> Self { @@ -22,7 +26,11 @@ LL | | } | help: try this | -LL | #[derive(Default)] +LL | impl Default for Bar { +LL | fn default() -> Self { +LL | Self::new() +LL | } +LL | } | error: you should consider adding a `Default` implementation for `LtKo<'c>` @@ -42,5 +50,22 @@ LL | } LL | } | -error: aborting due to 3 previous errors +error: you should consider adding a `Default` implementation for `NewNotEqualToDerive` + --> $DIR/new_without_default.rs:157:5 + | +LL | / pub fn new() -> Self { +LL | | NewNotEqualToDerive { foo: 1 } +LL | | } + | |_____^ + | +help: try this + | +LL | impl Default for NewNotEqualToDerive { +LL | fn default() -> Self { +LL | Self::new() +LL | } +LL | } + | + +error: aborting due to 4 previous errors