Auto merge of rust-lang#30533 - nikomatsakis:fulfillment-tree, r=aturon

This PR introduces an `ObligationForest` data structure that the fulfillment context can use to track what's going on, instead of the current flat vector. This enables a number of improvements:

1. transactional support, at least for pushing new obligations
2. remove the "errors will be reported" hack -- instead, we only add types to the global cache once their entire subtree has been proven safe. Before, we never knew when this point was reached because we didn't track the subtree.
   - this in turn allows us to limit coinductive reasoning to structural traits, which sidesteps rust-lang#29859
3. keeping the backtrace should allow for an improved error message, where we give the user full context
    - we can also remove chained obligation causes

This PR is not 100% complete. In particular:

- [x] Currently, types that embed themselves like `struct Foo { f: Foo }` give an overflow when evaluating whether `Foo: Sized`. This is not a very user-friendly error message, and this is a common beginner error. I plan to special-case this scenario, I think.
- [x] I should do some perf. measurements. (Update: 2% regression.)
- [x] More tests targeting rust-lang#29859
- [ ] The transactional support is not fully integrated, though that should be easy enough.
- [ ] The error messages are not taking advantage of the backtrace.

I'd certainly like to do 1 through 3 before landing, but 4 and 5 could come as separate PRs.

r? @aturon // good way to learn more about this part of the trait system
f? @arielb1 // already knows this part of the trait system :)

Author: bors

src/librustc/diagnostics.rs

@@ -679,6 +679,43 @@ There's no easy fix for this, generally code will need to be refactored so that
 you no longer need to derive from `Super<Self>`.
 "####,
 
+E0072: r##"
+When defining a recursive struct or enum, any use of the type being defined
+from inside the definition must occur behind a pointer (like `Box` or `&`).
+This is because structs and enums must have a well-defined size, and without
+the pointer the size of the type would need to be unbounded.
+
+Consider the following erroneous definition of a type for a list of bytes:
+
+```
+// error, invalid recursive struct type
+struct ListNode {
+    head: u8,
+    tail: Option<ListNode>,
+}
+```
+
+This type cannot have a well-defined size, because it needs to be arbitrarily
+large (since we would be able to nest `ListNode`s to any depth). Specifically,
+
+```plain
+size of `ListNode` = 1 byte for `head`
+                   + 1 byte for the discriminant of the `Option`
+                   + size of `ListNode`
+```
+
+One way to fix this is by wrapping `ListNode` in a `Box`, like so:
+
+```
+struct ListNode {
+    head: u8,
+    tail: Option<Box<ListNode>>,
+}
+```
+
+This works because `Box` is a pointer, so its size is well-known.
+"##,
+
 E0109: r##"
 You tried to give a type parameter to a type which doesn't need it. Erroneous
 code example:

src/librustc/middle/check_const.rs

@@ -125,7 +125,7 @@ impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> {
             None => self.tcx.empty_parameter_environment()
         };
 
-        let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, Some(param_env), false);
+        let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, Some(param_env));
 
         f(&mut euv::ExprUseVisitor::new(self, &infcx))
     }
@@ -280,7 +280,7 @@ impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> {
 
     fn check_static_type(&self, e: &hir::Expr) {
         let ty = self.tcx.node_id_to_type(e.id);
-        let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, None, false);
+        let infcx = infer::new_infer_ctxt(self.tcx, &self.tcx.tables, None);
         let cause = traits::ObligationCause::new(e.span, e.id, traits::SharedStatic);
         let mut fulfill_cx = infcx.fulfillment_cx.borrow_mut();
         fulfill_cx.register_builtin_bound(&infcx, ty, ty::BoundSync, cause);

src/librustc/middle/check_match.rs

@@ -1095,8 +1095,7 @@ fn check_legality_of_move_bindings(cx: &MatchCheckCtxt,
                         //FIXME: (@jroesch) this code should be floated up as well
                         let infcx = infer::new_infer_ctxt(cx.tcx,
                                                           &cx.tcx.tables,
-                                                          Some(cx.param_env.clone()),
-                                                          false);
+                                                          Some(cx.param_env.clone()));
                         if infcx.type_moves_by_default(pat_ty, pat.span) {
                             check_move(p, sub.as_ref().map(|p| &**p));
                         }
@@ -1128,8 +1127,7 @@ fn check_for_mutation_in_guard<'a, 'tcx>(cx: &'a MatchCheckCtxt<'a, 'tcx>,
 
     let infcx = infer::new_infer_ctxt(cx.tcx,
                                       &cx.tcx.tables,
-                                      Some(checker.cx.param_env.clone()),
-                                      false);
+                                      Some(checker.cx.param_env.clone()));
 
     let mut visitor = ExprUseVisitor::new(&mut checker, &infcx);
     visitor.walk_expr(guard);

src/librustc/middle/check_rvalues.rs

@@ -44,8 +44,7 @@ impl<'a, 'tcx, 'v> intravisit::Visitor<'v> for RvalueContext<'a, 'tcx> {
             let param_env = ParameterEnvironment::for_item(self.tcx, fn_id);
             let infcx = infer::new_infer_ctxt(self.tcx,
                                               &self.tcx.tables,
-                                              Some(param_env.clone()),
-                                              false);
+                                              Some(param_env.clone()));
             let mut delegate = RvalueContextDelegate { tcx: self.tcx, param_env: &param_env };
             let mut euv = euv::ExprUseVisitor::new(&mut delegate, &infcx);
             euv.walk_fn(fd, b);

src/librustc/middle/const_eval.rs

@@ -1262,7 +1262,7 @@ fn resolve_trait_associated_const<'a, 'tcx: 'a>(tcx: &'a ty::ctxt<'tcx>,
                                               substs: trait_substs });
 
     tcx.populate_implementations_for_trait_if_necessary(trait_ref.def_id());
-    let infcx = infer::new_infer_ctxt(tcx, &tcx.tables, None, false);
+    let infcx = infer::new_infer_ctxt(tcx, &tcx.tables, None);
 
     let mut selcx = traits::SelectionContext::new(&infcx);
     let obligation = traits::Obligation::new(traits::ObligationCause::dummy(),

src/librustc/middle/infer/mod.rs

@@ -354,16 +354,9 @@ pub fn fixup_err_to_string(f: FixupError) -> String {
     }
 }
 
-/// errors_will_be_reported is required to proxy to the fulfillment context
-/// FIXME -- a better option would be to hold back on modifying
-/// the global cache until we know that all dependent obligations
-/// are also satisfied. In that case, we could actually remove
-/// this boolean flag, and we'd also avoid the problem of squelching
-/// duplicate errors that occur across fns.
 pub fn new_infer_ctxt<'a, 'tcx>(tcx: &'a ty::ctxt<'tcx>,
                                 tables: &'a RefCell<ty::Tables<'tcx>>,
-                                param_env: Option<ty::ParameterEnvironment<'a, 'tcx>>,
-                                errors_will_be_reported: bool)
+                                param_env: Option<ty::ParameterEnvironment<'a, 'tcx>>)
                                 -> InferCtxt<'a, 'tcx> {
     InferCtxt {
         tcx: tcx,
@@ -373,7 +366,7 @@ pub fn new_infer_ctxt<'a, 'tcx>(tcx: &'a ty::ctxt<'tcx>,
         float_unification_table: RefCell::new(UnificationTable::new()),
         region_vars: RegionVarBindings::new(tcx),
         parameter_environment: param_env.unwrap_or(tcx.empty_parameter_environment()),
-        fulfillment_cx: RefCell::new(traits::FulfillmentContext::new(errors_will_be_reported)),
+        fulfillment_cx: RefCell::new(traits::FulfillmentContext::new()),
         reported_trait_errors: RefCell::new(FnvHashSet()),
         normalize: false,
         err_count_on_creation: tcx.sess.err_count()
@@ -383,7 +376,7 @@ pub fn new_infer_ctxt<'a, 'tcx>(tcx: &'a ty::ctxt<'tcx>,
 pub fn normalizing_infer_ctxt<'a, 'tcx>(tcx: &'a ty::ctxt<'tcx>,
                                         tables: &'a RefCell<ty::Tables<'tcx>>)
                                         -> InferCtxt<'a, 'tcx> {
-    let mut infcx = new_infer_ctxt(tcx, tables, None, false);
+    let mut infcx = new_infer_ctxt(tcx, tables, None);
     infcx.normalize = true;
     infcx
 }
@@ -522,7 +515,7 @@ pub fn normalize_associated_type<'tcx,T>(tcx: &ty::ctxt<'tcx>, value: &T) -> T
         return value;
     }
 
-    let infcx = new_infer_ctxt(tcx, &tcx.tables, None, true);
+    let infcx = new_infer_ctxt(tcx, &tcx.tables, None);
     let mut selcx = traits::SelectionContext::new(&infcx);
     let cause = traits::ObligationCause::dummy();
     let traits::Normalized { value: result, obligations } =

src/librustc/middle/traits/error_reporting.rs

@@ -182,7 +182,8 @@ fn report_on_unimplemented<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
 /// if the program type checks or not -- and they are unusual
 /// occurrences in any case.
 pub fn report_overflow_error<'a, 'tcx, T>(infcx: &InferCtxt<'a, 'tcx>,
-                                          obligation: &Obligation<'tcx, T>)
+                                          obligation: &Obligation<'tcx, T>,
+                                          suggest_increasing_limit: bool)
                                           -> !
     where T: fmt::Display + TypeFoldable<'tcx>
 {
@@ -192,7 +193,9 @@ pub fn report_overflow_error<'a, 'tcx, T>(infcx: &InferCtxt<'a, 'tcx>,
                                    "overflow evaluating the requirement `{}`",
                                    predicate);
 
-    suggest_new_overflow_limit(infcx.tcx, &mut err, obligation.cause.span);
+    if suggest_increasing_limit {
+        suggest_new_overflow_limit(infcx.tcx, &mut err, obligation.cause.span);
+    }
 
     note_obligation_cause(infcx, &mut err, obligation);
 
@@ -201,6 +204,142 @@ pub fn report_overflow_error<'a, 'tcx, T>(infcx: &InferCtxt<'a, 'tcx>,
     unreachable!();
 }
 
+/// Reports that a cycle was detected which led to overflow and halts
+/// compilation. This is equivalent to `report_overflow_error` except
+/// that we can give a more helpful error message (and, in particular,
+/// we do not suggest increasing the overflow limit, which is not
+/// going to help).
+pub fn report_overflow_error_cycle<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
+                                             cycle: &Vec<PredicateObligation<'tcx>>)
+                                             -> !
+{
+    assert!(cycle.len() > 1);
+
+    debug!("report_overflow_error_cycle(cycle length = {})", cycle.len());
+
+    let cycle = infcx.resolve_type_vars_if_possible(cycle);
+
+    debug!("report_overflow_error_cycle: cycle={:?}", cycle);
+
+    assert_eq!(&cycle[0].predicate, &cycle.last().unwrap().predicate);
+
+    try_report_overflow_error_type_of_infinite_size(infcx, &cycle);
+    report_overflow_error(infcx, &cycle[0], false);
+}
+
+/// If a cycle results from evaluated whether something is Sized, that
+/// is a particular special case that always results from a struct or
+/// enum definition that lacks indirection (e.g., `struct Foo { x: Foo
+/// }`). We wish to report a targeted error for this case.
+pub fn try_report_overflow_error_type_of_infinite_size<'a, 'tcx>(
+    infcx: &InferCtxt<'a, 'tcx>,
+    cycle: &[PredicateObligation<'tcx>])
+{
+    let sized_trait = match infcx.tcx.lang_items.sized_trait() {
+        Some(v) => v,
+        None => return,
+    };
+    let top_is_sized = {
+        match cycle[0].predicate {
+            ty::Predicate::Trait(ref data) => data.def_id() == sized_trait,
+            _ => false,
+        }
+    };
+    if !top_is_sized {
+        return;
+    }
+
+    // The only way to have a type of infinite size is to have,
+    // somewhere, a struct/enum type involved. Identify all such types
+    // and report the cycle to the user.
+
+    let struct_enum_tys: Vec<_> =
+        cycle.iter()
+             .flat_map(|obligation| match obligation.predicate {
+                 ty::Predicate::Trait(ref data) => {
+                     assert_eq!(data.def_id(), sized_trait);
+                     let self_ty = data.skip_binder().trait_ref.self_ty(); // (*)
+                     // (*) ok to skip binder because this is just
+                     // error reporting and regions don't really
+                     // matter
+                     match self_ty.sty {
+                         ty::TyEnum(..) | ty::TyStruct(..) => Some(self_ty),
+                         _ => None,
+                     }
+                 }
+                 _ => {
+                     infcx.tcx.sess.span_bug(obligation.cause.span,
+                                             &format!("Sized cycle involving non-trait-ref: {:?}",
+                                                      obligation.predicate));
+                 }
+             })
+             .collect();
+
+    assert!(!struct_enum_tys.is_empty());
+
+    // This is a bit tricky. We want to pick a "main type" in the
+    // listing that is local to the current crate, so we can give a
+    // good span to the user. But it might not be the first one in our
+    // cycle list. So find the first one that is local and then
+    // rotate.
+    let (main_index, main_def_id) =
+        struct_enum_tys.iter()
+                       .enumerate()
+                       .filter_map(|(index, ty)| match ty.sty {
+                           ty::TyEnum(adt_def, _) | ty::TyStruct(adt_def, _)
+                               if adt_def.did.is_local() =>
+                               Some((index, adt_def.did)),
+                           _ =>
+                               None,
+                       })
+                       .next()
+                       .unwrap(); // should always be SOME local type involved!
+
+    // Rotate so that the "main" type is at index 0.
+    let struct_enum_tys: Vec<_> =
+        struct_enum_tys.iter()
+                       .cloned()
+                       .skip(main_index)
+                       .chain(struct_enum_tys.iter().cloned().take(main_index))
+                       .collect();
+
+    let tcx = infcx.tcx;
+    let mut err = recursive_type_with_infinite_size_error(tcx, main_def_id);
+    let len = struct_enum_tys.len();
+    if len > 2 {
+        let span = tcx.map.span_if_local(main_def_id).unwrap();
+        err.fileline_note(span,
+                          &format!("type `{}` is embedded within `{}`...",
+                                   struct_enum_tys[0],
+                                   struct_enum_tys[1]));
+        for &next_ty in &struct_enum_tys[1..len-1] {
+            err.fileline_note(span,
+                              &format!("...which in turn is embedded within `{}`...", next_ty));
+        }
+        err.fileline_note(span,
+                          &format!("...which in turn is embedded within `{}`, \
+                                    completing the cycle.",
+                                   struct_enum_tys[len-1]));
+    }
+    err.emit();
+    infcx.tcx.sess.abort_if_errors();
+    unreachable!();
+}
+
+pub fn recursive_type_with_infinite_size_error<'tcx>(tcx: &ty::ctxt<'tcx>,
+                                                     type_def_id: DefId)
+                                                     -> DiagnosticBuilder<'tcx>
+{
+    assert!(type_def_id.is_local());
+    let span = tcx.map.span_if_local(type_def_id).unwrap();
+    let mut err = struct_span_err!(tcx.sess, span, E0072, "recursive type `{}` has infinite size",
+                                   tcx.item_path_str(type_def_id));
+    err.fileline_help(span, &format!("insert indirection (e.g., a `Box`, `Rc`, or `&`) \
+                                      at some point to make `{}` representable",
+                                     tcx.item_path_str(type_def_id)));
+    err
+}
+
 pub fn report_selection_error<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>,
                                         obligation: &PredicateObligation<'tcx>,
                                         error: &SelectionError<'tcx>)