Skip to content

Commit c41916d

Browse files
borsbors
committed
Auto merge of #5616 - euclio:no-derive-suggestion, r=phansch,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()`.
2 parents 7ca335a + a578bed commit c41916d

File tree

3 files changed

+64
-100
lines changed

3 files changed

+64
-100
lines changed

‎clippy_lints/src/new_without_default.rs

+23-95
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,20 @@
11
use crate::utils::paths;
22
use crate::utils::sugg::DiagnosticBuilderExt;
3-
use crate::utils::{get_trait_def_id, implements_trait, return_ty, same_tys, span_lint_hir_and_then};
3+
use crate::utils::{get_trait_def_id, return_ty, same_tys, span_lint_hir_and_then};
44
use if_chain::if_chain;
55
use rustc_errors::Applicability;
66
use rustc_hir as hir;
7-
use rustc_hir::def_id::DefId;
87
use rustc_hir::HirIdSet;
98
use rustc_lint::{LateContext, LateLintPass, LintContext};
109
use rustc_middle::lint::in_external_macro;
11-
use rustc_middle::ty::{self, Ty};
10+
use rustc_middle::ty::Ty;
1211
use rustc_session::{declare_tool_lint, impl_lint_pass};
13-
use rustc_span::source_map::Span;
1412

1513
declare_clippy_lint! {
1614
/// **What it does:** Checks for types with a `fn new() -> Self` method and no
1715
/// implementation of
1816
/// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html).
1917
///
20-
/// It detects both the case when a manual
21-
/// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html)
22-
/// implementation is required and also when it can be created with
23-
/// `#[derive(Default)]`
24-
///
2518
/// **Why is this bad?** The user might expect to be able to use
2619
/// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) as the
2720
/// type can be constructed without arguments.
@@ -40,46 +33,17 @@ declare_clippy_lint! {
4033
/// }
4134
/// ```
4235
///
43-
/// Instead, use:
36+
/// To fix the lint, and a `Default` implementation that delegates to `new`:
4437
///
4538
/// ```ignore
4639
/// struct Foo(Bar);
4740
///
4841
/// impl Default for Foo {
4942
/// fn default() -> Self {
50-
/// Foo(Bar::new())
43+
/// Foo::new()
5144
/// }
5245
/// }
5346
/// ```
54-
///
55-
/// Or, if
56-
/// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html)
57-
/// can be derived by `#[derive(Default)]`:
58-
///
59-
/// ```rust
60-
/// struct Foo;
61-
///
62-
/// impl Foo {
63-
/// fn new() -> Self {
64-
/// Foo
65-
/// }
66-
/// }
67-
/// ```
68-
///
69-
/// Instead, use:
70-
///
71-
/// ```rust
72-
/// #[derive(Default)]
73-
/// struct Foo;
74-
///
75-
/// impl Foo {
76-
/// fn new() -> Self {
77-
/// Foo
78-
/// }
79-
/// }
80-
/// ```
81-
///
82-
/// You can also have `new()` call `Default::default()`.
8347
pub NEW_WITHOUT_DEFAULT,
8448
style,
8549
"`fn new() -> Self` method without `Default` implementation"
@@ -158,46 +122,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault {
158122
}
159123
}
160124

161-
if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) {
162-
span_lint_hir_and_then(
163-
cx,
164-
NEW_WITHOUT_DEFAULT,
165-
id,
166-
impl_item.span,
167-
&format!(
168-
"you should consider deriving a `Default` implementation for `{}`",
169-
self_ty
170-
),
171-
|diag| {
172-
diag.suggest_item_with_attr(
173-
cx,
174-
sp,
175-
"try this",
176-
"#[derive(Default)]",
177-
Applicability::MaybeIncorrect,
178-
);
179-
});
180-
} else {
181-
span_lint_hir_and_then(
182-
cx,
183-
NEW_WITHOUT_DEFAULT,
184-
id,
185-
impl_item.span,
186-
&format!(
187-
"you should consider adding a `Default` implementation for `{}`",
188-
self_ty
189-
),
190-
|diag| {
191-
diag.suggest_prepend_item(
192-
cx,
193-
item.span,
194-
"try this",
195-
&create_new_without_default_suggest_msg(self_ty),
196-
Applicability::MaybeIncorrect,
197-
);
198-
},
199-
);
200-
}
125+
span_lint_hir_and_then(
126+
cx,
127+
NEW_WITHOUT_DEFAULT,
128+
id,
129+
impl_item.span,
130+
&format!(
131+
"you should consider adding a `Default` implementation for `{}`",
132+
self_ty
133+
),
134+
|diag| {
135+
diag.suggest_prepend_item(
136+
cx,
137+
item.span,
138+
"try this",
139+
&create_new_without_default_suggest_msg(self_ty),
140+
Applicability::MaybeIncorrect,
141+
);
142+
},
143+
);
201144
}
202145
}
203146
}
@@ -217,18 +160,3 @@ fn create_new_without_default_suggest_msg(ty: Ty<'_>) -> String {
217160
}}
218161
}}", ty)
219162
}
220-
221-
fn can_derive_default<'t, 'c>(ty: Ty<'t>, cx: &LateContext<'c, 't>, default_trait_id: DefId) -> Option<Span> {
222-
match ty.kind {
223-
ty::Adt(adt_def, substs) if adt_def.is_struct() => {
224-
for field in adt_def.all_fields() {
225-
let f_ty = field.ty(cx.tcx, substs);
226-
if !implements_trait(cx, f_ty, default_trait_id, &[]) {
227-
return None;
228-
}
229-
}
230-
Some(cx.tcx.def_span(adt_def.did))
231-
},
232-
_ => None,
233-
}
234-
}

‎tests/ui/new_without_default.rs

+11
Original file line numberDiff line numberDiff line change
@@ -148,4 +148,15 @@ impl AllowDerive {
148148
}
149149
}
150150

151+
pub struct NewNotEqualToDerive {
152+
foo: i32,
153+
}
154+
155+
impl NewNotEqualToDerive {
156+
// This `new` implementation is not equal to a derived `Default`, so do not suggest deriving.
157+
pub fn new() -> Self {
158+
NewNotEqualToDerive { foo: 1 }
159+
}
160+
}
161+
151162
fn main() {}

‎tests/ui/new_without_default.stderr

+30-5
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
error: you should consider deriving a `Default` implementation for `Foo`
1+
error: you should consider adding a `Default` implementation for `Foo`
22
--> $DIR/new_without_default.rs:8:5
33
|
44
LL | / pub fn new() -> Foo {
@@ -9,10 +9,14 @@ LL | | }
99
= note: `-D clippy::new-without-default` implied by `-D warnings`
1010
help: try this
1111
|
12-
LL | #[derive(Default)]
12+
LL | impl Default for Foo {
13+
LL | fn default() -> Self {
14+
LL | Self::new()
15+
LL | }
16+
LL | }
1317
|
1418

15-
error: you should consider deriving a `Default` implementation for `Bar`
19+
error: you should consider adding a `Default` implementation for `Bar`
1620
--> $DIR/new_without_default.rs:16:5
1721
|
1822
LL | / pub fn new() -> Self {
@@ -22,7 +26,11 @@ LL | | }
2226
|
2327
help: try this
2428
|
25-
LL | #[derive(Default)]
29+
LL | impl Default for Bar {
30+
LL | fn default() -> Self {
31+
LL | Self::new()
32+
LL | }
33+
LL | }
2634
|
2735

2836
error: you should consider adding a `Default` implementation for `LtKo<'c>`
@@ -42,5 +50,22 @@ LL | }
4250
LL | }
4351
|
4452

45-
error: aborting due to 3 previous errors
53+
error: you should consider adding a `Default` implementation for `NewNotEqualToDerive`
54+
--> $DIR/new_without_default.rs:157:5
55+
|
56+
LL | / pub fn new() -> Self {
57+
LL | | NewNotEqualToDerive { foo: 1 }
58+
LL | | }
59+
| |_____^
60+
|
61+
help: try this
62+
|
63+
LL | impl Default for NewNotEqualToDerive {
64+
LL | fn default() -> Self {
65+
LL | Self::new()
66+
LL | }
67+
LL | }
68+
|
69+
70+
error: aborting due to 4 previous errors
4671

0 commit comments

Comments
 (0)