From 944723b7731ec1eacdbc1946009bcd51d17a6301 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 28 Mar 2016 19:21:10 -0400 Subject: [PATCH 1/3] process cycles as soon as they are detected We used to wait for the recursion limit, but that might well be too long! --- src/librustc/traits/fulfill.rs | 288 ++++++++++++---------- src/test/compile-fail/issue-32326.rs | 20 ++ src/test/compile-fail/sized-cycle-note.rs | 10 +- 3 files changed, 188 insertions(+), 130 deletions(-) create mode 100644 src/test/compile-fail/issue-32326.rs diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index 321144126c9..6f3cc49daf2 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -320,103 +320,172 @@ impl<'tcx> FulfillmentContext<'tcx> { fn process_predicate<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, tree_cache: &mut LocalFulfilledPredicates<'tcx>, pending_obligation: &mut PendingPredicateObligation<'tcx>, - mut backtrace: Backtrace>, + backtrace: Backtrace>, region_obligations: &mut NodeMap>>) -> Result>>, FulfillmentErrorCode<'tcx>> { - match process_predicate1(selcx, pending_obligation, backtrace.clone(), region_obligations) { - Ok(Some(v)) => { - // FIXME(#30977) The code below is designed to detect (and - // permit) DAGs, while still ensuring that the reasoning - // is acyclic. However, it does a few things - // suboptimally. For example, it refreshes type variables - // a lot, probably more than needed, but also less than - // you might want. - // - // - more than needed: I want to be very sure we don't - // accidentally treat a cycle as a DAG, so I am - // refreshing type variables as we walk the ancestors; - // but we are going to repeat this a lot, which is - // sort of silly, and it would be nicer to refresh - // them *in place* so that later predicate processing - // can benefit from the same work; - // - less than you might want: we only add items in the cache here, - // but maybe we learn more about type variables and could add them into - // the cache later on. - - let tcx = selcx.tcx(); - - // Compute a little FnvHashSet for the ancestors. We only - // do this the first time that we care. - let mut cache = None; - let mut is_ancestor = |predicate: &ty::Predicate<'tcx>| { - if cache.is_none() { - let mut c = FnvHashSet(); - for ancestor in backtrace.by_ref() { - // Ugh. This just feels ridiculously - // inefficient. But we need to compare - // predicates without being concerned about - // the vagaries of type inference, so for now - // just ensure that they are always - // up-to-date. (I suppose we could just use a - // snapshot and check if they are unifiable?) - let resolved_predicate = - selcx.infcx().resolve_type_vars_if_possible( - &ancestor.obligation.predicate); - c.insert(resolved_predicate); - } - cache = Some(c); - } - - cache.as_ref().unwrap().contains(predicate) - }; - - let pending_predicate_obligations: Vec<_> = - v.into_iter() - .filter_map(|obligation| { - // Probably silly, but remove any inference - // variables. This is actually crucial to the - // ancestor check below, but it's not clear that - // it makes sense to ALWAYS do it. - let obligation = selcx.infcx().resolve_type_vars_if_possible(&obligation); - - // Screen out obligations that we know globally - // are true. This should really be the DAG check - // mentioned above. - if tcx.fulfilled_predicates.borrow().check_duplicate(&obligation.predicate) { - return None; - } - - // Check whether this obligation appears somewhere else in the tree. - if tree_cache.is_duplicate_or_add(&obligation.predicate) { - // If the obligation appears as a parent, - // allow it, because that is a cycle. - // Otherwise though we can just ignore - // it. Note that we have to be careful around - // inference variables here -- for the - // purposes of the ancestor check, we retain - // the invariant that all type variables are - // fully refreshed. - if !is_ancestor(&obligation.predicate) { - return None; - } - } - - Some(PendingPredicateObligation { - obligation: obligation, - stalled_on: vec![] - }) - }) - .collect(); - - Ok(Some(pending_predicate_obligations)) - } + match process_predicate1(selcx, pending_obligation, region_obligations) { + Ok(Some(v)) => process_child_obligations(selcx, + tree_cache, + &pending_obligation.obligation, + backtrace, + v), Ok(None) => Ok(None), Err(e) => Err(e) } } +fn process_child_obligations<'a,'tcx>( + selcx: &mut SelectionContext<'a,'tcx>, + tree_cache: &mut LocalFulfilledPredicates<'tcx>, + pending_obligation: &PredicateObligation<'tcx>, + backtrace: Backtrace>, + child_obligations: Vec>) + -> Result>>, + FulfillmentErrorCode<'tcx>> +{ + // FIXME(#30977) The code below is designed to detect (and + // permit) DAGs, while still ensuring that the reasoning + // is acyclic. However, it does a few things + // suboptimally. For example, it refreshes type variables + // a lot, probably more than needed, but also less than + // you might want. + // + // - more than needed: I want to be very sure we don't + // accidentally treat a cycle as a DAG, so I am + // refreshing type variables as we walk the ancestors; + // but we are going to repeat this a lot, which is + // sort of silly, and it would be nicer to refresh + // them *in place* so that later predicate processing + // can benefit from the same work; + // - less than you might want: we only add items in the cache here, + // but maybe we learn more about type variables and could add them into + // the cache later on. + + let tcx = selcx.tcx(); + + let mut ancestor_set = AncestorSet::new(&backtrace); + + let pending_predicate_obligations: Vec<_> = + child_obligations + .into_iter() + .filter_map(|obligation| { + // Probably silly, but remove any inference + // variables. This is actually crucial to the ancestor + // check marked (*) below, but it's not clear that it + // makes sense to ALWAYS do it. + let obligation = selcx.infcx().resolve_type_vars_if_possible(&obligation); + + // Screen out obligations that we know globally + // are true. + if tcx.fulfilled_predicates.borrow().check_duplicate(&obligation.predicate) { + return None; + } + + // Check whether this obligation appears + // somewhere else in the tree. If not, we have to + // process it for sure. + if !tree_cache.is_duplicate_or_add(&obligation.predicate) { + return Some(PendingPredicateObligation { + obligation: obligation, + stalled_on: vec![] + }); + } + + debug!("process_child_obligations: duplicate={:?}", + obligation.predicate); + + // OK, the obligation appears elsewhere in the tree. + // This is either a fatal error or else something we can + // ignore. If the obligation appears in our *ancestors* + // (rather than some more distant relative), that + // indicates a cycle. Cycles are either considered + // resolved (if this is a coinductive case) or a fatal + // error. + if let Some(index) = ancestor_set.has(selcx.infcx(), &obligation.predicate) { + // ~~~ (*) see above + debug!("process_child_obligations: cycle index = {}", index); + + if coinductive_match(selcx, &obligation, &backtrace) { + debug!("process_child_obligations: coinductive match"); + None + } else { + let backtrace = backtrace.clone(); + let cycle: Vec<_> = + iter::once(&obligation) + .chain(Some(pending_obligation)) + .chain(backtrace.take(index + 1).map(|p| &p.obligation)) + .cloned() + .collect(); + report_overflow_error_cycle(selcx.infcx(), &cycle); + } + } else { + // Not a cycle. Just ignore this obligation then, + // we're already in the process of proving it. + debug!("process_child_obligations: not a cycle"); + None + } + }) + .collect(); + + Ok(Some(pending_predicate_obligations)) +} + +struct AncestorSet<'b, 'tcx: 'b> { + populated: bool, + cache: FnvHashMap, usize>, + backtrace: Backtrace<'b, PendingPredicateObligation<'tcx>>, +} + +impl<'b, 'tcx> AncestorSet<'b, 'tcx> { + fn new(backtrace: &Backtrace<'b, PendingPredicateObligation<'tcx>>) -> Self { + AncestorSet { + populated: false, + cache: FnvHashMap(), + backtrace: backtrace.clone(), + } + } + + /// Checks whether any of the ancestors in the backtrace are equal + /// to `predicate` (`predicate` is assumed to be fully + /// type-resolved). Returns `None` if not; otherwise, returns + /// `Some` with the index within the backtrace. + fn has<'a>(&mut self, + infcx: &InferCtxt<'a, 'tcx>, + predicate: &ty::Predicate<'tcx>) + -> Option { + // the first time, we have to populate the cache + if !self.populated { + let backtrace = self.backtrace.clone(); + for (index, ancestor) in backtrace.enumerate() { + // Ugh. This just feels ridiculously + // inefficient. But we need to compare + // predicates without being concerned about + // the vagaries of type inference, so for now + // just ensure that they are always + // up-to-date. (I suppose we could just use a + // snapshot and check if they are unifiable?) + let resolved_predicate = + infcx.resolve_type_vars_if_possible( + &ancestor.obligation.predicate); + + // Though we try to avoid it, it can happen that a + // cycle already exists in the predecessors. This + // happens if the type variables were not fully known + // at the time that the ancestors were pushed. We'll + // just ignore such cycles for now, on the premise + // that they will repeat themselves and we'll deal + // with them properly then. + self.cache.entry(resolved_predicate) + .or_insert(index); + } + self.populated = true; + } + + self.cache.get(predicate).cloned() + } +} /// Return the set of type variables contained in a trait ref fn trait_ref_type_vars<'a, 'tcx>(selcx: &mut SelectionContext<'a, 'tcx>, @@ -438,7 +507,6 @@ fn trait_ref_type_vars<'a, 'tcx>(selcx: &mut SelectionContext<'a, 'tcx>, /// - `Err` if the predicate does not hold fn process_predicate1<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, pending_obligation: &mut PendingPredicateObligation<'tcx>, - backtrace: Backtrace>, region_obligations: &mut NodeMap>>) -> Result>>, FulfillmentErrorCode<'tcx>> @@ -461,16 +529,6 @@ fn process_predicate1<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, let obligation = &mut pending_obligation.obligation; - // If we exceed the recursion limit, take a moment to look for a - // cycle so we can give a better error report from here, where we - // have more context. - let recursion_limit = selcx.tcx().sess.recursion_limit.get(); - if obligation.recursion_depth >= recursion_limit { - if let Some(cycle) = scan_for_cycle(obligation, &backtrace) { - report_overflow_error_cycle(selcx.infcx(), &cycle); - } - } - if obligation.predicate.has_infer_types() { obligation.predicate = selcx.infcx().resolve_type_vars_if_possible(&obligation.predicate); } @@ -481,10 +539,6 @@ fn process_predicate1<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, return Ok(Some(vec![])); } - if coinductive_match(selcx, obligation, data, &backtrace) { - return Ok(Some(vec![])); - } - let trait_obligation = obligation.with(data.clone()); match selcx.select(&trait_obligation) { Ok(Some(vtable)) => { @@ -616,10 +670,15 @@ fn process_predicate1<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, /// also defaulted traits. fn coinductive_match<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, top_obligation: &PredicateObligation<'tcx>, - top_data: &ty::PolyTraitPredicate<'tcx>, backtrace: &Backtrace>) -> bool { + // only trait predicates can be coinductive matches + let top_data = match top_obligation.predicate { + ty::Predicate::Trait(ref data) => data, + _ => return false + }; + if selcx.tcx().trait_has_default_impl(top_data.def_id()) { debug!("coinductive_match: top_data={:?}", top_data); for bt_obligation in backtrace.clone() { @@ -647,27 +706,6 @@ fn coinductive_match<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, false } -fn scan_for_cycle<'a,'tcx>(top_obligation: &PredicateObligation<'tcx>, - backtrace: &Backtrace>) - -> Option>> -{ - let mut map = FnvHashMap(); - let all_obligations = - || iter::once(top_obligation) - .chain(backtrace.clone() - .map(|p| &p.obligation)); - for (index, bt_obligation) in all_obligations().enumerate() { - if let Some(&start) = map.get(&bt_obligation.predicate) { - // Found a cycle starting at position `start` and running - // until the current position (`index`). - return Some(all_obligations().skip(start).take(index - start + 1).cloned().collect()); - } else { - map.insert(bt_obligation.predicate.clone(), index); - } - } - None -} - fn register_region_obligation<'tcx>(t_a: Ty<'tcx>, r_b: ty::Region, cause: ObligationCause<'tcx>, diff --git a/src/test/compile-fail/issue-32326.rs b/src/test/compile-fail/issue-32326.rs new file mode 100644 index 00000000000..8af243afc22 --- /dev/null +++ b/src/test/compile-fail/issue-32326.rs @@ -0,0 +1,20 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Regression test for #32326. We ran out of memory because we +// attempted to expand this case up to the recursion limit, and 2^N is +// too big. + +enum Expr { //~ ERROR E0072 + Plus(Expr, Expr), + Literal(i64), +} + +fn main() { } diff --git a/src/test/compile-fail/sized-cycle-note.rs b/src/test/compile-fail/sized-cycle-note.rs index ec378d05ba5..3d7c4868e96 100644 --- a/src/test/compile-fail/sized-cycle-note.rs +++ b/src/test/compile-fail/sized-cycle-note.rs @@ -20,11 +20,11 @@ struct Baz { q: Option } struct Foo { q: Option } //~^ ERROR recursive type `Foo` has infinite size -//~| type `Foo` is embedded within `std::option::Option`... -//~| ...which in turn is embedded within `std::option::Option`... -//~| ...which in turn is embedded within `Baz`... -//~| ...which in turn is embedded within `std::option::Option`... -//~| ...which in turn is embedded within `Foo`, completing the cycle. +//~| NOTE type `Foo` is embedded within `std::option::Option`... +//~| NOTE ...which in turn is embedded within `std::option::Option`... +//~| NOTE ...which in turn is embedded within `Baz`... +//~| NOTE ...which in turn is embedded within `std::option::Option`... +//~| NOTE ...which in turn is embedded within `Foo`, completing the cycle. impl Foo { fn bar(&self) {} } From 6a749c7eb136e5ddc61a34d2adc4c8fc6a1725cf Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 29 Mar 2016 16:33:50 -0400 Subject: [PATCH 2/3] fix corner case around top of stack When deciding on a coinductive match, we were examining the new obligation and the backtrace, but not the *current* obligation that goes in between the two. Refactoring the code to just have the cycle given as input also made things a lot simpler. --- src/librustc/traits/fulfill.rs | 67 +++++++++---------- ...its-inductive-overflow-auto-normal-auto.rs | 32 +++++++++ 2 files changed, 62 insertions(+), 37 deletions(-) create mode 100644 src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index 6f3cc49daf2..810dfb960c6 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -407,17 +407,17 @@ fn process_child_obligations<'a,'tcx>( // ~~~ (*) see above debug!("process_child_obligations: cycle index = {}", index); - if coinductive_match(selcx, &obligation, &backtrace) { + let backtrace = backtrace.clone(); + let cycle: Vec<_> = + iter::once(&obligation) + .chain(Some(pending_obligation)) + .chain(backtrace.take(index + 1).map(|p| &p.obligation)) + .cloned() + .collect(); + if coinductive_match(selcx, &cycle) { debug!("process_child_obligations: coinductive match"); None } else { - let backtrace = backtrace.clone(); - let cycle: Vec<_> = - iter::once(&obligation) - .chain(Some(pending_obligation)) - .chain(backtrace.take(index + 1).map(|p| &p.obligation)) - .cloned() - .collect(); report_overflow_error_cycle(selcx.infcx(), &cycle); } } else { @@ -663,47 +663,40 @@ fn process_predicate1<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, /// For defaulted traits, we use a co-inductive strategy to solve, so /// that recursion is ok. This routine returns true if the top of the -/// stack (`top_obligation` and `top_data`): +/// stack (`cycle[0]`): /// - is a defaulted trait, and /// - it also appears in the backtrace at some position `X`; and, /// - all the predicates at positions `X..` between `X` an the top are /// also defaulted traits. fn coinductive_match<'a,'tcx>(selcx: &mut SelectionContext<'a,'tcx>, - top_obligation: &PredicateObligation<'tcx>, - backtrace: &Backtrace>) + cycle: &[PredicateObligation<'tcx>]) -> bool { - // only trait predicates can be coinductive matches - let top_data = match top_obligation.predicate { - ty::Predicate::Trait(ref data) => data, - _ => return false - }; + let len = cycle.len(); - if selcx.tcx().trait_has_default_impl(top_data.def_id()) { - debug!("coinductive_match: top_data={:?}", top_data); - for bt_obligation in backtrace.clone() { - debug!("coinductive_match: bt_obligation={:?}", bt_obligation); + assert_eq!(cycle[0].predicate, cycle[len - 1].predicate); - // *Everything* in the backtrace must be a defaulted trait. - match bt_obligation.obligation.predicate { - ty::Predicate::Trait(ref data) => { - if !selcx.tcx().trait_has_default_impl(data.def_id()) { - debug!("coinductive_match: trait does not have default impl"); - break; - } - } - _ => { break; } - } + cycle[0..len-1] + .iter() + .all(|bt_obligation| { + let result = coinductive_obligation(selcx, bt_obligation); + debug!("coinductive_match: bt_obligation={:?} coinductive={}", + bt_obligation, result); + result + }) +} - // And we must find a recursive match. - if bt_obligation.obligation.predicate == top_obligation.predicate { - debug!("coinductive_match: found a match in the backtrace"); - return true; - } +fn coinductive_obligation<'a, 'tcx>(selcx: &SelectionContext<'a, 'tcx>, + obligation: &PredicateObligation<'tcx>) + -> bool { + match obligation.predicate { + ty::Predicate::Trait(ref data) => { + selcx.tcx().trait_has_default_impl(data.def_id()) + } + _ => { + false } } - - false } fn register_region_obligation<'tcx>(t_a: Ty<'tcx>, diff --git a/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs b/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs new file mode 100644 index 00000000000..6015c5669cd --- /dev/null +++ b/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs @@ -0,0 +1,32 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test for a potential corner case in current impl where you have an +// auto trait (Magic1) that depends on a normal trait (Magic2) which +// in turn depends on the auto trait (Magic1). This was incorrectly +// being considered coinductive, but because of the normal trait +// interfering, it should not be. + +#![feature(optin_builtin_traits)] + +trait Magic1: Magic2 { } +impl Magic1 for .. {} + +trait Magic2 { } +impl Magic2 for T { } + +fn is_magic1() { } + +#[derive(Debug)] +struct NoClone; + +fn main() { + is_magic1::(); +} From e733b86ae3c2d8b86fe44f6d5c6c94ac36388ec2 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 4 Apr 2016 12:31:29 -0400 Subject: [PATCH 3/3] add error code to test --- .../compile-fail/traits-inductive-overflow-auto-normal-auto.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs b/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs index 6015c5669cd..cdf4b405fd8 100644 --- a/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs +++ b/src/test/compile-fail/traits-inductive-overflow-auto-normal-auto.rs @@ -28,5 +28,5 @@ fn is_magic1() { } struct NoClone; fn main() { - is_magic1::(); + is_magic1::(); //~ ERROR E0275 }