Auto merge of #41325 - eddyb:isolate-snapshots-for-good, r=arielb1

Ban registering obligations during InferCtxt snapshots.

Back in #33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse.

But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from #37658, which *have to* be rolled back *even* if they succeed.

We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse.
Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions.

As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned.

r? @nikomatsakis @arielb1

Author: bors

src/librustc/infer/mod.rs

@@ -199,10 +199,8 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     // `tained_by_errors`) to avoid reporting certain kinds of errors.
     err_count_on_creation: usize,
 
-    // This flag is used for debugging, and is set to true if there are
-    // any obligations set during the current snapshot. In that case, the
-    // snapshot can't be rolled back.
-    pub obligations_in_snapshot: Cell<bool>,
+    // This flag is true while there is an active snapshot.
+    in_snapshot: Cell<bool>,
 }
 
 /// A map returned by `skolemize_late_bound_regions()` indicating the skolemized
@@ -507,7 +505,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'gcx> {
             projection_mode: Reveal::UserFacing,
             tainted_by_errors_flag: Cell::new(false),
             err_count_on_creation: self.sess.err_count(),
-            obligations_in_snapshot: Cell::new(false),
+            in_snapshot: Cell::new(false),
         }
     }
 }
@@ -545,7 +543,7 @@ impl<'a, 'gcx, 'tcx> InferCtxtBuilder<'a, 'gcx, 'tcx> {
             projection_mode: projection_mode,
             tainted_by_errors_flag: Cell::new(false),
             err_count_on_creation: tcx.sess.err_count(),
-            obligations_in_snapshot: Cell::new(false),
+            in_snapshot: Cell::new(false),
         }))
     }
 }
@@ -573,7 +571,7 @@ pub struct CombinedSnapshot {
     int_snapshot: unify::Snapshot<ty::IntVid>,
     float_snapshot: unify::Snapshot<ty::FloatVid>,
     region_vars_snapshot: RegionSnapshot,
-    obligations_in_snapshot: bool,
+    was_in_snapshot: bool,
 }
 
 /// Helper trait for shortening the lifetimes inside a
@@ -734,6 +732,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         self.projection_mode
     }
 
+    pub fn is_in_snapshot(&self) -> bool {
+        self.in_snapshot.get()
+    }
+
     pub fn freshen<T:TypeFoldable<'tcx>>(&self, t: T) -> T {
         t.fold_with(&mut self.freshener())
     }
@@ -861,46 +863,45 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         result.map(move |t| InferOk { value: t, obligations: fields.obligations })
     }
 
-    // Clear the "obligations in snapshot" flag, invoke the closure,
+    // Clear the "currently in a snapshot" flag, invoke the closure,
     // then restore the flag to its original value. This flag is a
     // debugging measure designed to detect cases where we start a
-    // snapshot, create type variables, register obligations involving
-    // those type variables in the fulfillment cx, and then have to
-    // unroll the snapshot, leaving "dangling type variables" behind.
-    // In such cases, the flag will be set by the fulfillment cx, and
-    // an assertion will fail when rolling the snapshot back.  Very
-    // useful, much better than grovelling through megabytes of
-    // RUST_LOG output.
+    // snapshot, create type variables, and register obligations
+    // which may involve those type variables in the fulfillment cx,
+    // potentially leaving "dangling type variables" behind.
+    // In such cases, an assertion will fail when attempting to
+    // register obligations, within a snapshot. Very useful, much
+    // better than grovelling through megabytes of RUST_LOG output.
     //
-    // HOWEVER, in some cases the flag is wrong. In particular, we
+    // HOWEVER, in some cases the flag is unhelpful. In particular, we
     // sometimes create a "mini-fulfilment-cx" in which we enroll
     // obligations. As long as this fulfillment cx is fully drained
     // before we return, this is not a problem, as there won't be any
     // escaping obligations in the main cx. In those cases, you can
     // use this function.
-    pub fn save_and_restore_obligations_in_snapshot_flag<F, R>(&self, func: F) -> R
+    pub fn save_and_restore_in_snapshot_flag<F, R>(&self, func: F) -> R
         where F: FnOnce(&Self) -> R
     {
-        let flag = self.obligations_in_snapshot.get();
-        self.obligations_in_snapshot.set(false);
+        let flag = self.in_snapshot.get();
+        self.in_snapshot.set(false);
         let result = func(self);
-        self.obligations_in_snapshot.set(flag);
+        self.in_snapshot.set(flag);
         result
     }
 
     fn start_snapshot(&self) -> CombinedSnapshot {
         debug!("start_snapshot()");
 
-        let obligations_in_snapshot = self.obligations_in_snapshot.get();
-        self.obligations_in_snapshot.set(false);
+        let in_snapshot = self.in_snapshot.get();
+        self.in_snapshot.set(true);
 
         CombinedSnapshot {
             projection_cache_snapshot: self.projection_cache.borrow_mut().snapshot(),
             type_snapshot: self.type_variables.borrow_mut().snapshot(),
             int_snapshot: self.int_unification_table.borrow_mut().snapshot(),
             float_snapshot: self.float_unification_table.borrow_mut().snapshot(),
             region_vars_snapshot: self.region_vars.start_snapshot(),
-            obligations_in_snapshot: obligations_in_snapshot,
+            was_in_snapshot: in_snapshot,
         }
     }
 
@@ -911,10 +912,9 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                int_snapshot,
                                float_snapshot,
                                region_vars_snapshot,
-                               obligations_in_snapshot } = snapshot;
+                               was_in_snapshot } = snapshot;
 
-        assert!(!self.obligations_in_snapshot.get());
-        self.obligations_in_snapshot.set(obligations_in_snapshot);
+        self.in_snapshot.set(was_in_snapshot);
 
         self.projection_cache
             .borrow_mut()
@@ -939,9 +939,9 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                int_snapshot,
                                float_snapshot,
                                region_vars_snapshot,
-                               obligations_in_snapshot } = snapshot;
+                               was_in_snapshot } = snapshot;
 
-        self.obligations_in_snapshot.set(obligations_in_snapshot);
+        self.in_snapshot.set(was_in_snapshot);
 
         self.projection_cache
             .borrow_mut()

src/librustc/traits/fulfill.rs

@@ -171,7 +171,7 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> {
 
         debug!("register_predicate_obligation(obligation={:?})", obligation);
 
-        infcx.obligations_in_snapshot.set(true);
+        assert!(!infcx.is_in_snapshot());
 
         if infcx.tcx.fulfilled_predicates.borrow().check_duplicate(&obligation.predicate) {
             debug!("register_predicate_obligation: duplicate");

src/librustc/traits/specialize/mod.rs

@@ -242,7 +242,7 @@ fn fulfill_implication<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
     // attempt to prove all of the predicates for impl2 given those for impl1
     // (which are packed up in penv)
 
-    infcx.save_and_restore_obligations_in_snapshot_flag(|infcx| {
+    infcx.save_and_restore_in_snapshot_flag(|infcx| {
         let mut fulfill_cx = FulfillmentContext::new();
         for oblig in obligations.into_iter() {
             fulfill_cx.register_predicate_obligation(&infcx, oblig);

src/librustc_typeck/check/assoc.rs

@@ -149,22 +149,25 @@ impl<'a, 'gcx, 'tcx> Autoderef<'a, 'gcx, 'tcx> {
         self.fcx.resolve_type_vars_if_possible(&self.cur_ty)
     }
 
-    pub fn finalize<E>(self, pref: LvaluePreference, exprs: &[E])
-        where E: AsCoercionSite
-    {
+    pub fn finalize(self, pref: LvaluePreference, expr: &hir::Expr) {
         let fcx = self.fcx;
-        fcx.register_infer_ok_obligations(self.finalize_as_infer_ok(pref, exprs));
+        fcx.register_infer_ok_obligations(self.finalize_as_infer_ok(pref, &[expr]));
     }
 
     pub fn finalize_as_infer_ok<E>(self, pref: LvaluePreference, exprs: &[E])
                                    -> InferOk<'tcx, ()>
         where E: AsCoercionSite
     {
-        let methods: Vec<_> = self.steps
+        let Autoderef { fcx, span, mut obligations, steps, .. } = self;
+        let methods: Vec<_> = steps
             .iter()
             .map(|&(ty, kind)| {
                 if let AutoderefKind::Overloaded = kind {
-                    self.fcx.try_overloaded_deref(self.span, None, ty, pref)
+                    fcx.try_overloaded_deref(span, None, ty, pref)
+                        .map(|InferOk { value, obligations: o }| {
+                            obligations.extend(o);
+                            value
+                        })
                 } else {
                     None
                 }
@@ -174,22 +177,22 @@ impl<'a, 'gcx, 'tcx> Autoderef<'a, 'gcx, 'tcx> {
         debug!("finalize({:?}) - {:?},{:?}",
                pref,
                methods,
-               self.obligations);
+               obligations);
 
         for expr in exprs {
             let expr = expr.as_coercion_site();
             debug!("finalize - finalizing #{} - {:?}", expr.id, expr);
             for (n, method) in methods.iter().enumerate() {
                 if let &Some(method) = method {
                     let method_call = MethodCall::autoderef(expr.id, n as u32);
-                    self.fcx.tables.borrow_mut().method_map.insert(method_call, method);
+                    fcx.tables.borrow_mut().method_map.insert(method_call, method);
                 }
             }
         }
 
         InferOk {
             value: (),
-            obligations: self.obligations
+            obligations
         }
     }
 }
@@ -211,7 +214,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                                 base_expr: Option<&hir::Expr>,
                                 base_ty: Ty<'tcx>,
                                 lvalue_pref: LvaluePreference)
-                                -> Option<MethodCallee<'tcx>> {
+                                -> Option<InferOk<'tcx, MethodCallee<'tcx>>> {
         debug!("try_overloaded_deref({:?},{:?},{:?},{:?})",
                span,
                base_expr,

src/librustc_typeck/check/autoderef.rs

@@ -55,7 +55,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             })
             .next();
         let callee_ty = autoderef.unambiguous_final_ty();
-        autoderef.finalize(LvaluePreference::NoPreference, &[callee_expr]);
+        autoderef.finalize(LvaluePreference::NoPreference, callee_expr);
 
         let output = match result {
             None => {
@@ -173,7 +173,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                                                        adjusted_ty,
                                                        None) {
                 None => continue,
-                Some(method_callee) => {
+                Some(ok) => {
+                    let method_callee = self.register_infer_ok_obligations(ok);
                     return Some(method_callee);
                 }
             }

src/librustc_typeck/check/callee.rs

@@ -711,16 +711,15 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
 
         let cause = self.cause(expr.span, ObligationCauseCode::ExprAssignable);
         let coerce = Coerce::new(self, cause);
-        self.commit_if_ok(|_| {
-            let ok = coerce.coerce(&[expr], source, target)?;
-            let adjustment = self.register_infer_ok_obligations(ok);
-            self.apply_adjustment(expr.id, adjustment);
-
-            // We should now have added sufficient adjustments etc to
-            // ensure that the type of expression, post-adjustment, is
-            // a subtype of target.
-            Ok(target)
-        })
+        let ok = self.commit_if_ok(|_| coerce.coerce(&[expr], source, target))?;
+
+        let adjustment = self.register_infer_ok_obligations(ok);
+        self.apply_adjustment(expr.id, adjustment);
+
+        // We should now have added sufficient adjustments etc to
+        // ensure that the type of expression, post-adjustment, is
+        // a subtype of target.
+        Ok(target)
     }
 
     /// Given some expressions, their known unified type and another expression,