new lint: missing_field_in_debug

y21 · y21 · commit 7dba9644bee9 · 2023-04-09T04:56:03.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4718,6 +4718,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_field_in_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_field_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
@@ -428,6 +428,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_field_in_debug::MISSING_FIELD_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
@@ -202,6 +202,7 @@ mod missing_assert_message;
 mod missing_const_for_fn;
 mod missing_doc;
 mod missing_enforced_import_rename;
+mod missing_field_in_debug;
 mod missing_inline;
 mod missing_trait_methods;
 mod mixed_read_write_in_expression;
@@ -960,6 +961,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule));
     store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation));
     store.register_early_pass(|| Box::new(suspicious_doc_comments::SuspiciousDocComments));
+    store.register_late_pass(|_| Box::new(missing_field_in_debug::MissingFieldInDebug));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/missing_field_in_debug.rs b/clippy_lints/src/missing_field_in_debug.rs
@@ -0,0 +1,285 @@
+use std::{collections::HashSet, ops::ControlFlow};
+
+use clippy_utils::{
+    diagnostics::span_lint_and_then,
+    match_def_path,
+    visitors::{for_each_expr, Visitable},
+};
+use if_chain::if_chain;
+use rustc_hir::{
+    def::{DefKind, Res},
+    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_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::{sym, Span};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for manual [`core::fmt::Debug`](https://doc.rust-lang.org/stable/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/stable/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).
+    ///
+    /// ### 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_FIELD_IN_DEBUG,
+    pedantic,
+    "missing field in manual `Debug` implementation"
+}
+declare_lint_pass!(MissingFieldInDebug => [MISSING_FIELD_IN_DEBUG]);
+
+fn report_lints<'tcx, I>(cx: &LateContext<'tcx>, span: Span, span_notes: I)
+where
+    I: Iterator<Item = (Span, &'static str)>,
+{
+    span_lint_and_then(
+        cx,
+        MISSING_FIELD_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");
+        },
+    );
+}
+
+enum ShouldLint {
+    Yes,
+    No,
+}
+
+/// 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` or `Formatter::debug_tuple`
+/// and no call to `.finish_non_exhaustive()`.
+fn should_lint<'tcx>(
+    cx: &LateContext<'tcx>,
+    typeck_results: &TypeckResults<'tcx>,
+    block: impl Visitable<'tcx>,
+) -> ShouldLint {
+    let mut has_finish_non_exhaustive = false;
+    let mut has_debug_struct_tuple = false;
+
+    let _: Option<!> = for_each_expr(block, |expr| {
+        if let ExprKind::MethodCall(path, recv, ..) = &expr.kind {
+            if [sym::debug_struct, sym::debug_tuple].contains(&path.ident.name)
+                && let Some(ty) = typeck_results.expr_ty(recv).peel_refs().ty_adt_def()
+                && match_def_path(cx,ty.did(), &["core", "fmt", "Formatter"])
+            {
+                has_debug_struct_tuple = true;
+            }
+
+            if path.ident.name.as_str() == "finish_non_exhaustive"
+                && let Some(ty) = typeck_results.expr_ty(recv).peel_refs().ty_adt_def()
+                && match_def_path(cx,ty.did(), &["core", "fmt", "builders", "DebugStruct"])
+            {
+                has_finish_non_exhaustive = true;
+            }
+        }
+        ControlFlow::Continue(())
+    });
+
+    if !has_finish_non_exhaustive && has_debug_struct_tuple {
+        ShouldLint::Yes
+    } else {
+        ShouldLint::No
+    }
+}
+
+/// 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: &rustc_middle::ty::Ty<'tcx>,
+    item: &'tcx Item<'tcx>,
+    data: &VariantData<'_>,
+) {
+    if let ShouldLint::No = should_lint(cx, typeck_results, block) {
+        return;
+    }
+
+    let mut field_accesses = HashSet::new();
+
+    let _: Option<!> = for_each_expr(block, |expr| {
+        if_chain! {
+            if let ExprKind::Field(target, ident) = expr.kind;
+            let target_ty = typeck_results.expr_ty(target).peel_refs();
+            if target_ty == *self_ty;
+            then {
+                field_accesses.insert(ident);
+            }
+        }
+        ControlFlow::Continue(())
+    });
+
+    let span_notes = data
+        .fields()
+        .iter()
+        .filter_map(|field| {
+            if !field_accesses.contains(&field.ident) {
+                Some((field.span, "this field is unused"))
+            } else {
+                None
+            }
+        })
+        .collect::<Vec<_>>();
+
+    if !span_notes.is_empty() {
+        report_lints(cx, item.span, span_notes.into_iter());
+    }
+}
+
+/// 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` or `debug_tuple`
+fn check_enum<'tcx>(
+    cx: &LateContext<'tcx>,
+    typeck_results: &TypeckResults<'tcx>,
+    block: &'tcx Block<'tcx>,
+    self_ty: &rustc_middle::ty::Ty<'tcx>,
+    item: &'tcx Item<'tcx>,
+) {
+    let Some(arms) = for_each_expr(block, |expr| {
+        if_chain! {
+            if let ExprKind::Match(val, arms, MatchSource::Normal) = &expr.kind;
+            if let match_ty = typeck_results.expr_ty(val).peel_refs();
+            if match_ty == *self_ty;
+            then {
+                ControlFlow::Break(arms)
+            } else {
+                ControlFlow::Continue(())
+            }
+        }
+    }) else {
+        return;
+    };
+
+    let mut span_notes = Vec::new();
+    let lint_function = should_lint(cx, typeck_results, block);
+
+    for arm in *arms {
+        if let ShouldLint::No = lint_function && let ShouldLint::No = should_lint(cx, typeck_results, arm.body) {
+            continue;
+        }
+
+        arm.pat.walk_always(|pat| match &pat.kind {
+            PatKind::Wild => span_notes.push((pat.span, "wildcard used here")),
+            PatKind::Tuple(_, dotdot) | PatKind::TupleStruct(.., dotdot) if dotdot.as_opt_usize().is_some() => {
+                span_notes.push((pat.span, "more unused fields here due to dotdot pattern"));
+            },
+            PatKind::Struct(.., true) => span_notes.push((pat.span, "more unused fields here due to dotdot pattern")),
+            _ => {},
+        });
+
+        let mut field_accesses = HashSet::new();
+
+        let _: Option<!> = for_each_expr(arm.body, |expr| {
+            if_chain! {
+                if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind;
+                if let Some(segment) = path.segments.first();
+                then {
+                    arm.pat.each_binding(|_, _, _, pat_ident| {
+                        if segment.ident == pat_ident {
+                            field_accesses.insert(pat_ident);
+                        }
+                    });
+                }
+            }
+            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.into_iter());
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for MissingFieldInDebug {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::Item<'tcx>) {
+        if_chain! {
+            // is this an `impl Debug for X` block?
+            if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), self_ty, items, .. }) = item.kind;
+            if let Res::Def(DefKind::Trait, trait_def_id) = trait_ref.path.res;
+            if let TyKind::Path(QPath::Resolved(_, self_path)) = &self_ty.kind;
+            if cx.match_def_path(trait_def_id, &[sym::core, sym::fmt, sym::Debug]);
+
+            // find `Debug::fmt` function
+            if let Some(fmt_item) = items.iter().find(|i| i.ident.name == sym::fmt);
+            if let ImplItem { kind: ImplItemKind::Fn(_, body_id), .. } = cx.tcx.hir().impl_item(fmt_item.id);
+            let body = cx.tcx.hir().body(*body_id);
+            if let ExprKind::Block(block, _) = body.value.kind;
+
+            // inspect `self`
+            let self_ty = cx.tcx.type_of(self_path.res.def_id()).0.peel_refs();
+            if let Some(self_adt) = self_ty.ty_adt_def();
+            if let Some(self_def_id) = self_adt.did().as_local();
+            if let Some(Node::Item(self_item)) = cx.tcx.hir().find_by_def_id(self_def_id);
+
+            then {
+                // NB: can't call cx.typeck_results() as we are not in a body
+                let typeck_results = cx.tcx.typeck_body(*body_id);
+                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/tests/ui/missing_field_in_debug.rs b/tests/ui/missing_field_in_debug.rs
