Rollup merge of #73359 - jonas-schievink:do-the-shimmy, r=matthewjasper

shim.rs: avoid creating `Call` terminators calling `Self`

Also contains some cleanup and doc comment additions so I could make sense of the code.

Fixes https://github.com/rust-lang/rust/issues/73109
Closes https://github.com/rust-lang/rust/pull/73175

r? @matthewjasper

6 files changed, 123 insertions, 33 deletions

diff --git a/src/librustc_middle/ty/instance.rs b/src/librustc_middle/ty/instance.rs
index 1ce079821a2..d628d6783d5 100644
--- a/src/librustc_middle/ty/instance.rs
+++ b/src/librustc_middle/ty/instance.rs
@@ -9,6 +9,11 @@ use rustc_macros::HashStable;
 
 use std::fmt;
 
+/// A monomorphized `InstanceDef`.
+///
+/// Monomorphization happens on-the-fly and no monomorphized MIR is ever created. Instead, this type
+/// simply couples a potentially generic `InstanceDef` with some substs, and codegen and const eval
+/// will do all required substitution as they run.
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
 #[derive(HashStable, Lift)]
 pub struct Instance<'tcx> {
@@ -18,10 +23,26 @@ pub struct Instance<'tcx> {
 
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable, HashStable)]
 pub enum InstanceDef<'tcx> {
+    /// A user-defined callable item.
+    ///
+    /// This includes:
+    /// - `fn` items
+    /// - closures
+    /// - generators
     Item(DefId),
+
+    /// An intrinsic `fn` item (with `"rust-intrinsic"` or `"platform-intrinsic"` ABI).
+    ///
+    /// Alongside `Virtual`, this is the only `InstanceDef` that does not have its own callable MIR.
+    /// Instead, codegen and const eval "magically" evaluate calls to intrinsics purely in the
+    /// caller.
     Intrinsic(DefId),
 
-    /// `<T as Trait>::method` where `method` receives unsizeable `self: Self`.
+    /// `<T as Trait>::method` where `method` receives unsizeable `self: Self` (part of the
+    /// `unsized_locals` feature).
+    ///
+    /// The generated shim will take `Self` via `*mut Self` - conceptually this is `&owned Self` -
+    /// and dereference the argument to call the original function.
     VtableShim(DefId),
 
     /// `fn()` pointer where the function itself cannot be turned into a pointer.
@@ -37,7 +58,8 @@ pub enum InstanceDef<'tcx> {
     /// (the definition of the function itself).
     ReifyShim(DefId),
 
-    /// `<fn() as FnTrait>::call_*`
+    /// `<fn() as FnTrait>::call_*` (generated `FnTrait` implementation for `fn()` pointers).
+    ///
     /// `DefId` is `FnTrait::call_*`.
     ///
     /// NB: the (`fn` pointer) type must currently be monomorphic to avoid double substitution
@@ -45,19 +67,22 @@ pub enum InstanceDef<'tcx> {
     // FIXME(#69925) support polymorphic MIR shim bodies properly instead.
     FnPtrShim(DefId, Ty<'tcx>),
 
-    /// `<dyn Trait as Trait>::fn`, "direct calls" of which are implicitly
-    /// codegen'd as virtual calls.
+    /// Dynamic dispatch to `<dyn Trait as Trait>::fn`.
     ///
-    /// NB: if this is reified to a `fn` pointer, a `ReifyShim` is used
-    /// (see `ReifyShim` above for more details on that).
+    /// This `InstanceDef` does not have callable MIR. Calls to `Virtual` instances must be
+    /// codegen'd as virtual calls through the vtable.
+    ///
+    /// If this is reified to a `fn` pointer, a `ReifyShim` is used (see `ReifyShim` above for more
+    /// details on that).
     Virtual(DefId, usize),
 
-    /// `<[mut closure] as FnOnce>::call_once`
-    ClosureOnceShim {
-        call_once: DefId,
-    },
+    /// `<[FnMut closure] as FnOnce>::call_once`.
+    ///
+    /// The `DefId` is the ID of the `call_once` method in `FnOnce`.
+    ClosureOnceShim { call_once: DefId },
 
     /// `core::ptr::drop_in_place::<T>`.
+    ///
     /// The `DefId` is for `core::ptr::drop_in_place`.
     /// The `Option<Ty<'tcx>>` is either `Some(T)`, or `None` for empty drop
     /// glue.
@@ -67,7 +92,12 @@ pub enum InstanceDef<'tcx> {
     // FIXME(#69925) support polymorphic MIR shim bodies properly instead.
     DropGlue(DefId, Option<Ty<'tcx>>),
 
-    ///`<T as Clone>::clone` shim.
+    /// Compiler-generated `<T as Clone>::clone` implementation.
+    ///
+    /// For all types that automatically implement `Copy`, a trivial `Clone` impl is provided too.
+    /// Additionally, arrays, tuples, and closures get a `Clone` shim even if they aren't `Copy`.
+    ///
+    /// The `DefId` is for `Clone::clone`, the `Ty` is the type `T` with the builtin `Clone` impl.
     ///
     /// NB: the type must currently be monomorphic to avoid double substitution
     /// problems with the MIR shim bodies. `Instance::resolve` enforces this.
diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs
index 71fff851531..15a2e9130a3 100644
--- a/src/librustc_mir/shim.rs
+++ b/src/librustc_mir/shim.rs
@@ -32,13 +32,9 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
 
     let mut result = match instance {
         ty::InstanceDef::Item(..) => bug!("item {:?} passed to make_shim", instance),
-        ty::InstanceDef::VtableShim(def_id) => build_call_shim(
-            tcx,
-            instance,
-            Some(Adjustment::DerefMove),
-            CallKind::Direct(def_id),
-            None,
-        ),
+        ty::InstanceDef::VtableShim(def_id) => {
+            build_call_shim(tcx, instance, Some(Adjustment::Deref), CallKind::Direct(def_id), None)
+        }
         ty::InstanceDef::FnPtrShim(def_id, ty) => {
             // FIXME(eddyb) support generating shims for a "shallow type",
             // e.g. `Foo<_>` or `[_]` instead of requiring a fully monomorphic
@@ -60,7 +56,7 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
             let sig = tcx.erase_late_bound_regions(&ty.fn_sig(tcx));
             let arg_tys = sig.inputs();
 
-            build_call_shim(tcx, instance, Some(adjustment), CallKind::Indirect, Some(arg_tys))
+            build_call_shim(tcx, instance, Some(adjustment), CallKind::Indirect(ty), Some(arg_tys))
         }
         // We are generating a call back to our def-id, which the
         // codegen backend knows to turn to an actual call, be it
@@ -134,15 +130,28 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
 
 #[derive(Copy, Clone, Debug, PartialEq)]
 enum Adjustment {
+    /// Pass the receiver as-is.
     Identity,
+
+    /// We get passed `&[mut] self` and call the target with `*self`.
+    ///
+    /// This either copies `self` (if `Self: Copy`, eg. for function items), or moves out of it
+    /// (for `VtableShim`, which effectively is passed `&own Self`).
     Deref,
-    DerefMove,
+
+    /// We get passed `self: Self` and call the target with `&mut self`.
+    ///
+    /// In this case we need to ensure that the `Self` is dropped after the call, as the callee
+    /// won't do it for us.
     RefMut,
 }
 
 #[derive(Copy, Clone, Debug, PartialEq)]
-enum CallKind {
-    Indirect,
+enum CallKind<'tcx> {
+    /// Call the `FnPtr` that was passed as the receiver.
+    Indirect(Ty<'tcx>),
+
+    /// Call a known `FnDef`.
     Direct(DefId),
 }
 
@@ -662,7 +671,7 @@ fn build_call_shim<'tcx>(
     tcx: TyCtxt<'tcx>,
     instance: ty::InstanceDef<'tcx>,
     rcvr_adjustment: Option<Adjustment>,
-    call_kind: CallKind,
+    call_kind: CallKind<'tcx>,
     untuple_args: Option<&[Ty<'tcx>]>,
 ) -> Body<'tcx> {
     debug!(
@@ -675,6 +684,29 @@ fn build_call_shim<'tcx>(
     let sig = tcx.fn_sig(def_id);
     let mut sig = tcx.erase_late_bound_regions(&sig);
 
+    if let CallKind::Indirect(fnty) = call_kind {
+        // `sig` determines our local decls, and thus the callee type in the `Call` terminator. This
+        // can only be an `FnDef` or `FnPtr`, but currently will be `Self` since the types come from
+        // the implemented `FnX` trait.
+
+        // Apply the opposite adjustment to the MIR input.
+        let mut inputs_and_output = sig.inputs_and_output.to_vec();
+
+        // Initial signature is `fn(&? Self, Args) -> Self::Output` where `Args` is a tuple of the
+        // fn arguments. `Self` may be passed via (im)mutable reference or by-value.
+        assert_eq!(inputs_and_output.len(), 3);
+
+        // `Self` is always the original fn type `ty`. The MIR call terminator is only defined for
+        // `FnDef` and `FnPtr` callees, not the `Self` type param.
+        let self_arg = &mut inputs_and_output[0];
+        *self_arg = match rcvr_adjustment.unwrap() {
+            Adjustment::Identity => fnty,
+            Adjustment::Deref => tcx.mk_imm_ptr(fnty),
+            Adjustment::RefMut => tcx.mk_mut_ptr(fnty),
+        };
+        sig.inputs_and_output = tcx.intern_type_list(&inputs_and_output);
+    }
+
     // FIXME(eddyb) avoid having this snippet both here and in
     // `Instance::fn_sig` (introduce `InstanceDef::fn_sig`?).
     if let ty::InstanceDef::VtableShim(..) = instance {
@@ -701,8 +733,7 @@ fn build_call_shim<'tcx>(
 
     let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment {
         Adjustment::Identity => Operand::Move(rcvr_place()),
-        Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())), // Can't copy `&mut`
-        Adjustment::DerefMove => Operand::Move(tcx.mk_place_deref(rcvr_place())),
+        Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())),
         Adjustment::RefMut => {
             // let rcvr = &mut rcvr;
             let ref_rcvr = local_decls.push(
@@ -728,7 +759,10 @@ fn build_call_shim<'tcx>(
     });
 
     let (callee, mut args) = match call_kind {
-        CallKind::Indirect => (rcvr.unwrap(), vec![]),
+        // `FnPtr` call has no receiver. Args are untupled below.
+        CallKind::Indirect(_) => (rcvr.unwrap(), vec![]),
+
+        // `FnDef` call with optional receiver.
         CallKind::Direct(def_id) => {
             let ty = tcx.type_of(def_id);
             (
diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs
index 8150c328316..625f40cd792 100644
--- a/src/librustc_mir/transform/validate.rs
+++ b/src/librustc_mir/transform/validate.rs
@@ -9,7 +9,6 @@ use rustc_middle::{
     },
     ty::{self, ParamEnv, TyCtxt},
 };
-use rustc_span::def_id::DefId;
 
 #[derive(Copy, Clone, Debug)]
 enum EdgeKind {
@@ -24,15 +23,14 @@ pub struct Validator {
 
 impl<'tcx> MirPass<'tcx> for Validator {
     fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) {
-        let def_id = source.def_id();
-        let param_env = tcx.param_env(def_id);
-        TypeChecker { when: &self.when, def_id, body, tcx, param_env }.visit_body(body);
+        let param_env = tcx.param_env(source.def_id());
+        TypeChecker { when: &self.when, source, body, tcx, param_env }.visit_body(body);
     }
 }
 
 struct TypeChecker<'a, 'tcx> {
     when: &'a str,
-    def_id: DefId,
+    source: MirSource<'tcx>,
     body: &'a Body<'tcx>,
     tcx: TyCtxt<'tcx>,
     param_env: ParamEnv<'tcx>,
@@ -47,7 +45,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
             span,
             &format!(
                 "broken MIR in {:?} ({}) at {:?}:\n{}",
-                self.def_id,
+                self.source.instance,
                 self.when,
                 location,
                 msg.as_ref()
diff --git a/src/librustc_trait_selection/traits/util.rs b/src/librustc_trait_selection/traits/util.rs
index 8b5b4128e66..d3484b8af89 100644
--- a/src/librustc_trait_selection/traits/util.rs
+++ b/src/librustc_trait_selection/traits/util.rs
@@ -302,7 +302,7 @@ pub fn get_vtable_index_of_object_method<N>(
 ) -> usize {
     // Count number of methods preceding the one we are selecting and
     // add them to the total offset.
-    // Skip over associated types and constants.
+    // Skip over associated types and constants, as those aren't stored in the vtable.
     let mut entries = object.vtable_base;
     for trait_item in tcx.associated_items(object.upcast_trait_ref.def_id()).in_definition_order() {
         if trait_item.def_id == method_def_id {
diff --git a/src/test/mir-opt/fn-ptr-shim.rs b/src/test/mir-opt/fn-ptr-shim.rs
new file mode 100644
index 00000000000..08413c9f6fc
--- /dev/null
+++ b/src/test/mir-opt/fn-ptr-shim.rs
@@ -0,0 +1,15 @@
+// compile-flags: -Zmir-opt-level=0 -Zvalidate-mir
+
+// Tests that the `<fn() as Fn>` shim does not create a `Call` terminator with a `Self` callee
+// (as only `FnDef` and `FnPtr` callees are allowed in MIR).
+
+// EMIT_MIR rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir
+fn main() {
+    call(noop as fn());
+}
+
+fn noop() {}
+
+fn call<F: Fn()>(f: F) {
+    f();
+}
diff --git a/src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir b/src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir
new file mode 100644
index 00000000000..4ecc331afae
--- /dev/null
+++ b/src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir
@@ -0,0 +1,13 @@
+// MIR for `std::ops::Fn::call` before AddMovesForPackedDrops
+
+fn std::ops::Fn::call(_1: *const fn(), _2: Args) -> <Self as std::ops::FnOnce<Args>>::Output {
+    let mut _0: <Self as std::ops::FnOnce<Args>>::Output; // return place in scope 0 at $SRC_DIR/libcore/ops/function.rs:LL:COL
+
+    bb0: {
+        _0 = move (*_1)() -> bb1;        // scope 0 at $SRC_DIR/libcore/ops/function.rs:LL:COL
+    }
+
+    bb1: {
+        return;                          // scope 0 at $SRC_DIR/libcore/ops/function.rs:LL:COL
+    }
+}