More comments, reverse polarity of structural check

Author: compiler-errors

compiler/rustc_const_eval/src/check_consts/qualifs.rs

@@ -62,9 +62,12 @@ pub trait Qualif {
     /// It also determines the `Qualif`s for primitive types.
     fn in_any_value_of_ty<'tcx>(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool;
 
-    /// Returns `true` if the `Qualif` is not structural, i.e. that we should not recurse
-    /// into the operand.
-    fn is_non_structural<'tcx>(cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool;
+    /// Returns `true` if the `Qualif` is structural in its operand, i.e. that we may
+    /// recurse into the operand.
+    ///
+    /// If this returns false, `in_any_value_of_ty` will be invoked to determine the
+    /// final qualif for this operand.
+    fn is_structural_in_operand<'tcx>(cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool;
 
     /// Returns `true` if this `Qualif` behaves sructurally for pointers and references:
     /// the pointer/reference qualifies if and only if the pointee qualifies.
@@ -123,18 +126,17 @@ impl Qualif for HasMutInterior {
         !errors.is_empty()
     }
 
-    fn is_non_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool {
+    fn is_structural_in_operand<'tcx>(_cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool {
         // Exactly one type, `UnsafeCell`, has the `HasMutInterior` qualif inherently.
         // It arises structurally for all other types.
-        adt.is_unsafe_cell()
+        !adt.is_unsafe_cell()
     }
 
     fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
         false
     }
 }
 
-// FIXME(const_trait_impl): Get rid of this!
 /// Constant containing an ADT that implements `Drop`.
 /// This must be ruled out because implicit promotion would remove side-effects
 /// that occur as part of dropping that value. N.B., the implicit promotion has
@@ -154,8 +156,8 @@ impl Qualif for NeedsDrop {
         ty.needs_drop(cx.tcx, cx.param_env)
     }
 
-    fn is_non_structural<'tcx>(cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool {
-        adt.has_dtor(cx.tcx)
+    fn is_structural_in_operand<'tcx>(cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool {
+        !adt.has_dtor(cx.tcx)
     }
 
     fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
@@ -183,6 +185,11 @@ impl Qualif for NeedsNonConstDrop {
             return false;
         }
 
+        // We check that the type is `~const Destruct` since that will verify that
+        // the type is both `~const Drop` (if a drop impl exists for the adt), *and*
+        // that the components of this type are also `~const Destruct`. This
+        // amounts to verifying that there are no values in this ADT that may have
+        // a non-const drop.
         if cx.tcx.features().const_trait_impl() {
             let destruct_def_id = cx.tcx.require_lang_item(LangItem::Destruct, Some(cx.body.span));
             let infcx = cx.tcx.infer_ctxt().build(TypingMode::from_param_env(cx.param_env));
@@ -204,11 +211,18 @@ impl Qualif for NeedsNonConstDrop {
         }
     }
 
-    fn is_non_structural<'tcx>(cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool {
-        // Even a `const` dtor may have `~const` bounds that may need to
-        // be satisfied, so this becomes non-structural as soon as the
-        // ADT gets a destructor at all.
-        adt.has_dtor(cx.tcx)
+    fn is_structural_in_operand<'tcx>(cx: &ConstCx<'_, 'tcx>, adt: AdtDef<'tcx>) -> bool {
+        // As soon as an ADT has a destructor, then the drop becomes non-structural
+        // in its value since:
+        // 1. The destructor may have `~const` bounds that need to be satisfied on
+        //   top of checking that the components of a specific operand are const-drop.
+        //   While this could be instead satisfied by checking that the `~const Drop`
+        //   impl holds (i.e. replicating part of the `in_any_value_of_ty` logic above),
+        //   even in this case, we have another problem, which is,
+        // 2. The destructor may *modify* the operand being dropped, so even if we
+        //   did recurse on the components of the operand, we may not be even dropping
+        //   the same values that were present before the custom destructor was invoked.
+        !adt.has_dtor(cx.tcx)
     }
 
     fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
@@ -266,7 +280,11 @@ where
             // qualified.
             if let AggregateKind::Adt(adt_did, ..) = **kind {
                 let def = cx.tcx.adt_def(adt_did);
-                if def.is_union() || Q::is_non_structural(cx, def) {
+                // Don't do any value-based reasoning for unions.
+                // Also, if the ADT is not structural in its operand,
+                // then we cannot recurse on its components. Instead,
+                // we fall back to checking the qualif for *any* value.
+                if def.is_union() || !Q::is_structural_in_operand(cx, def) {
                     return Q::in_any_value_of_ty(cx, rvalue.ty(cx.body, cx.tcx));
                 }
             }