modify trait checker to track the variables on which trait resolution is

stalled rather than keeping this annoying mark; I checked that the
original compile-time regression that the mark was intended to
fix (rust-lang#18208) was still
reasonable, but I've not done exhaustive measurements to see how
important this "optimization" really is anymore

Author: nikomatsakis

src/librustc/middle/traits/fulfill.rs

@@ -57,12 +57,7 @@ pub struct FulfillmentContext<'tcx> {
 
     // A list of all obligations that have been registered with this
     // fulfillment context.
-    predicates: Vec<PredicateObligation<'tcx>>,
-
-    // Remembers the count of trait obligations that we have already
-    // attempted to select. This is used to avoid repeating work
-    // when `select_new_obligations` is called.
-    attempted_mark: usize,
+    predicates: Vec<PendingPredicateObligation<'tcx>>,
 
     // A set of constraints that regionck must validate. Each
     // constraint has the form `T:'a`, meaning "some type `T` must
@@ -100,6 +95,12 @@ pub struct RegionObligation<'tcx> {
     pub cause: ObligationCause<'tcx>,
 }
 
+#[derive(Clone, Debug)]
+pub struct PendingPredicateObligation<'tcx> {
+    pub obligation: PredicateObligation<'tcx>,
+    pub stalled_on: Vec<Ty<'tcx>>,
+}
+
 impl<'tcx> FulfillmentContext<'tcx> {
     /// Creates a new fulfillment context.
     ///
@@ -122,7 +123,6 @@ impl<'tcx> FulfillmentContext<'tcx> {
         FulfillmentContext {
             duplicate_set: FulfilledPredicates::new(),
             predicates: Vec::new(),
-            attempted_mark: 0,
             region_obligations: NodeMap(),
             errors_will_be_reported: errors_will_be_reported,
         }
@@ -198,6 +198,10 @@ impl<'tcx> FulfillmentContext<'tcx> {
         }
 
         debug!("register_predicate({:?})", obligation);
+        let obligation = PendingPredicateObligation {
+            obligation: obligation,
+            stalled_on: vec![]
+        };
         self.predicates.push(obligation);
     }
 
@@ -221,7 +225,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
         let errors: Vec<FulfillmentError> =
             self.predicates
             .iter()
-            .map(|o| FulfillmentError::new((*o).clone(), CodeAmbiguity))
+            .map(|o| FulfillmentError::new(o.obligation.clone(), CodeAmbiguity))
             .collect();
 
         if errors.is_empty() {
@@ -231,18 +235,6 @@ impl<'tcx> FulfillmentContext<'tcx> {
         }
     }
 
-    /// Attempts to select obligations that were registered since the call to a selection routine.
-    /// This is used by the type checker to eagerly attempt to resolve obligations in hopes of
-    /// gaining type information. It'd be equally valid to use `select_where_possible` but it
-    /// results in `O(n^2)` performance (#18208).
-    pub fn select_new_obligations<'a>(&mut self,
-                                      infcx: &InferCtxt<'a,'tcx>)
-                                      -> Result<(),Vec<FulfillmentError<'tcx>>>
-    {
-        let mut selcx = SelectionContext::new(infcx);
-        self.select(&mut selcx, true)
-    }
-
     pub fn select_where_possible<'a>(&mut self,
                                      infcx: &InferCtxt<'a,'tcx>)
                                      -> Result<(),Vec<FulfillmentError<'tcx>>>
@@ -251,7 +243,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
         self.select(&mut selcx, false)
     }
 
-    pub fn pending_obligations(&self) -> &[PredicateObligation<'tcx>] {
+    pub fn pending_obligations(&self) -> &[PendingPredicateObligation<'tcx>] {
         &self.predicates
     }
 
@@ -299,36 +291,25 @@ impl<'tcx> FulfillmentContext<'tcx> {
 
             let mut new_obligations = Vec::new();
 
-            // If we are only attempting obligations we haven't seen yet,
-            // then set `skip` to the number of obligations we've already
-            // seen.
-            let mut skip = if only_new_obligations {
-                self.attempted_mark
-            } else {
-                0
-            };
-
             // First pass: walk each obligation, retaining
             // only those that we cannot yet process.
             {
                 let region_obligations = &mut self.region_obligations;
-                self.predicates.retain(|predicate| {
-                    // Hack: Retain does not pass in the index, but we want
-                    // to avoid processing the first `start_count` entries.
-                    let processed =
-                        if skip == 0 {
-                            process_predicate(selcx, predicate,
-                                              &mut new_obligations, &mut errors, region_obligations)
-                        } else {
-                            skip -= 1;
-                            false
-                        };
-                    !processed
-                });
+                let mut i = 0;
+                while i < self.predicates.len() {
+                    let processed = process_predicate(selcx,
+                                                      &mut self.predicates[i],
+                                                      &mut new_obligations,
+                                                      &mut errors,
+                                                      region_obligations);
+                    if processed {
+                        self.predicates.swap_remove(i);
+                    } else {
+                        i += 1;
+                    }
+                }
             }
 
-            self.attempted_mark = self.predicates.len();
-
             if self.predicates.len() == count {
                 // Nothing changed.
                 break;
@@ -354,7 +335,7 @@ impl<'tcx> FulfillmentContext<'tcx> {
 }
 
 fn process_predicate<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>,
-                              obligation: &PredicateObligation<'tcx>,
+                              pending_obligation: &mut PendingPredicateObligation<'tcx>,
                               new_obligations: &mut Vec<PredicateObligation<'tcx>>,
                               errors: &mut Vec<FulfillmentError<'tcx>>,
                               region_obligations: &mut NodeMap<Vec<RegionObligation<'tcx>>>)
@@ -367,11 +348,53 @@ fn process_predicate<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>,
      * type inference.
      */
 
+    // if we were stalled on some unresolved variables, first check
+    // whether any of them have been resolved; if not, don't bother
+    // doing more work yet
+    if !pending_obligation.stalled_on.is_empty() {
+        if pending_obligation.stalled_on.iter().all(|&ty| {
+            let resolved_ty = selcx.infcx().resolve_type_vars_if_possible(&ty);
+            resolved_ty == ty // nothing changed here
+        }) {
+            debug!("process_predicate: pending obligation {:?} still stalled on {:?}",
+                   selcx.infcx().resolve_type_vars_if_possible(&pending_obligation.obligation),
+                   pending_obligation.stalled_on);
+            return false;
+        }
+        pending_obligation.stalled_on = vec![];
+    }
+
+    let obligation = &mut pending_obligation.obligation;
     match obligation.predicate {
         ty::Predicate::Trait(ref data) => {
             let trait_obligation = obligation.with(data.clone());
             match selcx.select(&trait_obligation) {
                 Ok(None) => {
+                    // This is a bit subtle: for the most part, the
+                    // only reason we can fail to make progress on
+                    // trait selection is because we don't have enough
+                    // information about the types in the trait. One
+                    // exception is that we sometimes haven't decided
+                    // what kind of closure a closure is. *But*, in
+                    // that case, it turns out, the type of the
+                    // closure will also change, because the closure
+                    // also includes references to its upvars as part
+                    // of its type, and those types are resolved at
+                    // the same time.
+                    pending_obligation.stalled_on =
+                        data.skip_binder() // ok b/c this check doesn't care about regions
+                        .input_types()
+                        .iter()
+                        .map(|t| selcx.infcx().resolve_type_vars_if_possible(t))
+                        .filter(|t| t.has_infer_types())
+                        .flat_map(|t| t.walk())
+                        .filter(|t| t.is_ty_var())
+                        .collect();
+
+                    debug!("process_predicate: pending obligation {:?} now stalled on {:?}",
+                           selcx.infcx().resolve_type_vars_if_possible(obligation),
+                           pending_obligation.stalled_on);
+
                     false
                 }
                 Ok(Some(s)) => {

src/librustc_typeck/check/closure.rs

@@ -141,6 +141,7 @@ fn deduce_expectations_from_obligations<'a,'tcx>(
         fulfillment_cx
         .pending_obligations()
         .iter()
+        .map(|obligation| &obligation.obligation)
         .filter_map(|obligation| {
             debug!("deduce_expectations_from_obligations: obligation.predicate={:?}",
                    obligation.predicate);
@@ -168,6 +169,7 @@ fn deduce_expectations_from_obligations<'a,'tcx>(
         fulfillment_cx
         .pending_obligations()
         .iter()
+        .map(|obligation| &obligation.obligation)
         .filter_map(|obligation| {
             let opt_trait_ref = match obligation.predicate {
                 ty::Predicate::Projection(ref data) => Some(data.to_poly_trait_ref()),

src/librustc_typeck/check/method/mod.rs

@@ -261,7 +261,7 @@ pub fn lookup_in_trait_adjusted<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
     // FIXME(#18653) -- Try to resolve obligations, giving us more
     // typing information, which can sometimes be needed to avoid
     // pathological region inference failures.
-    fcx.select_new_obligations();
+    fcx.select_obligations_where_possible();
 
     // Insert any adjustments needed (always an autoref of some mutability).
     match self_expr {

src/librustc_typeck/check/mod.rs

@@ -1235,15 +1235,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             return ty;
         }
 
-        // If not, try resolving any new fcx obligations that have cropped up.
-        self.select_new_obligations();
-        ty = self.infcx().resolve_type_vars_if_possible(&ty);
-        if !ty.has_infer_types() {
-            debug!("resolve_type_vars_if_possible: ty={:?}", ty);
-            return ty;
-        }
-
-        // If not, try resolving *all* pending obligations as much as
+        // If not, try resolving pending obligations as much as
         // possible. This can help substantially when there are
         // indirect dependencies that don't seem worth tracking
         // precisely.
@@ -2029,22 +2021,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             Err(errors) => { report_fulfillment_errors(self.infcx(), &errors); }
         }
     }
-
-    /// Try to select any fcx obligation that we haven't tried yet, in an effort
-    /// to improve inference. You could just call
-    /// `select_obligations_where_possible` except that it leads to repeated
-    /// work.
-    fn select_new_obligations(&self) {
-        match
-            self.inh.infcx.fulfillment_cx
-            .borrow_mut()
-            .select_new_obligations(self.infcx())
-        {
-            Ok(()) => { }
-            Err(errors) => { report_fulfillment_errors(self.infcx(), &errors); }
-        }
-    }
-
 }
 
 impl<'a, 'tcx> RegionScope for FnCtxt<'a, 'tcx> {
@@ -2496,7 +2472,7 @@ fn check_argument_types<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
         // an "opportunistic" vtable resolution of any trait bounds on
         // the call. This helps coercions.
         if check_blocks {
-            fcx.select_new_obligations();
+            fcx.select_obligations_where_possible();
         }
 
         // For variadic functions, we don't have a declared type for all of