Merge #54

54: Prevent collections from unnecessarily finalizing elems r=ltratt a=jacob-hughes

Collection types such as Vec<T> have drop methods which are
only necessary if T requires dropping. This can be managed partially by
implementing `NoFinalize` on `Vec<T>`:

    unsafe impl<T: NoFinalize> NoFinalize for Vec<T> {}

However, this will return true for needs_finalize if `T: !Drop` but also
doesn't implement `NoFinalize`.

To get around this, we introduce a trait which is expected only to be
used in the standard library called `OnlyFinalizeComponents`.
Implementing this trait on a type indicates to the collector that it can
ignore that type's drop method, but it must still recurse through the
type's components to see if any of them need dropping.


Co-authored-by: Jake Hughes <jh@jakehughes.uk>

compiler/rustc_hir/src/lang_items.rs

@@ -215,6 +215,7 @@ language_item_table! {
 
     Drop,                    sym::drop,                drop_trait,                 Target::Trait,          GenericRequirement::None;
     NoFinalize,              sym::no_finalize,         no_finalize_trait,          Target::Trait,          GenericRequirement::None;
+    FlzComps,                sym::flz_comps,           flz_comps_trait,            Target::Trait,          GenericRequirement::None;
 
     NoTrace,                 sym::notrace,             no_trace_trait,             Target::Trait,          GenericRequirement::None;
     Conservative,            sym::conservative,        conservative_trait,         Target::Trait,          GenericRequirement::None;

compiler/rustc_middle/src/query/mod.rs

@@ -1176,6 +1176,10 @@ rustc_queries! {
     query is_collectable_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
         desc { "computing whether `{}` is `Collectable`", env.value }
     }
+    /// Query backing `TyS::must_check_component_tys_for_finalizer`.
+    query must_check_component_tys_for_finalizer_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
+        desc { "computing whether `{}` contains types which might need finalizing", env.value }
+    }
     /// Query backing `TyS::needs_drop`.
     query needs_drop_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
         desc { "computing whether `{}` needs drop", env.value }

compiler/rustc_middle/src/ty/util.rs

@@ -749,6 +749,14 @@ impl<'tcx> ty::TyS<'tcx> {
         tcx_at.is_collectable_raw(param_env.and(self))
     }
 
+    pub fn must_check_component_tys_for_finalizer(
+        &'tcx self,
+        tcx_at: TyCtxtAt<'tcx>,
+        param_env: ty::ParamEnv<'tcx>,
+    ) -> bool {
+        tcx_at.must_check_component_tys_for_finalizer_raw(param_env.and(self))
+    }
+
     /// Fast path helper for testing if a type is `Freeze`.
     ///
     /// Returning true means the type is known to be `Freeze`. Returning

compiler/rustc_span/src/symbol.rs

@@ -655,6 +655,7 @@ symbols! {
         float_to_int_unchecked,
         floorf32,
         floorf64,
+        flz_comps,
         fmaf32,
         fmaf64,
         fmt,

compiler/rustc_ty_utils/src/common_traits.rs

@@ -34,6 +34,13 @@ fn is_collectable_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'
     is_item_raw(tcx, query, LangItem::Collectable)
 }
 
+fn must_check_component_tys_for_finalizer_raw<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>,
+) -> bool {
+    is_item_raw(tcx, query, LangItem::FlzComps)
+}
+
 fn is_unpin_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
     is_item_raw(tcx, query, LangItem::Unpin)
 }
@@ -65,6 +72,7 @@ pub(crate) fn provide(providers: &mut ty::query::Providers) {
         is_no_trace_raw,
         is_no_finalize_raw,
         is_collectable_raw,
+        must_check_component_tys_for_finalizer_raw,
         is_unpin_raw,
         ..*providers
     };

compiler/rustc_ty_utils/src/needs_finalizer.rs

@@ -14,8 +14,9 @@ fn needs_finalizer_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<
     if !tcx.sess.opts.gc_optimize_finalizers {
         return tcx.needs_drop_raw(query);
     }
-    let finalizer_fields =
-        move |adt_def: &ty::AdtDef| tcx.adt_finalizer_tys(adt_def.did).map(|tys| tys.iter());
+    let finalizer_fields = move |adt_def: &ty::AdtDef, _must_recurse: bool| {
+        tcx.adt_finalizer_tys(adt_def.did).map(|tys| tys.iter())
+    };
     let res =
         NeedsDropTypes::new(tcx, query.param_env, query.value, finalizer_fields).next().is_some();
     debug!("needs_finalize_raw({:?}) = {:?}", query, res);
@@ -59,7 +60,7 @@ impl<'tcx, F> NeedsDropTypes<'tcx, F> {
 
 impl<'tcx, F, I> Iterator for NeedsDropTypes<'tcx, F>
 where
-    F: Fn(&ty::AdtDef) -> NeedsDropResult<I>,
+    F: Fn(&ty::AdtDef, bool) -> NeedsDropResult<I>,
     I: Iterator<Item = Ty<'tcx>>,
 {
     type Item = NeedsDropResult<Ty<'tcx>>;
@@ -76,10 +77,6 @@ where
                 return Some(Err(AlwaysRequiresDrop));
             }
 
-            if ty.is_no_finalize_modulo_regions(tcx.at(DUMMY_SP), self.param_env) {
-                return None;
-            }
-
             let components = match needs_drop_components(ty, &tcx.data_layout) {
                 Err(e) => return Some(Err(e)),
                 Ok(components) => components,
@@ -94,7 +91,14 @@ where
 
             for component in components {
                 match *component.kind() {
-                    _ if component.is_copy_modulo_regions(tcx.at(DUMMY_SP), self.param_env) => (),
+                    _ if component.is_copy_modulo_regions(tcx.at(DUMMY_SP), self.param_env)
+                        && !component.must_check_component_tys_for_finalizer(
+                            tcx.at(DUMMY_SP),
+                            self.param_env,
+                        ) =>
+                    {
+                        ()
+                    }
 
                     ty::Closure(_, substs) => {
                         queue_type(self, substs.as_closure().tupled_upvars_ty());
@@ -125,7 +129,22 @@ where
                     // `ManuallyDrop`. If it's a struct or enum without a `Drop`
                     // impl then check whether the field types need `Drop`.
                     ty::Adt(adt_def, substs) => {
-                        let tys = match (self.adt_components)(adt_def) {
+                        if ty.is_no_finalize_modulo_regions(tcx.at(DUMMY_SP), self.param_env) {
+                            continue;
+                        }
+
+                        let must_recurse = component.must_check_component_tys_for_finalizer(
+                            tcx.at(DUMMY_SP),
+                            self.param_env,
+                        );
+
+                        if must_recurse {
+                            for subst_ty in substs.types() {
+                                queue_type(self, subst_ty);
+                            }
+                        }
+
+                        let tys = match (self.adt_components)(adt_def, must_recurse) {
                             Err(e) => return Some(Err(e)),
                             Ok(tys) => tys,
                         };
@@ -166,11 +185,11 @@ fn adt_finalizer_tys(
     def_id: DefId,
 ) -> Result<&ty::List<Ty<'_>>, AlwaysRequiresDrop> {
     let adt_has_dtor = |adt_def: &ty::AdtDef| adt_def.destructor(tcx).is_some();
-    let adt_components = move |adt_def: &ty::AdtDef| {
+    let adt_components = move |adt_def: &ty::AdtDef, must_recurse: bool| {
         if adt_def.is_manually_drop() {
             debug!("dt_finalizer_tys: `{:?}` is manually drop", adt_def);
             return Ok(Vec::new().into_iter());
-        } else if adt_has_dtor(adt_def) {
+        } else if adt_has_dtor(adt_def) && !must_recurse {
             debug!("adt_finalizer_tys: `{:?}` implements `Drop`", adt_def);
             // Special case tuples
             return Err(AlwaysRequiresDrop);

library/alloc/src/boxed.rs

@@ -139,7 +139,7 @@ use core::convert::{From, TryFrom};
 use core::fmt;
 use core::future::Future;
 use core::gc::Collectable;
-use core::gc::NoFinalize;
+use core::gc::{NoFinalize, OnlyFinalizeComponents};
 use core::hash::{Hash, Hasher};
 #[cfg(not(no_global_oom_handling))]
 use core::iter::FromIterator;
@@ -2014,7 +2014,13 @@ unsafe impl<T: NoFinalize> NoFinalize for Box<T> {}
 unsafe impl<T: NoFinalize, A: Allocator> NoFinalize for Box<T, A> {}
 
 #[unstable(feature = "gc", issue = "none")]
-unsafe impl<T, A: Allocator> Collectable for Box<T, A> {
+unsafe impl<T: ?Sized> OnlyFinalizeComponents for Box<T> {}
+
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T: ?Sized, A: Allocator> OnlyFinalizeComponents for Box<T, A> {}
+
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T: ?Sized, A: Allocator> Collectable for Box<T, A> {
     unsafe fn set_collectable(&self) {
         unsafe {
             crate::alloc::set_managed(self.0.as_ptr() as *mut u8);

library/alloc/src/raw_vec.rs

@@ -2,7 +2,7 @@
 
 use core::alloc::LayoutError;
 use core::cmp;
-use core::gc::NoFinalize;
+use core::gc::{NoFinalize, OnlyFinalizeComponents};
 use core::intrinsics;
 use core::mem::{self, ManuallyDrop, MaybeUninit};
 use core::ops::Drop;
@@ -482,6 +482,9 @@ unsafe impl<#[may_dangle] T, A: Allocator> Drop for RawVec<T, A> {
     }
 }
 
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T, A: Allocator> OnlyFinalizeComponents for RawVec<T, A> {}
+
 // Central function for reserve error handling.
 #[cfg(not(no_global_oom_handling))]
 #[inline]

library/alloc/src/vec/mod.rs

@@ -58,7 +58,7 @@ use core::cmp;
 use core::cmp::Ordering;
 use core::convert::TryFrom;
 use core::fmt;
-use core::gc::NoFinalize;
+use core::gc::{NoFinalize, OnlyFinalizeComponents};
 use core::hash::{Hash, Hasher};
 use core::intrinsics::{arith_offset, assume};
 use core::iter;
@@ -2806,6 +2806,12 @@ unsafe impl<T: NoFinalize, A: Allocator> NoFinalize for Vec<T, A> {}
 #[unstable(feature = "gc", issue = "none")]
 unsafe impl<T: NoFinalize> NoFinalize for Vec<T> {}
 
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T> OnlyFinalizeComponents for Vec<T> {}
+
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T, A: Allocator> OnlyFinalizeComponents for Vec<T, A> {}
+
 #[stable(feature = "rust1", since = "1.0.0")]
 unsafe impl<#[may_dangle] T, A: Allocator> Drop for Vec<T, A> {
     fn drop(&mut self) {

library/core/src/cell.rs

@@ -194,6 +194,7 @@
 
 use crate::cmp::Ordering;
 use crate::fmt::{self, Debug, Display};
+use crate::gc::NoFinalize;
 use crate::marker::Unsize;
 use crate::mem;
 use crate::ops::{CoerceUnsized, Deref, DerefMut};
@@ -240,6 +241,9 @@ pub struct Cell<T: ?Sized> {
 #[stable(feature = "rust1", since = "1.0.0")]
 unsafe impl<T: ?Sized> Send for Cell<T> where T: Send {}
 
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T: ?Sized> NoFinalize for Cell<T> where T: NoFinalize {}
+
 // Note that this negative impl isn't strictly necessary for correctness,
 // as `Cell` wraps `UnsafeCell`, which is itself `!Sync`.
 // However, given how important `Cell`'s `!Sync`-ness is,

library/core/src/gc.rs

@@ -26,6 +26,16 @@ pub trait Conservative {}
 #[cfg_attr(not(bootstrap), lang = "no_finalize")]
 pub unsafe trait NoFinalize {}
 
+#[cfg_attr(not(bootstrap), lang = "flz_comps")]
+/// Prevents a type from being finalized by GC if none of the component types
+/// need dropping. This can be thought of as a weaker version of `NoFinalize`.
+///
+/// # Safety
+///
+/// Unsafe because this should be used with care. Preventing drop from
+/// running can lead to surprising behaviour.
+pub unsafe trait OnlyFinalizeComponents {}
+
 #[unstable(feature = "gc", issue = "none")]
 #[cfg_attr(not(bootstrap), lang = "notrace")]
 pub auto trait NoTrace {}

library/core/src/option.rs

@@ -500,6 +500,7 @@
 
 #![stable(feature = "rust1", since = "1.0.0")]
 
+use crate::gc::NoFinalize;
 use crate::iter::{FromIterator, FusedIterator, TrustedLen};
 use crate::panicking::{panic, panic_str};
 use crate::pin::Pin;
@@ -2285,3 +2286,6 @@ impl<T> Option<Option<T>> {
         }
     }
 }
+
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T: NoFinalize> NoFinalize for Option<T> {}

library/core/src/ptr/unique.rs

@@ -5,6 +5,7 @@ use crate::mem;
 use crate::ops::{CoerceUnsized, DispatchFromDyn};
 
 use crate::gc::NoTrace;
+use crate::gc::OnlyFinalizeComponents;
 
 /// A wrapper around a raw non-null `*mut T` that indicates that the possessor
 /// of this wrapper owns the referent. Useful for building abstractions like
@@ -52,6 +53,9 @@ pub struct Unique<T: ?Sized> {
 #[unstable(feature = "ptr_internals", issue = "none")]
 unsafe impl<T: Send + ?Sized> Send for Unique<T> {}
 
+#[unstable(feature = "gc", issue = "none")]
+unsafe impl<T: ?Sized> OnlyFinalizeComponents for Unique<T> {}
+
 /// `Unique` pointers are `Sync` if `T` is `Sync` because the data they
 /// reference is unaliased. Note that this aliasing invariant is
 /// unenforced by the type system; the abstraction using the

src/test/ui/gc/needs_finalize.rs

@@ -21,6 +21,9 @@ struct FinalizedContainer<T>(T);
 struct MaybeFinalize<T>(T);
 struct ExplicitNoFinalize;
 
+// This struct doesn't need finalizing, but it's not annoted as such.
+struct NonAnnotated(usize);
+
 unsafe impl NoFinalize for ExplicitNoFinalize {}
 unsafe impl NoFinalize for HasDropNoFinalize {}
 
@@ -63,6 +66,9 @@ static OUTER_NEEDS_FINALIZING: bool = mem::needs_finalizer::<FinalizedContainer<
 static STATIC_MAYBE_FINALIZE_NO_COMPONENTS: bool = mem::needs_finalizer::<MaybeFinalize<ExplicitNoFinalize>>();
 static STATIC_MAYBE_FINALIZE_DROP_COMPONENTS: bool = mem::needs_finalizer::<MaybeFinalize<HasDrop>>();
 
+static VEC_COLLECTABLE_NO_DROP_ELEMENT: bool = mem::needs_finalizer::<Vec<NonAnnotated>>();
+static BOX_COLLECTABLE_NO_DROP_ELEMENT: bool = mem::needs_finalizer::<Box<NonAnnotated>>();
+
 fn main() {
     assert!(!CONST_U8);
     assert!(!CONST_STRING);
@@ -98,4 +104,7 @@ fn main() {
 
     assert!(!STATIC_MAYBE_FINALIZE_NO_COMPONENTS);
     assert!(STATIC_MAYBE_FINALIZE_DROP_COMPONENTS);
+
+    assert!(!VEC_COLLECTABLE_NO_DROP_ELEMENT);
+    assert!(!BOX_COLLECTABLE_NO_DROP_ELEMENT);
 }