rework the leak_check to take the outer_universe

clean up coherence to not rely on probes anymore

Author: lcnr

compiler/rustc_hir_typeck/src/coercion.rs

@@ -808,6 +808,8 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
         G: FnOnce(Ty<'tcx>) -> Vec<Adjustment<'tcx>>,
     {
         self.commit_if_ok(|snapshot| {
+            let outer_universe = self.infcx.universe();
+
             let result = if let ty::FnPtr(fn_ty_b) = b.kind()
                 && let (hir::Unsafety::Normal, hir::Unsafety::Unsafe) =
                     (fn_ty_a.unsafety(), fn_ty_b.unsafety())
@@ -824,7 +826,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
             // want the coerced type to be the actual supertype of these two,
             // but for now, we want to just error to ensure we don't lock
             // ourselves into a specific behavior with NLL.
-            self.leak_check(snapshot)?;
+            self.leak_check(outer_universe, Some(snapshot))?;
 
             result
         })

compiler/rustc_infer/src/infer/at.rs

@@ -70,8 +70,8 @@ impl<'tcx> InferCtxt<'tcx> {
             tcx: self.tcx,
             defining_use_anchor: self.defining_use_anchor,
             considering_regions: self.considering_regions,
+            skip_leak_check: self.skip_leak_check,
             inner: self.inner.clone(),
-            skip_leak_check: self.skip_leak_check.clone(),
             lexical_region_resolutions: self.lexical_region_resolutions.clone(),
             selection_cache: self.selection_cache.clone(),
             evaluation_cache: self.evaluation_cache.clone(),

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

@@ -105,24 +105,31 @@ impl<'tcx> InferCtxt<'tcx> {
         self.tcx.replace_bound_vars_uncached(binder, delegate)
     }
 
-    /// See [RegionConstraintCollector::leak_check][1].
+    /// See [RegionConstraintCollector::leak_check][1]. We only check placeholder
+    /// leaking into `outer_universe`, i.e. placeholders which cannot be named by that
+    /// universe.
     ///
     /// [1]: crate::infer::region_constraints::RegionConstraintCollector::leak_check
-    pub fn leak_check(&self, snapshot: &CombinedSnapshot<'tcx>) -> RelateResult<'tcx, ()> {
+    pub fn leak_check(
+        &self,
+        outer_universe: ty::UniverseIndex,
+        only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>,
+    ) -> RelateResult<'tcx, ()> {
         // If the user gave `-Zno-leak-check`, or we have been
         // configured to skip the leak check, then skip the leak check
         // completely. The leak check is deprecated. Any legitimate
         // subtyping errors that it would have caught will now be
         // caught later on, during region checking. However, we
         // continue to use it for a transition period.
-        if self.tcx.sess.opts.unstable_opts.no_leak_check || self.skip_leak_check.get() {
+        if self.tcx.sess.opts.unstable_opts.no_leak_check || self.skip_leak_check {
             return Ok(());
         }
 
         self.inner.borrow_mut().unwrap_region_constraints().leak_check(
             self.tcx,
+            outer_universe,
             self.universe(),
-            snapshot,
+            only_consider_snapshot,
         )
     }
 }

compiler/rustc_infer/src/infer/mod.rs

@@ -251,14 +251,13 @@ pub struct InferCtxt<'tcx> {
     /// solving is left to borrowck instead.
     pub considering_regions: bool,
 
-    pub inner: RefCell<InferCtxtInner<'tcx>>,
-
     /// If set, this flag causes us to skip the 'leak check' during
     /// higher-ranked subtyping operations. This flag is a temporary one used
     /// to manage the removal of the leak-check: for the time being, we still run the
-    /// leak-check, but we issue warnings. This flag can only be set to true
-    /// when entering a snapshot.
-    skip_leak_check: Cell<bool>,
+    /// leak-check, but we issue warnings.
+    skip_leak_check: bool,
+
+    pub inner: RefCell<InferCtxtInner<'tcx>>,
 
     /// Once region inference is done, the values for each variable.
     lexical_region_resolutions: RefCell<Option<LexicalRegionResolutions<'tcx>>>,
@@ -543,6 +542,7 @@ pub struct InferCtxtBuilder<'tcx> {
     tcx: TyCtxt<'tcx>,
     defining_use_anchor: DefiningAnchor,
     considering_regions: bool,
+    skip_leak_check: bool,
     /// Whether we are in coherence mode.
     intercrate: bool,
 }
@@ -557,6 +557,7 @@ impl<'tcx> TyCtxtInferExt<'tcx> for TyCtxt<'tcx> {
             tcx: self,
             defining_use_anchor: DefiningAnchor::Error,
             considering_regions: true,
+            skip_leak_check: false,
             intercrate: false,
         }
     }
@@ -584,6 +585,11 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
         self
     }
 
+    pub fn skip_leak_check(mut self, skip_leak_check: bool) -> Self {
+        self.skip_leak_check = skip_leak_check;
+        self
+    }
+
     /// Given a canonical value `C` as a starting point, create an
     /// inference context that contains each of the bound values
     /// within instantiated as a fresh variable. The `f` closure is
@@ -605,11 +611,18 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
     }
 
     pub fn build(&mut self) -> InferCtxt<'tcx> {
-        let InferCtxtBuilder { tcx, defining_use_anchor, considering_regions, intercrate } = *self;
+        let InferCtxtBuilder {
+            tcx,
+            defining_use_anchor,
+            considering_regions,
+            skip_leak_check,
+            intercrate,
+        } = *self;
         InferCtxt {
             tcx,
             defining_use_anchor,
             considering_regions,
+            skip_leak_check,
             inner: RefCell::new(InferCtxtInner::new()),
             lexical_region_resolutions: RefCell::new(None),
             selection_cache: Default::default(),
@@ -619,7 +632,6 @@ impl<'tcx> InferCtxtBuilder<'tcx> {
             tainted_by_errors: Cell::new(None),
             err_count_on_creation: tcx.sess.err_count(),
             in_snapshot: Cell::new(false),
-            skip_leak_check: Cell::new(false),
             universe: Cell::new(ty::UniverseIndex::ROOT),
             intercrate,
         }
@@ -815,32 +827,9 @@ impl<'tcx> InferCtxt<'tcx> {
         r
     }
 
-    /// If `should_skip` is true, then execute `f` then unroll any bindings it creates.
-    #[instrument(skip(self, f), level = "debug")]
-    pub fn probe_maybe_skip_leak_check<R, F>(&self, should_skip: bool, f: F) -> R
-    where
-        F: FnOnce(&CombinedSnapshot<'tcx>) -> R,
-    {
-        let snapshot = self.start_snapshot();
-        let was_skip_leak_check = self.skip_leak_check.get();
-        if should_skip {
-            self.skip_leak_check.set(true);
-        }
-        let r = f(&snapshot);
-        self.rollback_to("probe", snapshot);
-        self.skip_leak_check.set(was_skip_leak_check);
-        r
-    }
-
-    /// Scan the constraints produced since `snapshot` began and returns:
-    ///
-    /// - `None` -- if none of them involves "region outlives" constraints.
-    /// - `Some(true)` -- if there are `'a: 'b` constraints where `'a` or `'b` is a placeholder.
-    /// - `Some(false)` -- if there are `'a: 'b` constraints but none involve placeholders.
-    pub fn region_constraints_added_in_snapshot(
-        &self,
-        snapshot: &CombinedSnapshot<'tcx>,
-    ) -> Option<bool> {
+    /// Scan the constraints produced since `snapshot` and check whether
+    /// we added any region constraints.
+    pub fn region_constraints_added_in_snapshot(&self, snapshot: &CombinedSnapshot<'tcx>) -> bool {
         self.inner
             .borrow_mut()
             .unwrap_region_constraints()

compiler/rustc_infer/src/infer/region_constraints/leak_check.rs

@@ -3,7 +3,6 @@ use crate::infer::CombinedSnapshot;
 use rustc_data_structures::{
     fx::FxIndexMap,
     graph::{scc::Sccs, vec_graph::VecGraph},
-    undo_log::UndoLogs,
 };
 use rustc_index::Idx;
 use rustc_middle::ty::error::TypeError;
@@ -13,7 +12,9 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
     /// Searches new universes created during `snapshot`, looking for
     /// placeholders that may "leak" out from the universes they are contained
     /// in. If any leaking placeholders are found, then an `Err` is returned
-    /// (typically leading to the snapshot being reversed).
+    /// (typically leading to the snapshot being reversed). This algorithm
+    /// only looks at placeholders which cannot be named by `outer_universe`,
+    /// as this is the universe we're currently checking for a leak.
     ///
     /// The leak check *used* to be the only way we had to handle higher-ranked
     /// obligations. Now that we have integrated universes into the region
@@ -55,36 +56,34 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
     ///   * if they must also be equal to a placeholder P, and U cannot name P, report an error, as that
     ///     indicates `P: R` and `R` is in an incompatible universe
     ///
+    /// To improve performance and for the old trait solver caching to be sound, this takes
+    /// an optional snapshot in which case we only look at region constraints added in that
+    /// snapshot. If we were to not do that the `leak_check` during evaluation can rely on
+    /// region constraints added outside of that evaluation. As that is not reflected in the
+    /// cache key this would be unsound.
+    ///
     /// # Historical note
     ///
     /// Older variants of the leak check used to report errors for these
     /// patterns, but we no longer do:
     ///
     /// * R: P1, even if R cannot name P1, because R = 'static is a valid sol'n
     /// * R: P1, R: P2, as above
+    #[instrument(level = "debug", skip(self, tcx, only_consider_snapshot), ret)]
     pub fn leak_check(
         &mut self,
         tcx: TyCtxt<'tcx>,
+        outer_universe: ty::UniverseIndex,
         max_universe: ty::UniverseIndex,
-        snapshot: &CombinedSnapshot<'tcx>,
+        only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>,
     ) -> RelateResult<'tcx, ()> {
-        debug!(
-            "leak_check(max_universe={:?}, snapshot.universe={:?})",
-            max_universe, snapshot.universe
-        );
-
-        assert!(UndoLogs::<super::UndoLog<'_>>::in_snapshot(&self.undo_log));
-
-        let universe_at_start_of_snapshot = snapshot.universe;
-        if universe_at_start_of_snapshot == max_universe {
+        if outer_universe == max_universe {
             return Ok(());
         }
 
-        let mini_graph =
-            &MiniGraph::new(tcx, self.undo_log.region_constraints(), &self.storage.data.verifys);
+        let mini_graph = &MiniGraph::new(tcx, &self, only_consider_snapshot);
 
-        let mut leak_check =
-            LeakCheck::new(tcx, universe_at_start_of_snapshot, max_universe, mini_graph, self);
+        let mut leak_check = LeakCheck::new(tcx, outer_universe, max_universe, mini_graph, self);
         leak_check.assign_placeholder_values()?;
         leak_check.propagate_scc_value()?;
         Ok(())
@@ -93,7 +92,7 @@ impl<'tcx> RegionConstraintCollector<'_, 'tcx> {
 
 struct LeakCheck<'me, 'tcx> {
     tcx: TyCtxt<'tcx>,
-    universe_at_start_of_snapshot: ty::UniverseIndex,
+    outer_universe: ty::UniverseIndex,
     mini_graph: &'me MiniGraph<'tcx>,
     rcc: &'me RegionConstraintCollector<'me, 'tcx>,
 
@@ -121,15 +120,15 @@ struct LeakCheck<'me, 'tcx> {
 impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
     fn new(
         tcx: TyCtxt<'tcx>,
-        universe_at_start_of_snapshot: ty::UniverseIndex,
+        outer_universe: ty::UniverseIndex,
         max_universe: ty::UniverseIndex,
         mini_graph: &'me MiniGraph<'tcx>,
         rcc: &'me RegionConstraintCollector<'me, 'tcx>,
     ) -> Self {
         let dummy_scc_universe = SccUniverse { universe: max_universe, region: None };
         Self {
             tcx,
-            universe_at_start_of_snapshot,
+            outer_universe,
             mini_graph,
             rcc,
             scc_placeholders: IndexVec::from_elem_n(None, mini_graph.sccs.num_sccs()),
@@ -154,7 +153,7 @@ impl<'me, 'tcx> LeakCheck<'me, 'tcx> {
 
             // Detect those SCCs that directly contain a placeholder
             if let ty::RePlaceholder(placeholder) = **region {
-                if self.universe_at_start_of_snapshot.cannot_name(placeholder.universe) {
+                if self.outer_universe.cannot_name(placeholder.universe) {
                     self.assign_scc_value(scc, placeholder)?;
                 }
             }
@@ -364,56 +363,70 @@ struct MiniGraph<'tcx> {
 }
 
 impl<'tcx> MiniGraph<'tcx> {
-    fn new<'a>(
+    fn new(
         tcx: TyCtxt<'tcx>,
-        undo_log: impl Iterator<Item = &'a UndoLog<'tcx>>,
-        verifys: &[Verify<'tcx>],
-    ) -> Self
-    where
-        'tcx: 'a,
-    {
+        region_constraints: &RegionConstraintCollector<'_, 'tcx>,
+        only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>,
+    ) -> Self {
         let mut nodes = FxIndexMap::default();
         let mut edges = Vec::new();
 
         // Note that if `R2: R1`, we get a callback `r1, r2`, so `target` is first parameter.
-        Self::iterate_undo_log(tcx, undo_log, verifys, |target, source| {
-            let source_node = Self::add_node(&mut nodes, source);
-            let target_node = Self::add_node(&mut nodes, target);
-            edges.push((source_node, target_node));
-        });
+        Self::iterate_region_constraints(
+            tcx,
+            region_constraints,
+            only_consider_snapshot,
+            |target, source| {
+                let source_node = Self::add_node(&mut nodes, source);
+                let target_node = Self::add_node(&mut nodes, target);
+                edges.push((source_node, target_node));
+            },
+        );
         let graph = VecGraph::new(nodes.len(), edges);
         let sccs = Sccs::new(&graph);
         Self { nodes, sccs }
     }
 
     /// Invokes `each_edge(R1, R2)` for each edge where `R2: R1`
-    fn iterate_undo_log<'a>(
+    fn iterate_region_constraints(
         tcx: TyCtxt<'tcx>,
-        undo_log: impl Iterator<Item = &'a UndoLog<'tcx>>,
-        verifys: &[Verify<'tcx>],
+        region_constraints: &RegionConstraintCollector<'_, 'tcx>,
+        only_consider_snapshot: Option<&CombinedSnapshot<'tcx>>,
         mut each_edge: impl FnMut(ty::Region<'tcx>, ty::Region<'tcx>),
-    ) where
-        'tcx: 'a,
-    {
-        for undo_entry in undo_log {
-            match undo_entry {
-                &AddConstraint(Constraint::VarSubVar(a, b)) => {
-                    each_edge(ty::Region::new_var(tcx, a), ty::Region::new_var(tcx, b));
-                }
-                &AddConstraint(Constraint::RegSubVar(a, b)) => {
-                    each_edge(a, ty::Region::new_var(tcx, b));
-                }
-                &AddConstraint(Constraint::VarSubReg(a, b)) => {
-                    each_edge(ty::Region::new_var(tcx, a), b);
-                }
-                &AddConstraint(Constraint::RegSubReg(a, b)) => {
-                    each_edge(a, b);
+    ) {
+        let mut each_constraint = |constraint| match constraint {
+            &Constraint::VarSubVar(a, b) => {
+                each_edge(ty::Region::new_var(tcx, a), ty::Region::new_var(tcx, b));
+            }
+            &Constraint::RegSubVar(a, b) => {
+                each_edge(a, ty::Region::new_var(tcx, b));
+            }
+            &Constraint::VarSubReg(a, b) => {
+                each_edge(ty::Region::new_var(tcx, a), b);
+            }
+            &Constraint::RegSubReg(a, b) => {
+                each_edge(a, b);
+            }
+        };
+
+        if let Some(snapshot) = only_consider_snapshot {
+            for undo_entry in
+                region_constraints.undo_log.region_constraints_in_snapshot(&snapshot.undo_snapshot)
+            {
+                match undo_entry {
+                    AddConstraint(constraint) => {
+                        each_constraint(constraint);
+                    }
+                    &AddVerify(i) => span_bug!(
+                        region_constraints.data().verifys[i].origin.span(),
+                        "we never add verifications while doing higher-ranked things",
+                    ),
+                    &AddCombination(..) | &AddVar(..) => {}
                 }
-                &AddVerify(i) => span_bug!(
-                    verifys[i].origin.span(),
-                    "we never add verifications while doing higher-ranked things",
-                ),
-                &AddCombination(..) | &AddVar(..) => {}
+            }
+        } else {
+            for (constraint, _origin) in &region_constraints.data().constraints {
+                each_constraint(constraint)
             }
         }
     }