mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #5616 - euclio:no-derive-suggestion, r=phansch,flip1995

Browse Source 
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()`.
This commit is contained in:
bors 2020-05-25 17:19:00 +00:00
commit c41916d9bd
3 changed files with 64 additions and 100 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

118
clippy_lints/src/new_without_default.rs
View File

@ -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<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
tests/ui/new_without_default.rs
View File

@ -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() {}

35
tests/ui/new_without_default.stderr
View File

@ -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