Auto merge of #30492 - wesleywiser:fix_extra_drops, r=pnkfelix

Fixes #28159

Author: bors

src/librustc/middle/ty/mod.rs

@@ -46,7 +46,7 @@ use std::vec::IntoIter;
 use std::collections::{HashMap, HashSet};
 use syntax::ast::{self, CrateNum, Name, NodeId};
 use syntax::attr::{self, AttrMetaMethods};
-use syntax::codemap::Span;
+use syntax::codemap::{DUMMY_SP, Span};
 use syntax::parse::token::{InternedString, special_idents};
 
 use rustc_front::hir;
@@ -2394,6 +2394,39 @@ impl<'tcx> ctxt<'tcx> {
             || self.sess.cstore.item_super_predicates(self, did))
     }
 
+    /// If `type_needs_drop` returns true, then `ty` is definitely
+    /// non-copy and *might* have a destructor attached; if it returns
+    /// false, then `ty` definitely has no destructor (i.e. no drop glue).
+    ///
+    /// (Note that this implies that if `ty` has a destructor attached,
+    /// then `type_needs_drop` will definitely return `true` for `ty`.)
+    pub fn type_needs_drop_given_env<'a>(&self,
+                                         ty: Ty<'tcx>,
+                                         param_env: &ty::ParameterEnvironment<'a,'tcx>) -> bool {
+        // Issue #22536: We first query type_moves_by_default.  It sees a
+        // normalized version of the type, and therefore will definitely
+        // know whether the type implements Copy (and thus needs no
+        // cleanup/drop/zeroing) ...
+        let implements_copy = !ty.moves_by_default(param_env, DUMMY_SP);
+
+        if implements_copy { return false; }
+
+        // ... (issue #22536 continued) but as an optimization, still use
+        // prior logic of asking if the `needs_drop` bit is set; we need
+        // not zero non-Copy types if they have no destructor.
+
+        // FIXME(#22815): Note that calling `ty::type_contents` is a
+        // conservative heuristic; it may report that `needs_drop` is set
+        // when actual type does not actually have a destructor associated
+        // with it. But since `ty` absolutely did not have the `Copy`
+        // bound attached (see above), it is sound to treat it as having a
+        // destructor (e.g. zero its memory on move).
+
+        let contents = ty.type_contents(self);
+        debug!("type_needs_drop ty={:?} contents={:?}", ty, contents);
+        contents.needs_drop(self)
+    }
+
     /// Get the attributes of a definition.
     pub fn get_attrs(&self, did: DefId) -> Cow<'tcx, [ast::Attribute]> {
         if let Some(id) = self.map.as_local_node_id(did) {

src/librustc_mir/build/scope.rs

@@ -248,7 +248,7 @@ impl<'a,'tcx> Builder<'a,'tcx> {
                          kind: DropKind,
                          lvalue: &Lvalue<'tcx>,
                          lvalue_ty: Ty<'tcx>) {
-        if self.hir.needs_drop(lvalue_ty, span) {
+        if self.hir.needs_drop(lvalue_ty) {
             match self.scopes.iter_mut().rev().find(|s| s.extent == extent) {
                 Some(scope) => {
                     scope.drops.push((kind, span, lvalue.clone()));

src/librustc_mir/hair/cx/mod.rs

@@ -90,17 +90,8 @@ impl<'a,'tcx:'a> Cx<'a, 'tcx> {
             .collect()
     }
 
-    pub fn needs_drop(&mut self, ty: Ty<'tcx>, span: Span) -> bool {
-        if self.infcx.type_moves_by_default(ty, span) {
-            // FIXME(#21859) we should do an add'l check here to determine if
-            // any dtor will execute, but the relevant fn
-            // (`type_needs_drop`) is currently factored into
-            // `librustc_trans`, so we can't easily do so.
-            true
-        } else {
-            // if type implements Copy, cannot require drop
-            false
-        }
+    pub fn needs_drop(&mut self, ty: Ty<'tcx>) -> bool {
+        self.tcx.type_needs_drop_given_env(ty, &self.infcx.parameter_environment)
     }
 
     pub fn span_bug(&mut self, span: Span, message: &str) -> ! {

src/librustc_trans/trans/common.rs

@@ -73,45 +73,6 @@ pub fn type_is_fat_ptr<'tcx>(cx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool {
     }
 }
 
-/// If `type_needs_drop` returns true, then `ty` is definitely
-/// non-copy and *might* have a destructor attached; if it returns
-/// false, then `ty` definitely has no destructor (i.e. no drop glue).
-///
-/// (Note that this implies that if `ty` has a destructor attached,
-/// then `type_needs_drop` will definitely return `true` for `ty`.)
-pub fn type_needs_drop<'tcx>(cx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool {
-    type_needs_drop_given_env(cx, ty, &cx.empty_parameter_environment())
-}
-
-/// Core implementation of type_needs_drop, potentially making use of
-/// and/or updating caches held in the `param_env`.
-fn type_needs_drop_given_env<'a,'tcx>(cx: &ty::ctxt<'tcx>,
-                                      ty: Ty<'tcx>,
-                                      param_env: &ty::ParameterEnvironment<'a,'tcx>) -> bool {
-    // Issue #22536: We first query type_moves_by_default.  It sees a
-    // normalized version of the type, and therefore will definitely
-    // know whether the type implements Copy (and thus needs no
-    // cleanup/drop/zeroing) ...
-    let implements_copy = !ty.moves_by_default(param_env, DUMMY_SP);
-
-    if implements_copy { return false; }
-
-    // ... (issue #22536 continued) but as an optimization, still use
-    // prior logic of asking if the `needs_drop` bit is set; we need
-    // not zero non-Copy types if they have no destructor.
-
-    // FIXME(#22815): Note that calling `ty::type_contents` is a
-    // conservative heuristic; it may report that `needs_drop` is set
-    // when actual type does not actually have a destructor associated
-    // with it. But since `ty` absolutely did not have the `Copy`
-    // bound attached (see above), it is sound to treat it as having a
-    // destructor (e.g. zero its memory on move).
-
-    let contents = ty.type_contents(cx);
-    debug!("type_needs_drop ty={:?} contents={:?}", ty, contents);
-    contents.needs_drop(cx)
-}
-
 fn type_is_newtype_immediate<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
     match ty.sty {
         ty::TyStruct(def, substs) => {
@@ -518,7 +479,7 @@ impl<'a, 'tcx> FunctionContext<'a, 'tcx> {
     /// This is the same as `common::type_needs_drop`, except that it
     /// may use or update caches within this `FunctionContext`.
     pub fn type_needs_drop(&self, ty: Ty<'tcx>) -> bool {
-        type_needs_drop_given_env(self.ccx.tcx(), ty, &self.param_env)
+        self.ccx.tcx().type_needs_drop_given_env(ty, &self.param_env)
     }
 
     pub fn eh_personality(&self) -> ValueRef {

src/librustc_trans/trans/glue.rs

@@ -88,6 +88,10 @@ pub fn trans_exchange_free_ty<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
     }
 }
 
+fn type_needs_drop<'tcx>(tcx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool {
+    tcx.type_needs_drop_given_env(ty, &tcx.empty_parameter_environment())
+}
+
 pub fn get_drop_glue_type<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
                                     t: Ty<'tcx>) -> Ty<'tcx> {
     let tcx = ccx.tcx();
@@ -106,11 +110,11 @@ pub fn get_drop_glue_type<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     // returned `tcx.types.i8` does not appear unsound. The impact on
     // code quality is unknown at this time.)
 
-    if !type_needs_drop(tcx, t) {
+    if !type_needs_drop(&tcx, t) {
         return tcx.types.i8;
     }
     match t.sty {
-        ty::TyBox(typ) if !type_needs_drop(tcx, typ)
+        ty::TyBox(typ) if !type_needs_drop(&tcx, typ)
                          && type_is_sized(tcx, typ) => {
             let llty = sizing_type_of(ccx, typ);
             // `Box<ZeroSizeType>` does not allocate.