special-case PD, account for field access through Deref

y21 · y21 · commit 003ffb353ea3 · 2023-04-28T01:01:41.000+02:00
diff --git a/clippy_lints/src/missing_fields_in_debug.rs b/clippy_lints/src/missing_fields_in_debug.rs
@@ -16,8 +16,8 @@ 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};
 
@@ -41,6 +41,9 @@ declare_clippy_lint! {
     /// 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;
@@ -107,7 +110,9 @@ fn should_lint<'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| {
@@ -128,6 +133,8 @@ fn should_lint<'tcx>(
 
 /// 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>,
@@ -155,16 +162,19 @@ fn check_struct<'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(target).peel_refs()
+            && let target_ty = typeck_results.expr_ty_adjusted(target).peel_refs()
             && target_ty == self_ty
         {
-            has_direct_field_access = true;
             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);
         }
@@ -175,7 +185,8 @@ fn check_struct<'tcx>(
         .fields()
         .iter()
         .filter_map(|field| {
-            if field_accesses.contains(&field.ident.name) {
+            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"))
@@ -204,7 +215,7 @@ fn check_enum<'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(val).peel_refs()
+            && let match_ty = typeck_results.expr_ty_adjusted(val).peel_refs()
             && match_ty == self_ty
         {
             ControlFlow::Break(arms)
@@ -234,20 +245,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 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);
+                check_field_access(segment.ident.name, expr);
             } else if let Some(sym) = as_field_call(cx, typeck_results, expr) {
-                check_field_access(sym);
+                check_field_access(sym, expr);
             }
             ControlFlow::<!, _>::Continue(())
         });
diff --git a/tests/ui/missing_fields_in_debug.rs b/tests/ui/missing_fields_in_debug.rs
@@ -2,6 +2,8 @@
 #![warn(clippy::missing_fields_in_debug)]
 
 use std::fmt;
+use std::marker::PhantomData;
+use std::ops::Deref;
 
 struct NamedStruct1Ignored {
     data: u8,
@@ -269,4 +271,49 @@ impl fmt::Debug for Foo {
     }
 }
 
+// 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<String>,
+}
+
+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
@@ -1,5 +1,5 @@
 error: manual `Debug` impl does not include all fields
-  --> $DIR/missing_fields_in_debug.rs:11:1
+  --> $DIR/missing_fields_in_debug.rs:13:1
    |
 LL | / impl fmt::Debug for NamedStruct1Ignored {
 LL | |     // unused field: hidden
@@ -11,7 +11,7 @@ LL | | }
    | |_^
    |
 note: this field is unused
-  --> $DIR/missing_fields_in_debug.rs:8:5
+  --> $DIR/missing_fields_in_debug.rs:10:5
    |
 LL |     hidden: u32,
    |     ^^^^^^^^^^^
@@ -20,7 +20,7 @@ LL |     hidden: u32,
    = 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:29:1
+  --> $DIR/missing_fields_in_debug.rs:31:1
    |
 LL | / impl fmt::Debug for NamedStructMultipleIgnored {
 LL | |     // unused fields: hidden, hidden2, hidden4
@@ -32,25 +32,25 @@ LL | | }
    | |_^
    |
 note: this field is unused
-  --> $DIR/missing_fields_in_debug.rs:23:5
+  --> $DIR/missing_fields_in_debug.rs:25:5
    |
 LL |     hidden: u32,
    |     ^^^^^^^^^^^
 note: this field is unused
-  --> $DIR/missing_fields_in_debug.rs:24:5
+  --> $DIR/missing_fields_in_debug.rs:26:5
    |
 LL |     hidden2: String,
    |     ^^^^^^^^^^^^^^^
 note: this field is unused
-  --> $DIR/missing_fields_in_debug.rs:26:5
+  --> $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:90:1
+  --> $DIR/missing_fields_in_debug.rs:92:1
    |
 LL | / impl fmt::Debug for MultiExprDebugImpl {
 LL | |     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -62,15 +62,15 @@ LL | | }
    | |_^
    |
 note: this field is unused
-  --> $DIR/missing_fields_in_debug.rs:86:5
+  --> $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:117:1
+  --> $DIR/missing_fields_in_debug.rs:119:1
    |
 LL | / impl fmt::Debug for MultiVariantEnum {
 LL | |     // match arm Self::B ignores `b`
@@ -82,15 +82,15 @@ LL | | }
    | |_^
    |
 note: the field referenced by this binding is unused
-  --> $DIR/missing_fields_in_debug.rs:122:26
+  --> $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:168:1
+  --> $DIR/missing_fields_in_debug.rs:170:1
    |
 LL | / impl fmt::Debug for MultiVariantRest {
 LL | |     // `a` field ignored due to rest pattern
@@ -102,7 +102,7 @@ LL | | }
    | |_^
    |
 note: more unused fields here due to rest pattern `..`
-  --> $DIR/missing_fields_in_debug.rs:173:13
+  --> $DIR/missing_fields_in_debug.rs:175:13
    |
 LL |             Self::B { b, .. } => formatter.debug_struct("B").field("b", &b).finish(),
    |             ^^^^^^^^^^^^^^^^^
