`while_let_loop`: Include `let` assignment in suggestion (#14756)

Placeholders are still given for the content of the whole block.
However, if the result of the original `if let` or `match` expression
was assigned, the assignment is reflected in the suggestion.

No-op assignments (`let x = x;`) are skipped though, unless they contain
an explicit type which might help the compiler (`let x: u32 = x;` is
kept).

Closes rust-lang/rust-clippy#362

changelog: [`while_let_loop`]: include `let` assignment in suggestion

3 files changed, 271 insertions, 40 deletions

diff --git a/clippy_lints/src/loops/while_let_loop.rs b/clippy_lints/src/loops/while_let_loop.rs
index bd04827a1f0..845edb9cae1 100644
--- a/clippy_lints/src/loops/while_let_loop.rs
+++ b/clippy_lints/src/loops/while_let_loop.rs
@@ -1,68 +1,65 @@
 use super::WHILE_LET_LOOP;
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::higher;
-use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::source::{snippet, snippet_indent, snippet_opt};
 use clippy_utils::ty::needs_ordered_drop;
 use clippy_utils::visitors::any_temporaries_need_ordered_drop;
+use clippy_utils::{higher, peel_blocks};
+use rustc_ast::BindingMode;
 use rustc_errors::Applicability;
-use rustc_hir::{Block, Expr, ExprKind, LetStmt, MatchSource, Pat, StmtKind};
+use rustc_hir::{Block, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind, Path, QPath, StmtKind, Ty};
 use rustc_lint::LateContext;
 
 pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'tcx Block<'_>) {
-    let (init, has_trailing_exprs) = match (loop_block.stmts, loop_block.expr) {
-        ([stmt, stmts @ ..], expr) => {
-            if let StmtKind::Let(&LetStmt {
+    let (init, let_info) = match (loop_block.stmts, loop_block.expr) {
+        ([stmt, ..], _) => match stmt.kind {
+            StmtKind::Let(LetStmt {
                 init: Some(e),
                 els: None,
+                pat,
+                ty,
                 ..
-            })
-            | StmtKind::Semi(e)
-            | StmtKind::Expr(e) = stmt.kind
-            {
-                (e, !stmts.is_empty() || expr.is_some())
-            } else {
-                return;
-            }
+            }) => (*e, Some((*pat, *ty))),
+            StmtKind::Semi(e) | StmtKind::Expr(e) => (e, None),
+            _ => return,
         },
-        ([], Some(e)) => (e, false),
+        ([], Some(e)) => (e, None),
         _ => return,
     };
+    let has_trailing_exprs = loop_block.stmts.len() + usize::from(loop_block.expr.is_some()) > 1;
 
     if let Some(if_let) = higher::IfLet::hir(cx, init)
         && let Some(else_expr) = if_let.if_else
         && is_simple_break_expr(else_expr)
     {
-        could_be_while_let(cx, expr, if_let.let_pat, if_let.let_expr, has_trailing_exprs);
+        could_be_while_let(
+            cx,
+            expr,
+            if_let.let_pat,
+            if_let.let_expr,
+            has_trailing_exprs,
+            let_info,
+            if_let.if_then,
+        );
     } else if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal) = init.kind
         && arm1.guard.is_none()
         && arm2.guard.is_none()
         && is_simple_break_expr(arm2.body)
     {
-        could_be_while_let(cx, expr, arm1.pat, scrutinee, has_trailing_exprs);
+        could_be_while_let(cx, expr, arm1.pat, scrutinee, has_trailing_exprs, let_info, arm1.body);
     }
 }
 
-/// Returns `true` if expr contains a single break expression without a label or eub-expression.
+/// Returns `true` if expr contains a single break expression without a label or sub-expression,
+/// possibly embedded in blocks.
 fn is_simple_break_expr(e: &Expr<'_>) -> bool {
-    matches!(peel_blocks(e).kind, ExprKind::Break(dest, None) if dest.label.is_none())
-}
-
-/// Removes any blocks containing only a single expression.
-fn peel_blocks<'tcx>(e: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
     if let ExprKind::Block(b, _) = e.kind {
         match (b.stmts, b.expr) {
-            ([s], None) => {
-                if let StmtKind::Expr(e) | StmtKind::Semi(e) = s.kind {
-                    peel_blocks(e)
-                } else {
-                    e
-                }
-            },
-            ([], Some(e)) => peel_blocks(e),
-            _ => e,
+            ([s], None) => matches!(s.kind, StmtKind::Expr(e) | StmtKind::Semi(e) if is_simple_break_expr(e)),
+            ([], Some(e)) => is_simple_break_expr(e),
+            _ => false,
         }
     } else {
-        e
+        matches!(e.kind, ExprKind::Break(dest, None) if dest.label.is_none())
     }
 }
 
@@ -72,6 +69,8 @@ fn could_be_while_let<'tcx>(
     let_pat: &'tcx Pat<'_>,
     let_expr: &'tcx Expr<'_>,
     has_trailing_exprs: bool,
+    let_info: Option<(&Pat<'_>, Option<&Ty<'_>>)>,
+    inner_expr: &Expr<'_>,
 ) {
     if has_trailing_exprs
         && (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr))
@@ -86,7 +85,24 @@ fn could_be_while_let<'tcx>(
     // 1) it was ugly with big bodies;
     // 2) it was not indented properly;
     // 3) it wasn’t very smart (see #675).
-    let mut applicability = Applicability::HasPlaceholders;
+    let inner_content = if let Some((pat, ty)) = let_info
+        // Prevent trivial reassignments such as `let x = x;` or `let _ = …;`, but
+        // keep them if the type has been explicitly specified.
+        && (!is_trivial_assignment(pat, peel_blocks(inner_expr)) || ty.is_some())
+        && let Some(pat_str) = snippet_opt(cx, pat.span)
+        && let Some(init_str) = snippet_opt(cx, peel_blocks(inner_expr).span)
+    {
+        let ty_str = ty
+            .map(|ty| format!(": {}", snippet(cx, ty.span, "_")))
+            .unwrap_or_default();
+        format!(
+            "\n{indent}    let {pat_str}{ty_str} = {init_str};\n{indent}    ..\n{indent}",
+            indent = snippet_indent(cx, expr.span).unwrap_or_default(),
+        )
+    } else {
+        " .. ".into()
+    };
+
     span_lint_and_sugg(
         cx,
         WHILE_LET_LOOP,
@@ -94,10 +110,21 @@ fn could_be_while_let<'tcx>(
         "this loop could be written as a `while let` loop",
         "try",
         format!(
-            "while let {} = {} {{ .. }}",
-            snippet_with_applicability(cx, let_pat.span, "..", &mut applicability),
-            snippet_with_applicability(cx, let_expr.span, "..", &mut applicability),
+            "while let {} = {} {{{inner_content}}}",
+            snippet(cx, let_pat.span, ".."),
+            snippet(cx, let_expr.span, ".."),
         ),
-        applicability,
+        Applicability::HasPlaceholders,
     );
 }
+
+fn is_trivial_assignment(pat: &Pat<'_>, init: &Expr<'_>) -> bool {
+    match (pat.kind, init.kind) {
+        (PatKind::Wild, _) => true,
+        (
+            PatKind::Binding(BindingMode::NONE, _, pat_ident, None),
+            ExprKind::Path(QPath::Resolved(None, Path { segments: [init], .. })),
+        ) => pat_ident.name == init.ident.name,
+        _ => false,
+    }
+}
diff --git a/tests/ui/while_let_loop.rs b/tests/ui/while_let_loop.rs
index d591ab984cf..95062c9f46c 100644
--- a/tests/ui/while_let_loop.rs
+++ b/tests/ui/while_let_loop.rs
@@ -154,3 +154,89 @@ fn issue_5715(mut m: core::cell::RefCell<Option<u32>>) {
         m = core::cell::RefCell::new(Some(x + 1));
     }
 }
+
+mod issue_362 {
+    pub fn merge_sorted<T>(xs: Vec<T>, ys: Vec<T>) -> Vec<T>
+    where
+        T: PartialOrd,
+    {
+        let total_len = xs.len() + ys.len();
+        let mut res = Vec::with_capacity(total_len);
+        let mut ix = xs.into_iter().peekable();
+        let mut iy = ys.into_iter().peekable();
+        loop {
+            //~^ while_let_loop
+            let lt = match (ix.peek(), iy.peek()) {
+                (Some(x), Some(y)) => x < y,
+                _ => break,
+            };
+            res.push(if lt { &mut ix } else { &mut iy }.next().unwrap());
+        }
+        res.extend(ix);
+        res.extend(iy);
+        res
+    }
+}
+
+fn let_assign() {
+    loop {
+        //~^ while_let_loop
+        let x = if let Some(y) = Some(3) {
+            y
+        } else {
+            break;
+        };
+        if x == 3 {
+            break;
+        }
+    }
+
+    loop {
+        //~^ while_let_loop
+        let x: u32 = if let Some(y) = Some(3) {
+            y
+        } else {
+            break;
+        };
+        if x == 3 {
+            break;
+        }
+    }
+
+    loop {
+        //~^ while_let_loop
+        let x = if let Some(x) = Some(3) {
+            x
+        } else {
+            break;
+        };
+        if x == 3 {
+            break;
+        }
+    }
+
+    loop {
+        //~^ while_let_loop
+        let x: u32 = if let Some(x) = Some(3) {
+            x
+        } else {
+            break;
+        };
+        if x == 3 {
+            break;
+        }
+    }
+
+    loop {
+        //~^ while_let_loop
+        let x = if let Some(x) = Some(2) {
+            let t = 1;
+            t + x
+        } else {
+            break;
+        };
+        if x == 3 {
+            break;
+        }
+    }
+}
diff --git a/tests/ui/while_let_loop.stderr b/tests/ui/while_let_loop.stderr
index bd482857e67..ed42628a53e 100644
--- a/tests/ui/while_let_loop.stderr
+++ b/tests/ui/while_let_loop.stderr
@@ -57,7 +57,125 @@ LL | |         let (e, l) = match "".split_whitespace().next() {
 ...  |
 LL | |         let _ = (e, l);
 LL | |     }
-   | |_____^ help: try: `while let Some(word) = "".split_whitespace().next() { .. }`
+   | |_____^
+   |
+help: try
+   |
+LL ~     while let Some(word) = "".split_whitespace().next() {
+LL +         let (e, l) = (word.is_empty(), word.len());
+LL +         ..
+LL +     }
+   |
+
+error: this loop could be written as a `while let` loop
+  --> tests/ui/while_let_loop.rs:167:9
+   |
+LL | /         loop {
+LL | |
+LL | |             let lt = match (ix.peek(), iy.peek()) {
+LL | |                 (Some(x), Some(y)) => x < y,
+...  |
+LL | |             res.push(if lt { &mut ix } else { &mut iy }.next().unwrap());
+LL | |         }
+   | |_________^
+   |
+help: try
+   |
+LL ~         while let (Some(x), Some(y)) = (ix.peek(), iy.peek()) {
+LL +             let lt = x < y;
+LL +             ..
+LL +         }
+   |
+
+error: this loop could be written as a `while let` loop
+  --> tests/ui/while_let_loop.rs:182:5
+   |
+LL | /     loop {
+LL | |
+LL | |         let x = if let Some(y) = Some(3) {
+LL | |             y
+...  |
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     while let Some(y) = Some(3) {
+LL +         let x = y;
+LL +         ..
+LL +     }
+   |
+
+error: this loop could be written as a `while let` loop
+  --> tests/ui/while_let_loop.rs:194:5
+   |
+LL | /     loop {
+LL | |
+LL | |         let x: u32 = if let Some(y) = Some(3) {
+LL | |             y
+...  |
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     while let Some(y) = Some(3) {
+LL +         let x: u32 = y;
+LL +         ..
+LL +     }
+   |
+
+error: this loop could be written as a `while let` loop
+  --> tests/ui/while_let_loop.rs:206:5
+   |
+LL | /     loop {
+LL | |
+LL | |         let x = if let Some(x) = Some(3) {
+LL | |             x
+...  |
+LL | |     }
+   | |_____^ help: try: `while let Some(x) = Some(3) { .. }`
+
+error: this loop could be written as a `while let` loop
+  --> tests/ui/while_let_loop.rs:218:5
+   |
+LL | /     loop {
+LL | |
+LL | |         let x: u32 = if let Some(x) = Some(3) {
+LL | |             x
+...  |
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     while let Some(x) = Some(3) {
+LL +         let x: u32 = x;
+LL +         ..
+LL +     }
+   |
+
+error: this loop could be written as a `while let` loop
+  --> tests/ui/while_let_loop.rs:230:5
+   |
+LL | /     loop {
+LL | |
+LL | |         let x = if let Some(x) = Some(2) {
+LL | |             let t = 1;
+...  |
+LL | |     }
+   | |_____^
+   |
+help: try
+   |
+LL ~     while let Some(x) = Some(2) {
+LL +         let x = {
+LL +             let t = 1;
+LL +             t + x
+LL +         };
+LL +         ..
+LL +     }
+   |
 
-error: aborting due to 5 previous errors
+error: aborting due to 11 previous errors