Merge pull request #22101 from ChayimFriedman2/port-infer-call

fix: Port call expr type checking and closure upvar inference from rustc

Author: ChayimFriedman2

Cargo.lock

@@ -1,3 +1,4 @@
+//- minicore: fn
 use crate::ast::{AngleBracketedArgs, ParenthesizedArgs, AttrStyle, BareFnTy};
 use crate::ast::{GenericBound, TraitBoundModifier};
 use crate::ast::Unsafety;

bench_data/glorious_old_parser

@@ -675,6 +675,9 @@ impl ExpressionStore {
                 f(*expr);
                 arms.iter().for_each(|arm| {
                     f(arm.expr);
+                    if let Some(guard) = arm.guard {
+                        f(guard);
+                    }
                     self.walk_exprs_in_pat(arm.pat, &mut f);
                 });
             }
@@ -926,6 +929,13 @@ impl ExpressionStore {
         // We keep the async closure exactly one expr before.
         ExprId::from_raw(la_arena::RawIdx::from_u32(coroutine_closure.into_raw().into_u32() - 1))
     }
+
+    /// The opposite of [`Self::coroutine_for_closure()`].
+    #[inline]
+    pub fn closure_for_coroutine(coroutine: ExprId) -> ExprId {
+        // We keep the async closure exactly one expr before.
+        ExprId::from_raw(la_arena::RawIdx::from_u32(coroutine.into_raw().into_u32() + 1))
+    }
 }
 
 impl Index<ExprId> for ExpressionStore {

crates/hir-def/src/expr_store.rs

@@ -133,7 +133,7 @@ impl Body {
         expr: ExprId,
         edition: Edition,
     ) -> String {
-        pretty::print_expr_hir(db, self, owner, expr, edition)
+        pretty::print_expr_hir(db, self, owner.into(), expr, edition)
     }
 
     pub fn pretty_print_pat(
@@ -144,7 +144,7 @@ impl Body {
         oneline: bool,
         edition: Edition,
     ) -> String {
-        pretty::print_pat_hir(db, self, owner, pat, oneline, edition)
+        pretty::print_pat_hir(db, self, owner.into(), pat, oneline, edition)
     }
 }
 

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

@@ -945,12 +945,19 @@ impl<'db> ExprCollector<'db> {
         })
     }
 
-    /// An `async fn` needs to capture all parameters in the generated `async` block, even if they have
-    /// non-captured patterns such as wildcards (to ensure consistent drop order).
-    fn lower_async_fn(&mut self, params: &mut Vec<PatId>, body: ExprId) -> ExprId {
+    /// Lowers a desugared coroutine body after moving all of the arguments
+    /// into the body. This is to make sure that the future actually owns the
+    /// arguments that are passed to the function, and to ensure things like
+    /// drop order are stable.
+    fn lower_async_block_with_moved_arguments(
+        &mut self,
+        params: &mut [PatId],
+        body: ExprId,
+        coroutine_source: CoroutineSource,
+    ) -> ExprId {
         let mut statements = Vec::new();
         for param in params {
-            let name = match self.store.pats[*param] {
+            let (name, hygiene) = match self.store.pats[*param] {
                 Pat::Bind { id, .. }
                     if matches!(
                         self.store.bindings[id].mode,
@@ -962,14 +969,16 @@ impl<'db> ExprCollector<'db> {
                 }
                 Pat::Bind { id, .. } => {
                     // If this is a `ref` binding, we can't leave it as is but we can at least reuse the name, for better display.
-                    self.store.bindings[id].name.clone()
+                    (self.store.bindings[id].name.clone(), self.store.bindings[id].hygiene)
                 }
-                _ => self.generate_new_name(),
+                _ => (self.generate_new_name(), HygieneId::ROOT),
             };
-            let binding_id =
-                self.alloc_binding(name.clone(), BindingAnnotation::Mutable, HygieneId::ROOT);
+            let binding_id = self.alloc_binding(name.clone(), BindingAnnotation::Mutable, hygiene);
             let pat_id = self.alloc_pat_desugared(Pat::Bind { id: binding_id, subpat: None });
             let expr = self.alloc_expr_desugared(Expr::Path(name.into()));
+            if !hygiene.is_root() {
+                self.store.ident_hygiene.insert(expr.into(), hygiene);
+            }
             statements.push(Statement::Let {
                 pat: *param,
                 type_ref: None,
@@ -980,12 +989,17 @@ impl<'db> ExprCollector<'db> {
         }
 
         let async_ = self.async_block(
-            CoroutineSource::Fn,
-            CaptureBy::Value,
+            coroutine_source,
+            // The default capture mode here is by-ref. Later on during upvar analysis,
+            // we will force the captured arguments to by-move, but for async closures,
+            // we want to make sure that we avoid unnecessarily moving captures, or else
+            // all async closures would default to `FnOnce` as their calling mode.
+            CaptureBy::Ref,
             None,
             statements.into_boxed_slice(),
             Some(body),
         );
+        // It's important that this comes last, see the lowering of async closures for why.
         self.alloc_expr_desugared(async_)
     }
 
@@ -1010,14 +1024,18 @@ impl<'db> ExprCollector<'db> {
 
     fn collect(
         &mut self,
-        params: &mut Vec<PatId>,
+        params: &mut [PatId],
         expr: Option<ast::Expr>,
         awaitable: Awaitable,
     ) -> ExprId {
         self.awaitable_context.replace(awaitable);
         self.with_label_rib(RibKind::Closure, |this| {
             let body = this.collect_expr_opt(expr);
-            if awaitable == Awaitable::Yes { this.lower_async_fn(params, body) } else { body }
+            if awaitable == Awaitable::Yes {
+                this.lower_async_block_with_moved_arguments(params, body, CoroutineSource::Fn)
+            } else {
+                body
+            }
         })
     }
 
@@ -1407,9 +1425,11 @@ impl<'db> ExprCollector<'db> {
                 }
             }
             ast::Expr::ClosureExpr(e) => self.with_label_rib(RibKind::Closure, |this| {
-                this.with_binding_owner(|this| {
+                this.with_binding_owner_and_return(|this| {
                     let mut args = Vec::new();
                     let mut arg_types = Vec::new();
+                    // For coroutine closures, the body, aka. the coroutine is the bindings owner, and not the closure.
+                    let mut body_is_bindings_owner = false;
                     if let Some(pl) = e.param_list() {
                         let num_params = pl.params().count();
                         args.reserve_exact(num_params);
@@ -1448,18 +1468,12 @@ impl<'db> ExprCollector<'db> {
                     } else if e.async_token().is_some() {
                         // It's important that this expr is allocated immediately before the closure.
                         // We rely on it for `coroutine_for_closure()`.
-                        body = this.alloc_expr_desugared(Expr::Closure {
-                            args: Box::default(),
-                            arg_types: Box::default(),
-                            ret_type: None,
+                        body = this.lower_async_block_with_moved_arguments(
+                            &mut args,
                             body,
-                            closure_kind: ClosureKind::AsyncBlock {
-                                source: CoroutineSource::Closure,
-                            },
-                            // The block may need to capture by move, but we cannot know it now.
-                            // It will be fixed in capture analysis.
-                            capture_by: CaptureBy::Ref,
-                        });
+                            CoroutineSource::Closure,
+                        );
+                        body_is_bindings_owner = true;
 
                         ClosureKind::AsyncClosure
                     } else {
@@ -1469,7 +1483,7 @@ impl<'db> ExprCollector<'db> {
                         if e.move_token().is_some() { CaptureBy::Value } else { CaptureBy::Ref };
                     this.is_lowering_coroutine = prev_is_lowering_coroutine;
                     this.current_try_block = prev_try_block;
-                    this.alloc_expr(
+                    let closure = this.alloc_expr(
                         Expr::Closure {
                             args: args.into(),
                             arg_types: arg_types.into(),
@@ -1479,7 +1493,9 @@ impl<'db> ExprCollector<'db> {
                             capture_by,
                         },
                         syntax_ptr,
-                    )
+                    );
+
+                    (if body_is_bindings_owner { body } else { closure }, closure)
                 })
             }),
             ast::Expr::BinExpr(e) => {
@@ -1781,13 +1797,24 @@ impl<'db> ExprCollector<'db> {
         }
     }
 
-    fn with_binding_owner(&mut self, create_expr: impl FnOnce(&mut Self) -> ExprId) -> ExprId {
+    /// The callback should return two exprs: the first is the bindings owner, the second is the expr to return.
+    fn with_binding_owner_and_return(
+        &mut self,
+        create_expr: impl FnOnce(&mut Self) -> (ExprId, ExprId),
+    ) -> ExprId {
         let prev_unowned_bindings_len = self.unowned_bindings.len();
-        let expr_id = create_expr(self);
+        let (bindings_owner, expr_to_return) = create_expr(self);
         for binding in self.unowned_bindings.drain(prev_unowned_bindings_len..) {
-            self.store.binding_owners.insert(binding, expr_id);
+            self.store.binding_owners.insert(binding, bindings_owner);
         }
-        expr_id
+        expr_to_return
+    }
+
+    fn with_binding_owner(&mut self, create_expr: impl FnOnce(&mut Self) -> ExprId) -> ExprId {
+        self.with_binding_owner_and_return(move |this| {
+            let expr = create_expr(this);
+            (expr, expr)
+        })
     }
 
     /// Desugar `try { <stmts>; <expr> }` into `'<new_label>: { <stmts>; ::std::ops::Try::from_output(<expr>) }`,

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

@@ -401,7 +401,7 @@ fn print_generic_params(db: &dyn DefDatabase, generic_params: &GenericParams, p:
 pub fn print_expr_hir(
     db: &dyn DefDatabase,
     store: &ExpressionStore,
-    _owner: DefWithBodyId,
+    _owner: ExpressionStoreOwnerId,
     expr: ExprId,
     edition: Edition,
 ) -> String {
@@ -420,7 +420,7 @@ pub fn print_expr_hir(
 pub fn print_pat_hir(
     db: &dyn DefDatabase,
     store: &ExpressionStore,
-    _owner: DefWithBodyId,
+    _owner: ExpressionStoreOwnerId,
     pat: PatId,
     oneline: bool,
     edition: Edition,

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

@@ -652,7 +652,7 @@ fn async_fn_weird_param_patterns() {
 async fn main(&self, param1: i32, ref mut param2: i32, _: i32, param4 @ _: i32, 123: i32) {}
 "#,
         expect![[r#"
-            fn main(self, param1, mut param2, mut <ra@gennew>0, param4 @ _, mut <ra@gennew>1) async move {
+            fn main(self, param1, mut param2, mut <ra@gennew>0, param4 @ _, mut <ra@gennew>1) async {
                 let ref mut param2 = param2;
                 let _ = <ra@gennew>0;
                 let 123 = <ra@gennew>1;

crates/hir-def/src/expr_store/tests/body.rs

@@ -306,6 +306,7 @@ language_item_table! { LangItems =>
     /// Trait injected by `#[derive(Eq)]`, (i.e. "Total EQ"; no, I will not apologize).
     StructuralTeq,           sym::structural_teq,      TraitId;
     Copy,                    sym::copy,                TraitId;
+    UseCloned,               sym::use_cloned,          TraitId;
     Clone,                   sym::clone,               TraitId;
     TrivialClone,            sym::trivial_clone,       TraitId;
     Sync,                    sym::sync,                TraitId;
@@ -324,6 +325,7 @@ language_item_table! { LangItems =>
 
     Drop,                    sym::drop,                TraitId;
     Destruct,                sym::destruct,            TraitId;
+    BikeshedGuaranteedNoDrop,sym::bikeshed_guaranteed_no_drop, TraitId;
 
     CoerceUnsized,           sym::coerce_unsized,      TraitId;
     DispatchFromDyn,         sym::dispatch_from_dyn,   TraitId;
@@ -373,6 +375,8 @@ language_item_table! { LangItems =>
     AsyncFn,                 sym::async_fn,            TraitId;
     AsyncFnMut,              sym::async_fn_mut,        TraitId;
     AsyncFnOnce,             sym::async_fn_once,       TraitId;
+    AsyncFnKindHelper,       sym::async_fn_kind_helper,TraitId;
+    AsyncFnKindUpvars,       sym::async_fn_kind_upvars,TypeAliasId;
 
     CallRefFuture,           sym::call_ref_future,     TypeAliasId;
     CallOnceFuture,          sym::call_once_future,    TypeAliasId;
@@ -489,6 +493,8 @@ language_item_table! { LangItems =>
     IntoIterIntoIter,        sym::into_iter,           FunctionId;
     IteratorNext,            sym::next,                FunctionId;
     Iterator,                sym::iterator,            TraitId;
+    FusedIterator,           sym::fused_iterator,      TraitId;
+    AsyncIterator,           sym::async_iterator,      TraitId;
 
     PinNewUnchecked,         sym::new_unchecked,       FunctionId;
 
@@ -509,6 +515,10 @@ language_item_table! { LangItems =>
     CStr,                    sym::CStr,                StructId;
     Ordering,                sym::Ordering,            EnumId;
 
+    Field,                   sym::field,               TraitId;
+    FieldBase,               sym::field_base,          TypeAliasId;
+    FieldType,               sym::field_type,          TypeAliasId;
+
     @non_lang_core_traits:
     core::default, Default;
     core::fmt, Debug;

crates/hir-def/src/lang_item.rs

@@ -1792,14 +1792,14 @@ const GOAL: i32 = {
 fn closure_capture_unsized_type() {
     check_number(
         r#"
-    //- minicore: fn, copy, slice, index, coerce_unsized
+    //- minicore: fn, copy, slice, index, coerce_unsized, sized
     fn f<T: A>(x: &<T as A>::Ty) -> &<T as A>::Ty {
         let c = || &*x;
         c()
     }
 
     trait A {
-        type Ty;
+        type Ty: ?Sized;
     }
 
     impl A for i32 {
@@ -1810,7 +1810,7 @@ fn closure_capture_unsized_type() {
         let k: &[u8] = &[1, 2, 3];
         let k = f::<i32>(k);
         k[0] + k[1] + k[2]
-    }
+    };
     "#,
         6,
     );

crates/hir-ty/src/consteval/tests.rs

@@ -238,8 +238,7 @@ impl<'db> ExprValidator<'db> {
             if (pat_ty == scrut_ty
                 || scrut_ty
                     .as_reference()
-                    .map(|(match_expr_ty, ..)| match_expr_ty == pat_ty)
-                    .unwrap_or(false))
+                    .is_none_or(|(match_expr_ty, ..)| match_expr_ty == pat_ty))
                 && types_of_subpatterns_do_match(arm.pat, self.body, self.infer)
             {
                 // If we had a NotUsefulMatchArm diagnostic, we could