Do not deduce parameter attributes during CTFE

Deducing parameter attributes can depend upon typeck, so can lead to
cycles if performed during constant evaluation.

This patch breaks a subquery out from `fn_abi_of_interface`,
`fn_abi_of_interface_unadjusted`, which returns the ABI before deduced
parameter attributes are applied; and that new subquery is used in CTFE
instead.

Author: eggyal

compiler/rustc_const_eval/src/interpret/call.rs

@@ -351,7 +351,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
 
         // Compute callee information.
         // FIXME: for variadic support, do we have to somehow determine callee's extra_args?
-        let callee_fn_abi = self.fn_abi_of_instance(instance, ty::List::empty())?;
+        let callee_fn_abi = self.fn_abi_of_instance_unadjusted(instance, ty::List::empty())?;
 
         if callee_fn_abi.c_variadic || caller_fn_abi.c_variadic {
             throw_unsup_format!("calling a c-variadic function is not supported");
@@ -839,7 +839,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 enter_trace_span!(M, resolve::resolve_drop_in_place, ty = ?place.layout.ty);
             ty::Instance::resolve_drop_in_place(*self.tcx, place.layout.ty)
         };
-        let fn_abi = self.fn_abi_of_instance(instance, ty::List::empty())?;
+        let fn_abi = self.fn_abi_of_instance_unadjusted(instance, ty::List::empty())?;
 
         let arg = self.mplace_to_ref(&place)?;
         let ret = MPlaceTy::fake_alloc_zst(self.layout_of(self.tcx.types.unit)?);

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -153,16 +153,16 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
     }
 
     /// This inherent method takes priority over the trait method with the same name in FnAbiOf,
-    /// and allows wrapping the actual [FnAbiOf::fn_abi_of_instance] with a tracing span.
-    /// See [FnAbiOf::fn_abi_of_instance] for the original documentation.
+    /// and allows wrapping the actual [FnAbiOf::fn_abi_of_instance_unadjusted] with a tracing span.
+    /// See [FnAbiOf::fn_abi_of_instance_unadjusted] for the original documentation.
     #[inline(always)]
-    pub fn fn_abi_of_instance(
+    pub fn fn_abi_of_instance_unadjusted(
         &self,
         instance: ty::Instance<'tcx>,
         extra_args: &'tcx ty::List<Ty<'tcx>>,
     ) -> <Self as FnAbiOfHelpers<'tcx>>::FnAbiOfResult {
         let _trace = enter_trace_span!(M, layouting::fn_abi_of_instance, ?instance, ?extra_args);
-        FnAbiOf::fn_abi_of_instance(self, instance, extra_args)
+        FnAbiOf::fn_abi_of_instance_unadjusted(self, instance, extra_args)
     }
 }
 

compiler/rustc_const_eval/src/interpret/step.rs

@@ -476,7 +476,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 let instance = self.resolve(def_id, args)?;
                 (
                     FnVal::Instance(instance),
-                    self.fn_abi_of_instance(instance, extra_args)?,
+                    self.fn_abi_of_instance_unadjusted(instance, extra_args)?,
                     instance.def.requires_caller_location(*self.tcx),
                 )
             }

compiler/rustc_middle/src/query/mod.rs

@@ -1811,6 +1811,20 @@ rustc_queries! {
         desc { "computing call ABI of `{}` function pointers", key.value.0 }
     }
 
+    /// Compute a `FnAbi` suitable for declaring/defining an `fn` instance, and for
+    /// direct calls to an `fn`. This ABI does not have deduced parameter attributes set, and
+    /// therefore may produce less optimal codegen than `fn_abi_of_instance` (which should be
+    /// preferred instead). However, CTFE may wish to avoid such parameter attribute deduction,
+    /// as it can otherwise result in loops during typeck.
+    ///
+    /// NB: that includes virtual calls, which are represented by "direct calls"
+    /// to an `InstanceKind::Virtual` instance (of `<dyn Trait as Trait>::fn`).
+    query fn_abi_of_instance_unadjusted(
+        key: ty::PseudoCanonicalInput<'tcx, (ty::Instance<'tcx>, &'tcx ty::List<Ty<'tcx>>)>
+    ) -> Result<&'tcx rustc_target::callconv::FnAbi<'tcx, Ty<'tcx>>, &'tcx ty::layout::FnAbiError<'tcx>> {
+        desc { "computing unadjusted call ABI of `{}`", key.value.0 }
+    }
+
     /// Compute a `FnAbi` suitable for declaring/defining an `fn` instance, and for
     /// direct calls to an `fn`.
     ///

compiler/rustc_middle/src/ty/layout.rs

@@ -1360,6 +1360,44 @@ pub trait FnAbiOf<'tcx>: FnAbiOfHelpers<'tcx> {
         )
     }
 
+    /// Compute a `FnAbi` suitable for declaring/defining an `fn` instance, and for
+    /// direct calls to an `fn`. This ABI does not have deduced parameter attributes set, and
+    /// therefore may produce less optimal codegen than `fn_abi_of_instance` (which should be
+    /// preferred instead). However, CTFE may wish to avoid such parameter attribute deduction,
+    /// as it can otherwise result in loops during typeck.
+    ///
+    /// NB: that includes virtual calls, which are represented by "direct calls"
+    /// to an `InstanceKind::Virtual` instance (of `<dyn Trait as Trait>::fn`).
+    #[inline]
+    #[tracing::instrument(level = "debug", skip(self))]
+    fn fn_abi_of_instance_unadjusted(
+        &self,
+        instance: ty::Instance<'tcx>,
+        extra_args: &'tcx ty::List<Ty<'tcx>>,
+    ) -> Self::FnAbiOfResult {
+        // FIXME(eddyb) get a better `span` here.
+        let span = self.layout_tcx_at_span();
+        let tcx = self.tcx().at(span);
+
+        MaybeResult::from(
+            tcx.fn_abi_of_instance_unadjusted(
+                self.typing_env().as_query_input((instance, extra_args)),
+            )
+            .map_err(|err| {
+                // HACK(eddyb) at least for definitions of/calls to `Instance`s,
+                // we can get some kind of span even if one wasn't provided.
+                // However, we don't do this early in order to avoid calling
+                // `def_span` unconditionally (which may have a perf penalty).
+                let span = if !span.is_dummy() { span } else { tcx.def_span(instance.def_id()) };
+                self.handle_fn_abi_err(
+                    *err,
+                    span,
+                    FnAbiRequest::OfInstance { instance, extra_args },
+                )
+            }),
+        )
+    }
+
     /// Compute a `FnAbi` suitable for declaring/defining an `fn` instance, and for
     /// direct calls to an `fn`.
     ///

compiler/rustc_ty_utils/src/abi.rs

@@ -21,7 +21,12 @@ use rustc_target::callconv::{
 use tracing::debug;
 
 pub(crate) fn provide(providers: &mut Providers) {
-    *providers = Providers { fn_abi_of_fn_ptr, fn_abi_of_instance, ..*providers };
+    *providers = Providers {
+        fn_abi_of_fn_ptr,
+        fn_abi_of_instance_unadjusted,
+        fn_abi_of_instance,
+        ..*providers
+    };
 }
 
 // NOTE(eddyb) this is private to avoid using it from outside of
@@ -250,25 +255,71 @@ fn fn_abi_of_fn_ptr<'tcx>(
     let ty::PseudoCanonicalInput { typing_env, value: (sig, extra_args) } = query;
     fn_abi_new_uncached(
         &LayoutCx::new(tcx, typing_env),
-        tcx.instantiate_bound_regions_with_erased(sig),
-        extra_args,
+        tcx.normalize_erasing_regions(typing_env, tcx.instantiate_bound_regions_with_erased(sig)),
+        None,
         None,
+        false,
+        extra_args,
     )
 }
 
-fn fn_abi_of_instance<'tcx>(
+fn instance_params<'tcx>(
     tcx: TyCtxt<'tcx>,
     query: ty::PseudoCanonicalInput<'tcx, (ty::Instance<'tcx>, &'tcx ty::List<Ty<'tcx>>)>,
-) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
+) -> (LayoutCx<'tcx>, ty::FnSig<'tcx>, Option<DefId>, Option<Ty<'tcx>>, bool, &'tcx [Ty<'tcx>]) {
     let ty::PseudoCanonicalInput { typing_env, value: (instance, extra_args) } = query;
+    let cx = LayoutCx::new(tcx, typing_env);
+    let sig =
+        tcx.normalize_erasing_regions(typing_env, fn_sig_for_fn_abi(tcx, instance, typing_env));
+    let is_virtual_call = matches!(instance.def, ty::InstanceKind::Virtual(..));
+    let is_tls_shim_call = matches!(instance.def, ty::InstanceKind::ThreadLocalShim(_));
+    (
+        cx,
+        sig,
+        (!is_virtual_call && !is_tls_shim_call).then(|| instance.def_id()),
+        instance.def.requires_caller_location(tcx).then(|| tcx.caller_location_ty()),
+        is_virtual_call,
+        extra_args,
+    )
+}
+
+fn fn_abi_of_instance_unadjusted<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    query: ty::PseudoCanonicalInput<'tcx, (ty::Instance<'tcx>, &'tcx ty::List<Ty<'tcx>>)>,
+) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
+    let (cx, sig, determined_fn_def_id, caller_location, is_virtual_call, extra_args) =
+        instance_params(tcx, query);
     fn_abi_new_uncached(
-        &LayoutCx::new(tcx, typing_env),
-        fn_sig_for_fn_abi(tcx, instance, typing_env),
+        &cx,
+        sig,
+        caller_location,
+        determined_fn_def_id,
+        is_virtual_call,
         extra_args,
-        Some(instance),
     )
 }
 
+fn fn_abi_of_instance<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    query: ty::PseudoCanonicalInput<'tcx, (ty::Instance<'tcx>, &'tcx ty::List<Ty<'tcx>>)>,
+) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
+    tcx.fn_abi_of_instance_unadjusted(query).map(|fn_abi| {
+        let (cx, sig, determined_fn_def_id, ..) = instance_params(tcx, query);
+        determined_fn_def_id.map_or(fn_abi, |determined_fn_def_id| {
+            fn_abi_adjust_for_deduced_attrs(
+                &cx,
+                fn_abi,
+                sig.abi,
+                // If this is a virtual call, we cannot pass the `fn_def_id`, as it might call other
+                // functions from vtable. And for a tls shim, passing the `fn_def_id` would refer to
+                // the underlying static. Internally, `deduced_param_attrs` attempts to infer attributes
+                // by visit the function body.
+                determined_fn_def_id,
+            )
+        })
+    })
+}
+
 // Handle safe Rust thin and wide pointers.
 fn arg_attrs_for_rust_scalar<'tcx>(
     cx: LayoutCx<'tcx>,
@@ -479,27 +530,19 @@ fn fn_abi_sanity_check<'tcx>(
     fn_arg_sanity_check(cx, fn_abi, spec_abi, &fn_abi.ret);
 }
 
-#[tracing::instrument(level = "debug", skip(cx, instance))]
+#[tracing::instrument(
+    level = "debug",
+    skip(cx, caller_location, determined_fn_def_id, is_virtual_call)
+)]
 fn fn_abi_new_uncached<'tcx>(
     cx: &LayoutCx<'tcx>,
     sig: ty::FnSig<'tcx>,
+    caller_location: Option<Ty<'tcx>>,
+    determined_fn_def_id: Option<DefId>,
+    is_virtual_call: bool,
     extra_args: &[Ty<'tcx>],
-    instance: Option<ty::Instance<'tcx>>,
 ) -> Result<&'tcx FnAbi<'tcx, Ty<'tcx>>, &'tcx FnAbiError<'tcx>> {
     let tcx = cx.tcx();
-    let (caller_location, determined_fn_def_id, is_virtual_call) = if let Some(instance) = instance
-    {
-        let is_virtual_call = matches!(instance.def, ty::InstanceKind::Virtual(..));
-        let is_tls_shim_call = matches!(instance.def, ty::InstanceKind::ThreadLocalShim(_));
-        (
-            instance.def.requires_caller_location(tcx).then(|| tcx.caller_location_ty()),
-            if is_virtual_call || is_tls_shim_call { None } else { Some(instance.def_id()) },
-            is_virtual_call,
-        )
-    } else {
-        (None, None, false)
-    };
-    let sig = tcx.normalize_erasing_regions(cx.typing_env, sig);
 
     let abi_map = AbiMap::from_target(&tcx.sess.target);
     let conv = abi_map.canonize_abi(sig.abi, sig.c_variadic).unwrap();
@@ -575,16 +618,7 @@ fn fn_abi_new_uncached<'tcx>(
             sig.abi,
         ),
     };
-    fn_abi_adjust_for_abi(
-        cx,
-        &mut fn_abi,
-        sig.abi,
-        // If this is a virtual call, we cannot pass the `fn_def_id`, as it might call other
-        // functions from vtable. And for a tls shim, passing the `fn_def_id` would refer to
-        // the underlying static. Internally, `deduced_param_attrs` attempts to infer attributes
-        // by visit the function body.
-        determined_fn_def_id,
-    );
+    fn_abi_adjust_for_abi(cx, &mut fn_abi, sig.abi);
     debug!("fn_abi_new_uncached = {:?}", fn_abi);
     fn_abi_sanity_check(cx, &fn_abi, sig.abi);
     Ok(tcx.arena.alloc(fn_abi))
@@ -595,7 +629,6 @@ fn fn_abi_adjust_for_abi<'tcx>(
     cx: &LayoutCx<'tcx>,
     fn_abi: &mut FnAbi<'tcx, Ty<'tcx>>,
     abi: ExternAbi,
-    fn_def_id: Option<DefId>,
 ) {
     if abi == ExternAbi::Unadjusted {
         // The "unadjusted" ABI passes aggregates in "direct" mode. That's fragile but needed for
@@ -616,30 +649,43 @@ fn fn_abi_adjust_for_abi<'tcx>(
         for arg in fn_abi.args.iter_mut() {
             unadjust(arg);
         }
-        return;
+    } else if abi.is_rustic_abi() {
+        fn_abi.adjust_for_rust_abi(cx);
+    } else {
+        fn_abi.adjust_for_foreign_abi(cx, abi);
     }
+}
 
+#[tracing::instrument(level = "trace", skip(cx))]
+fn fn_abi_adjust_for_deduced_attrs<'tcx>(
+    cx: &LayoutCx<'tcx>,
+    fn_abi: &'tcx FnAbi<'tcx, Ty<'tcx>>,
+    abi: ExternAbi,
+    fn_def_id: DefId,
+) -> &'tcx FnAbi<'tcx, Ty<'tcx>> {
     let tcx = cx.tcx();
-
-    if abi.is_rustic_abi() {
-        fn_abi.adjust_for_rust_abi(cx);
-        // Look up the deduced parameter attributes for this function, if we have its def ID and
-        // we're optimizing in non-incremental mode. We'll tag its parameters with those attributes
-        // as appropriate.
-        let deduced =
-            if tcx.sess.opts.optimize != OptLevel::No && tcx.sess.opts.incremental.is_none() {
-                fn_def_id.map(|fn_def_id| tcx.deduced_param_attrs(fn_def_id)).unwrap_or_default()
-            } else {
-                &[]
-            };
-        if !deduced.is_empty() {
-            apply_deduced_attributes(cx, deduced, 0, &mut fn_abi.ret);
-            for (arg_idx, arg) in fn_abi.args.iter_mut().enumerate() {
-                apply_deduced_attributes(cx, deduced, arg_idx + 1, arg);
-            }
-        }
+    // Look up the deduced parameter attributes for this function, if we have its def ID and
+    // we're optimizing in non-incremental mode. We'll tag its parameters with those attributes
+    // as appropriate.
+    let deduced = if abi.is_rustic_abi()
+        && tcx.sess.opts.optimize != OptLevel::No
+        && tcx.sess.opts.incremental.is_none()
+    {
+        tcx.deduced_param_attrs(fn_def_id)
     } else {
-        fn_abi.adjust_for_foreign_abi(cx, abi);
+        &[]
+    };
+    if deduced.is_empty() {
+        fn_abi
+    } else {
+        let mut fn_abi = fn_abi.clone();
+        apply_deduced_attributes(cx, deduced, 0, &mut fn_abi.ret);
+        for (arg_idx, arg) in fn_abi.args.iter_mut().enumerate() {
+            apply_deduced_attributes(cx, deduced, arg_idx + 1, arg);
+        }
+        debug!("fn_abi_adjust_for_deduced_attrs = {:?}", fn_abi);
+        fn_abi_sanity_check(cx, &fn_abi, abi);
+        tcx.arena.alloc(fn_abi)
     }
 }
 

tests/ui/coroutine/avoid-query-cycle-in-ctfe.rs

@@ -0,0 +1,18 @@
+//! Items whose type depends on CTFE (such as the async closure/coroutine beneath, whose type
+//! depends upon evaluating `do_nothing`) should not cause a query cycle owing to the deduction of
+//! the function's parameter attributes, which are only required for codegen and not for CTFE.
+//!
+//! Regression test for https://github.com/rust-lang/rust/issues/151748
+//@ compile-flags: -O
+//@ edition: 2018
+//@ check-pass
+
+fn main() {
+    let _ = async || {
+        let COMPLEX_CONSTANT = ();
+    };
+}
+
+const fn do_nothing() {}
+
+const COMPLEX_CONSTANT: () = do_nothing();