From d049e5d19e4058dfdbf1ce54770a0e3ca36e5dd3 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 23 Nov 2017 23:03:47 +0200 Subject: [PATCH 1/2] avoid type-live-for-region obligations on dummy nodes Type-live-for-region obligations on DUMMY_NODE_ID cause an ICE, and it turns out that in the few cases they are needed, these obligations are not needed anyway because they are verified elsewhere. Fixes #46069. --- src/librustc/infer/outlives/obligations.rs | 1 + src/librustc/traits/fulfill.rs | 64 ++++++++++++++----- src/librustc/traits/mod.rs | 20 ++++-- src/librustc/traits/specialize/mod.rs | 2 +- src/librustc/traits/structural_impls.rs | 15 +++-- .../transform/nll/constraint_generation.rs | 13 +++- src/test/run-pass/issue-46069.rs | 32 ++++++++++ 7 files changed, 117 insertions(+), 30 deletions(-) create mode 100644 src/test/run-pass/issue-46069.rs diff --git a/src/librustc/infer/outlives/obligations.rs b/src/librustc/infer/outlives/obligations.rs index c7081e59ec36f..221d378d62186 100644 --- a/src/librustc/infer/outlives/obligations.rs +++ b/src/librustc/infer/outlives/obligations.rs @@ -88,6 +88,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { body_id: ast::NodeId, obligation: RegionObligation<'tcx>, ) { + debug!("register_region_obligation({:?}, {:?})", body_id, obligation); self.region_obligations .borrow_mut() .push((body_id, obligation)); diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index 6b681322c9bf5..f56c3853de0ab 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -46,6 +46,19 @@ pub struct FulfillmentContext<'tcx> { // A list of all obligations that have been registered with this // fulfillment context. predicates: ObligationForest>, + // Should this fulfillment context register type-lives-for-region + // obligations on its parent infcx? In some cases, region + // obligations are either already known to hold (normalization) or + // hopefully verifed elsewhere (type-impls-bound), and therefore + // should not be checked. + // + // Note that if we are normalizing a type that we already + // know is well-formed, there should be no harm setting this + // to true - all the region variables should be determinable + // using the RFC 447 rules, which don't depend on + // type-lives-for-region constraints, and because the type + // is well-formed, the constraints should hold. + register_region_obligations: bool, } #[derive(Clone, Debug)] @@ -59,6 +72,14 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> { pub fn new() -> FulfillmentContext<'tcx> { FulfillmentContext { predicates: ObligationForest::new(), + register_region_obligations: true + } + } + + pub fn new_ignoring_regions() -> FulfillmentContext<'tcx> { + FulfillmentContext { + predicates: ObligationForest::new(), + register_region_obligations: false } } @@ -191,7 +212,10 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> { debug!("select: starting another iteration"); // Process pending obligations. - let outcome = self.predicates.process_obligations(&mut FulfillProcessor { selcx }); + let outcome = self.predicates.process_obligations(&mut FulfillProcessor { + selcx, + register_region_obligations: self.register_region_obligations + }); debug!("select: outcome={:?}", outcome); // FIXME: if we kept the original cache key, we could mark projection @@ -220,6 +244,7 @@ impl<'a, 'gcx, 'tcx> FulfillmentContext<'tcx> { struct FulfillProcessor<'a, 'b: 'a, 'gcx: 'tcx, 'tcx: 'b> { selcx: &'a mut SelectionContext<'b, 'gcx, 'tcx>, + register_region_obligations: bool } impl<'a, 'b, 'gcx, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'gcx, 'tcx> { @@ -230,7 +255,7 @@ impl<'a, 'b, 'gcx, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'gcx, obligation: &mut Self::Obligation) -> Result>, Self::Error> { - process_predicate(self.selcx, obligation) + process_predicate(self.selcx, obligation, self.register_region_obligations) .map(|os| os.map(|os| os.into_iter().map(|o| PendingPredicateObligation { obligation: o, stalled_on: vec![] @@ -269,7 +294,8 @@ fn trait_ref_type_vars<'a, 'gcx, 'tcx>(selcx: &mut SelectionContext<'a, 'gcx, 't /// - `Err` if the predicate does not hold fn process_predicate<'a, 'gcx, 'tcx>( selcx: &mut SelectionContext<'a, 'gcx, 'tcx>, - pending_obligation: &mut PendingPredicateObligation<'tcx>) + pending_obligation: &mut PendingPredicateObligation<'tcx>, + register_region_obligations: bool) -> Result>>, FulfillmentErrorCode<'tcx>> { @@ -391,26 +417,30 @@ fn process_predicate<'a, 'gcx, 'tcx>( // `for<'a> T: 'a where 'a not in T`, which we can treat as `T: 'static`. Some(t_a) => { let r_static = selcx.tcx().types.re_static; - selcx.infcx().register_region_obligation( - obligation.cause.body_id, - RegionObligation { - sup_type: t_a, - sub_region: r_static, - cause: obligation.cause.clone(), - }); + if register_region_obligations { + selcx.infcx().register_region_obligation( + obligation.cause.body_id, + RegionObligation { + sup_type: t_a, + sub_region: r_static, + cause: obligation.cause.clone(), + }); + } Ok(Some(vec![])) } } } // If there aren't, register the obligation. Some(ty::OutlivesPredicate(t_a, r_b)) => { - selcx.infcx().register_region_obligation( - obligation.cause.body_id, - RegionObligation { - sup_type: t_a, - sub_region: r_b, - cause: obligation.cause.clone() - }); + if register_region_obligations { + selcx.infcx().register_region_obligation( + obligation.cause.body_id, + RegionObligation { + sup_type: t_a, + sub_region: r_b, + cause: obligation.cause.clone() + }); + } Ok(Some(vec![])) } } diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 55b1a913f0d1d..55bd10e922535 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -431,7 +431,7 @@ pub fn type_known_to_meet_bound<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx // this function's result remains infallible, we must confirm // that guess. While imperfect, I believe this is sound. - let mut fulfill_cx = FulfillmentContext::new(); + let mut fulfill_cx = FulfillmentContext::new_ignoring_regions(); // We can use a dummy node-id here because we won't pay any mind // to region obligations that arise (there shouldn't really be any @@ -583,9 +583,6 @@ pub fn fully_normalize<'a, 'gcx, 'tcx, T>(infcx: &InferCtxt<'a, 'gcx, 'tcx>, -> Result>> where T : TypeFoldable<'tcx> { - debug!("fully_normalize(value={:?})", value); - - let selcx = &mut SelectionContext::new(infcx); // FIXME (@jroesch) ISSUE 26721 // I'm not sure if this is a bug or not, needs further investigation. // It appears that by reusing the fulfillment_cx here we incur more @@ -599,8 +596,21 @@ pub fn fully_normalize<'a, 'gcx, 'tcx, T>(infcx: &InferCtxt<'a, 'gcx, 'tcx>, // // I think we should probably land this refactor and then come // back to this is a follow-up patch. - let mut fulfill_cx = FulfillmentContext::new(); + let fulfillcx = FulfillmentContext::new(); + fully_normalize_with_fulfillcx(infcx, fulfillcx, cause, param_env, value) +} +pub fn fully_normalize_with_fulfillcx<'a, 'gcx, 'tcx, T>( + infcx: &InferCtxt<'a, 'gcx, 'tcx>, + mut fulfill_cx: FulfillmentContext<'tcx>, + cause: ObligationCause<'tcx>, + param_env: ty::ParamEnv<'tcx>, + value: &T) + -> Result>> + where T : TypeFoldable<'tcx> +{ + debug!("fully_normalize_with_fulfillcx(value={:?})", value); + let selcx = &mut SelectionContext::new(infcx); let Normalized { value: normalized_value, obligations } = project::normalize(selcx, param_env, cause, value); debug!("fully_normalize: normalized_value={:?} obligations={:?}", diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index d8d0715ff3957..f83e131c32765 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -241,7 +241,7 @@ fn fulfill_implication<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>, // (which are packed up in penv) infcx.save_and_restore_in_snapshot_flag(|infcx| { - let mut fulfill_cx = FulfillmentContext::new(); + let mut fulfill_cx = FulfillmentContext::new_ignoring_regions(); for oblig in obligations.into_iter() { fulfill_cx.register_predicate_obligation(&infcx, oblig); } diff --git a/src/librustc/traits/structural_impls.rs b/src/librustc/traits/structural_impls.rs index 9231995018065..e1e2798ecb51c 100644 --- a/src/librustc/traits/structural_impls.rs +++ b/src/librustc/traits/structural_impls.rs @@ -10,7 +10,7 @@ use traits; use traits::project::Normalized; -use ty::{Lift, TyCtxt}; +use ty::{self, Lift, TyCtxt}; use ty::fold::{TypeFoldable, TypeFolder, TypeVisitor}; use std::fmt; @@ -28,9 +28,16 @@ impl<'tcx, T: fmt::Debug> fmt::Debug for Normalized<'tcx, T> { impl<'tcx, O: fmt::Debug> fmt::Debug for traits::Obligation<'tcx, O> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "Obligation(predicate={:?},depth={})", - self.predicate, - self.recursion_depth) + if ty::tls::with(|tcx| tcx.sess.verbose()) { + write!(f, "Obligation(predicate={:?},cause={:?},depth={})", + self.predicate, + self.cause, + self.recursion_depth) + } else { + write!(f, "Obligation(predicate={:?},depth={})", + self.predicate, + self.recursion_depth) + } } } diff --git a/src/librustc_mir/transform/nll/constraint_generation.rs b/src/librustc_mir/transform/nll/constraint_generation.rs index 1f905d32f84e8..479680cc7fe79 100644 --- a/src/librustc_mir/transform/nll/constraint_generation.rs +++ b/src/librustc_mir/transform/nll/constraint_generation.rs @@ -152,10 +152,17 @@ impl<'cx, 'gcx, 'tcx> ConstraintGeneration<'cx, 'gcx, 'tcx> { // associated types and parameters). We need to normalize // associated types here and possibly recursively process. for ty in dtorck_types { - // FIXME -- I think that this may disregard some region obligations - // or something. Do we care? -nmatsakis let cause = ObligationCause::dummy(); - match traits::fully_normalize(self.infcx, cause, self.param_env, &ty) { + // We know that our original `dropped_ty` is well-formed, + // so region obligations resulting from this normalization + // should always hold. + // + // Therefore we ignore them instead of trying to match + // them up with a location. + let fulfillcx = traits::FulfillmentContext::new_ignoring_regions(); + match traits::fully_normalize_with_fulfillcx( + self.infcx, fulfillcx, cause, self.param_env, &ty + ) { Ok(ty) => match ty.sty { ty::TyParam(..) | ty::TyProjection(..) | ty::TyAnon(..) => { self.add_regular_live_constraint(ty, location); diff --git a/src/test/run-pass/issue-46069.rs b/src/test/run-pass/issue-46069.rs new file mode 100644 index 0000000000000..70db20e4a6c92 --- /dev/null +++ b/src/test/run-pass/issue-46069.rs @@ -0,0 +1,32 @@ +// Copyright 2017 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. + +use std::iter::{Fuse, Cloned}; +use std::slice::Iter; + +struct Foo<'a, T: 'a>(&'a T); +impl<'a, T: 'a> Copy for Foo<'a, T> {} +impl<'a, T: 'a> Clone for Foo<'a, T> { + fn clone(&self) -> Self { *self } +} + +fn copy_ex() { + let s = 2; + let k1 = || s; + let upvar = Foo(&k1); + let k = || upvar; + k(); +} + +fn main() { + let _f = 0 as *mut >> as Iterator>::Item; + + copy_ex(); +} From ebd219ab8e7be56020436f9c6d6a44c6d4f4a5cf Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 23 Nov 2017 23:21:56 +0200 Subject: [PATCH 2/2] comments --- src/librustc/infer/outlives/obligations.rs | 12 --------- src/librustc/traits/mod.rs | 31 ++++++++++++++-------- src/librustc/traits/specialize/mod.rs | 11 ++++++++ 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/librustc/infer/outlives/obligations.rs b/src/librustc/infer/outlives/obligations.rs index 221d378d62186..3c3aba372fbd0 100644 --- a/src/librustc/infer/outlives/obligations.rs +++ b/src/librustc/infer/outlives/obligations.rs @@ -181,18 +181,6 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { TypeOutlives::new(self, region_bound_pairs, implicit_region_bound, param_env); outlives.type_must_outlive(origin, ty, region); } - - /// Ignore the region obligations, not bothering to prove - /// them. This function should not really exist; it is used to - /// accommodate some older code for the time being. - pub fn ignore_region_obligations(&self) { - assert!( - !self.in_snapshot.get(), - "cannot ignore registered region obligations in a snapshot" - ); - - self.region_obligations.borrow_mut().clear(); - } } #[must_use] // you ought to invoke `into_accrued_obligations` when you are done =) diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 55bd10e922535..a3a5c26ec186a 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -431,6 +431,9 @@ pub fn type_known_to_meet_bound<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx // this function's result remains infallible, we must confirm // that guess. While imperfect, I believe this is sound. + // The handling of regions in this area of the code is terrible, + // see issue #29149. We should be able to improve on this with + // NLL. let mut fulfill_cx = FulfillmentContext::new_ignoring_regions(); // We can use a dummy node-id here because we won't pay any mind @@ -511,8 +514,24 @@ pub fn normalize_param_env_or_error<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, unnormalized_env.reveal); tcx.infer_ctxt().enter(|infcx| { - let predicates = match fully_normalize( + // FIXME. We should really... do something with these region + // obligations. But this call just continues the older + // behavior (i.e., doesn't cause any new bugs), and it would + // take some further refactoring to actually solve them. In + // particular, we would have to handle implied bounds + // properly, and that code is currently largely confined to + // regionck (though I made some efforts to extract it + // out). -nmatsakis + // + // @arielby: In any case, these obligations are checked + // by wfcheck anyway, so I'm not sure we have to check + // them here too, and we will remove this function when + // we move over to lazy normalization *anyway*. + let fulfill_cx = FulfillmentContext::new_ignoring_regions(); + + let predicates = match fully_normalize_with_fulfillcx( &infcx, + fulfill_cx, cause, elaborated_env, // You would really want to pass infcx.param_env.caller_bounds here, @@ -537,16 +556,6 @@ pub fn normalize_param_env_or_error<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let region_scope_tree = region::ScopeTree::default(); let free_regions = FreeRegionMap::new(); - // FIXME. We should really... do something with these region - // obligations. But this call just continues the older - // behavior (i.e., doesn't cause any new bugs), and it would - // take some further refactoring to actually solve them. In - // particular, we would have to handle implied bounds - // properly, and that code is currently largely confined to - // regionck (though I made some efforts to extract it - // out). -nmatsakis - let _ = infcx.ignore_region_obligations(); - infcx.resolve_regions_and_report_errors(region_context, ®ion_scope_tree, &free_regions); let predicates = match infcx.fully_resolve(&predicates) { Ok(predicates) => predicates, diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index f83e131c32765..1b5b0d35ba390 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -241,6 +241,17 @@ fn fulfill_implication<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>, // (which are packed up in penv) infcx.save_and_restore_in_snapshot_flag(|infcx| { + // If we came from `translate_substs`, we already know that the + // predicates for our impl hold (after all, we know that a more + // specialized impl holds, so our impl must hold too), and + // we only want to process the projections to determine the + // the types in our substs using RFC 447, so we can safely + // ignore region obligations, which allows us to avoid threading + // a node-id to assign them with. + // + // If we came from specialization graph construction, then + // we already make a mockery out of the region system, so + // why not ignore them a bit earlier? let mut fulfill_cx = FulfillmentContext::new_ignoring_regions(); for oblig in obligations.into_iter() { fulfill_cx.register_predicate_obligation(&infcx, oblig);