Rollup merge of #140641 - lcnr:opaque-type-storage-entries, r=compiler-errors

detect additional uses of opaques after writeback

Based on #140607. It's a lot harder to encounter in practice than I though 😅 😁 I've still added it with the expectation that somebody will encounter it at some point.

Also modifies the `EvalCtxt` to use the same impl to detect newly added opaque types.

r? ``@compiler-errors``

Author: matthiaskrgr

compiler/rustc_hir_typeck/src/lib.rs

@@ -32,6 +32,7 @@ mod gather_locals;
 mod intrinsicck;
 mod method;
 mod op;
+mod opaque_types;
 mod pat;
 mod place_op;
 mod rvalue_scopes;
@@ -245,9 +246,7 @@ fn typeck_with_inspect<'tcx>(
 
     let typeck_results = fcx.resolve_type_vars_in_body(body);
 
-    // We clone the defined opaque types during writeback in the new solver
-    // because we have to use them during normalization.
-    let _ = fcx.infcx.take_opaque_types();
+    fcx.detect_opaque_types_added_during_writeback();
 
     // Consistency check our TypeckResults instance can hold all ItemLocalIds
     // it will need to hold.

compiler/rustc_hir_typeck/src/opaque_types.rs

@@ -0,0 +1,26 @@
+use super::FnCtxt;
+impl<'tcx> FnCtxt<'_, 'tcx> {
+    /// We may in theory add further uses of an opaque after cloning the opaque
+    /// types storage during writeback when computing the defining uses.
+    ///
+    /// Silently ignoring them is dangerous and could result in ICE or even in
+    /// unsoundness, so we make sure we catch such cases here. There's currently
+    /// no known code where this actually happens, even with the new solver which
+    /// does normalize types in writeback after cloning the opaque type storage.
+    ///
+    /// FIXME(@lcnr): I believe this should be possible in theory and would like
+    /// an actual test here. After playing around with this for an hour, I wasn't
+    /// able to do anything which didn't already try to normalize the opaque before
+    /// then, either allowing compilation to succeed or causing an ambiguity error.
+    pub(super) fn detect_opaque_types_added_during_writeback(&self) {
+        let num_entries = self.checked_opaque_types_storage_entries.take().unwrap();
+        for (key, hidden_type) in
+            self.inner.borrow_mut().opaque_types().opaque_types_added_since(num_entries)
+        {
+            let opaque_type_string = self.tcx.def_path_str(key.def_id);
+            let msg = format!("unexpected cyclic definition of `{opaque_type_string}`");
+            self.dcx().span_delayed_bug(hidden_type.span, msg);
+        }
+        let _ = self.take_opaque_types();
+    }
+}

compiler/rustc_hir_typeck/src/typeck_root_ctxt.rs

@@ -1,10 +1,10 @@
-use std::cell::RefCell;
+use std::cell::{Cell, RefCell};
 use std::ops::Deref;
 
 use rustc_data_structures::unord::{UnordMap, UnordSet};
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::{self as hir, HirId, HirIdMap, LangItem};
-use rustc_infer::infer::{InferCtxt, InferOk, TyCtxtInferExt};
+use rustc_infer::infer::{InferCtxt, InferOk, OpaqueTypeStorageEntries, TyCtxtInferExt};
 use rustc_middle::span_bug;
 use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt, TypingMode};
 use rustc_span::Span;
@@ -37,6 +37,11 @@ pub(crate) struct TypeckRootCtxt<'tcx> {
 
     pub(super) fulfillment_cx: RefCell<Box<dyn TraitEngine<'tcx, FulfillmentError<'tcx>>>>,
 
+    // Used to detect opaque types uses added after we've already checked them.
+    //
+    // See [FnCtxt::detect_opaque_types_added_during_writeback] for more details.
+    pub(super) checked_opaque_types_storage_entries: Cell<Option<OpaqueTypeStorageEntries>>,
+
     /// Some additional `Sized` obligations badly affect type inference.
     /// These obligations are added in a later stage of typeck.
     /// Removing these may also cause additional complications, see #101066.
@@ -85,12 +90,14 @@ impl<'tcx> TypeckRootCtxt<'tcx> {
         let infcx =
             tcx.infer_ctxt().ignoring_regions().build(TypingMode::typeck_for_body(tcx, def_id));
         let typeck_results = RefCell::new(ty::TypeckResults::new(hir_owner));
+        let fulfillment_cx = RefCell::new(<dyn TraitEngine<'_, _>>::new(&infcx));
 
         TypeckRootCtxt {
-            typeck_results,
-            fulfillment_cx: RefCell::new(<dyn TraitEngine<'_, _>>::new(&infcx)),
             infcx,
+            typeck_results,
             locals: RefCell::new(Default::default()),
+            fulfillment_cx,
+            checked_opaque_types_storage_entries: Cell::new(None),
             deferred_sized_obligations: RefCell::new(Vec::new()),
             deferred_call_resolutions: RefCell::new(Default::default()),
             deferred_cast_checks: RefCell::new(Vec::new()),

compiler/rustc_hir_typeck/src/writeback.rs

@@ -535,13 +535,10 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
         let tcx = self.tcx();
         // We clone the opaques instead of stealing them here as they are still used for
         // normalization in the next generation trait solver.
-        //
-        // FIXME(-Znext-solver): Opaque types defined after this would simply get dropped
-        // at the end of typeck. While this seems unlikely to happen in practice this
-        // should still get fixed. Either by preventing writeback from defining new opaque
-        // types or by using this function at the end of writeback and running it as a
-        // fixpoint.
         let opaque_types = self.fcx.infcx.clone_opaque_types();
+        let num_entries = self.fcx.inner.borrow_mut().opaque_types().num_entries();
+        let prev = self.fcx.checked_opaque_types_storage_entries.replace(Some(num_entries));
+        debug_assert_eq!(prev, None);
         for (opaque_type_key, hidden_type) in opaque_types {
             let hidden_type = self.resolve(hidden_type, &hidden_type.span);
             let opaque_type_key = self.resolve(opaque_type_key, &hidden_type.span);

compiler/rustc_infer/src/infer/context.rs

@@ -6,7 +6,10 @@ use rustc_middle::ty::relate::combine::PredicateEmittingRelation;
 use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable};
 use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span};
 
-use super::{BoundRegionConversionTime, InferCtxt, RegionVariableOrigin, SubregionOrigin};
+use super::{
+    BoundRegionConversionTime, InferCtxt, OpaqueTypeStorageEntries, RegionVariableOrigin,
+    SubregionOrigin,
+};
 
 impl<'tcx> rustc_type_ir::InferCtxtLike for InferCtxt<'tcx> {
     type Interner = TyCtxt<'tcx>;
@@ -213,4 +216,58 @@ impl<'tcx> rustc_type_ir::InferCtxtLike for InferCtxt<'tcx> {
     fn register_ty_outlives(&self, ty: Ty<'tcx>, r: ty::Region<'tcx>, span: Span) {
         self.register_region_obligation_with_cause(ty, r, &ObligationCause::dummy_with_span(span));
     }
+
+    type OpaqueTypeStorageEntries = OpaqueTypeStorageEntries;
+    fn opaque_types_storage_num_entries(&self) -> OpaqueTypeStorageEntries {
+        self.inner.borrow_mut().opaque_types().num_entries()
+    }
+    fn clone_opaque_types_lookup_table(&self) -> Vec<(ty::OpaqueTypeKey<'tcx>, Ty<'tcx>)> {
+        self.inner.borrow_mut().opaque_types().iter_lookup_table().map(|(k, h)| (k, h.ty)).collect()
+    }
+    fn clone_duplicate_opaque_types(&self) -> Vec<(ty::OpaqueTypeKey<'tcx>, Ty<'tcx>)> {
+        self.inner
+            .borrow_mut()
+            .opaque_types()
+            .iter_duplicate_entries()
+            .map(|(k, h)| (k, h.ty))
+            .collect()
+    }
+    fn clone_opaque_types_added_since(
+        &self,
+        prev_entries: OpaqueTypeStorageEntries,
+    ) -> Vec<(ty::OpaqueTypeKey<'tcx>, Ty<'tcx>)> {
+        self.inner
+            .borrow_mut()
+            .opaque_types()
+            .opaque_types_added_since(prev_entries)
+            .map(|(k, h)| (k, h.ty))
+            .collect()
+    }
+
+    fn register_hidden_type_in_storage(
+        &self,
+        opaque_type_key: ty::OpaqueTypeKey<'tcx>,
+        hidden_ty: Ty<'tcx>,
+        span: Span,
+    ) -> Option<Ty<'tcx>> {
+        self.register_hidden_type_in_storage(
+            opaque_type_key,
+            ty::OpaqueHiddenType { span, ty: hidden_ty },
+        )
+    }
+    fn add_duplicate_opaque_type(
+        &self,
+        opaque_type_key: ty::OpaqueTypeKey<'tcx>,
+        hidden_ty: Ty<'tcx>,
+        span: Span,
+    ) {
+        self.inner
+            .borrow_mut()
+            .opaque_types()
+            .add_duplicate(opaque_type_key, ty::OpaqueHiddenType { span, ty: hidden_ty })
+    }
+
+    fn reset_opaque_types(&self) {
+        let _ = self.take_opaque_types();
+    }
 }

compiler/rustc_infer/src/infer/mod.rs

@@ -9,7 +9,7 @@ use free_regions::RegionRelations;
 pub use freshen::TypeFreshener;
 use lexical_region_resolve::LexicalRegionResolutions;
 pub use lexical_region_resolve::RegionResolutionError;
-use opaque_types::OpaqueTypeStorage;
+pub use opaque_types::{OpaqueTypeStorage, OpaqueTypeStorageEntries, OpaqueTypeTable};
 use region_constraints::{
     GenericKind, RegionConstraintCollector, RegionConstraintStorage, VarInfos, VerifyBound,
 };

compiler/rustc_infer/src/infer/opaque_types/mod.rs

@@ -18,7 +18,7 @@ use crate::traits::{self, Obligation, PredicateObligations};
 
 mod table;
 
-pub(crate) use table::{OpaqueTypeStorage, OpaqueTypeTable};
+pub use table::{OpaqueTypeStorage, OpaqueTypeStorageEntries, OpaqueTypeTable};
 
 impl<'tcx> InferCtxt<'tcx> {
     /// This is a backwards compatibility hack to prevent breaking changes from

compiler/rustc_infer/src/infer/opaque_types/table.rs

@@ -14,6 +14,16 @@ pub struct OpaqueTypeStorage<'tcx> {
     duplicate_entries: Vec<(OpaqueTypeKey<'tcx>, OpaqueHiddenType<'tcx>)>,
 }
 
+/// The number of entries in the opaque type storage at a given point.
+///
+/// Used to check that we haven't added any new opaque types after checking
+/// the opaque types currently in the storage.
+#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)]
+pub struct OpaqueTypeStorageEntries {
+    opaque_types: usize,
+    duplicate_entries: usize,
+}
+
 impl<'tcx> OpaqueTypeStorage<'tcx> {
     #[instrument(level = "debug")]
     pub(crate) fn remove(
@@ -49,6 +59,24 @@ impl<'tcx> OpaqueTypeStorage<'tcx> {
         std::mem::take(opaque_types).into_iter().chain(std::mem::take(duplicate_entries))
     }
 
+    pub fn num_entries(&self) -> OpaqueTypeStorageEntries {
+        OpaqueTypeStorageEntries {
+            opaque_types: self.opaque_types.len(),
+            duplicate_entries: self.duplicate_entries.len(),
+        }
+    }
+
+    pub fn opaque_types_added_since(
+        &self,
+        prev_entries: OpaqueTypeStorageEntries,
+    ) -> impl Iterator<Item = (OpaqueTypeKey<'tcx>, OpaqueHiddenType<'tcx>)> {
+        self.opaque_types
+            .iter()
+            .skip(prev_entries.opaque_types)
+            .map(|(k, v)| (*k, *v))
+            .chain(self.duplicate_entries.iter().skip(prev_entries.duplicate_entries).copied())
+    }
+
     /// Only returns the opaque types from the lookup table. These are used
     /// when normalizing opaque types and have a unique key.
     ///

compiler/rustc_next_trait_solver/src/delegate.rs

@@ -39,13 +39,6 @@ pub trait SolverDelegate: Deref<Target = Self::Infcx> + Sized {
         term: <Self::Interner as Interner>::Term,
     ) -> Option<Vec<Goal<Self::Interner, <Self::Interner as Interner>::Predicate>>>;
 
-    fn clone_opaque_types_lookup_table(
-        &self,
-    ) -> Vec<(ty::OpaqueTypeKey<Self::Interner>, <Self::Interner as Interner>::Ty)>;
-    fn clone_duplicate_opaque_types(
-        &self,
-    ) -> Vec<(ty::OpaqueTypeKey<Self::Interner>, <Self::Interner as Interner>::Ty)>;
-
     fn make_deduplicated_outlives_constraints(
         &self,
     ) -> Vec<ty::OutlivesPredicate<Self::Interner, <Self::Interner as Interner>::GenericArg>>;
@@ -64,20 +57,6 @@ pub trait SolverDelegate: Deref<Target = Self::Infcx> + Sized {
         span: <Self::Interner as Interner>::Span,
         universe_map: impl Fn(ty::UniverseIndex) -> ty::UniverseIndex,
     ) -> <Self::Interner as Interner>::GenericArg;
-
-    fn register_hidden_type_in_storage(
-        &self,
-        opaque_type_key: ty::OpaqueTypeKey<Self::Interner>,
-        hidden_ty: <Self::Interner as Interner>::Ty,
-        span: <Self::Interner as Interner>::Span,
-    ) -> Option<<Self::Interner as Interner>::Ty>;
-    fn add_duplicate_opaque_type(
-        &self,
-        opaque_type_key: ty::OpaqueTypeKey<Self::Interner>,
-        hidden_ty: <Self::Interner as Interner>::Ty,
-        span: <Self::Interner as Interner>::Span,
-    );
-
     fn add_item_bounds_for_hidden_type(
         &self,
         def_id: <Self::Interner as Interner>::DefId,
@@ -86,7 +65,6 @@ pub trait SolverDelegate: Deref<Target = Self::Infcx> + Sized {
         hidden_ty: <Self::Interner as Interner>::Ty,
         goals: &mut Vec<Goal<Self::Interner, <Self::Interner as Interner>::Predicate>>,
     );
-    fn reset_opaque_types(&self);
 
     fn fetch_eligible_assoc_item(
         &self,

compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs

@@ -250,13 +250,7 @@ where
         // to the `var_values`.
         let opaque_types = self
             .delegate
-            .clone_opaque_types_lookup_table()
-            .into_iter()
-            .filter(|(a, _)| {
-                self.predefined_opaques_in_body.opaque_types.iter().all(|(pa, _)| pa != a)
-            })
-            .chain(self.delegate.clone_duplicate_opaque_types())
-            .collect();
+            .clone_opaque_types_added_since(self.initial_opaque_types_storage_num_entries);
 
         ExternalConstraintsData { region_constraints, opaque_types, normalization_nested_goals }
     }

compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs

@@ -23,8 +23,7 @@ use crate::solve::inspect::{self, ProofTreeBuilder};
 use crate::solve::search_graph::SearchGraph;
 use crate::solve::{
     CanonicalInput, Certainty, FIXPOINT_STEP_LIMIT, Goal, GoalEvaluationKind, GoalSource,
-    HasChanged, NestedNormalizationGoals, NoSolution, PredefinedOpaquesData, QueryInput,
-    QueryResult,
+    HasChanged, NestedNormalizationGoals, NoSolution, QueryInput, QueryResult,
 };
 
 pub(super) mod canonical;
@@ -99,8 +98,6 @@ where
     current_goal_kind: CurrentGoalKind,
     pub(super) var_values: CanonicalVarValues<I>,
 
-    predefined_opaques_in_body: I::PredefinedOpaques,
-
     /// The highest universe index nameable by the caller.
     ///
     /// When we enter a new binder inside of the query we create new universes
@@ -111,6 +108,10 @@ where
     /// if we have a coinductive cycle and because that's the only way we can return
     /// new placeholders to the caller.
     pub(super) max_input_universe: ty::UniverseIndex,
+    /// The opaque types from the canonical input. We only need to return opaque types
+    /// which have been added to the storage while evaluating this goal.
+    pub(super) initial_opaque_types_storage_num_entries:
+        <D::Infcx as InferCtxtLike>::OpaqueTypeStorageEntries,
 
     pub(super) search_graph: &'a mut SearchGraph<D>,
 
@@ -305,10 +306,8 @@ where
 
             // Only relevant when canonicalizing the response,
             // which we don't do within this evaluation context.
-            predefined_opaques_in_body: delegate
-                .cx()
-                .mk_predefined_opaques_in_body(PredefinedOpaquesData::default()),
             max_input_universe: ty::UniverseIndex::ROOT,
+            initial_opaque_types_storage_num_entries: Default::default(),
             variables: Default::default(),
             var_values: CanonicalVarValues::dummy(),
             current_goal_kind: CurrentGoalKind::Misc,
@@ -342,25 +341,10 @@ where
         canonical_goal_evaluation: &mut ProofTreeBuilder<D>,
         f: impl FnOnce(&mut EvalCtxt<'_, D>, Goal<I, I::Predicate>) -> R,
     ) -> R {
-        let (ref delegate, input, var_values) =
-            SolverDelegate::build_with_canonical(cx, &canonical_input);
-
-        let mut ecx = EvalCtxt {
-            delegate,
-            variables: canonical_input.canonical.variables,
-            var_values,
-            current_goal_kind: CurrentGoalKind::from_query_input(cx, input),
-            predefined_opaques_in_body: input.predefined_opaques_in_body,
-            max_input_universe: canonical_input.canonical.max_universe,
-            search_graph,
-            nested_goals: Default::default(),
-            origin_span: I::Span::dummy(),
-            tainted: Ok(()),
-            inspect: canonical_goal_evaluation.new_goal_evaluation_step(var_values),
-        };
+        let (ref delegate, input, var_values) = D::build_with_canonical(cx, &canonical_input);
 
         for &(key, ty) in &input.predefined_opaques_in_body.opaque_types {
-            let prev = ecx.delegate.register_hidden_type_in_storage(key, ty, ecx.origin_span);
+            let prev = delegate.register_hidden_type_in_storage(key, ty, I::Span::dummy());
             // It may be possible that two entries in the opaque type storage end up
             // with the same key after resolving contained inference variables.
             //
@@ -373,13 +357,24 @@ where
             // the canonical input. This is more annoying to implement and may cause a
             // perf regression, so we do it inside of the query for now.
             if let Some(prev) = prev {
-                debug!(?key, ?ty, ?prev, "ignore duplicate in `opaque_type_storage`");
+                debug!(?key, ?ty, ?prev, "ignore duplicate in `opaque_types_storage`");
             }
         }
 
-        if !ecx.nested_goals.is_empty() {
-            panic!("prepopulating opaque types shouldn't add goals: {:?}", ecx.nested_goals);
-        }
+        let initial_opaque_types_storage_num_entries = delegate.opaque_types_storage_num_entries();
+        let mut ecx = EvalCtxt {
+            delegate,
+            variables: canonical_input.canonical.variables,
+            var_values,
+            current_goal_kind: CurrentGoalKind::from_query_input(cx, input),
+            max_input_universe: canonical_input.canonical.max_universe,
+            initial_opaque_types_storage_num_entries,
+            search_graph,
+            nested_goals: Default::default(),
+            origin_span: I::Span::dummy(),
+            tainted: Ok(()),
+            inspect: canonical_goal_evaluation.new_goal_evaluation_step(var_values),
+        };
 
         let result = f(&mut ecx, input.goal);
         ecx.inspect.probe_final_state(ecx.delegate, ecx.max_input_universe);