move some strings into consts, more tests

y21 · y21 · commit f3dd1c6deab4 · 2023-04-09T13:58:24.000+02:00
diff --git a/clippy_lints/src/missing_field_in_debug.rs b/clippy_lints/src/missing_field_in_debug.rs
@@ -2,7 +2,7 @@ use std::{collections::HashSet, ops::ControlFlow};
 
 use clippy_utils::{
     diagnostics::span_lint_and_then,
-    match_def_path,
+    match_def_path, paths,
     visitors::{for_each_expr, Visitable},
 };
 use if_chain::if_chain;
@@ -20,15 +20,25 @@ 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.
+    /// Checks for manual [`core::fmt::Debug`] 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)
+    /// At the same time, it also acts as a style lint to suggest using [`core::fmt::DebugStruct::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`] and [`DebugTuple`] 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.
+    ///
+    ///
     /// ### Example
     /// ```rust
     /// use std::fmt;
@@ -70,28 +80,24 @@ declare_clippy_lint! {
 }
 declare_lint_pass!(MissingFieldInDebug => [MISSING_FIELD_IN_DEBUG]);
 
+const WARN: &str = "manual `Debug` impl does not include all fields";
+const HELP_WILDCARD: &str = "unused field here due to wildcard `_`";
+const HELP_UNUSED_REST: &str = "more unused fields here due to rest pattern `..`";
+const HELP_FIELD_UNUSED: &str = "this field is unused";
+const HELP_FIELD_REFERENCE: &str = "the field referenced by this binding is unused";
+const HELP_INCLUDE_ALL_FIELDS: &str = "consider including all fields in this `Debug` impl";
+const HELP_FINISH_NON_EXHAUSTIVE: &str = "consider calling `.finish_non_exhaustive()` if you intend to ignore fields";
+
 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,
+    span_lint_and_then(cx, MISSING_FIELD_IN_DEBUG, span, WARN, |diag| {
+        for (span, note) in span_notes {
+            diag.span_note(span, note);
+        }
+        diag.help(HELP_INCLUDE_ALL_FIELDS).help(HELP_FINISH_NON_EXHAUSTIVE);
+    });
 }
 
 /// Checks if we should lint in a block of code
@@ -103,34 +109,36 @@ fn should_lint<'tcx>(
     cx: &LateContext<'tcx>,
     typeck_results: &TypeckResults<'tcx>,
     block: impl Visitable<'tcx>,
-) -> ShouldLint {
+) -> bool {
     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"])
-            {
+        let ExprKind::MethodCall(path, recv, ..) = &expr.kind else {
+            return ControlFlow::Continue(());
+        };
+
+        if_chain! {
+            if [sym::debug_struct, sym::debug_tuple].contains(&path.ident.name);
+            if let Some(ty) = typeck_results.expr_ty(recv).peel_refs().ty_adt_def();
+            if match_def_path(cx,ty.did(), &paths::FORMATTER);
+            then {
                 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"])
-            {
+        if_chain! {
+            if path.ident.name.as_str() == "finish_non_exhaustive";
+            if let Some(ty) = typeck_results.expr_ty(recv).peel_refs().ty_adt_def();
+            if match_def_path(cx,ty.did(), &paths::DEBUG_STRUCT);
+            then {
                 has_finish_non_exhaustive = true;
             }
         }
         ControlFlow::Continue(())
     });
 
-    if !has_finish_non_exhaustive && has_debug_struct_tuple {
-        ShouldLint::Yes
-    } else {
-        ShouldLint::No
-    }
+    !has_finish_non_exhaustive && has_debug_struct_tuple
 }
 
 /// Attempts to find unused fields assuming that the item is a struct
@@ -142,10 +150,6 @@ fn check_struct<'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| {
@@ -165,7 +169,7 @@ fn check_struct<'tcx>(
         .iter()
         .filter_map(|field| {
             if !field_accesses.contains(&field.ident) {
-                Some((field.span, "this field is unused"))
+                Some((field.span, HELP_FIELD_UNUSED))
             } else {
                 None
             }
@@ -182,6 +186,7 @@ fn check_struct<'tcx>(
 ///
 /// Currently, only simple cases are detected where the user
 /// matches on `self` and calls `debug_struct` or `debug_tuple`
+/// inside of the arms
 fn check_enum<'tcx>(
     cx: &LateContext<'tcx>,
     typeck_results: &TypeckResults<'tcx>,
@@ -205,19 +210,18 @@ fn check_enum<'tcx>(
     };
 
     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) {
+        if !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::Wild => span_notes.push((pat.span, HELP_WILDCARD)),
+            PatKind::Tuple(_, rest) | PatKind::TupleStruct(.., rest) if rest.as_opt_usize().is_some() => {
+                span_notes.push((pat.span, HELP_UNUSED_REST));
             },
-            PatKind::Struct(.., true) => span_notes.push((pat.span, "more unused fields here due to dotdot pattern")),
+            PatKind::Struct(.., true) => span_notes.push((pat.span, HELP_UNUSED_REST)),
             _ => {},
         });
 
@@ -240,7 +244,7 @@ fn check_enum<'tcx>(
 
         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"));
+                span_notes.push((span, HELP_FIELD_REFERENCE));
             }
         });
     }
@@ -271,9 +275,10 @@ impl<'tcx> LateLintPass<'tcx> for MissingFieldInDebug {
             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);
 
+            // NB: can't call cx.typeck_results() as we are not in a body
+            let typeck_results = cx.tcx.typeck_body(*body_id);
+            if should_lint(cx, typeck_results, block);
             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/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs
@@ -159,3 +159,5 @@ pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"];
 pub const PTR_NON_NULL: [&str; 4] = ["core", "ptr", "non_null", "NonNull"];
 pub const INSTANT_NOW: [&str; 4] = ["std", "time", "Instant", "now"];
 pub const INSTANT: [&str; 3] = ["std", "time", "Instant"];
+pub const FORMATTER: [&str; 3] = ["core", "fmt", "Formatter"];
+pub const DEBUG_STRUCT: [&str; 4] = ["core", "fmt", "builders", "DebugStruct"];
diff --git a/tests/ui/missing_field_in_debug.rs b/tests/ui/missing_field_in_debug.rs
@@ -9,6 +9,7 @@ struct NamedStruct1Ignored {
 }
 
 impl fmt::Debug for NamedStruct1Ignored {
+    // unused field: hidden
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter
             .debug_struct("NamedStruct1Ignored")
@@ -26,6 +27,7 @@ struct NamedStructMultipleIgnored {
 }
 
 impl fmt::Debug for NamedStructMultipleIgnored {
+    // unused fields: hidden, hidden2, hidden4
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter
             .debug_struct("NamedStructMultipleIgnored")
@@ -37,6 +39,7 @@ impl fmt::Debug for NamedStructMultipleIgnored {
 
 struct Unit;
 
+// ok
 impl fmt::Debug for Unit {
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter.debug_struct("Unit").finish()
@@ -54,6 +57,7 @@ impl fmt::Debug for UnnamedStruct1Ignored {
 struct UnnamedStructMultipleIgnored(String, Vec<u8>, i32);
 
 impl fmt::Debug for UnnamedStructMultipleIgnored {
+    // unused fields: 0, 2
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter
             .debug_tuple("UnnamedStructMultipleIgnored")
@@ -67,6 +71,7 @@ struct NamedStructNonExhaustive {
     b: String,
 }
 
+// ok
 impl fmt::Debug for NamedStructNonExhaustive {
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter
@@ -81,7 +86,7 @@ struct MultiExprDebugImpl {
     b: String,
 }
 
-// Debug impl split up across multiple expressions
+// ok
 impl fmt::Debug for MultiExprDebugImpl {
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut f = formatter.debug_struct("MultiExprDebugImpl");
@@ -94,10 +99,11 @@ 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(), // OK
+            Self::A(n) => formatter.debug_tuple("A").field(&n).finish(),
         }
     }
 }
@@ -109,9 +115,10 @@ enum MultiVariantEnum {
 }
 
 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(), // OK
+            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(),
         }
@@ -124,11 +131,12 @@ enum MultiVariantEnumOk {
     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(), // OK
-            Self::B { a, b } => formatter.debug_struct("B").field("a", &a).field("b", &b).finish(), // OK
+            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(),
         }
     }
@@ -140,16 +148,73 @@ enum MultiVariantEnumNonExhaustive {
     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(), // OK
-            Self::B { a, b } => formatter.debug_struct("B").field("b", &b).finish_non_exhaustive(), // OK
+            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(),
         }
     }
 }
 
-fn main() {
-    // test code goes here
+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 {}
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/missing_field_in_debug.stderr b/tests/ui/missing_field_in_debug.stderr
