Auto merge of rust-lang#70174 - JohnTitor:rollup-0lum0jh, r=JohnTitor

Rollup of 9 pull requests

Successful merges:

 - rust-lang#69618 (Clarify the relationship between `forget()` and `ManuallyDrop`.)
 - rust-lang#69768 (Compute the correct layout for variants of uninhabited enums)
 - rust-lang#69935 (codegen/mir: support polymorphic `InstanceDef`s)
 - rust-lang#70103 (Clean up E0437 explanation)
 - rust-lang#70131 (Add regression test for TAIT lifetime inference (issue rust-lang#55099))
 - rust-lang#70133 (remove unused imports)
 - rust-lang#70145 (doc: Add quote to .init_array)
 - rust-lang#70146 (Clean up e0438 explanation)
 - rust-lang#70150 (triagebot.toml: accept cleanup-crew)

Failed merges:

r? @ghost

Author: bors

src/libcore/mem/mod.rs

@@ -58,7 +58,9 @@ pub use crate::intrinsics::transmute;
 ///
 /// # Examples
 ///
-/// Leak an I/O object, never closing the file:
+/// The canonical safe use of `mem::forget` is to circumvent a value's destructor
+/// implemented by the `Drop` trait. For example, this will leak a `File`, i.e. reclaim
+/// the space taken by the variable but never close the underlying system resource:
 ///
 /// ```no_run
 /// use std::mem;
@@ -68,9 +70,40 @@ pub use crate::intrinsics::transmute;
 /// mem::forget(file);
 /// ```
 ///
-/// The practical use cases for `forget` are rather specialized and mainly come
-/// up in unsafe or FFI code. However, [`ManuallyDrop`] is usually preferred
-/// for such cases, e.g.:
+/// This is useful when the ownership of the underlying resource was previously
+/// transferred to code outside of Rust, for example by transmitting the raw
+/// file descriptor to C code.
+///
+/// # Relationship with `ManuallyDrop`
+///
+/// While `mem::forget` can also be used to transfer *memory* ownership, doing so is error-prone.
+/// [`ManuallyDrop`] should be used instead. Consider, for example, this code:
+///
+/// ```
+/// use std::mem;
+///
+/// let mut v = vec![65, 122];
+/// // Build a `String` using the contents of `v`
+/// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), v.len(), v.capacity()) };
+/// // leak `v` because its memory is now managed by `s`
+/// mem::forget(v);  // ERROR - v is invalid and must not be passed to a function
+/// assert_eq!(s, "Az");
+/// // `s` is implicitly dropped and its memory deallocated.
+/// ```
+///
+/// There are two issues with the above example:
+///
+/// * If more code were added between the construction of `String` and the invocation of
+///   `mem::forget()`, a panic within it would cause a double free because the same memory
+///   is handled by both `v` and `s`.
+/// * After calling `v.as_mut_ptr()` and transmitting the ownership of the data to `s`,
+///   the `v` value is invalid. Even when a value is just moved to `mem::forget` (which won't
+///   inspect it), some types have strict requirements on their values that
+///   make them invalid when dangling or no longer owned. Using invalid values in any
+///   way, including passing them to or returning them from functions, constitutes
+///   undefined behavior and may break the assumptions made by the compiler.
+///
+/// Switching to `ManuallyDrop` avoids both issues:
 ///
 /// ```
 /// use std::mem::ManuallyDrop;
@@ -80,24 +113,24 @@ pub use crate::intrinsics::transmute;
 /// // does not get dropped!
 /// let mut v = ManuallyDrop::new(v);
 /// // Now disassemble `v`. These operations cannot panic, so there cannot be a leak.
-/// let ptr = v.as_mut_ptr();
-/// let cap = v.capacity();
+/// let (ptr, len, cap) = (v.as_mut_ptr(), v.len(), v.capacity());
 /// // Finally, build a `String`.
-/// let s = unsafe { String::from_raw_parts(ptr, 2, cap) };
+/// let s = unsafe { String::from_raw_parts(ptr, len, cap) };
 /// assert_eq!(s, "Az");
 /// // `s` is implicitly dropped and its memory deallocated.
 /// ```
 ///
-/// Using `ManuallyDrop` here has two advantages:
+/// `ManuallyDrop` robustly prevents double-free because we disable `v`'s destructor
+/// before doing anything else. `mem::forget()` doesn't allow this because it consumes its
+/// argument, forcing us to call it only after extracting anything we need from `v`. Even
+/// if a panic were introduced between construction of `ManuallyDrop` and building the
+/// string (which cannot happen in the code as shown), it would result in a leak and not a
+/// double free. In other words, `ManuallyDrop` errs on the side of leaking instead of
+/// erring on the side of (double-)dropping.
 ///
-/// * We do not "touch" `v` after disassembling it. For some types, operations
-///   such as passing ownership (to a function like `mem::forget`) requires them to actually
-///   be fully owned right now; that is a promise we do not want to make here as we are
-///   in the process of transferring ownership to the new `String` we are building.
-/// * In case of an unexpected panic, `ManuallyDrop` is not dropped, but if the panic
-///   occurs before `mem::forget` was called we might end up dropping invalid data,
-///   or double-dropping. In other words, `ManuallyDrop` errs on the side of leaking
-///   instead of erring on the side of dropping.
+/// Also, `ManuallyDrop` prevents us from having to "touch" `v` after transferring the
+/// ownership to `s` - the final step of interacting with `v` to dispoe of it without
+/// running its destructor is entirely avoided.
 ///
 /// [drop]: fn.drop.html
 /// [uninit]: fn.uninitialized.html

src/libpanic_unwind/hermit.rs

@@ -4,7 +4,6 @@
 
 use alloc::boxed::Box;
 use core::any::Any;
-use core::ptr;
 
 pub unsafe fn cleanup(_ptr: *mut u8) -> Box<dyn Any + Send> {
     extern "C" {

src/librustc/ty/instance.rs

@@ -381,6 +381,32 @@ impl<'tcx> Instance<'tcx> {
         Instance { def, substs }
     }
 
+    /// FIXME(#69925) Depending on the kind of `InstanceDef`, the MIR body associated with an
+    /// instance is expressed in terms of the generic parameters of `self.def_id()`, and in other
+    /// cases the MIR body is expressed in terms of the types found in the substitution array.
+    /// In the former case, we want to substitute those generic types and replace them with the
+    /// values from the substs when monomorphizing the function body. But in the latter case, we
+    /// don't want to do that substitution, since it has already been done effectively.
+    ///
+    /// This function returns `Some(substs)` in the former case and None otherwise -- i.e., if
+    /// this function returns `None`, then the MIR body does not require substitution during
+    /// monomorphization.
+    pub fn substs_for_mir_body(&self) -> Option<SubstsRef<'tcx>> {
+        match self.def {
+            InstanceDef::CloneShim(..)
+            | InstanceDef::DropGlue(_, Some(_)) => None,
+            InstanceDef::ClosureOnceShim { .. }
+            | InstanceDef::DropGlue(..)
+            // FIXME(#69925): `FnPtrShim` should be in the other branch.
+            | InstanceDef::FnPtrShim(..)
+            | InstanceDef::Item(_)
+            | InstanceDef::Intrinsic(..)
+            | InstanceDef::ReifyShim(..)
+            | InstanceDef::Virtual(..)
+            | InstanceDef::VtableShim(..) => Some(self.substs),
+        }
+    }
+
     pub fn is_vtable_shim(&self) -> bool {
         if let InstanceDef::VtableShim(..) = self.def { true } else { false }
     }

src/librustc/ty/layout.rs

@@ -780,8 +780,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
                     present_first @ Some(_) => present_first,
                     // Uninhabited because it has no variants, or only absent ones.
                     None if def.is_enum() => return tcx.layout_raw(param_env.and(tcx.types.never)),
-                    // if it's a struct, still compute a layout so that we can still compute the
-                    // field offsets
+                    // If it's a struct, still compute a layout so that we can still compute the
+                    // field offsets.
                     None => Some(VariantIdx::new(0)),
                 };
 
@@ -1987,7 +1987,15 @@ where
 {
     fn for_variant(this: TyLayout<'tcx>, cx: &C, variant_index: VariantIdx) -> TyLayout<'tcx> {
         let details = match this.variants {
-            Variants::Single { index } if index == variant_index => this.details,
+            Variants::Single { index }
+                // If all variants but one are uninhabited, the variant layout is the enum layout.
+                if index == variant_index &&
+                // Don't confuse variants of uninhabited enums with the enum itself.
+                // For more details see https://github.com/rust-lang/rust/issues/69763.
+                this.fields != FieldPlacement::Union(0) =>
+            {
+                this.details
+            }
 
             Variants::Single { index } => {
                 // Deny calling for_variant more than once for non-Single enums.

src/librustc_codegen_ssa/mir/mod.rs

@@ -86,13 +86,18 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
 impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
     pub fn monomorphize<T>(&self, value: &T) -> T
     where
-        T: TypeFoldable<'tcx>,
+        T: Copy + TypeFoldable<'tcx>,
     {
-        self.cx.tcx().subst_and_normalize_erasing_regions(
-            self.instance.substs,
-            ty::ParamEnv::reveal_all(),
-            value,
-        )
+        debug!("monomorphize: self.instance={:?}", self.instance);
+        if let Some(substs) = self.instance.substs_for_mir_body() {
+            self.cx.tcx().subst_and_normalize_erasing_regions(
+                substs,
+                ty::ParamEnv::reveal_all(),
+                &value,
+            )
+        } else {
+            self.cx.tcx().normalize_erasing_regions(ty::ParamEnv::reveal_all(), *value)
+        }
     }
 }
 

src/librustc_error_codes/error_codes/E0437.md

@@ -1,7 +1,5 @@
-Trait implementations can only implement associated types that are members of
-the trait in question. This error indicates that you attempted to implement
-an associated type whose name does not match the name of any associated type
-in the trait.
+An associated type whose name does not match any of the associated types
+in the trait was used when implementing the trait.
 
 Erroneous code example:
 
@@ -13,6 +11,9 @@ impl Foo for i32 {
 }
 ```
 
+Trait implementations can only implement associated types that are members of
+the trait in question.
+
 The solution to this problem is to remove the extraneous associated type:
 
 ```

src/librustc_error_codes/error_codes/E0438.md

@@ -1,7 +1,5 @@
-Trait implementations can only implement associated constants that are
-members of the trait in question. This error indicates that you
-attempted to implement an associated constant whose name does not
-match the name of any associated constant in the trait.
+An associated constant whose name does not match any of the associated constants
+in the trait was used when implementing the trait.
 
 Erroneous code example:
 
@@ -13,6 +11,9 @@ impl Foo for i32 {
 }
 ```
 
+Trait implementations can only implement associated constants that are
+members of the trait in question.
+
 The solution to this problem is to remove the extraneous associated constant:
 
 ```

src/librustc_mir/interpret/eval_context.rs

@@ -335,15 +335,25 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
     /// Call this on things you got out of the MIR (so it is as generic as the current
     /// stack frame), to bring it into the proper environment for this interpreter.
+    pub(super) fn subst_from_current_frame_and_normalize_erasing_regions<T: TypeFoldable<'tcx>>(
+        &self,
+        value: T,
+    ) -> T {
+        self.subst_from_frame_and_normalize_erasing_regions(self.frame(), value)
+    }
+
+    /// Call this on things you got out of the MIR (so it is as generic as the provided
+    /// stack frame), to bring it into the proper environment for this interpreter.
     pub(super) fn subst_from_frame_and_normalize_erasing_regions<T: TypeFoldable<'tcx>>(
         &self,
+        frame: &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
         value: T,
     ) -> T {
-        self.tcx.subst_and_normalize_erasing_regions(
-            self.frame().instance.substs,
-            self.param_env,
-            &value,
-        )
+        if let Some(substs) = frame.instance.substs_for_mir_body() {
+            self.tcx.subst_and_normalize_erasing_regions(substs, self.param_env, &value)
+        } else {
+            self.tcx.normalize_erasing_regions(self.param_env, value)
+        }
     }
 
     /// The `substs` are assumed to already be in our interpreter "universe" (param_env).
@@ -371,11 +381,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             None => {
                 let layout = crate::interpret::operand::from_known_layout(layout, || {
                     let local_ty = frame.body.local_decls[local].ty;
-                    let local_ty = self.tcx.subst_and_normalize_erasing_regions(
-                        frame.instance.substs,
-                        self.param_env,
-                        &local_ty,
-                    );
+                    let local_ty =
+                        self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty);
                     self.layout_of(local_ty)
                 })?;
                 if let Some(state) = frame.locals.get(local) {

src/librustc_mir/interpret/operand.rs

@@ -355,7 +355,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
         let base = match op.try_as_mplace(self) {
             Ok(mplace) => {
-                // The easy case
+                // We can reuse the mplace field computation logic for indirect operands.
                 let field = self.mplace_field(mplace, field)?;
                 return Ok(field.into());
             }
@@ -490,7 +490,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             Copy(ref place) | Move(ref place) => self.eval_place_to_op(place, layout)?,
 
             Constant(ref constant) => {
-                let val = self.subst_from_frame_and_normalize_erasing_regions(constant.literal);
+                let val =
+                    self.subst_from_current_frame_and_normalize_erasing_regions(constant.literal);
                 self.eval_const_to_op(val, layout)?
             }
         };

src/librustc_mir/interpret/place.rs

@@ -410,14 +410,6 @@ where
                 stride * field
             }
             layout::FieldPlacement::Union(count) => {
-                // This is a narrow bug-fix for rust-lang/rust#69191: if we are
-                // trying to access absent field of uninhabited variant, then
-                // signal UB (but don't ICE the compiler).
-                // FIXME temporary hack to work around incoherence between
-                // layout computation and MIR building
-                if field >= count as u64 && base.layout.abi == layout::Abi::Uninhabited {
-                    throw_ub!(Unreachable);
-                }
                 assert!(
                     field < count as u64,
                     "Tried to access field {} of union {:#?} with {} fields",
@@ -648,9 +640,11 @@ where
                         // bail out.
                         None => Place::null(&*self),
                     },
-                    layout: self.layout_of(self.subst_from_frame_and_normalize_erasing_regions(
-                        self.frame().body.return_ty(),
-                    ))?,
+                    layout: self.layout_of(
+                        self.subst_from_current_frame_and_normalize_erasing_regions(
+                            self.frame().body.return_ty(),
+                        ),
+                    )?,
                 }
             }
             local => PlaceTy {