Auto merge of #24765 - pnkfelix:fsk-enum-swapindrop, r=nikomatsakis

Inspect enum discriminant *after* calling its destructor

Includes some drive-by cleanup (e.g. changed some field and method names to reflect fill-on-drop; added comments about zero-variant enums being classified as `_match::Single`).

Probably the most invasive change was the expansion of the maps `available_drop_glues` and `drop_glues` to now hold two different kinds of drop glues; there is the (old) normal drop glue, and there is (new) drop-contents glue that jumps straight to dropping the contents of a struct or enum, skipping its destructor.

 * For all types that do not have user-defined Drop implementations, the normal glue is generated as usual (i.e. recursively dropping the fields of the data structure).

  (And this actually is exactly what the newly-added drop-contents glue does as well.)

 * For types that have user-defined Drop implementations, the "normal" drop glue now schedules a cleanup before invoking the `Drop::drop` method that will call the drop-contents glue after that invocation returns.

Fix #23611.

----

Is this a breaking change?  The prior behavior was totally unsound, and it seems unreasonable that anyone was actually relying on it.

Nonetheless, since there is a user-visible change to the language semantics, I guess I will conservatively mark this as a:

[breaking-change]

(To see an example of what sort of user-visible change this causes, see the comments in the regression test.)

Author: bors

src/librustc_trans/trans/_match.rs

@@ -916,8 +916,8 @@ fn insert_lllocals<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
 
         let datum = Datum::new(llval, binding_info.ty, Lvalue);
         if let Some(cs) = cs {
-            bcx.fcx.schedule_drop_and_zero_mem(cs, llval, binding_info.ty);
             bcx.fcx.schedule_lifetime_end(cs, binding_info.llmatch);
+            bcx.fcx.schedule_drop_and_fill_mem(cs, llval, binding_info.ty);
         }
 
         debug!("binding {} to {}", binding_info.id, bcx.val_to_string(llval));

src/librustc_trans/trans/adt.rs

@@ -771,6 +771,7 @@ pub fn trans_switch<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
             (_match::Switch, Some(trans_get_discr(bcx, r, scrutinee, None)))
         }
         Univariant(..) => {
+            // N.B.: Univariant means <= 1 enum variants (*not* == 1 variants).
             (_match::Single, None)
         }
     }
@@ -1062,7 +1063,7 @@ pub fn trans_drop_flag_ptr<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>, r: &Repr<'tcx
             ));
             bcx = fold_variants(bcx, r, val, |variant_cx, st, value| {
                 let ptr = struct_field_ptr(variant_cx, st, value, (st.fields.len() - 1), false);
-                datum::Datum::new(ptr, ptr_ty, datum::Rvalue::new(datum::ByRef))
+                datum::Datum::new(ptr, ptr_ty, datum::Lvalue)
                     .store_to(variant_cx, scratch.val)
             });
             let expr_datum = scratch.to_expr_datum();

src/librustc_trans/trans/base.rs

@@ -471,8 +471,11 @@ pub fn iter_structural_ty<'blk, 'tcx, F>(cx: Block<'blk, 'tcx>,
 
           match adt::trans_switch(cx, &*repr, av) {
               (_match::Single, None) => {
-                  cx = iter_variant(cx, &*repr, av, &*(*variants)[0],
-                                    substs, &mut f);
+                  if n_variants != 0 {
+                      assert!(n_variants == 1);
+                      cx = iter_variant(cx, &*repr, av, &*(*variants)[0],
+                                        substs, &mut f);
+                  }
               }
               (_match::Switch, Some(lldiscrim_a)) => {
                   cx = f(cx, lldiscrim_a, cx.tcx().types.isize);

src/librustc_trans/trans/cleanup.rs

@@ -393,19 +393,22 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
             must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
             val: val,
             ty: ty,
-            zero: false
+            fill_on_drop: false,
+            skip_dtor: false,
         };
 
-        debug!("schedule_drop_mem({:?}, val={}, ty={})",
+        debug!("schedule_drop_mem({:?}, val={}, ty={}) fill_on_drop={} skip_dtor={}",
                cleanup_scope,
                self.ccx.tn().val_to_string(val),
-               ty.repr(self.ccx.tcx()));
+               ty.repr(self.ccx.tcx()),
+               drop.fill_on_drop,
+               drop.skip_dtor);
 
         self.schedule_clean(cleanup_scope, drop as CleanupObj);
     }
 
-    /// Schedules a (deep) drop and zero-ing of `val`, which is a pointer to an instance of `ty`
-    fn schedule_drop_and_zero_mem(&self,
+    /// Schedules a (deep) drop and filling of `val`, which is a pointer to an instance of `ty`
+    fn schedule_drop_and_fill_mem(&self,
                                   cleanup_scope: ScopeId,
                                   val: ValueRef,
                                   ty: Ty<'tcx>) {
@@ -416,14 +419,48 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
             must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
             val: val,
             ty: ty,
-            zero: true
+            fill_on_drop: true,
+            skip_dtor: false,
+        };
+
+        debug!("schedule_drop_and_fill_mem({:?}, val={}, ty={}, fill_on_drop={}, skip_dtor={})",
+               cleanup_scope,
+               self.ccx.tn().val_to_string(val),
+               ty.repr(self.ccx.tcx()),
+               drop.fill_on_drop,
+               drop.skip_dtor);
+
+        self.schedule_clean(cleanup_scope, drop as CleanupObj);
+    }
+
+    /// Issue #23611: Schedules a (deep) drop of the contents of
+    /// `val`, which is a pointer to an instance of struct/enum type
+    /// `ty`. The scheduled code handles extracting the discriminant
+    /// and dropping the contents associated with that variant
+    /// *without* executing any associated drop implementation.
+    fn schedule_drop_enum_contents(&self,
+                                   cleanup_scope: ScopeId,
+                                   val: ValueRef,
+                                   ty: Ty<'tcx>) {
+        // `if` below could be "!contents_needs_drop"; skipping drop
+        // is just an optimization, so sound to be conservative.
+        if !self.type_needs_drop(ty) { return; }
+
+        let drop = box DropValue {
+            is_immediate: false,
+            must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
+            val: val,
+            ty: ty,
+            fill_on_drop: false,
+            skip_dtor: true,
         };
 
-        debug!("schedule_drop_and_zero_mem({:?}, val={}, ty={}, zero={})",
+        debug!("schedule_drop_enum_contents({:?}, val={}, ty={}) fill_on_drop={} skip_dtor={}",
                cleanup_scope,
                self.ccx.tn().val_to_string(val),
                ty.repr(self.ccx.tcx()),
-               true);
+               drop.fill_on_drop,
+               drop.skip_dtor);
 
         self.schedule_clean(cleanup_scope, drop as CleanupObj);
     }
@@ -440,13 +477,16 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
             must_unwind: common::type_needs_unwind_cleanup(self.ccx, ty),
             val: val,
             ty: ty,
-            zero: false
+            fill_on_drop: false,
+            skip_dtor: false,
         };
 
-        debug!("schedule_drop_immediate({:?}, val={}, ty={:?})",
+        debug!("schedule_drop_immediate({:?}, val={}, ty={:?}) fill_on_drop={} skip_dtor={}",
                cleanup_scope,
                self.ccx.tn().val_to_string(val),
-               ty.repr(self.ccx.tcx()));
+               ty.repr(self.ccx.tcx()),
+               drop.fill_on_drop,
+               drop.skip_dtor);
 
         self.schedule_clean(cleanup_scope, drop as CleanupObj);
     }
@@ -987,7 +1027,8 @@ pub struct DropValue<'tcx> {
     must_unwind: bool,
     val: ValueRef,
     ty: Ty<'tcx>,
-    zero: bool
+    fill_on_drop: bool,
+    skip_dtor: bool,
 }
 
 impl<'tcx> Cleanup<'tcx> for DropValue<'tcx> {
@@ -1007,13 +1048,18 @@ impl<'tcx> Cleanup<'tcx> for DropValue<'tcx> {
                    bcx: Block<'blk, 'tcx>,
                    debug_loc: DebugLoc)
                    -> Block<'blk, 'tcx> {
-        let _icx = base::push_ctxt("<DropValue as Cleanup>::trans");
+        let skip_dtor = self.skip_dtor;
+        let _icx = if skip_dtor {
+            base::push_ctxt("<DropValue as Cleanup>::trans skip_dtor=true")
+        } else {
+            base::push_ctxt("<DropValue as Cleanup>::trans skip_dtor=false")
+        };
         let bcx = if self.is_immediate {
-            glue::drop_ty_immediate(bcx, self.val, self.ty, debug_loc)
+            glue::drop_ty_immediate(bcx, self.val, self.ty, debug_loc, self.skip_dtor)
         } else {
-            glue::drop_ty(bcx, self.val, self.ty, debug_loc)
+            glue::drop_ty_core(bcx, self.val, self.ty, debug_loc, self.skip_dtor)
         };
-        if self.zero {
+        if self.fill_on_drop {
             base::drop_done_fill_mem(bcx, self.val, self.ty);
         }
         bcx
@@ -1190,10 +1236,14 @@ pub trait CleanupMethods<'blk, 'tcx> {
                          cleanup_scope: ScopeId,
                          val: ValueRef,
                          ty: Ty<'tcx>);
-    fn schedule_drop_and_zero_mem(&self,
+    fn schedule_drop_and_fill_mem(&self,
                                   cleanup_scope: ScopeId,
                                   val: ValueRef,
                                   ty: Ty<'tcx>);
+    fn schedule_drop_enum_contents(&self,
+                                   cleanup_scope: ScopeId,
+                                   val: ValueRef,
+                                   ty: Ty<'tcx>);
     fn schedule_drop_immediate(&self,
                                cleanup_scope: ScopeId,
                                val: ValueRef,

src/librustc_trans/trans/context.rs

@@ -21,6 +21,7 @@ use trans::builder::Builder;
 use trans::common::{ExternMap,BuilderRef_res};
 use trans::debuginfo;
 use trans::declare;
+use trans::glue::DropGlueKind;
 use trans::monomorphize::MonoId;
 use trans::type_::{Type, TypeNames};
 use middle::subst::Substs;
@@ -73,7 +74,7 @@ pub struct SharedCrateContext<'tcx> {
     check_drop_flag_for_sanity: bool,
 
     available_monomorphizations: RefCell<FnvHashSet<String>>,
-    available_drop_glues: RefCell<FnvHashMap<Ty<'tcx>, String>>,
+    available_drop_glues: RefCell<FnvHashMap<DropGlueKind<'tcx>, String>>,
 }
 
 /// The local portion of a `CrateContext`.  There is one `LocalCrateContext`
@@ -89,7 +90,7 @@ pub struct LocalCrateContext<'tcx> {
     item_vals: RefCell<NodeMap<ValueRef>>,
     needs_unwind_cleanup_cache: RefCell<FnvHashMap<Ty<'tcx>, bool>>,
     fn_pointer_shims: RefCell<FnvHashMap<Ty<'tcx>, ValueRef>>,
-    drop_glues: RefCell<FnvHashMap<Ty<'tcx>, ValueRef>>,
+    drop_glues: RefCell<FnvHashMap<DropGlueKind<'tcx>, ValueRef>>,
     /// Track mapping of external ids to local items imported for inlining
     external: RefCell<DefIdMap<Option<ast::NodeId>>>,
     /// Backwards version of the `external` map (inlined items to where they
@@ -574,7 +575,7 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
         &self.local.fn_pointer_shims
     }
 
-    pub fn drop_glues<'a>(&'a self) -> &'a RefCell<FnvHashMap<Ty<'tcx>, ValueRef>> {
+    pub fn drop_glues<'a>(&'a self) -> &'a RefCell<FnvHashMap<DropGlueKind<'tcx>, ValueRef>> {
         &self.local.drop_glues
     }
 
@@ -660,7 +661,7 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
         &self.shared.available_monomorphizations
     }
 
-    pub fn available_drop_glues<'a>(&'a self) -> &'a RefCell<FnvHashMap<Ty<'tcx>, String>> {
+    pub fn available_drop_glues(&self) -> &RefCell<FnvHashMap<DropGlueKind<'tcx>, String>> {
         &self.shared.available_drop_glues
     }