Skip to content

Commit

Permalink
Auto merge of rust-lang#5616 - euclio:no-derive-suggestion, r=phansch…
Browse files Browse the repository at this point in the history 
…,flip1995

new_without_default: do not suggest deriving

---

changelog: do not suggest deriving `Default` in `new_without_default`

This commit changes the behavior of the `new_without_default` lint to not suggest deriving `Default`. This suggestion is misleading if the `new` implementation does something different than what a derived `Default` implementation would do, because then the two methods would not be equivalent.

Instead, the `can_derive_default` check is removed, and we always suggest implementing `Default` in terms of `new()`.
  • Loading branch information
@bors
bors committed May 25, 2020
2 parents 7ca335a + a578bed commit c41916d
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 100 deletions.
118 changes: 23 additions & 95 deletions clippy_lints/src/new_without_default.rs
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -40,46 +33,17 @@ declare_clippy_lint! {
/// }
/// ```
///
/// 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"
Expand Down Expand Up @@ -158,46 +122,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault {
}
}

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,
);
},
);
}
}
}
Expand All @@ -217,18 +160,3 @@ fn create_new_without_default_suggest_msg(ty: Ty<'_>) -> String {
}}
}}", ty)
}

fn can_derive_default<'t, 'c>(ty: Ty<'t>, cx: &LateContext<'c, 't>, default_trait_id: DefId) -> Option<Span> {
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,
}
}
11 changes: 11 additions & 0 deletions tests/ui/new_without_default.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,4 +148,15 @@ impl AllowDerive {
}
}

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() {}
35 changes: 30 additions & 5 deletions tests/ui/new_without_default.stderr
View file
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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 {
Expand All @@ -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>`
Expand All @@ -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

0 comments on commit c41916d

Please sign in to comment.