fix(let_unit_value): create the suggestion "differentially" (#15788)

I.e, instead of constructing a whole new `LetStmt` to replace the old
one, suggest to only change the part of the `LetStmt` that we don't
like. This makes the suggestion less likely to introduce spurious
changes, but also makes the diagnostic much nicer imo.

Fixes #15771 by not adding a semicolon that the
initial `LetStmt` didn't have
Fixes #15784 by using the `init.span` with the
correct `SyntaxContext`

changelog: [`let_unit_value`]: don't add spurious semicolon
changelog: [`let_unit_value`]: don't mangle macro calls

Author: samueltardieu

clippy_lints/src/unit_types/let_unit_value.rs

@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 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::source::{snippet_indent, walk_span_to_context};
 use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
 use core::ops::ControlFlow;
 use rustc_ast::{FormatArgs, FormatArgumentKind};
@@ -74,10 +74,10 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsStorag
                 "this let-binding has unit value",
                 |diag| {
                     let mut suggestions = Vec::new();
+                    let init_new_span = walk_span_to_context(init.span, local.span.ctxt()).unwrap();
 
                     // Suggest omitting the `let` binding
-                    let mut app = Applicability::MachineApplicable;
-                    let snip = snippet_with_context(cx, init.span, local.span.ctxt(), "()", &mut app).0;
+                    let app = Applicability::MachineApplicable;
 
                     // If this is a binding pattern, we need to add suggestions to remove any usages
                     // of the variable
@@ -89,35 +89,48 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, format_args: &FormatArgsStorag
                         walk_body(&mut visitor, body);
 
                         let mut has_in_format_capture = false;
-                        suggestions.extend(visitor.spans.iter().filter_map(|span| match span {
+                        suggestions.extend(visitor.spans.into_iter().filter_map(|span| match span {
                             MaybeInFormatCapture::Yes => {
                                 has_in_format_capture = true;
                                 None
                             },
-                            MaybeInFormatCapture::No(span) => Some((*span, "()".to_string())),
+                            MaybeInFormatCapture::No(span) => Some((span, "()".to_string())),
                         }));
 
                         if has_in_format_capture {
+                            // In a case like this:
+                            // ```
+                            // let unit = returns_unit();
+                            // eprintln!("{unit}");
+                            // ```
+                            // we can't remove the `unit` binding and replace its uses with a `()`,
+                            // because the `eprintln!` would break.
+                            //
+                            // So do the following instead:
+                            // ```
+                            // let unit = ();
+                            // returns_unit();
+                            // eprintln!("{unit}");
+                            // ```
+                            // TODO: find a less awkward way to do this
                             suggestions.push((
-                                init.span,
-                                format!("();\n{}", reindent_multiline(&snip, false, indent_of(cx, local.span))),
+                                init_new_span.shrink_to_lo(),
+                                format!("();\n{}", snippet_indent(cx, local.span).as_deref().unwrap_or("")),
                             ));
-                            diag.multipart_suggestion(
-                                "replace variable usages with `()`",
-                                suggestions,
-                                Applicability::MachineApplicable,
-                            );
+                            diag.multipart_suggestion_verbose("replace variable usages with `()`", suggestions, app);
                             return;
                         }
                     }
 
-                    suggestions.push((local.span, format!("{snip};")));
+                    // let local = returns_unit();
+                    // ^^^^^^^^^^^^ remove this
+                    suggestions.push((local.span.until(init_new_span), String::new()));
                     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);
+                    diag.multipart_suggestion_verbose(message, suggestions, app);
                 },
             );
         }
@@ -132,6 +145,12 @@ struct UnitVariableCollector<'a, 'tcx> {
     macro_call: Option<&'a FormatArgs>,
 }
 
+/// Whether the unit variable is captured in a `format!`:
+///
+/// ```ignore
+/// let unit = ();
+/// eprintln!("{unit}");
+/// ```
 enum MaybeInFormatCapture {
     Yes,
     No(Span),

tests/ui/let_unit.fixed

@@ -1,5 +1,10 @@
 #![warn(clippy::let_unit_value)]
-#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]
+#![allow(
+    clippy::no_effect,
+    clippy::needless_late_init,
+    path_statements,
+    clippy::match_single_binding
+)]
 
 macro_rules! let_and_return {
     ($n:expr) => {{
@@ -15,12 +20,12 @@ fn main() {
     if true {
         // do not lint this, since () is explicit
         let _a = ();
-        let () = dummy();
+        let () = returns_unit();
         let () = ();
-        () = dummy();
+        () = returns_unit();
         () = ();
         let _a: () = ();
-        let _a: () = dummy();
+        let _a: () = returns_unit();
     }
 
     consume_units_with_for_loop(); // should be fine as well
@@ -30,7 +35,7 @@ fn main() {
     let_and_return!(()) // should be fine
 }
 
-fn dummy() {}
+fn returns_unit() {}
 
 // Related to issue #1964
 fn consume_units_with_for_loop() {
@@ -181,8 +186,6 @@ async fn issue10433() {
 pub async fn issue11502(a: ()) {}
 
 pub fn issue12594() {
-    fn returns_unit() {}
-
     fn returns_result<T>(res: T) -> Result<T, ()> {
         Ok(res)
     }
@@ -199,13 +202,30 @@ pub fn issue12594() {
     }
 }
 
+fn takes_unit(x: ()) {}
+
 fn issue15061() {
-    fn return_unit() {}
-    fn do_something(x: ()) {}
+    let res = ();
+    returns_unit();
+    //~^ let_unit_value
+    takes_unit(());
+    println!("{res:?}");
+}
+
+fn issue15771() {
+    match "Example String" {
+        _ => returns_unit(),
+        //~^ let_unit_value
+    }
+
+    if true {}
+    //~^ let_unit_value
+}
 
+fn issue_15784() {
     let res = ();
-    return_unit();
+    eprintln!("I return unit");
     //~^ let_unit_value
-    do_something(());
+    takes_unit(());
     println!("{res:?}");
 }

tests/ui/let_unit.rs

@@ -1,5 +1,10 @@
 #![warn(clippy::let_unit_value)]
-#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]
+#![allow(
+    clippy::no_effect,
+    clippy::needless_late_init,
+    path_statements,
+    clippy::match_single_binding
+)]
 
 macro_rules! let_and_return {
     ($n:expr) => {{
@@ -15,12 +20,12 @@ fn main() {
     if true {
         // do not lint this, since () is explicit
         let _a = ();
-        let () = dummy();
+        let () = returns_unit();
         let () = ();
-        () = dummy();
+        () = returns_unit();
         () = ();
         let _a: () = ();
-        let _a: () = dummy();
+        let _a: () = returns_unit();
     }
 
     consume_units_with_for_loop(); // should be fine as well
@@ -30,7 +35,7 @@ fn main() {
     let_and_return!(()) // should be fine
 }
 
-fn dummy() {}
+fn returns_unit() {}
 
 // Related to issue #1964
 fn consume_units_with_for_loop() {
@@ -181,8 +186,6 @@ async fn issue10433() {
 pub async fn issue11502(a: ()) {}
 
 pub fn issue12594() {
-    fn returns_unit() {}
-
     fn returns_result<T>(res: T) -> Result<T, ()> {
         Ok(res)
     }
@@ -199,12 +202,28 @@ pub fn issue12594() {
     }
 }
 
+fn takes_unit(x: ()) {}
+
 fn issue15061() {
-    fn return_unit() {}
-    fn do_something(x: ()) {}
+    let res = returns_unit();
+    //~^ let_unit_value
+    takes_unit(res);
+    println!("{res:?}");
+}
+
+fn issue15771() {
+    match "Example String" {
+        _ => _ = returns_unit(),
+        //~^ let_unit_value
+    }
+
+    _ = if true {}
+    //~^ let_unit_value
+}
 
-    let res = return_unit();
+fn issue_15784() {
+    let res = eprintln!("I return unit");
     //~^ let_unit_value
-    do_something(res);
+    takes_unit(res);
     println!("{res:?}");
 }

tests/ui/let_unit.stderr

@@ -1,14 +1,19 @@
 error: this let-binding has unit value
-  --> tests/ui/let_unit.rs:11:5
+  --> tests/ui/let_unit.rs:16:5
    |
 LL |     let _x = println!("x");
-   |     ^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `println!("x");`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::let-unit-value` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::let_unit_value)]`
+help: omit the `let` binding
+   |
+LL -     let _x = println!("x");
+LL +     println!("x");
+   |
 
 error: this let-binding has unit value
-  --> tests/ui/let_unit.rs:60:5
+  --> tests/ui/let_unit.rs:65:5
    |
 LL | /     let _ = v
 LL | |
@@ -21,18 +26,12 @@ LL | |         .unwrap();
    |
 help: omit the `let` binding
    |
-LL ~     v
-LL +
-LL +         .into_iter()
-LL +         .map(|i| i * 2)
-LL +         .filter(|i| i.is_multiple_of(2))
-LL +         .map(|_| ())
-LL +         .next()
-LL +         .unwrap();
+LL -     let _ = v
+LL +     v
    |
 
 error: this let-binding has unit value
-  --> tests/ui/let_unit.rs:110:5
+  --> tests/ui/let_unit.rs:115:5
    |
 LL | /     let x = match Some(0) {
 LL | |
@@ -45,17 +44,12 @@ LL | |     };
    |
 help: omit the `let` binding
    |
-LL ~     match Some(0) {
-LL +
-LL +         None => f2(1),
-LL +         Some(0) => f(),
-LL +         Some(1) => f2(3),
-LL +         Some(_) => (),
-LL +     };
+LL -     let x = match Some(0) {
+LL +     match Some(0) {
    |
 
 error: this let-binding has unit value
-  --> tests/ui/let_unit.rs:192:9
+  --> tests/ui/let_unit.rs:195:9
    |
 LL |         let res = returns_unit();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -69,18 +63,56 @@ LL ~         returns_result(()).unwrap();
    |
 
 error: this let-binding has unit value
-  --> tests/ui/let_unit.rs:206:5
+  --> tests/ui/let_unit.rs:208:5
+   |
+LL |     let res = returns_unit();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace variable usages with `()`
+   |
+LL ~     let res = ();
+LL ~     returns_unit();
+LL |
+LL ~     takes_unit(());
+   |
+
+error: this let-binding has unit value
+  --> tests/ui/let_unit.rs:216:14
+   |
+LL |         _ => _ = returns_unit(),
+   |              ^^^^^^^^^^^^^^^^^^
+   |
+help: omit the `let` binding
+   |
+LL -         _ => _ = returns_unit(),
+LL +         _ => returns_unit(),
+   |
+
+error: this let-binding has unit value
+  --> tests/ui/let_unit.rs:220:5
+   |
+LL |     _ = if true {}
+   |     ^^^^^^^^^^^^^^
+   |
+help: omit the `let` binding
+   |
+LL -     _ = if true {}
+LL +     if true {}
+   |
+
+error: this let-binding has unit value
+  --> tests/ui/let_unit.rs:225:5
    |
-LL |     let res = return_unit();
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+LL |     let res = eprintln!("I return unit");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 help: replace variable usages with `()`
    |
 LL ~     let res = ();
-LL ~     return_unit();
+LL ~     eprintln!("I return unit");
 LL |
-LL ~     do_something(());
+LL ~     takes_unit(());
    |
 
-error: aborting due to 5 previous errors
+error: aborting due to 8 previous errors