From f74ec6b1b858c13449dc4cfd9231a3a0d82e50df Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 9 Apr 2023 04:56:03 +0200 Subject: [PATCH 1/2] new lint: `missing_field_in_debug` move some strings into consts, more tests s/missing_field_in_debug/missing_fields_in_debug dont trigger in macro expansions make dogfood tests happy minor cleanups replace HashSet with FxHashSet replace match_def_path with match_type if_chain -> let chains, fix markdown, allow newtype pattern fmt consider string literal in `.field()` calls as used don't intern defined symbol, remove mentions of 'debug_tuple' special-case PD, account for field access through `Deref` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/missing_fields_in_debug.rs | 311 +++++++++++++++++++ clippy_utils/src/paths.rs | 2 + tests/ui/missing_fields_in_debug.rs | 319 ++++++++++++++++++++ tests/ui/missing_fields_in_debug.stderr | 113 +++++++ 7 files changed, 749 insertions(+) create mode 100644 clippy_lints/src/missing_fields_in_debug.rs create mode 100644 tests/ui/missing_fields_in_debug.rs create mode 100644 tests/ui/missing_fields_in_debug.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index fdf0cacad362..8b609b47d819 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4963,6 +4963,7 @@ Released 2018-09-13 [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items [`missing_enforced_import_renames`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_enforced_import_renames [`missing_errors_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc +[`missing_fields_in_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_fields_in_debug [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items [`missing_panics_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 4ade25e1257e..a7067d8b86aa 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -430,6 +430,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::missing_const_for_fn::MISSING_CONST_FOR_FN_INFO, crate::missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS_INFO, crate::missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES_INFO, + crate::missing_fields_in_debug::MISSING_FIELDS_IN_DEBUG_INFO, crate::missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS_INFO, crate::missing_trait_methods::MISSING_TRAIT_METHODS_INFO, crate::mixed_read_write_in_expression::DIVERGING_SUB_EXPRESSION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 10404d51ba55..a0a89e4967b8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -203,6 +203,7 @@ mod missing_assert_message; mod missing_const_for_fn; mod missing_doc; mod missing_enforced_import_rename; +mod missing_fields_in_debug; mod missing_inline; mod missing_trait_methods; mod mixed_read_write_in_expression; @@ -994,6 +995,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(|| Box::new(ref_patterns::RefPatterns)); store.register_late_pass(|_| Box::new(default_constructed_unit_structs::DefaultConstructedUnitStructs)); store.register_early_pass(|| Box::new(needless_else::NeedlessElse)); + store.register_late_pass(|_| Box::new(missing_fields_in_debug::MissingFieldsInDebug)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/missing_fields_in_debug.rs b/clippy_lints/src/missing_fields_in_debug.rs new file mode 100644 index 000000000000..389a8d04dc28 --- /dev/null +++ b/clippy_lints/src/missing_fields_in_debug.rs @@ -0,0 +1,311 @@ +use std::ops::ControlFlow; + +use clippy_utils::{ + diagnostics::span_lint_and_then, + paths, + ty::match_type, + visitors::{for_each_expr, Visitable}, +}; +use rustc_ast::LitKind; +use rustc_data_structures::fx::FxHashSet; +use rustc_hir::{ + def::{DefKind, Res}, + Expr, ImplItemKind, MatchSource, Node, +}; +use rustc_hir::{Block, PatKind}; +use rustc_hir::{ExprKind, Impl, ItemKind, QPath, TyKind}; +use rustc_hir::{ImplItem, Item, VariantData}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::TypeckResults; +use rustc_middle::ty::{EarlyBinder, Ty}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{sym, Span, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for manual [`core::fmt::Debug`](https://doc.rust-lang.org/core/fmt/trait.Debug.html) implementations that do not use all fields. + /// + /// ### Why is this bad? + /// A common mistake is to forget to update manual `Debug` implementations when adding a new field + /// to a struct or a new variant to an enum. + /// + /// 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) + /// for the times when the user intentionally wants to leave out certain fields (e.g. to hide implementation details). + /// + /// ### Known problems + /// This lint works based on the `DebugStruct` helper types provided by the `Formatter`, + /// so this won't detect `Debug` impls that use the `write!` macro. + /// Oftentimes there is more logic to a `Debug` impl if it uses `write!` macro, so it tries + /// to be on the conservative side and not lint in those cases in an attempt to prevent false positives. + /// + /// This lint also does not look through function calls, so calling `.field(self.as_slice())` for example + /// does not consider fields used inside of `as_slice()` as used by the `Debug` impl. + /// + /// Lastly, it also ignores tuple structs as their `DebugTuple` formatter does not have a `finish_non_exhaustive` + /// method. + /// + /// ### Example + /// ```rust + /// use std::fmt; + /// struct Foo { + /// data: String, + /// // implementation detail + /// hidden_data: i32 + /// } + /// impl fmt::Debug for Foo { + /// fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + /// formatter + /// .debug_struct("Foo") + /// .field("data", &self.data) + /// .finish() + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// use std::fmt; + /// struct Foo { + /// data: String, + /// // implementation detail + /// hidden_data: i32 + /// } + /// impl fmt::Debug for Foo { + /// fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + /// formatter + /// .debug_struct("Foo") + /// .field("data", &self.data) + /// .finish_non_exhaustive() + /// } + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub MISSING_FIELDS_IN_DEBUG, + pedantic, + "missing fields in manual `Debug` implementation" +} +declare_lint_pass!(MissingFieldsInDebug => [MISSING_FIELDS_IN_DEBUG]); + +fn report_lints(cx: &LateContext<'_>, span: Span, span_notes: Vec<(Span, &'static str)>) { + span_lint_and_then( + cx, + MISSING_FIELDS_IN_DEBUG, + span, + "manual `Debug` impl does not include all fields", + |diag| { + for (span, note) in span_notes { + diag.span_note(span, note); + } + diag.help("consider including all fields in this `Debug` impl") + .help("consider calling `.finish_non_exhaustive()` if you intend to ignore fields"); + }, + ); +} + +/// Checks if we should lint in a block of code +/// +/// The way we check for this condition is by checking if there is +/// a call to `Formatter::debug_struct` but no call to `.finish_non_exhaustive()`. +fn should_lint<'tcx>( + cx: &LateContext<'tcx>, + typeck_results: &TypeckResults<'tcx>, + block: impl Visitable<'tcx>, +) -> bool { + // Is there a call to `DebugStruct::finish_non_exhaustive`? Don't lint if there is. + let mut has_finish_non_exhaustive = false; + // Is there a call to `DebugStruct::debug_struct`? Do lint if there is. + let mut has_debug_struct = false; + + for_each_expr(block, |expr| { + if let ExprKind::MethodCall(path, recv, ..) = &expr.kind { + let recv_ty = typeck_results.expr_ty(recv).peel_refs(); + + if path.ident.name == sym::debug_struct && match_type(cx, recv_ty, &paths::FORMATTER) { + has_debug_struct = true; + } else if path.ident.name == sym!(finish_non_exhaustive) && match_type(cx, recv_ty, &paths::DEBUG_STRUCT) { + has_finish_non_exhaustive = true; + } + } + ControlFlow::::Continue(()) + }); + + !has_finish_non_exhaustive && has_debug_struct +} + +/// Checks if the given expression is a call to `DebugStruct::field` +/// and the first argument to it is a string literal and if so, returns it +/// +/// Example: `.field("foo", ....)` returns `Some("foo")` +fn as_field_call<'tcx>( + cx: &LateContext<'tcx>, + typeck_results: &TypeckResults<'tcx>, + expr: &Expr<'_>, +) -> Option { + if let ExprKind::MethodCall(path, recv, [debug_field, _], _) = &expr.kind + && let recv_ty = typeck_results.expr_ty(recv).peel_refs() + && match_type(cx, recv_ty, &paths::DEBUG_STRUCT) + && path.ident.name == sym::field + && let ExprKind::Lit(lit) = &debug_field.kind + && let LitKind::Str(sym, ..) = lit.node + { + Some(sym) + } else { + None + } +} + +/// Attempts to find unused fields assuming that the item is a struct +fn check_struct<'tcx>( + cx: &LateContext<'tcx>, + typeck_results: &TypeckResults<'tcx>, + block: &'tcx Block<'tcx>, + self_ty: Ty<'tcx>, + item: &'tcx Item<'tcx>, + data: &VariantData<'_>, +) { + // Is there a "direct" field access anywhere (i.e. self.foo)? + // We don't want to lint if there is not, because the user might have + // a newtype struct and use fields from the wrapped type only. + let mut has_direct_field_access = false; + let mut field_accesses = FxHashSet::default(); + + for_each_expr(block, |expr| { + if let ExprKind::Field(target, ident) = expr.kind + && let target_ty = typeck_results.expr_ty_adjusted(target).peel_refs() + && target_ty == self_ty + { + field_accesses.insert(ident.name); + has_direct_field_access = true; + } else if let Some(sym) = as_field_call(cx, typeck_results, expr) { + field_accesses.insert(sym); + } + ControlFlow::::Continue(()) + }); + + let span_notes = data + .fields() + .iter() + .filter_map(|field| { + let EarlyBinder(field_ty) = cx.tcx.type_of(field.def_id); + if field_accesses.contains(&field.ident.name) || field_ty.is_phantom_data() { + None + } else { + Some((field.span, "this field is unused")) + } + }) + .collect::>(); + + // only lint if there's also at least one direct field access to allow patterns + // where one might have a newtype struct and uses fields from the wrapped type + if !span_notes.is_empty() && has_direct_field_access { + report_lints(cx, item.span, span_notes); + } +} + +/// Attempts to find unused fields in variants assuming that +/// the item is an enum. +/// +/// Currently, only simple cases are detected where the user +/// matches on `self` and calls `debug_struct` inside of the arms +fn check_enum<'tcx>( + cx: &LateContext<'tcx>, + typeck_results: &TypeckResults<'tcx>, + block: &'tcx Block<'tcx>, + self_ty: Ty<'tcx>, + item: &'tcx Item<'tcx>, +) { + let Some(arms) = for_each_expr(block, |expr| { + if let ExprKind::Match(val, arms, MatchSource::Normal) = expr.kind + && let match_ty = typeck_results.expr_ty_adjusted(val).peel_refs() + && match_ty == self_ty + { + ControlFlow::Break(arms) + } else { + ControlFlow::Continue(()) + } + }) else { + return; + }; + + let mut span_notes = Vec::new(); + + for arm in arms { + if !should_lint(cx, typeck_results, arm.body) { + continue; + } + + arm.pat.walk_always(|pat| match pat.kind { + PatKind::Wild => span_notes.push((pat.span, "unused field here due to wildcard `_`")), + PatKind::Tuple(_, rest) | PatKind::TupleStruct(.., rest) if rest.as_opt_usize().is_some() => { + span_notes.push((pat.span, "more unused fields here due to rest pattern `..`")); + }, + PatKind::Struct(.., true) => { + span_notes.push((pat.span, "more unused fields here due to rest pattern `..`")); + }, + _ => {}, + }); + + let mut field_accesses = FxHashSet::default(); + let mut check_field_access = |sym, expr| { + if !typeck_results.expr_ty(expr).is_phantom_data() { + arm.pat.each_binding(|_, _, _, pat_ident| { + if sym == pat_ident.name { + field_accesses.insert(pat_ident); + } + }); + } + }; + + for_each_expr(arm.body, |expr| { + if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind && let Some(segment) = path.segments.first() + { + check_field_access(segment.ident.name, expr); + } else if let Some(sym) = as_field_call(cx, typeck_results, expr) { + check_field_access(sym, expr); + } + ControlFlow::::Continue(()) + }); + + arm.pat.each_binding(|_, _, span, pat_ident| { + if !field_accesses.contains(&pat_ident) { + span_notes.push((span, "the field referenced by this binding is unused")); + } + }); + } + + if !span_notes.is_empty() { + report_lints(cx, item.span, span_notes); + } +} + +impl<'tcx> LateLintPass<'tcx> for MissingFieldsInDebug { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'tcx>) { + // is this an `impl Debug for X` block? + if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), self_ty, items, .. }) = item.kind + && let Res::Def(DefKind::Trait, trait_def_id) = trait_ref.path.res + && let TyKind::Path(QPath::Resolved(_, self_path)) = &self_ty.kind + && cx.match_def_path(trait_def_id, &[sym::core, sym::fmt, sym::Debug]) + // don't trigger if this impl was derived + && !cx.tcx.has_attr(item.owner_id, sym::automatically_derived) + && !item.span.from_expansion() + // find `Debug::fmt` function + && let Some(fmt_item) = items.iter().find(|i| i.ident.name == sym::fmt) + && let ImplItem { kind: ImplItemKind::Fn(_, body_id), .. } = cx.tcx.hir().impl_item(fmt_item.id) + && let body = cx.tcx.hir().body(*body_id) + && let ExprKind::Block(block, _) = body.value.kind + // inspect `self` + && let self_ty = cx.tcx.type_of(self_path.res.def_id()).0.peel_refs() + && let Some(self_adt) = self_ty.ty_adt_def() + && let Some(self_def_id) = self_adt.did().as_local() + && let Some(Node::Item(self_item)) = cx.tcx.hir().find_by_def_id(self_def_id) + // NB: can't call cx.typeck_results() as we are not in a body + && let typeck_results = cx.tcx.typeck_body(*body_id) + && should_lint(cx, typeck_results, block) + { + match &self_item.kind { + ItemKind::Struct(data, _) => check_struct(cx, typeck_results, block, self_ty, item, data), + ItemKind::Enum(..) => check_enum(cx, typeck_results, block, self_ty, item), + _ => {} + } + } + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 0f0792fdaa96..3a2b0a72a3de 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -163,3 +163,5 @@ pub const VEC_IS_EMPTY: [&str; 4] = ["alloc", "vec", "Vec", "is_empty"]; pub const VEC_POP: [&str; 4] = ["alloc", "vec", "Vec", "pop"]; pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"]; pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"]; +pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"]; +pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"]; diff --git a/tests/ui/missing_fields_in_debug.rs b/tests/ui/missing_fields_in_debug.rs new file mode 100644 index 000000000000..72b9e0e2aae5 --- /dev/null +++ b/tests/ui/missing_fields_in_debug.rs @@ -0,0 +1,319 @@ +#![allow(unused)] +#![warn(clippy::missing_fields_in_debug)] + +use std::fmt; +use std::marker::PhantomData; +use std::ops::Deref; + +struct NamedStruct1Ignored { + data: u8, + hidden: u32, +} + +impl fmt::Debug for NamedStruct1Ignored { + // unused field: hidden + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("NamedStruct1Ignored") + .field("data", &self.data) + .finish() + } +} + +struct NamedStructMultipleIgnored { + data: u8, + hidden: u32, + hidden2: String, + hidden3: Vec>, + hidden4: ((((u8), u16), u32), u64), +} + +impl fmt::Debug for NamedStructMultipleIgnored { + // unused fields: hidden, hidden2, hidden4 + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("NamedStructMultipleIgnored") + .field("data", &self.data) + .field("hidden3", &self.hidden3) + .finish() + } +} + +struct Unit; + +// ok +impl fmt::Debug for Unit { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.debug_struct("Unit").finish() + } +} + +struct UnnamedStruct1Ignored(String); + +impl fmt::Debug for UnnamedStruct1Ignored { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.debug_tuple("UnnamedStruct1Ignored").finish() + } +} + +struct UnnamedStructMultipleIgnored(String, Vec, i32); + +// tuple structs are not linted +impl fmt::Debug for UnnamedStructMultipleIgnored { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_tuple("UnnamedStructMultipleIgnored") + .field(&self.1) + .finish() + } +} + +struct NamedStructNonExhaustive { + a: u8, + b: String, +} + +// ok +impl fmt::Debug for NamedStructNonExhaustive { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("NamedStructNonExhaustive") + .field("a", &self.a) + .finish_non_exhaustive() // should not warn here + } +} + +struct MultiExprDebugImpl { + a: u8, + b: String, +} + +// ok +impl fmt::Debug for MultiExprDebugImpl { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut f = formatter.debug_struct("MultiExprDebugImpl"); + f.field("a", &self.a); + f.finish() + } +} + +enum SingleVariantEnumUnnamed { + A(u8), +} + +// ok +impl fmt::Debug for SingleVariantEnumUnnamed { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + } + } +} + +enum MultiVariantEnum { + A(u8), + B { a: u8, b: String }, + C, +} + +impl fmt::Debug for MultiVariantEnum { + // match arm Self::B ignores `b` + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + Self::B { a, b } => formatter.debug_struct("B").field("a", &a).finish(), + Self::C => formatter.debug_struct("C").finish(), + } + } +} + +enum MultiVariantEnumOk { + A(u8), + B { a: u8, b: String }, + C, +} + +// ok +impl fmt::Debug for MultiVariantEnumOk { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + Self::B { a, b } => formatter.debug_struct("B").field("a", &a).field("b", &b).finish(), + Self::C => formatter.debug_struct("C").finish(), + } + } +} + +enum MultiVariantEnumNonExhaustive { + A(u8), + B { a: u8, b: String }, + C, +} + +// ok +impl fmt::Debug for MultiVariantEnumNonExhaustive { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + Self::B { a, b } => formatter.debug_struct("B").field("b", &b).finish_non_exhaustive(), + Self::C => formatter.debug_struct("C").finish(), + } + } +} + +enum MultiVariantRest { + A(u8), + B { a: u8, b: String }, + C, +} + +impl fmt::Debug for MultiVariantRest { + // `a` field ignored due to rest pattern + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish(), + Self::C => formatter.debug_struct("C").finish(), + } + } +} + +enum MultiVariantRestNonExhaustive { + A(u8), + B { a: u8, b: String }, + C, +} + +// ok +impl fmt::Debug for MultiVariantRestNonExhaustive { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish_non_exhaustive(), + Self::C => formatter.debug_struct("C").finish(), + } + } +} + +enum Wildcard { + A(u8), + B(String), +} + +// ok +impl fmt::Debug for Wildcard { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), + _ => todo!(), + } + } +} + +enum Empty {} + +// ok +impl fmt::Debug for Empty { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match *self {} + } +} + +#[derive(Debug)] +struct DerivedStruct { + a: u8, + b: i32, +} + +#[derive(Debug)] +enum DerivedEnum { + A(i32), + B { a: String }, +} + +// https://github.com/rust-lang/rust-clippy/pull/10616#discussion_r1166846953 + +struct Inner { + a: usize, + b: usize, +} + +struct HasInner { + inner: Inner, +} + +impl HasInner { + fn get(&self) -> &Inner { + &self.inner + } +} + +impl fmt::Debug for HasInner { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let inner = self.get(); + + f.debug_struct("HasInner") + .field("a", &inner.a) + .field("b", &inner.b) + .finish() + } +} + +// https://github.com/rust-lang/rust-clippy/pull/10616#discussion_r1170306053 +struct Foo { + a: u8, + b: u8, +} + +impl fmt::Debug for Foo { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Foo").field("a", &self.a).field("b", &()).finish() + } +} + +// https://github.com/rust-lang/rust-clippy/pull/10616#discussion_r1175473620 +mod comment1175473620 { + use super::*; + + struct Inner { + a: usize, + b: usize, + } + struct Wrapper(Inner); + + impl Deref for Wrapper { + type Target = Inner; + + fn deref(&self) -> &Self::Target { + &self.0 + } + } + + impl fmt::Debug for Wrapper { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Wrapper") + .field("a", &self.a) + .field("b", &self.b) + .finish() + } + } +} + +// https://github.com/rust-lang/rust-clippy/pull/10616#discussion_r1175488757 +// PhantomData is an exception and does not need to be included +struct WithPD { + a: u8, + b: u8, + c: PhantomData, +} + +impl fmt::Debug for WithPD { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("WithPD") + .field("a", &self.a) + .field("b", &self.b) + .finish() + } +} + +fn main() {} diff --git a/tests/ui/missing_fields_in_debug.stderr b/tests/ui/missing_fields_in_debug.stderr new file mode 100644 index 000000000000..1dc7c0d65e5d --- /dev/null +++ b/tests/ui/missing_fields_in_debug.stderr @@ -0,0 +1,113 @@ +error: manual `Debug` impl does not include all fields + --> $DIR/missing_fields_in_debug.rs:13:1 + | +LL | / impl fmt::Debug for NamedStruct1Ignored { +LL | | // unused field: hidden +LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { +LL | | formatter +... | +LL | | } +LL | | } + | |_^ + | +note: this field is unused + --> $DIR/missing_fields_in_debug.rs:10:5 + | +LL | hidden: u32, + | ^^^^^^^^^^^ + = help: consider including all fields in this `Debug` impl + = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields + = note: `-D clippy::missing-fields-in-debug` implied by `-D warnings` + +error: manual `Debug` impl does not include all fields + --> $DIR/missing_fields_in_debug.rs:31:1 + | +LL | / impl fmt::Debug for NamedStructMultipleIgnored { +LL | | // unused fields: hidden, hidden2, hidden4 +LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { +LL | | formatter +... | +LL | | } +LL | | } + | |_^ + | +note: this field is unused + --> $DIR/missing_fields_in_debug.rs:25:5 + | +LL | hidden: u32, + | ^^^^^^^^^^^ +note: this field is unused + --> $DIR/missing_fields_in_debug.rs:26:5 + | +LL | hidden2: String, + | ^^^^^^^^^^^^^^^ +note: this field is unused + --> $DIR/missing_fields_in_debug.rs:28:5 + | +LL | hidden4: ((((u8), u16), u32), u64), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: consider including all fields in this `Debug` impl + = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields + +error: manual `Debug` impl does not include all fields + --> $DIR/missing_fields_in_debug.rs:92:1 + | +LL | / impl fmt::Debug for MultiExprDebugImpl { +LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { +LL | | let mut f = formatter.debug_struct("MultiExprDebugImpl"); +LL | | f.field("a", &self.a); +LL | | f.finish() +LL | | } +LL | | } + | |_^ + | +note: this field is unused + --> $DIR/missing_fields_in_debug.rs:88:5 + | +LL | b: String, + | ^^^^^^^^^ + = help: consider including all fields in this `Debug` impl + = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields + +error: manual `Debug` impl does not include all fields + --> $DIR/missing_fields_in_debug.rs:119:1 + | +LL | / impl fmt::Debug for MultiVariantEnum { +LL | | // match arm Self::B ignores `b` +LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { +LL | | match self { +... | +LL | | } +LL | | } + | |_^ + | +note: the field referenced by this binding is unused + --> $DIR/missing_fields_in_debug.rs:124:26 + | +LL | Self::B { a, b } => formatter.debug_struct("B").field("a", &a).finish(), + | ^ + = help: consider including all fields in this `Debug` impl + = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields + +error: manual `Debug` impl does not include all fields + --> $DIR/missing_fields_in_debug.rs:170:1 + | +LL | / impl fmt::Debug for MultiVariantRest { +LL | | // `a` field ignored due to rest pattern +LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { +LL | | match self { +... | +LL | | } +LL | | } + | |_^ + | +note: more unused fields here due to rest pattern `..` + --> $DIR/missing_fields_in_debug.rs:175:13 + | +LL | Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish(), + | ^^^^^^^^^^^^^^^^^ + = help: consider including all fields in this `Debug` impl + = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields + +error: aborting due to 5 previous errors + From a859b0e6dfae12169d6a239e2a63aef8227e1dce Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 31 May 2023 23:08:49 +0200 Subject: [PATCH 2/2] don't lint enums, update note in lint description --- clippy_lints/src/missing_fields_in_debug.rs | 101 ++------------- tests/ui/missing_fields_in_debug.rs | 128 -------------------- tests/ui/missing_fields_in_debug.stderr | 42 +------ 3 files changed, 13 insertions(+), 258 deletions(-) diff --git a/clippy_lints/src/missing_fields_in_debug.rs b/clippy_lints/src/missing_fields_in_debug.rs index 389a8d04dc28..8332d638f92a 100644 --- a/clippy_lints/src/missing_fields_in_debug.rs +++ b/clippy_lints/src/missing_fields_in_debug.rs @@ -2,22 +2,22 @@ use std::ops::ControlFlow; use clippy_utils::{ diagnostics::span_lint_and_then, - paths, + is_path_lang_item, paths, ty::match_type, visitors::{for_each_expr, Visitable}, }; use rustc_ast::LitKind; use rustc_data_structures::fx::FxHashSet; +use rustc_hir::Block; use rustc_hir::{ def::{DefKind, Res}, - Expr, ImplItemKind, MatchSource, Node, + Expr, ImplItemKind, LangItem, Node, }; -use rustc_hir::{Block, PatKind}; use rustc_hir::{ExprKind, Impl, ItemKind, QPath, TyKind}; use rustc_hir::{ImplItem, Item, VariantData}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; use rustc_middle::ty::TypeckResults; -use rustc_middle::ty::{EarlyBinder, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{sym, Span, Symbol}; @@ -38,11 +38,12 @@ declare_clippy_lint! { /// Oftentimes there is more logic to a `Debug` impl if it uses `write!` macro, so it tries /// to be on the conservative side and not lint in those cases in an attempt to prevent false positives. /// - /// This lint also does not look through function calls, so calling `.field(self.as_slice())` for example - /// does not consider fields used inside of `as_slice()` as used by the `Debug` impl. + /// This lint also does not look through function calls, so calling a function does not consider fields + /// used inside of that function as used by the `Debug` impl. /// /// Lastly, it also ignores tuple structs as their `DebugTuple` formatter does not have a `finish_non_exhaustive` - /// method. + /// method, as well as enums because their exhaustiveness is already checked by the compiler when matching on the enum, + /// making it much less likely to accidentally forget to update the `Debug` impl when adding a new variant. /// /// ### Example /// ```rust @@ -185,8 +186,7 @@ fn check_struct<'tcx>( .fields() .iter() .filter_map(|field| { - let EarlyBinder(field_ty) = cx.tcx.type_of(field.def_id); - if field_accesses.contains(&field.ident.name) || field_ty.is_phantom_data() { + if field_accesses.contains(&field.ident.name) || is_path_lang_item(cx, field.ty, LangItem::PhantomData) { None } else { Some((field.span, "this field is unused")) @@ -201,82 +201,6 @@ fn check_struct<'tcx>( } } -/// Attempts to find unused fields in variants assuming that -/// the item is an enum. -/// -/// Currently, only simple cases are detected where the user -/// matches on `self` and calls `debug_struct` inside of the arms -fn check_enum<'tcx>( - cx: &LateContext<'tcx>, - typeck_results: &TypeckResults<'tcx>, - block: &'tcx Block<'tcx>, - self_ty: Ty<'tcx>, - item: &'tcx Item<'tcx>, -) { - let Some(arms) = for_each_expr(block, |expr| { - if let ExprKind::Match(val, arms, MatchSource::Normal) = expr.kind - && let match_ty = typeck_results.expr_ty_adjusted(val).peel_refs() - && match_ty == self_ty - { - ControlFlow::Break(arms) - } else { - ControlFlow::Continue(()) - } - }) else { - return; - }; - - let mut span_notes = Vec::new(); - - for arm in arms { - if !should_lint(cx, typeck_results, arm.body) { - continue; - } - - arm.pat.walk_always(|pat| match pat.kind { - PatKind::Wild => span_notes.push((pat.span, "unused field here due to wildcard `_`")), - PatKind::Tuple(_, rest) | PatKind::TupleStruct(.., rest) if rest.as_opt_usize().is_some() => { - span_notes.push((pat.span, "more unused fields here due to rest pattern `..`")); - }, - PatKind::Struct(.., true) => { - span_notes.push((pat.span, "more unused fields here due to rest pattern `..`")); - }, - _ => {}, - }); - - let mut field_accesses = FxHashSet::default(); - let mut check_field_access = |sym, expr| { - if !typeck_results.expr_ty(expr).is_phantom_data() { - arm.pat.each_binding(|_, _, _, pat_ident| { - if sym == pat_ident.name { - field_accesses.insert(pat_ident); - } - }); - } - }; - - for_each_expr(arm.body, |expr| { - if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind && let Some(segment) = path.segments.first() - { - check_field_access(segment.ident.name, expr); - } else if let Some(sym) = as_field_call(cx, typeck_results, expr) { - check_field_access(sym, expr); - } - ControlFlow::::Continue(()) - }); - - arm.pat.each_binding(|_, _, span, pat_ident| { - if !field_accesses.contains(&pat_ident) { - span_notes.push((span, "the field referenced by this binding is unused")); - } - }); - } - - if !span_notes.is_empty() { - report_lints(cx, item.span, span_notes); - } -} - impl<'tcx> LateLintPass<'tcx> for MissingFieldsInDebug { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'tcx>) { // is this an `impl Debug for X` block? @@ -301,10 +225,9 @@ impl<'tcx> LateLintPass<'tcx> for MissingFieldsInDebug { && let typeck_results = cx.tcx.typeck_body(*body_id) && should_lint(cx, typeck_results, block) { - match &self_item.kind { - ItemKind::Struct(data, _) => check_struct(cx, typeck_results, block, self_ty, item, data), - ItemKind::Enum(..) => check_enum(cx, typeck_results, block, self_ty, item), - _ => {} + // we intentionally only lint structs, see lint description + if let ItemKind::Struct(data, _) = &self_item.kind { + check_struct(cx, typeck_results, block, self_ty, item, data); } } } diff --git a/tests/ui/missing_fields_in_debug.rs b/tests/ui/missing_fields_in_debug.rs index 72b9e0e2aae5..c156d394ecea 100644 --- a/tests/ui/missing_fields_in_debug.rs +++ b/tests/ui/missing_fields_in_debug.rs @@ -97,140 +97,12 @@ impl fmt::Debug for MultiExprDebugImpl { } } -enum SingleVariantEnumUnnamed { - A(u8), -} - -// ok -impl fmt::Debug for SingleVariantEnumUnnamed { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - } - } -} - -enum MultiVariantEnum { - A(u8), - B { a: u8, b: String }, - C, -} - -impl fmt::Debug for MultiVariantEnum { - // match arm Self::B ignores `b` - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - Self::B { a, b } => formatter.debug_struct("B").field("a", &a).finish(), - Self::C => formatter.debug_struct("C").finish(), - } - } -} - -enum MultiVariantEnumOk { - A(u8), - B { a: u8, b: String }, - C, -} - -// ok -impl fmt::Debug for MultiVariantEnumOk { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - Self::B { a, b } => formatter.debug_struct("B").field("a", &a).field("b", &b).finish(), - Self::C => formatter.debug_struct("C").finish(), - } - } -} - -enum MultiVariantEnumNonExhaustive { - A(u8), - B { a: u8, b: String }, - C, -} - -// ok -impl fmt::Debug for MultiVariantEnumNonExhaustive { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - Self::B { a, b } => formatter.debug_struct("B").field("b", &b).finish_non_exhaustive(), - Self::C => formatter.debug_struct("C").finish(), - } - } -} - -enum MultiVariantRest { - A(u8), - B { a: u8, b: String }, - C, -} - -impl fmt::Debug for MultiVariantRest { - // `a` field ignored due to rest pattern - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish(), - Self::C => formatter.debug_struct("C").finish(), - } - } -} - -enum MultiVariantRestNonExhaustive { - A(u8), - B { a: u8, b: String }, - C, -} - -// ok -impl fmt::Debug for MultiVariantRestNonExhaustive { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish_non_exhaustive(), - Self::C => formatter.debug_struct("C").finish(), - } - } -} - -enum Wildcard { - A(u8), - B(String), -} - -// ok -impl fmt::Debug for Wildcard { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::A(n) => formatter.debug_tuple("A").field(&n).finish(), - _ => todo!(), - } - } -} - -enum Empty {} - -// ok -impl fmt::Debug for Empty { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self {} - } -} - #[derive(Debug)] struct DerivedStruct { a: u8, b: i32, } -#[derive(Debug)] -enum DerivedEnum { - A(i32), - B { a: String }, -} - // https://github.com/rust-lang/rust-clippy/pull/10616#discussion_r1166846953 struct Inner { diff --git a/tests/ui/missing_fields_in_debug.stderr b/tests/ui/missing_fields_in_debug.stderr index 1dc7c0d65e5d..ef9d02abab7d 100644 --- a/tests/ui/missing_fields_in_debug.stderr +++ b/tests/ui/missing_fields_in_debug.stderr @@ -69,45 +69,5 @@ LL | b: String, = help: consider including all fields in this `Debug` impl = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields -error: manual `Debug` impl does not include all fields - --> $DIR/missing_fields_in_debug.rs:119:1 - | -LL | / impl fmt::Debug for MultiVariantEnum { -LL | | // match arm Self::B ignores `b` -LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { -LL | | match self { -... | -LL | | } -LL | | } - | |_^ - | -note: the field referenced by this binding is unused - --> $DIR/missing_fields_in_debug.rs:124:26 - | -LL | Self::B { a, b } => formatter.debug_struct("B").field("a", &a).finish(), - | ^ - = help: consider including all fields in this `Debug` impl - = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields - -error: manual `Debug` impl does not include all fields - --> $DIR/missing_fields_in_debug.rs:170:1 - | -LL | / impl fmt::Debug for MultiVariantRest { -LL | | // `a` field ignored due to rest pattern -LL | | fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { -LL | | match self { -... | -LL | | } -LL | | } - | |_^ - | -note: more unused fields here due to rest pattern `..` - --> $DIR/missing_fields_in_debug.rs:175:13 - | -LL | Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish(), - | ^^^^^^^^^^^^^^^^^ - = help: consider including all fields in this `Debug` impl - = help: consider calling `.finish_non_exhaustive()` if you intend to ignore fields - -error: aborting due to 5 previous errors +error: aborting due to 3 previous errors