fix: `let_unit_value` suggests wrongly for format macros

6 files changed, 131 insertions, 35 deletions

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 96a6dee5885..792acec9e52 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -523,7 +523,8 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(|_| Box::new(same_name_method::SameNameMethod));
     store.register_late_pass(move |_| Box::new(index_refutable_slice::IndexRefutableSlice::new(conf)));
     store.register_late_pass(|_| Box::<shadow::Shadow>::default());
-    store.register_late_pass(|_| Box::new(unit_types::UnitTypes));
+    let format_args = format_args_storage.clone();
+    store.register_late_pass(move |_| Box::new(unit_types::UnitTypes::new(format_args.clone())));
     store.register_late_pass(move |_| Box::new(loops::Loops::new(conf)));
     store.register_late_pass(|_| Box::<main_recursion::MainRecursion>::default());
     store.register_late_pass(move |_| Box::new(lifetimes::Lifetimes::new(conf)));
diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs
index 87f184e13ce..d5b6c175854 100644
--- a/clippy_lints/src/unit_types/let_unit_value.rs
+++ b/clippy_lints/src/unit_types/let_unit_value.rs
@@ -1,17 +1,20 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::source::snippet_with_context;
+use clippy_utils::macros::{FormatArgsStorage, find_format_arg_expr, is_format_macro, root_macro_call_first_node};
+use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_context};
 use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
 use core::ops::ControlFlow;
+use rustc_ast::{FormatArgs, FormatArgumentKind};
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::intravisit::{Visitor, walk_body};
+use rustc_hir::intravisit::{Visitor, walk_body, walk_expr};
 use rustc_hir::{Expr, ExprKind, HirId, HirIdSet, LetStmt, MatchSource, Node, PatKind, QPath, TyKind};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::ty;
+use rustc_span::Span;
 
 use super::LET_UNIT_VALUE;
 
-pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsStorage, local: &'tcx LetStmt<'_>) {
     // skip `let () = { ... }`
     if let PatKind::Tuple(fields, ..) = local.pat.kind
         && fields.is_empty()
@@ -73,11 +76,8 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
                     let mut suggestions = Vec::new();
 
                     // Suggest omitting the `let` binding
-                    if let Some(expr) = &local.init {
-                        let mut app = Applicability::MachineApplicable;
-                        let snip = snippet_with_context(cx, expr.span, local.span.ctxt(), "()", &mut app).0;
-                        suggestions.push((local.span, format!("{snip};")));
-                    }
+                    let mut app = Applicability::MachineApplicable;
+                    let snip = snippet_with_context(cx, init.span, local.span.ctxt(), "()", &mut app).0;
 
                     // If this is a binding pattern, we need to add suggestions to remove any usages
                     // of the variable
@@ -85,53 +85,102 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
                         && let Some(body_id) = cx.enclosing_body.as_ref()
                     {
                         let body = cx.tcx.hir_body(*body_id);
-
-                        // Collect variable usages
-                        let mut visitor = UnitVariableCollector::new(binding_hir_id);
+                        let mut visitor = UnitVariableCollector::new(cx, format_args, binding_hir_id);
                         walk_body(&mut visitor, body);
 
-                        // Add suggestions for replacing variable usages
-                        suggestions.extend(visitor.spans.into_iter().map(|span| (span, "()".to_string())));
-                    }
+                        let mut has_in_format_capture = false;
+                        suggestions.extend(visitor.spans.iter().filter_map(|span| match span {
+                            MaybeInFormatCapture::Yes => {
+                                has_in_format_capture = true;
+                                None
+                            },
+                            MaybeInFormatCapture::No(span) => Some((*span, "()".to_string())),
+                        }));
 
-                    // Emit appropriate diagnostic based on whether there are usages of the let binding
-                    if !suggestions.is_empty() {
-                        let message = if suggestions.len() == 1 {
-                            "omit the `let` binding"
-                        } else {
-                            "omit the `let` binding and replace variable usages with `()`"
-                        };
-                        diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
+                        if has_in_format_capture {
+                            suggestions.push((
+                                init.span,
+                                format!("();\n{}", reindent_multiline(&snip, false, indent_of(cx, local.span))),
+                            ));
+                            diag.multipart_suggestion(
+                                "replace variable usages with `()`",
+                                suggestions,
+                                Applicability::MachineApplicable,
+                            );
+                            return;
+                        }
                     }
+
+                    suggestions.push((local.span, format!("{snip};")));
+                    let message = if suggestions.len() == 1 {
+                        "omit the `let` binding"
+                    } else {
+                        "omit the `let` binding and replace variable usages with `()`"
+                    };
+                    diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
                 },
             );
         }
     }
 }
 
-struct UnitVariableCollector {
+struct UnitVariableCollector<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    format_args: &'a FormatArgsStorage,
     id: HirId,
-    spans: Vec<rustc_span::Span>,
+    spans: Vec<MaybeInFormatCapture>,
+    macro_call: Option<&'a FormatArgs>,
 }
 
-impl UnitVariableCollector {
-    fn new(id: HirId) -> Self {
-        Self { id, spans: vec![] }
+enum MaybeInFormatCapture {
+    Yes,
+    No(Span),
+}
+
+impl<'a, 'tcx> UnitVariableCollector<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'tcx>, format_args: &'a FormatArgsStorage, id: HirId) -> Self {
+        Self {
+            cx,
+            format_args,
+            id,
+            spans: Vec::new(),
+            macro_call: None,
+        }
     }
 }
 
 /**
  * Collect all instances where a variable is used based on its `HirId`.
  */
-impl<'tcx> Visitor<'tcx> for UnitVariableCollector {
+impl<'tcx> Visitor<'tcx> for UnitVariableCollector<'_, 'tcx> {
     fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) -> Self::Result {
+        if let Some(macro_call) = root_macro_call_first_node(self.cx, ex)
+            && is_format_macro(self.cx, macro_call.def_id)
+            && let Some(format_args) = self.format_args.get(self.cx, ex, macro_call.expn)
+        {
+            let parent_macro_call = self.macro_call;
+            self.macro_call = Some(format_args);
+            walk_expr(self, ex);
+            self.macro_call = parent_macro_call;
+            return;
+        }
+
         if let ExprKind::Path(QPath::Resolved(None, path)) = ex.kind
             && let Res::Local(id) = path.res
             && id == self.id
         {
-            self.spans.push(path.span);
+            if let Some(macro_call) = self.macro_call
+                && macro_call.arguments.all_args().iter().any(|arg| {
+                    matches!(arg.kind, FormatArgumentKind::Captured(_)) && find_format_arg_expr(ex, arg).is_some()
+                })
+            {
+                self.spans.push(MaybeInFormatCapture::Yes);
+            } else {
+                self.spans.push(MaybeInFormatCapture::No(path.span));
+            }
         }
-        rustc_hir::intravisit::walk_expr(self, ex);
+
+        walk_expr(self, ex);
     }
 }
 
diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs
index e016bd3434b..4ffcc247acf 100644
--- a/clippy_lints/src/unit_types/mod.rs
+++ b/clippy_lints/src/unit_types/mod.rs
@@ -3,9 +3,10 @@ mod unit_arg;
 mod unit_cmp;
 mod utils;
 
+use clippy_utils::macros::FormatArgsStorage;
 use rustc_hir::{Expr, LetStmt};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_session::declare_lint_pass;
+use rustc_session::impl_lint_pass;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -96,11 +97,21 @@ declare_clippy_lint! {
     "passing unit to a function"
 }
 
-declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);
+pub struct UnitTypes {
+    format_args: FormatArgsStorage,
+}
+
+impl_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);
+
+impl UnitTypes {
+    pub fn new(format_args: FormatArgsStorage) -> Self {
+        Self { format_args }
+    }
+}
 
 impl<'tcx> LateLintPass<'tcx> for UnitTypes {
     fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx LetStmt<'tcx>) {
-        let_unit_value::check(cx, local);
+        let_unit_value::check(cx, &self.format_args, local);
     }
 
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
diff --git a/tests/ui/let_unit.fixed b/tests/ui/let_unit.fixed
index 5e7a2ad37a8..0484ab29730 100644
--- a/tests/ui/let_unit.fixed
+++ b/tests/ui/let_unit.fixed
@@ -198,3 +198,14 @@ pub fn issue12594() {
         returns_result(res).unwrap();
     }
 }
+
+fn issue15061() {
+    fn return_unit() {}
+    fn do_something(x: ()) {}
+
+    let res = ();
+    return_unit();
+    //~^ let_unit_value
+    do_something(());
+    println!("{res:?}");
+}
diff --git a/tests/ui/let_unit.rs b/tests/ui/let_unit.rs
index 7b06f694012..a0afc995b6f 100644
--- a/tests/ui/let_unit.rs
+++ b/tests/ui/let_unit.rs
@@ -198,3 +198,13 @@ pub fn issue12594() {
         returns_result(res).unwrap();
     }
 }
+
+fn issue15061() {
+    fn return_unit() {}
+    fn do_something(x: ()) {}
+
+    let res = return_unit();
+    //~^ let_unit_value
+    do_something(res);
+    println!("{res:?}");
+}
diff --git a/tests/ui/let_unit.stderr b/tests/ui/let_unit.stderr
index d7d01d304ca..0bc4da8b9c6 100644
--- a/tests/ui/let_unit.stderr
+++ b/tests/ui/let_unit.stderr
@@ -68,5 +68,19 @@ LL ~         returns_result(()).unwrap();
 LL ~         returns_result(()).unwrap();
    |
 
-error: aborting due to 4 previous errors
+error: this let-binding has unit value
+  --> tests/ui/let_unit.rs:206:5
+   |
+LL |     let res = return_unit();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace variable usages with `()`
+   |
+LL ~     let res = ();
+LL ~     return_unit();
+LL |
+LL ~     do_something(());
+   |
+
+error: aborting due to 5 previous errors