auto merge of rust-lang#18386 : nikomatsakis/rust/issue-18208, r=pnkfelix

Avoid O(n^2) performance by reconsidering the full set of obligations only when we are about to report an error (rust-lang#18208). I found it is still important to consider the full set in order to make tests like `let x: Vec<_> = obligations.iter().collect()` work.

I think we lack the infrastructure to write a regression test for this, but when I did manual testing I found a massive reduction in type-checking time for extreme examples like those found in rust-lang#18208 vs stage0.

f? @dotdash

Author: bors

src/librustc/middle/traits/fulfill.rs

@@ -35,12 +35,18 @@ pub struct FulfillmentContext {
     // A list of all obligations that have been registered with this
     // fulfillment context.
     trait_obligations: Vec<Obligation>,
+
+    // 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: uint,
 }
 
 impl FulfillmentContext {
     pub fn new() -> FulfillmentContext {
         FulfillmentContext {
             trait_obligations: Vec::new(),
+            attempted_mark: 0,
         }
     }
 
@@ -74,18 +80,49 @@ impl FulfillmentContext {
         }
     }
 
+    pub fn select_new_obligations<'a,'tcx>(&mut self,
+                                           infcx: &InferCtxt<'a,'tcx>,
+                                           param_env: &ty::ParameterEnvironment,
+                                           typer: &Typer<'tcx>)
+                                           -> Result<(),Vec<FulfillmentError>>
+    {
+        /*!
+         * 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).
+         */
+
+        let mut selcx = SelectionContext::new(infcx, param_env, typer);
+        self.select(&mut selcx, true)
+    }
+
     pub fn select_where_possible<'a,'tcx>(&mut self,
                                           infcx: &InferCtxt<'a,'tcx>,
                                           param_env: &ty::ParameterEnvironment,
                                           typer: &Typer<'tcx>)
                                           -> Result<(),Vec<FulfillmentError>>
     {
-        let tcx = infcx.tcx;
         let mut selcx = SelectionContext::new(infcx, param_env, typer);
+        self.select(&mut selcx, false)
+    }
 
-        debug!("select_where_possible({} obligations) start",
-               self.trait_obligations.len());
+    fn select(&mut self,
+              selcx: &mut SelectionContext,
+              only_new_obligations: bool)
+              -> Result<(),Vec<FulfillmentError>>
+    {
+        /*!
+         * Attempts to select obligations using `selcx`. If
+         * `only_new_obligations` is true, then it only attempts to
+         * select obligations that haven't been seen before.
+         */
+        debug!("select({} obligations, only_new_obligations={}) start",
+               self.trait_obligations.len(),
+               only_new_obligations);
 
+        let tcx = selcx.tcx();
         let mut errors = Vec::new();
 
         loop {
@@ -96,30 +133,47 @@ impl FulfillmentContext {
 
             let mut selections = 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.
             self.trait_obligations.retain(|obligation| {
-                match selcx.select(obligation) {
-                    Ok(None) => {
-                        true
-                    }
-                    Ok(Some(s)) => {
-                        selections.push(s);
-                        false
-                    }
-                    Err(selection_err) => {
-                        debug!("obligation: {} error: {}",
-                               obligation.repr(tcx),
-                               selection_err.repr(tcx));
-
-                        errors.push(FulfillmentError::new(
-                            (*obligation).clone(),
-                            CodeSelectionError(selection_err)));
-                        false
+                // Hack: Retain does not pass in the index, but we want
+                // to avoid processing the first `start_count` entries.
+                if skip > 0 {
+                    skip -= 1;
+                    true
+                } else {
+                    match selcx.select(obligation) {
+                        Ok(None) => {
+                            true
+                        }
+                        Ok(Some(s)) => {
+                            selections.push(s);
+                            false
+                        }
+                        Err(selection_err) => {
+                            debug!("obligation: {} error: {}",
+                                   obligation.repr(tcx),
+                                   selection_err.repr(tcx));
+                            errors.push(FulfillmentError::new(
+                                (*obligation).clone(),
+                                CodeSelectionError(selection_err)));
+                            false
+                        }
                     }
                 }
             });
 
+            self.attempted_mark = self.trait_obligations.len();
+
             if self.trait_obligations.len() == count {
                 // Nothing changed.
                 break;
@@ -133,7 +187,7 @@ impl FulfillmentContext {
             }
         }
 
-        debug!("select_where_possible({} obligations, {} errors) done",
+        debug!("select({} obligations, {} errors) done",
                self.trait_obligations.len(),
                errors.len());
 

src/librustc/middle/typeck/check/method.rs

@@ -88,7 +88,7 @@ use middle::ty;
 use middle::typeck::astconv::AstConv;
 use middle::typeck::check::{FnCtxt, NoPreference, PreferMutLvalue};
 use middle::typeck::check::{impl_self_ty};
-use middle::typeck::check::vtable::select_fcx_obligations_where_possible;
+use middle::typeck::check::vtable::select_new_fcx_obligations;
 use middle::typeck::check;
 use middle::typeck::infer;
 use middle::typeck::{MethodCall, MethodCallee};
@@ -1302,7 +1302,7 @@ impl<'a, 'tcx> LookupContext<'a, 'tcx> {
         // the `Self` trait).
         let callee = self.confirm_candidate(rcvr_ty, &candidate);
 
-        select_fcx_obligations_where_possible(self.fcx);
+        select_new_fcx_obligations(self.fcx);
 
         Some(Ok(callee))
     }

src/librustc/middle/typeck/check/mod.rs

@@ -101,7 +101,6 @@ use middle::typeck::check::method::{AutoderefReceiver};
 use middle::typeck::check::method::{CheckTraitsAndInherentMethods};
 use middle::typeck::check::regionmanip::replace_late_bound_regions;
 use middle::typeck::CrateCtxt;
-use middle::typeck::infer::{resolve_type, force_tvar};
 use middle::typeck::infer;
 use middle::typeck::rscope::RegionScope;
 use middle::typeck::{lookup_def_ccx};
@@ -1412,7 +1411,7 @@ fn check_cast(fcx: &FnCtxt,
         }
         // casts from C-like enums are allowed
     } else if t_1_is_char {
-        let t_e = fcx.infcx().resolve_type_vars_if_possible(t_e);
+        let t_e = fcx.infcx().shallow_resolve(t_e);
         if ty::get(t_e).sty != ty::ty_uint(ast::TyU8) {
             fcx.type_error_message(span, |actual| {
                 format!("only `u8` can be cast as \
@@ -2564,7 +2563,7 @@ fn check_argument_types<'a>(fcx: &FnCtxt,
         // an "opportunistic" vtable resolution of any trait
         // bounds on the call.
         if check_blocks {
-            vtable::select_fcx_obligations_where_possible(fcx);
+            vtable::select_new_fcx_obligations(fcx);
         }
 
         // For variadic functions, we don't have a declared type for all of
@@ -2988,9 +2987,11 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
         // 'else' branch.
         let expected = match expected.only_has_type() {
             ExpectHasType(ety) => {
-                match infer::resolve_type(fcx.infcx(), Some(sp), ety, force_tvar) {
-                    Ok(rty) if !ty::type_is_ty_var(rty) => ExpectHasType(rty),
-                    _ => NoExpectation
+                let ety = fcx.infcx().shallow_resolve(ety);
+                if !ty::type_is_ty_var(ety) {
+                    ExpectHasType(ety)
+                } else {
+                    NoExpectation
                 }
             }
             _ => NoExpectation
@@ -4037,7 +4038,7 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
       ast::ExprForLoop(ref pat, ref head, ref block, _) => {
         check_expr(fcx, &**head);
         let typ = lookup_method_for_for_loop(fcx, &**head, expr.id);
-        vtable::select_fcx_obligations_where_possible(fcx);
+        vtable::select_new_fcx_obligations(fcx);
 
         let pcx = pat_ctxt {
             fcx: fcx,
@@ -5393,18 +5394,32 @@ pub fn instantiate_path(fcx: &FnCtxt,
 
 // Resolves `typ` by a single level if `typ` is a type variable.  If no
 // resolution is possible, then an error is reported.
-pub fn structurally_resolved_type(fcx: &FnCtxt, sp: Span, tp: ty::t) -> ty::t {
-    match infer::resolve_type(fcx.infcx(), Some(sp), tp, force_tvar) {
-        Ok(t_s) if !ty::type_is_ty_var(t_s) => t_s,
-        _ => {
-            fcx.type_error_message(sp, |_actual| {
-                "the type of this value must be known in this \
-                 context".to_string()
-            }, tp, None);
-            demand::suptype(fcx, sp, ty::mk_err(), tp);
-            tp
-        }
+pub fn structurally_resolved_type(fcx: &FnCtxt, sp: Span, mut ty: ty::t) -> ty::t {
+    // If `ty` is a type variable, see whether we already know what it is.
+    ty = fcx.infcx().shallow_resolve(ty);
+
+    // If not, try resolve pending fcx obligations. Those can shed light.
+    //
+    // FIXME(#18391) -- This current strategy can lead to bad performance in
+    // extreme cases.  We probably ought to smarter in general about
+    // only resolving when we need help and only resolving obligations
+    // will actually help.
+    if ty::type_is_ty_var(ty) {
+        vtable::select_fcx_obligations_where_possible(fcx);
+        ty = fcx.infcx().shallow_resolve(ty);
     }
+
+    // If not, error.
+    if ty::type_is_ty_var(ty) {
+        fcx.type_error_message(sp, |_actual| {
+            "the type of this value must be known in this \
+             context".to_string()
+        }, ty, None);
+        demand::suptype(fcx, sp, ty::mk_err(), ty);
+        ty = ty::mk_err();
+    }
+
+    ty
 }
 
 // Returns the one-level-deep structure of the given type.

src/librustc/middle/typeck/check/vtable.rs

@@ -339,6 +339,24 @@ pub fn select_fcx_obligations_where_possible(fcx: &FnCtxt) {
     }
 }
 
+pub fn select_new_fcx_obligations(fcx: &FnCtxt) {
+    /*!
+     * Try to select any fcx obligation that we haven't tried yet,
+     * in an effort to improve inference. You could just call
+     * `select_fcx_obligations_where_possible` except that it leads
+     * to repeated work.
+     */
+
+    match
+        fcx.inh.fulfillment_cx
+        .borrow_mut()
+        .select_new_obligations(fcx.infcx(), &fcx.inh.param_env, fcx)
+    {
+        Ok(()) => { }
+        Err(errors) => { report_fulfillment_errors(fcx, &errors); }
+    }
+}
+
 fn note_obligation_cause(fcx: &FnCtxt,
                          obligation: &Obligation) {
     let tcx = fcx.tcx();