if_chain -> let chains, fix markdown, allow newtype pattern

Author: y21

clippy_lints/src/missing_fields_in_debug.rs

@@ -6,7 +6,6 @@ use clippy_utils::{
     ty::match_type,
     visitors::{for_each_expr, Visitable},
 };
-use if_chain::if_chain;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::{
     def::{DefKind, Res},
@@ -23,25 +22,24 @@ use rustc_span::{sym, Span};
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for manual [`core::fmt::Debug`] implementations that do not use all fields.
+    /// 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`]
+    /// 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`] 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
+    /// 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;
@@ -83,21 +81,20 @@ declare_clippy_lint! {
 }
 declare_lint_pass!(MissingFieldInDebug => [MISSING_FIELDS_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(cx: &LateContext<'_>, span: Span, span_notes: Vec<(Span, &'static str)>) {
-    span_lint_and_then(cx, MISSING_FIELDS_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);
-    });
+    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
@@ -119,20 +116,14 @@ fn should_lint<'tcx>(
         };
         let recv_ty = typeck_results.expr_ty(recv).peel_refs();
 
-        if_chain! {
-            if [sym::debug_struct, sym::debug_tuple].contains(&path.ident.name);
-            if match_type(cx, recv_ty, &paths::FORMATTER);
-            then {
-                has_debug_struct_tuple = true;
-            }
+        if [sym::debug_struct, sym::debug_tuple].contains(&path.ident.name)
+            && match_type(cx, recv_ty, &paths::FORMATTER)
+        {
+            has_debug_struct_tuple = true;
         }
 
-        if_chain! {
-            if path.ident.name.as_str() == "finish_non_exhaustive";
-            if match_type(cx, recv_ty, &paths::DEBUG_STRUCT);
-            then {
-                has_finish_non_exhaustive = true;
-            }
+        if path.ident.name.as_str() == "finish_non_exhaustive" && match_type(cx, recv_ty, &paths::DEBUG_STRUCT) {
+            has_finish_non_exhaustive = true;
         }
         ControlFlow::Continue(())
     });
@@ -152,13 +143,11 @@ fn check_struct<'tcx>(
     let mut field_accesses = FxHashSet::default();
 
     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);
-            }
+        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);
         }
         ControlFlow::Continue(())
     });
@@ -170,12 +159,14 @@ fn check_struct<'tcx>(
             if field_accesses.contains(&field.ident) {
                 None
             } else {
-                Some((field.span, HELP_FIELD_UNUSED))
+                Some((field.span, "this field is unused"))
             }
         })
         .collect::<Vec<_>>();
 
-    if !span_notes.is_empty() {
+    // only lint if there's also at least one 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() {
         report_lints(cx, item.span, span_notes);
     }
 }
@@ -194,56 +185,54 @@ fn check_enum<'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(())
-            }
+        if let ExprKind::Match(val, arms, MatchSource::Normal) = expr.kind
+            && let match_ty = typeck_results.expr_ty(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 {
+    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, HELP_WILDCARD)),
+        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, HELP_UNUSED_REST));
+                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 `..`"))
             },
-            PatKind::Struct(.., true) => span_notes.push((pat.span, HELP_UNUSED_REST)),
             _ => {},
         });
 
         let mut field_accesses = FxHashSet::default();
 
         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);
-                        }
-                    });
-                }
+            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);
+                    }
+                });
             }
             ControlFlow::Continue(())
         });
 
         arm.pat.each_binding(|_, _, span, pat_ident| {
             if !field_accesses.contains(&pat_ident) {
-                span_notes.push((span, HELP_FIELD_REFERENCE));
+                span_notes.push((span, "the field referenced by this binding is unused"));
             }
         });
     }
@@ -255,38 +244,36 @@ fn check_enum<'tcx>(
 
 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]);
-
+        // 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
-            if !cx.tcx.has_attr(item.owner_id, sym::automatically_derived);
-            if !item.span.from_expansion();
-
+            && !cx.tcx.has_attr(item.owner_id, sym::automatically_derived)
+            && !item.span.from_expansion()
+            
             // 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;
-
+            && 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();
-            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);
-
+            && 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);
-            if should_lint(cx, typeck_results, block);
-            then {
-                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),
-                    _ => {}
-                }
+            && 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),
+                _ => {}
             }
         }
     }

tests/ui/missing_field_in_debug.rs renamed to tests/ui/missing_fields_in_debug.rs

@@ -229,4 +229,32 @@ enum DerivedEnum {
     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()
+    }
+}
+
 fn main() {}