Skip to content

Commit 189b4c3

Browse files
samueltardieusamueltardieu
authored
fix(new_without_default): if new has #[cfg], copy that onto impl Default (#15720)
Fixes #14552 changelog: [`new_without_default`]: if `new` has `#[cfg]`, copy that onto `impl Default`
2 parents 1a415e6 + 7a04ada commit 189b4c3

File tree

4 files changed

+300
-99
lines changed

4 files changed

+300
-99
lines changed

‎clippy_lints/src/new_without_default.rs‎

Lines changed: 103 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use clippy_utils::diagnostics::span_lint_hir_and_then;
22
use clippy_utils::return_ty;
3-
use clippy_utils::source::snippet;
3+
use clippy_utils::source::snippet_with_applicability;
44
use clippy_utils::sugg::DiagExt;
55
use rustc_errors::Applicability;
66
use rustc_hir as hir;
@@ -58,116 +58,132 @@ impl_lint_pass!(NewWithoutDefault => [NEW_WITHOUT_DEFAULT]);
5858

5959
impl<'tcx> LateLintPass<'tcx> for NewWithoutDefault {
6060
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
61-
if let hir::ItemKind::Impl(hir::Impl {
61+
let hir::ItemKind::Impl(hir::Impl {
6262
of_trait: None,
6363
generics,
6464
self_ty: impl_self_ty,
6565
..
6666
}) = item.kind
67+
else {
68+
return;
69+
};
70+
71+
for assoc_item in cx
72+
.tcx
73+
.associated_items(item.owner_id.def_id)
74+
.filter_by_name_unhygienic(sym::new)
6775
{
68-
for assoc_item in cx
69-
.tcx
70-
.associated_items(item.owner_id.def_id)
71-
.filter_by_name_unhygienic(sym::new)
76+
if let AssocKind::Fn { has_self: false, .. } = assoc_item.kind
77+
&& let assoc_item_hir_id = cx.tcx.local_def_id_to_hir_id(assoc_item.def_id.expect_local())
78+
&& let impl_item = cx.tcx.hir_node(assoc_item_hir_id).expect_impl_item()
79+
&& !impl_item.span.in_external_macro(cx.sess().source_map())
80+
&& let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind
81+
&& let id = impl_item.owner_id
82+
// can't be implemented for unsafe new
83+
&& !sig.header.is_unsafe()
84+
// shouldn't be implemented when it is hidden in docs
85+
&& !cx.tcx.is_doc_hidden(impl_item.owner_id.def_id)
86+
// when the result of `new()` depends on a parameter we should not require
87+
// an impl of `Default`
88+
&& impl_item.generics.params.is_empty()
89+
&& sig.decl.inputs.is_empty()
90+
&& cx.effective_visibilities.is_exported(impl_item.owner_id.def_id)
91+
&& let self_ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
92+
&& self_ty == return_ty(cx, impl_item.owner_id)
93+
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
7294
{
73-
if let AssocKind::Fn { has_self: false, .. } = assoc_item.kind {
74-
let impl_item = cx
75-
.tcx
76-
.hir_node_by_def_id(assoc_item.def_id.expect_local())
77-
.expect_impl_item();
78-
if impl_item.span.in_external_macro(cx.sess().source_map()) {
79-
return;
80-
}
81-
if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind {
82-
let id = impl_item.owner_id;
83-
if sig.header.is_unsafe() {
84-
// can't be implemented for unsafe new
85-
return;
86-
}
87-
if cx.tcx.is_doc_hidden(impl_item.owner_id.def_id) {
88-
// shouldn't be implemented when it is hidden in docs
89-
return;
90-
}
91-
if !impl_item.generics.params.is_empty() {
92-
// when the result of `new()` depends on a parameter we should not require
93-
// an impl of `Default`
94-
return;
95-
}
96-
if sig.decl.inputs.is_empty()
97-
&& cx.effective_visibilities.is_exported(impl_item.owner_id.def_id)
98-
&& let self_ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
99-
&& self_ty == return_ty(cx, impl_item.owner_id)
100-
&& let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
95+
if self.impling_types.is_none() {
96+
let mut impls = HirIdSet::default();
97+
for &d in cx.tcx.local_trait_impls(default_trait_id) {
98+
let ty = cx.tcx.type_of(d).instantiate_identity();
99+
if let Some(ty_def) = ty.ty_adt_def()
100+
&& let Some(local_def_id) = ty_def.did().as_local()
101101
{
102-
if self.impling_types.is_none() {
103-
let mut impls = HirIdSet::default();
104-
for &d in cx.tcx.local_trait_impls(default_trait_id) {
105-
let ty = cx.tcx.type_of(d).instantiate_identity();
106-
if let Some(ty_def) = ty.ty_adt_def()
107-
&& let Some(local_def_id) = ty_def.did().as_local()
108-
{
109-
impls.insert(cx.tcx.local_def_id_to_hir_id(local_def_id));
110-
}
111-
}
112-
self.impling_types = Some(impls);
113-
}
102+
impls.insert(cx.tcx.local_def_id_to_hir_id(local_def_id));
103+
}
104+
}
105+
self.impling_types = Some(impls);
106+
}
114107

115-
// Check if a Default implementation exists for the Self type, regardless of
116-
// generics
117-
if let Some(ref impling_types) = self.impling_types
118-
&& let self_def = cx.tcx.type_of(item.owner_id).instantiate_identity()
119-
&& let Some(self_def) = self_def.ty_adt_def()
120-
&& let Some(self_local_did) = self_def.did().as_local()
121-
&& let self_id = cx.tcx.local_def_id_to_hir_id(self_local_did)
122-
&& impling_types.contains(&self_id)
123-
{
124-
return;
125-
}
108+
// Check if a Default implementation exists for the Self type, regardless of
109+
// generics
110+
if let Some(ref impling_types) = self.impling_types
111+
&& let self_def = cx.tcx.type_of(item.owner_id).instantiate_identity()
112+
&& let Some(self_def) = self_def.ty_adt_def()
113+
&& let Some(self_local_did) = self_def.did().as_local()
114+
&& let self_id = cx.tcx.local_def_id_to_hir_id(self_local_did)
115+
&& impling_types.contains(&self_id)
116+
{
117+
return;
118+
}
126119

127-
let generics_sugg = snippet(cx, generics.span, "");
128-
let where_clause_sugg = if generics.has_where_clause_predicates {
129-
format!("\n{}\n", snippet(cx, generics.where_clause_span, ""))
130-
} else {
131-
String::new()
132-
};
133-
let self_ty_fmt = self_ty.to_string();
134-
let self_type_snip = snippet(cx, impl_self_ty.span, &self_ty_fmt);
135-
span_lint_hir_and_then(
136-
cx,
137-
NEW_WITHOUT_DEFAULT,
138-
id.into(),
139-
impl_item.span,
140-
format!("you should consider adding a `Default` implementation for `{self_type_snip}`"),
141-
|diag| {
142-
diag.suggest_prepend_item(
143-
cx,
144-
item.span,
145-
"try adding this",
146-
&create_new_without_default_suggest_msg(
147-
&self_type_snip,
148-
&generics_sugg,
149-
&where_clause_sugg,
150-
),
151-
Applicability::MachineApplicable,
152-
);
153-
},
154-
);
120+
let mut app = Applicability::MachineApplicable;
121+
let attrs_sugg = {
122+
let mut sugg = String::new();
123+
for attr in cx.tcx.hir_attrs(assoc_item_hir_id) {
124+
if !attr.has_name(sym::cfg_trace) {
125+
// This might be some other attribute that the `impl Default` ought to inherit.
126+
// But it could also be one of the many attributes that:
127+
// - can't be put on an impl block -- like `#[inline]`
128+
// - we can't even build a suggestion for, since `Attribute::span` may panic.
129+
//
130+
// Because of all that, remain on the safer side -- don't inherit this attr, and just
131+
// reduce the applicability
132+
app = Applicability::MaybeIncorrect;
133+
continue;
155134
}
135+
136+
sugg.push_str(&snippet_with_applicability(cx.sess(), attr.span(), "_", &mut app));
137+
sugg.push('\n');
156138
}
157-
}
139+
sugg
140+
};
141+
let generics_sugg = snippet_with_applicability(cx, generics.span, "", &mut app);
142+
let where_clause_sugg = if generics.has_where_clause_predicates {
143+
format!(
144+
"\n{}\n",
145+
snippet_with_applicability(cx, generics.where_clause_span, "", &mut app)
146+
)
147+
} else {
148+
String::new()
149+
};
150+
let self_ty_fmt = self_ty.to_string();
151+
let self_type_snip = snippet_with_applicability(cx, impl_self_ty.span, &self_ty_fmt, &mut app);
152+
span_lint_hir_and_then(
153+
cx,
154+
NEW_WITHOUT_DEFAULT,
155+
id.into(),
156+
impl_item.span,
157+
format!("you should consider adding a `Default` implementation for `{self_type_snip}`"),
158+
|diag| {
159+
diag.suggest_prepend_item(
160+
cx,
161+
item.span,
162+
"try adding this",
163+
&create_new_without_default_suggest_msg(
164+
&attrs_sugg,
165+
&self_type_snip,
166+
&generics_sugg,
167+
&where_clause_sugg,
168+
),
169+
app,
170+
);
171+
},
172+
);
158173
}
159174
}
160175
}
161176
}
162177

163178
fn create_new_without_default_suggest_msg(
179+
attrs_sugg: &str,
164180
self_type_snip: &str,
165181
generics_sugg: &str,
166182
where_clause_sugg: &str,
167183
) -> String {
168184
#[rustfmt::skip]
169185
format!(
170-
"impl{generics_sugg} Default for {self_type_snip}{where_clause_sugg} {{
186+
"{attrs_sugg}impl{generics_sugg} Default for {self_type_snip}{where_clause_sugg} {{
171187
fn default() -> Self {{
172188
Self::new()
173189
}}

‎tests/ui/new_without_default.fixed‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#![allow(
2-
dead_code,
32
clippy::missing_safety_doc,
43
clippy::extra_unused_lifetimes,
54
clippy::extra_unused_type_parameters,
@@ -323,6 +322,74 @@ where
323322
}
324323
}
325324

325+
// From issue #14552, but with `#[cfg]`s that are actually `true` in the uitest context
326+
327+
pub struct NewWithCfg;
328+
#[cfg(not(test))]
329+
impl Default for NewWithCfg {
330+
fn default() -> Self {
331+
Self::new()
332+
}
333+
}
334+
335+
impl NewWithCfg {
336+
#[cfg(not(test))]
337+
pub fn new() -> Self {
338+
//~^ new_without_default
339+
unimplemented!()
340+
}
341+
}
342+
343+
pub struct NewWith2Cfgs;
344+
#[cfg(not(test))]
345+
#[cfg(panic = "unwind")]
346+
impl Default for NewWith2Cfgs {
347+
fn default() -> Self {
348+
Self::new()
349+
}
350+
}
351+
352+
impl NewWith2Cfgs {
353+
#[cfg(not(test))]
354+
#[cfg(panic = "unwind")]
355+
pub fn new() -> Self {
356+
//~^ new_without_default
357+
unimplemented!()
358+
}
359+
}
360+
361+
pub struct NewWithExtraneous;
362+
impl Default for NewWithExtraneous {
363+
fn default() -> Self {
364+
Self::new()
365+
}
366+
}
367+
368+
impl NewWithExtraneous {
369+
#[inline]
370+
pub fn new() -> Self {
371+
//~^ new_without_default
372+
unimplemented!()
373+
}
374+
}
375+
376+
pub struct NewWithCfgAndExtraneous;
377+
#[cfg(not(test))]
378+
impl Default for NewWithCfgAndExtraneous {
379+
fn default() -> Self {
380+
Self::new()
381+
}
382+
}
383+
384+
impl NewWithCfgAndExtraneous {
385+
#[cfg(not(test))]
386+
#[inline]
387+
pub fn new() -> Self {
388+
//~^ new_without_default
389+
unimplemented!()
390+
}
391+
}
392+
326393
mod issue15778 {
327394
pub struct Foo(Vec<i32>);
328395

‎tests/ui/new_without_default.rs‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#![allow(
2-
dead_code,
32
clippy::missing_safety_doc,
43
clippy::extra_unused_lifetimes,
54
clippy::extra_unused_type_parameters,
@@ -266,6 +265,46 @@ where
266265
}
267266
}
268267

268+
// From issue #14552, but with `#[cfg]`s that are actually `true` in the uitest context
269+
270+
pub struct NewWithCfg;
271+
impl NewWithCfg {
272+
#[cfg(not(test))]
273+
pub fn new() -> Self {
274+
//~^ new_without_default
275+
unimplemented!()
276+
}
277+
}
278+
279+
pub struct NewWith2Cfgs;
280+
impl NewWith2Cfgs {
281+
#[cfg(not(test))]
282+
#[cfg(panic = "unwind")]
283+
pub fn new() -> Self {
284+
//~^ new_without_default
285+
unimplemented!()
286+
}
287+
}
288+
289+
pub struct NewWithExtraneous;
290+
impl NewWithExtraneous {
291+
#[inline]
292+
pub fn new() -> Self {
293+
//~^ new_without_default
294+
unimplemented!()
295+
}
296+
}
297+
298+
pub struct NewWithCfgAndExtraneous;
299+
impl NewWithCfgAndExtraneous {
300+
#[cfg(not(test))]
301+
#[inline]
302+
pub fn new() -> Self {
303+
//~^ new_without_default
304+
unimplemented!()
305+
}
306+
}
307+
269308
mod issue15778 {
270309
pub struct Foo(Vec<i32>);
271310

0 commit comments

Comments
 (0)