Auto merge of #14347 - HKalbasi:dev, r=lnicola

Add storage dead for let bindings without initializer

fix #14344

Author: bors

crates/hir-def/src/body.rs

@@ -24,7 +24,9 @@ use syntax::{ast, AstPtr, SyntaxNode, SyntaxNodePtr};
 use crate::{
     attr::Attrs,
     db::DefDatabase,
-    expr::{dummy_expr_id, Binding, BindingId, Expr, ExprId, Label, LabelId, Pat, PatId},
+    expr::{
+        dummy_expr_id, Binding, BindingId, Expr, ExprId, Label, LabelId, Pat, PatId, RecordFieldPat,
+    },
     item_scope::BuiltinShadowMode,
     macro_id_to_def_id,
     nameres::DefMap,
@@ -432,6 +434,44 @@ impl Body {
         pats.shrink_to_fit();
         bindings.shrink_to_fit();
     }
+
+    pub fn walk_bindings_in_pat(&self, pat_id: PatId, mut f: impl FnMut(BindingId)) {
+        self.walk_pats(pat_id, &mut |pat| {
+            if let Pat::Bind { id, .. } = pat {
+                f(*id);
+            }
+        });
+    }
+
+    pub fn walk_pats(&self, pat_id: PatId, f: &mut impl FnMut(&Pat)) {
+        let pat = &self[pat_id];
+        f(pat);
+        match pat {
+            Pat::Range { .. }
+            | Pat::Lit(..)
+            | Pat::Path(..)
+            | Pat::ConstBlock(..)
+            | Pat::Wild
+            | Pat::Missing => {}
+            &Pat::Bind { subpat, .. } => {
+                if let Some(subpat) = subpat {
+                    self.walk_pats(subpat, f);
+                }
+            }
+            Pat::Or(args) | Pat::Tuple { args, .. } | Pat::TupleStruct { args, .. } => {
+                args.iter().copied().for_each(|p| self.walk_pats(p, f));
+            }
+            Pat::Ref { pat, .. } => self.walk_pats(*pat, f),
+            Pat::Slice { prefix, slice, suffix } => {
+                let total_iter = prefix.iter().chain(slice.iter()).chain(suffix.iter());
+                total_iter.copied().for_each(|p| self.walk_pats(p, f));
+            }
+            Pat::Record { args, .. } => {
+                args.iter().for_each(|RecordFieldPat { pat, .. }| self.walk_pats(*pat, f));
+            }
+            Pat::Box { inner } => self.walk_pats(*inner, f),
+        }
+    }
 }
 
 impl Default for Body {

crates/hir-ty/src/infer/pat.rs

@@ -5,10 +5,7 @@ use std::iter::repeat_with;
 use chalk_ir::Mutability;
 use hir_def::{
     body::Body,
-    expr::{
-        Binding, BindingAnnotation, BindingId, Expr, ExprId, ExprOrPatId, Literal, Pat, PatId,
-        RecordFieldPat,
-    },
+    expr::{Binding, BindingAnnotation, BindingId, Expr, ExprId, ExprOrPatId, Literal, Pat, PatId},
     path::Path,
 };
 use hir_expand::name::Name;
@@ -439,38 +436,8 @@ fn is_non_ref_pat(body: &hir_def::body::Body, pat: PatId) -> bool {
 
 pub(super) fn contains_explicit_ref_binding(body: &Body, pat_id: PatId) -> bool {
     let mut res = false;
-    walk_pats(body, pat_id, &mut |pat| {
+    body.walk_pats(pat_id, &mut |pat| {
         res |= matches!(pat, Pat::Bind { id, .. } if body.bindings[*id].mode == BindingAnnotation::Ref);
     });
     res
 }
-
-fn walk_pats(body: &Body, pat_id: PatId, f: &mut impl FnMut(&Pat)) {
-    let pat = &body[pat_id];
-    f(pat);
-    match pat {
-        Pat::Range { .. }
-        | Pat::Lit(..)
-        | Pat::Path(..)
-        | Pat::ConstBlock(..)
-        | Pat::Wild
-        | Pat::Missing => {}
-        &Pat::Bind { subpat, .. } => {
-            if let Some(subpat) = subpat {
-                walk_pats(body, subpat, f);
-            }
-        }
-        Pat::Or(args) | Pat::Tuple { args, .. } | Pat::TupleStruct { args, .. } => {
-            args.iter().copied().for_each(|p| walk_pats(body, p, f));
-        }
-        Pat::Ref { pat, .. } => walk_pats(body, *pat, f),
-        Pat::Slice { prefix, slice, suffix } => {
-            let total_iter = prefix.iter().chain(slice.iter()).chain(suffix.iter());
-            total_iter.copied().for_each(|p| walk_pats(body, p, f));
-        }
-        Pat::Record { args, .. } => {
-            args.iter().for_each(|RecordFieldPat { pat, .. }| walk_pats(body, *pat, f));
-        }
-        Pat::Box { inner } => walk_pats(body, *inner, f),
-    }
-}

crates/hir-ty/src/mir/lower.rs

@@ -1113,7 +1113,7 @@ impl MirLowerCtx<'_> {
                 if matches!(mode, BindingAnnotation::Ref | BindingAnnotation::RefMut) {
                     binding_mode = mode;
                 }
-                self.push_storage_live(*id, current)?;
+                self.push_storage_live(*id, current);
                 self.push_assignment(
                     current,
                     target_place.into(),
@@ -1327,8 +1327,9 @@ impl MirLowerCtx<'_> {
         is_ty_uninhabited_from(&self.infer[expr_id], self.owner.module(self.db.upcast()), self.db)
     }
 
-    /// This function push `StorageLive` statements for each binding in the pattern.
-    fn push_storage_live(&mut self, b: BindingId, current: BasicBlockId) -> Result<()> {
+    /// This function push `StorageLive` statement for the binding, and applies changes to add `StorageDead` in
+    /// the appropriated places.
+    fn push_storage_live(&mut self, b: BindingId, current: BasicBlockId) {
         // Current implementation is wrong. It adds no `StorageDead` at the end of scope, and before each break
         // and continue. It just add a `StorageDead` before the `StorageLive`, which is not wrong, but unneeeded in
         // the proper implementation. Due this limitation, implementing a borrow checker on top of this mir will falsely
@@ -1356,7 +1357,6 @@ impl MirLowerCtx<'_> {
         let l = self.result.binding_locals[b];
         self.push_statement(current, StatementKind::StorageDead(l).with_span(span));
         self.push_statement(current, StatementKind::StorageLive(l).with_span(span));
-        Ok(())
     }
 
     fn resolve_lang_item(&self, item: LangItem) -> Result<LangItemTarget> {
@@ -1381,10 +1381,10 @@ impl MirLowerCtx<'_> {
                     if let Some(expr_id) = initializer {
                         let else_block;
                         let Some((init_place, c)) =
-                        self.lower_expr_as_place(current, *expr_id, true)?
-                    else {
-                        return Ok(None);
-                    };
+                            self.lower_expr_as_place(current, *expr_id, true)?
+                        else {
+                            return Ok(None);
+                        };
                         current = c;
                         (current, else_block) = self.pattern_match(
                             current,
@@ -1407,6 +1407,10 @@ impl MirLowerCtx<'_> {
                                 }
                             }
                         }
+                    } else {
+                        self.body.walk_bindings_in_pat(*pat, |b| {
+                            self.push_storage_live(b, current);
+                        });
                     }
                 }
                 hir_def::expr::Statement::Expr { expr, has_semi: _ } => {

crates/ide-diagnostics/src/handlers/mutability_errors.rs

@@ -505,6 +505,30 @@ fn main() {
         );
     }
 
+    #[test]
+    fn initialization_is_not_mutation_in_loop() {
+        check_diagnostics(
+            r#"
+fn main() {
+    let a;
+    loop {
+        let c @ (
+            mut b,
+          //^^^^^ 💡 weak: variable does not need to be mutable
+            mut d
+          //^^^^^ 💡 weak: variable does not need to be mutable
+        );
+        a = 1;
+      //^^^^^ 💡 error: cannot mutate immutable variable `a`
+        b = 1;
+        c = (2, 3);
+        d = 3;
+    }
+}
+"#,
+        );
+    }
+
     #[test]
     fn function_arguments_are_initialized() {
         check_diagnostics(