Merge pull request #21209 from ChayimFriedman2/stale-expr

internal: Do not create stale expressions in body lowering

crates/hir-def/src/expr_store/lower.rs

@@ -434,7 +434,7 @@ pub struct ExprCollector<'db> {
     current_try_block_label: Option<LabelId>,
 
     label_ribs: Vec<LabelRib>,
-    current_binding_owner: Option<ExprId>,
+    unowned_bindings: Vec<BindingId>,
 
     awaitable_context: Option<Awaitable>,
     krate: base_db::Crate,
@@ -538,7 +538,7 @@ impl<'db> ExprCollector<'db> {
             current_try_block_label: None,
             is_lowering_coroutine: false,
             label_ribs: Vec::new(),
-            current_binding_owner: None,
+            unowned_bindings: Vec::new(),
             awaitable_context: None,
             current_block_legacy_macro_defs_count: FxHashMap::default(),
             outer_impl_trait: false,
@@ -1065,12 +1065,10 @@ impl<'db> ExprCollector<'db> {
                 Some(ast::BlockModifier::Const(_)) => {
                     self.with_label_rib(RibKind::Constant, |this| {
                         this.with_awaitable_block(Awaitable::No("constant block"), |this| {
-                            let (result_expr_id, prev_binding_owner) =
+                            this.with_binding_owner(|this| {
-                                this.initialize_binding_owner(syntax_ptr);
                                 let inner_expr = this.collect_block(e);
-                            this.store.exprs[result_expr_id] = Expr::Const(inner_expr);
+                                this.alloc_expr(Expr::Const(inner_expr), syntax_ptr)
-                            this.current_binding_owner = prev_binding_owner;
+                            })
-                            result_expr_id
                         })
                     })
                 }
@@ -1281,8 +1279,7 @@ impl<'db> ExprCollector<'db> {
                 }
             }
             ast::Expr::ClosureExpr(e) => self.with_label_rib(RibKind::Closure, |this| {
-                let (result_expr_id, prev_binding_owner) =
+                this.with_binding_owner(|this| {
-                    this.initialize_binding_owner(syntax_ptr);
                     let mut args = Vec::new();
                     let mut arg_types = Vec::new();
                     if let Some(pl) = e.param_list() {
@@ -1310,8 +1307,8 @@ impl<'db> ExprCollector<'db> {
                     } else {
                         Awaitable::No("non-async closure")
                     };
-                let body =
+                    let body = this
-                    this.with_awaitable_block(awaitable, |this| this.collect_expr_opt(e.body()));
+                        .with_awaitable_block(awaitable, |this| this.collect_expr_opt(e.body()));
 
                     let closure_kind = if this.is_lowering_coroutine {
                         let movability = if e.static_token().is_some() {
@@ -1328,17 +1325,19 @@ impl<'db> ExprCollector<'db> {
                     let capture_by =
                         if e.move_token().is_some() { CaptureBy::Value } else { CaptureBy::Ref };
                     this.is_lowering_coroutine = prev_is_lowering_coroutine;
-                this.current_binding_owner = prev_binding_owner;
                     this.current_try_block_label = prev_try_block_label;
-                this.store.exprs[result_expr_id] = Expr::Closure {
+                    this.alloc_expr(
+                        Expr::Closure {
                             args: args.into(),
                             arg_types: arg_types.into(),
                             ret_type,
                             body,
                             closure_kind,
                             capture_by,
-                };
+                        },
-                result_expr_id
+                        syntax_ptr,
+                    )
+                })
             }),
             ast::Expr::BinExpr(e) => {
                 let op = e.op_kind();
@@ -1374,11 +1373,7 @@ impl<'db> ExprCollector<'db> {
                         let initializer = self.collect_expr_opt(initializer);
                         let repeat = self.with_label_rib(RibKind::Constant, |this| {
                             if let Some(repeat) = repeat {
-                                let syntax_ptr = AstPtr::new(&repeat);
+                                this.with_binding_owner(|this| this.collect_expr(repeat))
-                                this.collect_as_a_binding_owner_bad(
-                                    |this| this.collect_expr(repeat),
-                                    syntax_ptr,
-                                )
                             } else {
                                 this.missing_expr()
                             }
@@ -1635,31 +1630,13 @@ impl<'db> ExprCollector<'db> {
         }
     }
 
-    fn initialize_binding_owner(
+    fn with_binding_owner(&mut self, create_expr: impl FnOnce(&mut Self) -> ExprId) -> ExprId {
-        &mut self,
+        let prev_unowned_bindings_len = self.unowned_bindings.len();
-        syntax_ptr: AstPtr<ast::Expr>,
+        let expr_id = create_expr(self);
-    ) -> (ExprId, Option<ExprId>) {
+        for binding in self.unowned_bindings.drain(prev_unowned_bindings_len..) {
-        let result_expr_id = self.alloc_expr(Expr::Missing, syntax_ptr);
+            self.store.binding_owners.insert(binding, expr_id);
-        let prev_binding_owner = self.current_binding_owner.take();
-        self.current_binding_owner = Some(result_expr_id);
-
-        (result_expr_id, prev_binding_owner)
         }
-
+        expr_id
-    /// FIXME: This function is bad. It will produce a dangling `Missing` expr which wastes memory. Currently
-    /// it is used only for const blocks and repeat expressions, which are also hacky and ideally should have
-    /// their own body. Don't add more usage for this function so that we can remove this function after
-    /// separating those bodies.
-    fn collect_as_a_binding_owner_bad(
-        &mut self,
-        job: impl FnOnce(&mut ExprCollector<'_>) -> ExprId,
-        syntax_ptr: AstPtr<ast::Expr>,
-    ) -> ExprId {
-        let (id, prev_owner) = self.initialize_binding_owner(syntax_ptr);
-        let tmp = job(self);
-        self.store.exprs[id] = mem::replace(&mut self.store.exprs[tmp], Expr::Missing);
-        self.current_binding_owner = prev_owner;
-        id
     }
 
     /// Desugar `try { <stmts>; <expr> }` into `'<new_label>: { <stmts>; ::std::ops::Try::from_output(<expr>) }`,
@@ -2371,11 +2348,7 @@ impl<'db> ExprCollector<'db> {
             ast::Pat::ConstBlockPat(const_block_pat) => {
                 if let Some(block) = const_block_pat.block_expr() {
                     let expr_id = self.with_label_rib(RibKind::Constant, |this| {
-                        let syntax_ptr = AstPtr::new(&block.clone().into());
+                        this.with_binding_owner(|this| this.collect_block(block))
-                        this.collect_as_a_binding_owner_bad(
-                            |this| this.collect_block(block),
-                            syntax_ptr,
-                        )
                     });
                     Pat::ConstBlock(expr_id)
                 } else {
@@ -3379,9 +3352,7 @@ impl ExprCollector<'_> {
         hygiene: HygieneId,
     ) -> BindingId {
         let binding = self.store.bindings.alloc(Binding { name, mode, problems: None, hygiene });
-        if let Some(owner) = self.current_binding_owner {
+        self.unowned_bindings.push(binding);
-            self.store.binding_owners.insert(binding, owner);
-        }
         binding
     }
 

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

@@ -995,6 +995,10 @@ fn fn_once(mut x: impl FnOnce(u8) -> u8) -> u8 {
         }
                     "#,
         );
+        // FIXME: There should be no "unused variable" here, and there should be a mutability error,
+        // but our MIR infra is horribly broken and due to the order in which expressions are lowered
+        // there is no `StorageLive` for `x` in the closure (in fact, `x` should not even be a variable
+        // of the closure, the environment should be, but as I said, our MIR infra is horribly broken).
         check_diagnostics(
             r#"
 //- minicore: copy, fn
@@ -1003,8 +1007,8 @@ fn f() {
         || {
             || {
                 let x = 2;
+                 // ^ 💡 warn: unused variable
                 || { || { x = 5; } }
-                        //^^^^^ 💡 error: cannot mutate immutable variable `x`
             }
         }
     };