Skip to content

Commit 652b4c7

Browse files
committed
Auto merge of rust-lang#10616 - y21:missing_field_in_debug, r=Alexendoo
new lint: `missing_fields_in_debug` Fixes rust-lang#10429 This PR adds a new lint that looks for manual `Debug` implementations that do not "use" all of the fields. This often happens when adding a new field to a struct. It also acts as a style lint in case leaving out a field was intentional. In that case, it's preferred to use [`DebugStruct::finish_non_exhaustive`](https://doc.rust-lang.org/stable/std/fmt/struct.DebugStruct.html#method.finish_non_exhaustive), which indicates that there are more fields that were explicitly not shown. ``` changelog: [`missing_fields_in_debug`]: missing fields in manual `Debug` implementation ```
2 parents 594a2cb + a859b0e commit 652b4c7

File tree

7 files changed

+504
-0
lines changed

7 files changed

+504
-0
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -4963,6 +4963,7 @@ Released 2018-09-13
49634963
[`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items
49644964
[`missing_enforced_import_renames`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_enforced_import_renames
49654965
[`missing_errors_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc
4966+
[`missing_fields_in_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_fields_in_debug
49664967
[`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items
49674968
[`missing_panics_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc
49684969
[`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc

clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
430430
crate::missing_const_for_fn::MISSING_CONST_FOR_FN_INFO,
431431
crate::missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS_INFO,
432432
crate::missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES_INFO,
433+
crate::missing_fields_in_debug::MISSING_FIELDS_IN_DEBUG_INFO,
433434
crate::missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS_INFO,
434435
crate::missing_trait_methods::MISSING_TRAIT_METHODS_INFO,
435436
crate::mixed_read_write_in_expression::DIVERGING_SUB_EXPRESSION_INFO,

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ mod missing_assert_message;
203203
mod missing_const_for_fn;
204204
mod missing_doc;
205205
mod missing_enforced_import_rename;
206+
mod missing_fields_in_debug;
206207
mod missing_inline;
207208
mod missing_trait_methods;
208209
mod mixed_read_write_in_expression;
@@ -994,6 +995,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
994995
store.register_early_pass(|| Box::new(ref_patterns::RefPatterns));
995996
store.register_late_pass(|_| Box::new(default_constructed_unit_structs::DefaultConstructedUnitStructs));
996997
store.register_early_pass(|| Box::new(needless_else::NeedlessElse));
998+
store.register_late_pass(|_| Box::new(missing_fields_in_debug::MissingFieldsInDebug));
997999
// add lints here, do not remove this comment, it's used in `new_lint`
9981000
}
9991001

+234
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
use std::ops::ControlFlow;
2+
3+
use clippy_utils::{
4+
diagnostics::span_lint_and_then,
5+
is_path_lang_item, paths,
6+
ty::match_type,
7+
visitors::{for_each_expr, Visitable},
8+
};
9+
use rustc_ast::LitKind;
10+
use rustc_data_structures::fx::FxHashSet;
11+
use rustc_hir::Block;
12+
use rustc_hir::{
13+
def::{DefKind, Res},
14+
Expr, ImplItemKind, LangItem, Node,
15+
};
16+
use rustc_hir::{ExprKind, Impl, ItemKind, QPath, TyKind};
17+
use rustc_hir::{ImplItem, Item, VariantData};
18+
use rustc_lint::{LateContext, LateLintPass};
19+
use rustc_middle::ty::Ty;
20+
use rustc_middle::ty::TypeckResults;
21+
use rustc_session::{declare_lint_pass, declare_tool_lint};
22+
use rustc_span::{sym, Span, Symbol};
23+
24+
declare_clippy_lint! {
25+
/// ### What it does
26+
/// Checks for manual [`core::fmt::Debug`](https://doc.rust-lang.org/core/fmt/trait.Debug.html) implementations that do not use all fields.
27+
///
28+
/// ### Why is this bad?
29+
/// A common mistake is to forget to update manual `Debug` implementations when adding a new field
30+
/// to a struct or a new variant to an enum.
31+
///
32+
/// At the same time, it also acts as a style lint to suggest using [`core::fmt::DebugStruct::finish_non_exhaustive`](https://doc.rust-lang.org/core/fmt/struct.DebugStruct.html#method.finish_non_exhaustive)
33+
/// for the times when the user intentionally wants to leave out certain fields (e.g. to hide implementation details).
34+
///
35+
/// ### Known problems
36+
/// This lint works based on the `DebugStruct` helper types provided by the `Formatter`,
37+
/// so this won't detect `Debug` impls that use the `write!` macro.
38+
/// Oftentimes there is more logic to a `Debug` impl if it uses `write!` macro, so it tries
39+
/// to be on the conservative side and not lint in those cases in an attempt to prevent false positives.
40+
///
41+
/// This lint also does not look through function calls, so calling a function does not consider fields
42+
/// used inside of that function as used by the `Debug` impl.
43+
///
44+
/// Lastly, it also ignores tuple structs as their `DebugTuple` formatter does not have a `finish_non_exhaustive`
45+
/// method, as well as enums because their exhaustiveness is already checked by the compiler when matching on the enum,
46+
/// making it much less likely to accidentally forget to update the `Debug` impl when adding a new variant.
47+
///
48+
/// ### Example
49+
/// ```rust
50+
/// use std::fmt;
51+
/// struct Foo {
52+
/// data: String,
53+
/// // implementation detail
54+
/// hidden_data: i32
55+
/// }
56+
/// impl fmt::Debug for Foo {
57+
/// fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
58+
/// formatter
59+
/// .debug_struct("Foo")
60+
/// .field("data", &self.data)
61+
/// .finish()
62+
/// }
63+
/// }
64+
/// ```
65+
/// Use instead:
66+
/// ```rust
67+
/// use std::fmt;
68+
/// struct Foo {
69+
/// data: String,
70+
/// // implementation detail
71+
/// hidden_data: i32
72+
/// }
73+
/// impl fmt::Debug for Foo {
74+
/// fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
75+
/// formatter
76+
/// .debug_struct("Foo")
77+
/// .field("data", &self.data)
78+
/// .finish_non_exhaustive()
79+
/// }
80+
/// }
81+
/// ```
82+
#[clippy::version = "1.70.0"]
83+
pub MISSING_FIELDS_IN_DEBUG,
84+
pedantic,
85+
"missing fields in manual `Debug` implementation"
86+
}
87+
declare_lint_pass!(MissingFieldsInDebug => [MISSING_FIELDS_IN_DEBUG]);
88+
89+
fn report_lints(cx: &LateContext<'_>, span: Span, span_notes: Vec<(Span, &'static str)>) {
90+
span_lint_and_then(
91+
cx,
92+
MISSING_FIELDS_IN_DEBUG,
93+
span,
94+
"manual `Debug` impl does not include all fields",
95+
|diag| {
96+
for (span, note) in span_notes {
97+
diag.span_note(span, note);
98+
}
99+
diag.help("consider including all fields in this `Debug` impl")
100+
.help("consider calling `.finish_non_exhaustive()` if you intend to ignore fields");
101+
},
102+
);
103+
}
104+
105+
/// Checks if we should lint in a block of code
106+
///
107+
/// The way we check for this condition is by checking if there is
108+
/// a call to `Formatter::debug_struct` but no call to `.finish_non_exhaustive()`.
109+
fn should_lint<'tcx>(
110+
cx: &LateContext<'tcx>,
111+
typeck_results: &TypeckResults<'tcx>,
112+
block: impl Visitable<'tcx>,
113+
) -> bool {
114+
// Is there a call to `DebugStruct::finish_non_exhaustive`? Don't lint if there is.
115+
let mut has_finish_non_exhaustive = false;
116+
// Is there a call to `DebugStruct::debug_struct`? Do lint if there is.
117+
let mut has_debug_struct = false;
118+
119+
for_each_expr(block, |expr| {
120+
if let ExprKind::MethodCall(path, recv, ..) = &expr.kind {
121+
let recv_ty = typeck_results.expr_ty(recv).peel_refs();
122+
123+
if path.ident.name == sym::debug_struct && match_type(cx, recv_ty, &paths::FORMATTER) {
124+
has_debug_struct = true;
125+
} else if path.ident.name == sym!(finish_non_exhaustive) && match_type(cx, recv_ty, &paths::DEBUG_STRUCT) {
126+
has_finish_non_exhaustive = true;
127+
}
128+
}
129+
ControlFlow::<!, _>::Continue(())
130+
});
131+
132+
!has_finish_non_exhaustive && has_debug_struct
133+
}
134+
135+
/// Checks if the given expression is a call to `DebugStruct::field`
136+
/// and the first argument to it is a string literal and if so, returns it
137+
///
138+
/// Example: `.field("foo", ....)` returns `Some("foo")`
139+
fn as_field_call<'tcx>(
140+
cx: &LateContext<'tcx>,
141+
typeck_results: &TypeckResults<'tcx>,
142+
expr: &Expr<'_>,
143+
) -> Option<Symbol> {
144+
if let ExprKind::MethodCall(path, recv, [debug_field, _], _) = &expr.kind
145+
&& let recv_ty = typeck_results.expr_ty(recv).peel_refs()
146+
&& match_type(cx, recv_ty, &paths::DEBUG_STRUCT)
147+
&& path.ident.name == sym::field
148+
&& let ExprKind::Lit(lit) = &debug_field.kind
149+
&& let LitKind::Str(sym, ..) = lit.node
150+
{
151+
Some(sym)
152+
} else {
153+
None
154+
}
155+
}
156+
157+
/// Attempts to find unused fields assuming that the item is a struct
158+
fn check_struct<'tcx>(
159+
cx: &LateContext<'tcx>,
160+
typeck_results: &TypeckResults<'tcx>,
161+
block: &'tcx Block<'tcx>,
162+
self_ty: Ty<'tcx>,
163+
item: &'tcx Item<'tcx>,
164+
data: &VariantData<'_>,
165+
) {
166+
// Is there a "direct" field access anywhere (i.e. self.foo)?
167+
// We don't want to lint if there is not, because the user might have
168+
// a newtype struct and use fields from the wrapped type only.
169+
let mut has_direct_field_access = false;
170+
let mut field_accesses = FxHashSet::default();
171+
172+
for_each_expr(block, |expr| {
173+
if let ExprKind::Field(target, ident) = expr.kind
174+
&& let target_ty = typeck_results.expr_ty_adjusted(target).peel_refs()
175+
&& target_ty == self_ty
176+
{
177+
field_accesses.insert(ident.name);
178+
has_direct_field_access = true;
179+
} else if let Some(sym) = as_field_call(cx, typeck_results, expr) {
180+
field_accesses.insert(sym);
181+
}
182+
ControlFlow::<!, _>::Continue(())
183+
});
184+
185+
let span_notes = data
186+
.fields()
187+
.iter()
188+
.filter_map(|field| {
189+
if field_accesses.contains(&field.ident.name) || is_path_lang_item(cx, field.ty, LangItem::PhantomData) {
190+
None
191+
} else {
192+
Some((field.span, "this field is unused"))
193+
}
194+
})
195+
.collect::<Vec<_>>();
196+
197+
// only lint if there's also at least one direct field access to allow patterns
198+
// where one might have a newtype struct and uses fields from the wrapped type
199+
if !span_notes.is_empty() && has_direct_field_access {
200+
report_lints(cx, item.span, span_notes);
201+
}
202+
}
203+
204+
impl<'tcx> LateLintPass<'tcx> for MissingFieldsInDebug {
205+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'tcx>) {
206+
// is this an `impl Debug for X` block?
207+
if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), self_ty, items, .. }) = item.kind
208+
&& let Res::Def(DefKind::Trait, trait_def_id) = trait_ref.path.res
209+
&& let TyKind::Path(QPath::Resolved(_, self_path)) = &self_ty.kind
210+
&& cx.match_def_path(trait_def_id, &[sym::core, sym::fmt, sym::Debug])
211+
// don't trigger if this impl was derived
212+
&& !cx.tcx.has_attr(item.owner_id, sym::automatically_derived)
213+
&& !item.span.from_expansion()
214+
// find `Debug::fmt` function
215+
&& let Some(fmt_item) = items.iter().find(|i| i.ident.name == sym::fmt)
216+
&& let ImplItem { kind: ImplItemKind::Fn(_, body_id), .. } = cx.tcx.hir().impl_item(fmt_item.id)
217+
&& let body = cx.tcx.hir().body(*body_id)
218+
&& let ExprKind::Block(block, _) = body.value.kind
219+
// inspect `self`
220+
&& let self_ty = cx.tcx.type_of(self_path.res.def_id()).0.peel_refs()
221+
&& let Some(self_adt) = self_ty.ty_adt_def()
222+
&& let Some(self_def_id) = self_adt.did().as_local()
223+
&& let Some(Node::Item(self_item)) = cx.tcx.hir().find_by_def_id(self_def_id)
224+
// NB: can't call cx.typeck_results() as we are not in a body
225+
&& let typeck_results = cx.tcx.typeck_body(*body_id)
226+
&& should_lint(cx, typeck_results, block)
227+
{
228+
// we intentionally only lint structs, see lint description
229+
if let ItemKind::Struct(data, _) = &self_item.kind {
230+
check_struct(cx, typeck_results, block, self_ty, item, data);
231+
}
232+
}
233+
}
234+
}

clippy_utils/src/paths.rs

+2
Original file line numberDiff line numberDiff line change
@@ -163,3 +163,5 @@ pub const VEC_IS_EMPTY: [&str; 4] = ["alloc", "vec", "Vec", "is_empty"];
163163
pub const VEC_POP: [&str; 4] = ["alloc", "vec", "Vec", "pop"];
164164
pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
165165
pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"];
166+
pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"];
167+
pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];

0 commit comments

Comments
 (0)