consider string literal in .field() calls as used

Author: y21

clippy_lints/src/lib.rs

@@ -961,7 +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_fields_in_debug::MissingFieldInDebug));
+    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`
 }
 

clippy_lints/src/missing_fields_in_debug.rs

@@ -6,10 +6,11 @@ use clippy_utils::{
     ty::match_type,
     visitors::{for_each_expr, Visitable},
 };
+use rustc_ast::LitKind;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::{
     def::{DefKind, Res},
-    ImplItemKind, MatchSource, Node,
+    Expr, ImplItemKind, MatchSource, Node,
 };
 use rustc_hir::{Block, PatKind};
 use rustc_hir::{ExprKind, Impl, ItemKind, QPath, TyKind};
@@ -18,7 +19,7 @@ use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::Ty;
 use rustc_middle::ty::TypeckResults;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{sym, Span};
+use rustc_span::{sym, Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -32,8 +33,8 @@ declare_clippy_lint! {
     /// 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.
+    /// 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.
     ///
@@ -79,7 +80,7 @@ declare_clippy_lint! {
     pedantic,
     "missing fields in manual `Debug` implementation"
 }
-declare_lint_pass!(MissingFieldInDebug => [MISSING_FIELDS_IN_DEBUG]);
+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(
@@ -100,35 +101,49 @@ fn report_lints(cx: &LateContext<'_>, span: Span, span_notes: Vec<(Span, &'stati
 /// 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()`.
+/// 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 {
     let mut has_finish_non_exhaustive = false;
-    let mut has_debug_struct_tuple = false;
+    let mut has_debug_struct = false;
 
-    let _: Option<!> = for_each_expr(block, |expr| {
-        let ExprKind::MethodCall(path, recv, ..) = &expr.kind else {
-            return ControlFlow::Continue(());
-        };
-        let recv_ty = typeck_results.expr_ty(recv).peel_refs();
-
-        if [sym::debug_struct, sym::debug_tuple].contains(&path.ident.name)
-            && match_type(cx, recv_ty, &paths::FORMATTER)
-        {
-            has_debug_struct_tuple = true;
-        }
+    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.as_str() == "finish_non_exhaustive" && match_type(cx, recv_ty, &paths::DEBUG_STRUCT) {
-            has_finish_non_exhaustive = true;
+            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(())
+        ControlFlow::<!, _>::Continue(())
     });
 
-    !has_finish_non_exhaustive && has_debug_struct_tuple
+    !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
+fn as_field_call<'tcx>(
+    cx: &LateContext<'tcx>,
+    typeck_results: &TypeckResults<'tcx>,
+    expr: &Expr<'_>,
+) -> Option<Symbol> {
+    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
@@ -140,33 +155,37 @@ fn check_struct<'tcx>(
     item: &'tcx Item<'tcx>,
     data: &VariantData<'_>,
 ) {
+    let mut has_direct_field_access = false;
     let mut field_accesses = FxHashSet::default();
 
-    let _: Option<!> = for_each_expr(block, |expr| {
+    for_each_expr(block, |expr| {
         if let ExprKind::Field(target, ident) = expr.kind
             && let target_ty = typeck_results.expr_ty(target).peel_refs()
             && target_ty == self_ty
         {
-            field_accesses.insert(ident);
+            has_direct_field_access = true;
+            field_accesses.insert(ident.name);
+        } else if let Some(sym) = as_field_call(cx, typeck_results, expr) {
+            field_accesses.insert(sym);
         }
-        ControlFlow::Continue(())
+        ControlFlow::<!, _>::Continue(())
     });
 
     let span_notes = data
         .fields()
         .iter()
         .filter_map(|field| {
-            if field_accesses.contains(&field.ident) {
+            if field_accesses.contains(&field.ident.name) {
                 None
             } else {
                 Some((field.span, "this field is unused"))
             }
         })
         .collect::<Vec<_>>();
 
-    // only lint if there's also at least one field access to allow patterns
+    // 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() && !field_accesses.is_empty() {
+    if !span_notes.is_empty() && has_direct_field_access {
         report_lints(cx, item.span, span_notes);
     }
 }
@@ -216,18 +235,22 @@ fn check_enum<'tcx>(
         });
 
         let mut field_accesses = FxHashSet::default();
+        let mut check_field_access = |sym| {
+            arm.pat.each_binding(|_, _, _, pat_ident| {
+                if sym == pat_ident.name {
+                    field_accesses.insert(pat_ident);
+                }
+            });
+        };
 
-        let _: Option<!> = for_each_expr(arm.body, |expr| {
-            if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind
-                && let Some(segment) = path.segments.first()
+        for_each_expr(arm.body, |expr| {
+            if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind && let Some(segment) = path.segments.first()
             {
-                arm.pat.each_binding(|_, _, _, pat_ident| {
-                    if segment.ident == pat_ident {
-                        field_accesses.insert(pat_ident);
-                    }
-                });
+                check_field_access(segment.ident.name);
+            } else if let Some(sym) = as_field_call(cx, typeck_results, expr) {
+                check_field_access(sym);
             }
-            ControlFlow::Continue(())
+            ControlFlow::<!, _>::Continue(())
         });
 
         arm.pat.each_binding(|_, _, span, pat_ident| {
@@ -242,7 +265,7 @@ fn check_enum<'tcx>(
     }
 }
 
-impl<'tcx> LateLintPass<'tcx> for MissingFieldInDebug {
+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

tests/ui/missing_fields_in_debug.rs

@@ -56,8 +56,8 @@ impl fmt::Debug for UnnamedStruct1Ignored {
 
 struct UnnamedStructMultipleIgnored(String, Vec<u8>, i32);
 
+// tuple structs are not linted
 impl fmt::Debug for UnnamedStructMultipleIgnored {
-    // unused fields: 0, 2
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter
             .debug_tuple("UnnamedStructMultipleIgnored")
@@ -257,4 +257,16 @@ impl fmt::Debug for HasInner {
     }
 }
 
+// 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()
+    }
+}
+
 fn main() {}

tests/ui/missing_fields_in_debug.stderr

@@ -49,31 +49,6 @@ 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:59:1
-   |
-LL | / impl fmt::Debug for UnnamedStructMultipleIgnored {
-LL | |     // unused fields: 0, 2
-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:57:37
-   |
-LL | struct UnnamedStructMultipleIgnored(String, Vec<u8>, i32);
-   |                                     ^^^^^^
-note: this field is unused
-  --> $DIR/missing_fields_in_debug.rs:57:54
-   |
-LL | struct UnnamedStructMultipleIgnored(String, Vec<u8>, i32);
-   |                                                      ^^^
-   = 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:90:1
    |
@@ -134,5 +109,5 @@ LL |             Self::B { b, .. } => formatter.debug_struct("B").field("b", &b)
    = 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 6 previous errors
+error: aborting due to 5 previous errors