From 168c14a1a59de279534c09a1b4e23b3b7f8f774d Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sun, 10 Feb 2019 09:53:52 +0000 Subject: [PATCH 1/3] Avoid propagating redundant outlives constraints from closures --- .../borrow_check/nll/region_infer/mod.rs | 142 ++++++++++++------ 1 file changed, 93 insertions(+), 49 deletions(-) diff --git a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs index 6de05777fe888..7a0cd2ac26c3b 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs @@ -15,7 +15,7 @@ use rustc::mir::{ ConstraintCategory, Local, Location, Mir, }; use rustc::ty::{self, RegionVid, Ty, TyCtxt, TypeFoldable}; -use rustc::util::common; +use rustc::util::common::{self, ErrorReported}; use rustc_data_structures::bit_set::BitSet; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::graph::scc::Sccs; @@ -1157,63 +1157,107 @@ impl<'tcx> RegionInferenceContext<'tcx> { .is_none() ); + // Only check all of the relations for the main representative of each + // SCC, otherwise just check that we outlive said representative. This + // reduces the number of redundant relations propagated out of + // closures. + // Note that the representative will be a universal region if there is + // one in this SCC, so we will always check the representative here. + let representative = self.scc_representatives[longer_fr_scc]; + if representative != longer_fr { + self.check_universal_region_relation( + longer_fr, + representative, + infcx, + mir, + mir_def_id, + propagated_outlives_requirements, + errors_buffer, + ); + return; + } + // Find every region `o` such that `fr: o` // (because `fr` includes `end(o)`). for shorter_fr in self.scc_values.universal_regions_outlived_by(longer_fr_scc) { - // If it is known that `fr: o`, carry on. - if self.universal_region_relations - .outlives(longer_fr, shorter_fr) - { - continue; + if let Some(ErrorReported) = self.check_universal_region_relation( + longer_fr, + shorter_fr, + infcx, + mir, + mir_def_id, + propagated_outlives_requirements, + errors_buffer, + ) { + // continuing to iterate just reports more errors than necessary + return; } + } + } - debug!( - "check_universal_region: fr={:?} does not outlive shorter_fr={:?}", - longer_fr, shorter_fr, - ); + fn check_universal_region_relation( + &self, + longer_fr: RegionVid, + shorter_fr: RegionVid, + infcx: &InferCtxt<'_, 'gcx, 'tcx>, + mir: &Mir<'tcx>, + mir_def_id: DefId, + propagated_outlives_requirements: &mut Option<&mut Vec>>, + errors_buffer: &mut Vec, + ) -> Option { + // If it is known that `fr: o`, carry on. + if self.universal_region_relations + .outlives(longer_fr, shorter_fr) + { + return None; + } - let blame_span_category = self.find_outlives_blame_span(mir, longer_fr, shorter_fr); - - if let Some(propagated_outlives_requirements) = propagated_outlives_requirements { - // Shrink `fr` until we find a non-local region (if we do). - // We'll call that `fr-` -- it's ever so slightly smaller than `fr`. - if let Some(fr_minus) = self.universal_region_relations - .non_local_lower_bound(longer_fr) - { - debug!("check_universal_region: fr_minus={:?}", fr_minus); - - // Grow `shorter_fr` until we find a non-local - // region. (We always will.) We'll call that - // `shorter_fr+` -- it's ever so slightly larger than - // `fr`. - let shorter_fr_plus = self.universal_region_relations - .non_local_upper_bound(shorter_fr); - debug!( - "check_universal_region: shorter_fr_plus={:?}", - shorter_fr_plus - ); + debug!( + "check_universal_region_relation: fr={:?} does not outlive shorter_fr={:?}", + longer_fr, shorter_fr, + ); - // Push the constraint `fr-: shorter_fr+` - propagated_outlives_requirements.push(ClosureOutlivesRequirement { - subject: ClosureOutlivesSubject::Region(fr_minus), - outlived_free_region: shorter_fr_plus, - blame_span: blame_span_category.1, - category: blame_span_category.0, - }); - continue; - } - } - // If we are not in a context where we can propagate - // errors, or we could not shrink `fr` to something - // smaller, then just report an error. - // - // Note: in this case, we use the unapproximated regions - // to report the error. This gives better error messages - // in some cases. - self.report_error(mir, infcx, mir_def_id, longer_fr, shorter_fr, errors_buffer); - return; // continuing to iterate just reports more errors than necessary + if let Some(propagated_outlives_requirements) = propagated_outlives_requirements { + // Shrink `fr` until we find a non-local region (if we do). + // We'll call that `fr-` -- it's ever so slightly smaller than `fr`. + if let Some(fr_minus) = self.universal_region_relations + .non_local_lower_bound(longer_fr) + { + debug!("check_universal_region: fr_minus={:?}", fr_minus); + + let blame_span_category = self.find_outlives_blame_span(mir, longer_fr, shorter_fr); + + // Grow `shorter_fr` until we find a non-local + // region. (We always will.) We'll call that + // `shorter_fr+` -- it's ever so slightly larger than + // `fr`. + let shorter_fr_plus = self.universal_region_relations + .non_local_upper_bound(shorter_fr); + debug!( + "check_universal_region: shorter_fr_plus={:?}", + shorter_fr_plus + ); + + // Push the constraint `fr-: shorter_fr+` + propagated_outlives_requirements.push(ClosureOutlivesRequirement { + subject: ClosureOutlivesSubject::Region(fr_minus), + outlived_free_region: shorter_fr_plus, + blame_span: blame_span_category.1, + category: blame_span_category.0, + }); + return None; + } } + + // If we are not in a context where we can't propagate errors, or we + // could not shrink `fr` to something smaller, then just report an + // error. + // + // Note: in this case, we use the unapproximated regions to report the + // error. This gives better error messages in some cases. + self.report_error(mir, infcx, mir_def_id, longer_fr, shorter_fr, errors_buffer); + Some(ErrorReported) } fn check_bound_universal_region<'gcx>( From ea613f30d9728e9bc283ec213225ea8bdaa18f7c Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 6 Feb 2019 14:11:09 +0100 Subject: [PATCH 2/3] Buffer and migrate nice region errors --- .../error_reporting/nice_region_error/mod.rs | 6 +++-- .../nice_region_error/named_anon_conflict.rs | 27 ++++++++++--------- .../nice_region_error/placeholder_error.rs | 8 +++--- .../nll/region_infer/error_reporting/mod.rs | 3 ++- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/librustc/infer/error_reporting/nice_region_error/mod.rs b/src/librustc/infer/error_reporting/nice_region_error/mod.rs index dad1e3ba80da3..d995fe92337c4 100644 --- a/src/librustc/infer/error_reporting/nice_region_error/mod.rs +++ b/src/librustc/infer/error_reporting/nice_region_error/mod.rs @@ -1,9 +1,10 @@ use crate::infer::InferCtxt; use crate::infer::lexical_region_resolve::RegionResolutionError; use crate::infer::lexical_region_resolve::RegionResolutionError::*; -use syntax::source_map::Span; use crate::ty::{self, TyCtxt}; use crate::util::common::ErrorReported; +use errors::DiagnosticBuilder; +use syntax::source_map::Span; mod different_lifetimes; mod find_anon_type; @@ -59,7 +60,7 @@ impl<'cx, 'gcx, 'tcx> NiceRegionError<'cx, 'gcx, 'tcx> { self.infcx.tcx } - pub fn try_report_from_nll(&self) -> Option { + pub fn try_report_from_nll(&self) -> Option> { // Due to the improved diagnostics returned by the MIR borrow checker, only a subset of // the nice region errors are required when running under the MIR borrow checker. self.try_report_named_anon_conflict() @@ -68,6 +69,7 @@ impl<'cx, 'gcx, 'tcx> NiceRegionError<'cx, 'gcx, 'tcx> { pub fn try_report(&self) -> Option { self.try_report_from_nll() + .map(|mut diag| { diag.emit(); ErrorReported }) .or_else(|| self.try_report_anon_anon_conflict()) .or_else(|| self.try_report_outlives_closure()) .or_else(|| self.try_report_static_impl_trait()) diff --git a/src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs b/src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs index b10af400f2b6c..3821484d38e5f 100644 --- a/src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs +++ b/src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs @@ -2,13 +2,12 @@ //! where one region is named and the other is anonymous. use crate::infer::error_reporting::nice_region_error::NiceRegionError; use crate::ty; -use crate::util::common::ErrorReported; -use errors::Applicability; +use errors::{Applicability, DiagnosticBuilder}; impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> { /// When given a `ConcreteFailure` for a function with arguments containing a named region and /// an anonymous region, emit an descriptive diagnostic error. - pub(super) fn try_report_named_anon_conflict(&self) -> Option { + pub(super) fn try_report_named_anon_conflict(&self) -> Option> { let (span, sub, sup) = self.get_regions(); debug!( @@ -96,21 +95,23 @@ impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> { ("parameter type".to_owned(), "type".to_owned()) }; - struct_span_err!( + let mut diag = struct_span_err!( self.tcx().sess, span, E0621, "explicit lifetime required in {}", error_var - ).span_suggestion( - new_ty_span, - &format!("add explicit lifetime `{}` to {}", named, span_label_var), - new_ty.to_string(), - Applicability::Unspecified, - ) - .span_label(span, format!("lifetime `{}` required", named)) - .emit(); - return Some(ErrorReported); + ); + + diag.span_suggestion( + new_ty_span, + &format!("add explicit lifetime `{}` to {}", named, span_label_var), + new_ty.to_string(), + Applicability::Unspecified, + ) + .span_label(span, format!("lifetime `{}` required", named)); + + Some(diag) } // This method returns whether the given Region is Named diff --git a/src/librustc/infer/error_reporting/nice_region_error/placeholder_error.rs b/src/librustc/infer/error_reporting/nice_region_error/placeholder_error.rs index 843fa8b0182e2..3b2fb7d41008e 100644 --- a/src/librustc/infer/error_reporting/nice_region_error/placeholder_error.rs +++ b/src/librustc/infer/error_reporting/nice_region_error/placeholder_error.rs @@ -8,13 +8,12 @@ use crate::traits::{ObligationCause, ObligationCauseCode}; use crate::ty; use crate::ty::error::ExpectedFound; use crate::ty::subst::Substs; -use crate::util::common::ErrorReported; use crate::util::ppaux::RegionHighlightMode; impl NiceRegionError<'me, 'gcx, 'tcx> { /// When given a `ConcreteFailure` for a function with arguments containing a named region and /// an anonymous region, emit a descriptive diagnostic error. - pub(super) fn try_report_placeholder_conflict(&self) -> Option { + pub(super) fn try_report_placeholder_conflict(&self) -> Option> { match &self.error { /////////////////////////////////////////////////////////////////////////// // NB. The ordering of cases in this match is very @@ -178,7 +177,7 @@ impl NiceRegionError<'me, 'gcx, 'tcx> { trait_def_id: DefId, expected_substs: &'tcx Substs<'tcx>, actual_substs: &'tcx Substs<'tcx>, - ) -> ErrorReported { + ) -> DiagnosticBuilder<'me> { debug!( "try_report_placeholders_trait(\ vid={:?}, \ @@ -295,8 +294,7 @@ impl NiceRegionError<'me, 'gcx, 'tcx> { any_self_ty_has_vid, ); - err.emit(); - ErrorReported + err } /// Add notes with details about the expected and actual trait refs, with attention to cases diff --git a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs index f741af0b228b5..081c458bfc17a 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs @@ -244,7 +244,8 @@ impl<'tcx> RegionInferenceContext<'tcx> { if let (Some(f), Some(o)) = (self.to_error_region(fr), self.to_error_region(outlived_fr)) { let tables = infcx.tcx.typeck_tables_of(mir_def_id); let nice = NiceRegionError::new_from_span(infcx, span, o, f, Some(tables)); - if let Some(_error_reported) = nice.try_report_from_nll() { + if let Some(diag) = nice.try_report_from_nll() { + diag.buffer(errors_buffer); return; } } From 79e8c311765df4c35558aa9683f1e2004719175a Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 6 Feb 2019 14:13:01 +0100 Subject: [PATCH 3/3] Propagate region constraints more precisely from closures --- .../borrow_check/nll/region_infer/mod.rs | 66 +++++++----- .../nll/type_check/free_region_relations.rs | 102 +++++++++++------- .../issue-58127-mutliple-requirements.rs | 40 +++++++ 3 files changed, 142 insertions(+), 66 deletions(-) create mode 100644 src/test/ui/nll/closure-requirements/issue-58127-mutliple-requirements.rs diff --git a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs index 7a0cd2ac26c3b..cbeb5dc206ee6 100644 --- a/src/librustc_mir/borrow_check/nll/region_infer/mod.rs +++ b/src/librustc_mir/borrow_check/nll/region_infer/mod.rs @@ -763,20 +763,26 @@ impl<'tcx> RegionInferenceContext<'tcx> { debug!("try_promote_type_test: ur={:?}", ur); - let non_local_ub = self.universal_region_relations.non_local_upper_bound(ur); + let non_local_ub = self.universal_region_relations.non_local_upper_bounds(&ur); debug!("try_promote_type_test: non_local_ub={:?}", non_local_ub); - assert!(self.universal_regions.is_universal_region(non_local_ub)); - assert!(!self.universal_regions.is_local_free_region(non_local_ub)); - - let requirement = ClosureOutlivesRequirement { - subject, - outlived_free_region: non_local_ub, - blame_span: locations.span(mir), - category: ConstraintCategory::Boring, - }; - debug!("try_promote_type_test: pushing {:#?}", requirement); - propagated_outlives_requirements.push(requirement); + // This is slightly too conservative. To show T: '1, given `'2: '1` + // and `'3: '1` we only need to prove that T: '2 *or* T: '3, but to + // avoid potential non-determinism we approximate this by requiring + // T: '1 and T: '2. + for &upper_bound in non_local_ub { + debug_assert!(self.universal_regions.is_universal_region(upper_bound)); + debug_assert!(!self.universal_regions.is_local_free_region(upper_bound)); + + let requirement = ClosureOutlivesRequirement { + subject, + outlived_free_region: upper_bound, + blame_span: locations.span(mir), + category: ConstraintCategory::Boring, + }; + debug!("try_promote_type_test: pushing {:#?}", requirement); + propagated_outlives_requirements.push(requirement); + } } true } @@ -1217,35 +1223,37 @@ impl<'tcx> RegionInferenceContext<'tcx> { longer_fr, shorter_fr, ); - if let Some(propagated_outlives_requirements) = propagated_outlives_requirements { - // Shrink `fr` until we find a non-local region (if we do). - // We'll call that `fr-` -- it's ever so slightly smaller than `fr`. - if let Some(fr_minus) = self.universal_region_relations + // Shrink `longer_fr` until we find a non-local region (if we do). + // We'll call it `fr-` -- it's ever so slightly smaller than + // `longer_fr`. + + if let Some(fr_minus) = self + .universal_region_relations .non_local_lower_bound(longer_fr) { debug!("check_universal_region: fr_minus={:?}", fr_minus); let blame_span_category = self.find_outlives_blame_span(mir, longer_fr, shorter_fr); - // Grow `shorter_fr` until we find a non-local - // region. (We always will.) We'll call that - // `shorter_fr+` -- it's ever so slightly larger than - // `fr`. + // Grow `shorter_fr` until we find some non-local regions. (We + // always will.) We'll call them `shorter_fr+` -- they're ever + // so slightly larger than `shorter_fr`. let shorter_fr_plus = self.universal_region_relations - .non_local_upper_bound(shorter_fr); + .non_local_upper_bounds(&shorter_fr); debug!( "check_universal_region: shorter_fr_plus={:?}", shorter_fr_plus ); - - // Push the constraint `fr-: shorter_fr+` - propagated_outlives_requirements.push(ClosureOutlivesRequirement { - subject: ClosureOutlivesSubject::Region(fr_minus), - outlived_free_region: shorter_fr_plus, - blame_span: blame_span_category.1, - category: blame_span_category.0, - }); + for &&fr in &shorter_fr_plus { + // Push the constraint `fr-: shorter_fr+` + propagated_outlives_requirements.push(ClosureOutlivesRequirement { + subject: ClosureOutlivesSubject::Region(fr_minus), + outlived_free_region: fr, + blame_span: blame_span_category.1, + category: blame_span_category.0, + }); + } return None; } } diff --git a/src/librustc_mir/borrow_check/nll/type_check/free_region_relations.rs b/src/librustc_mir/borrow_check/nll/type_check/free_region_relations.rs index 7ddfd55dbbb94..3b663ef6dad61 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/free_region_relations.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/free_region_relations.rs @@ -105,44 +105,89 @@ impl UniversalRegionRelations<'tcx> { /// Finds an "upper bound" for `fr` that is not local. In other /// words, returns the smallest (*) known region `fr1` that (a) - /// outlives `fr` and (b) is not local. This cannot fail, because - /// we will always find `'static` at worst. + /// outlives `fr` and (b) is not local. /// - /// (*) If there are multiple competing choices, we pick the "postdominating" - /// one. See `TransitiveRelation::postdom_upper_bound` for details. - crate fn non_local_upper_bound(&self, fr: RegionVid) -> RegionVid { + /// (*) If there are multiple competing choices, we return all of them. + crate fn non_local_upper_bounds(&'a self, fr: &'a RegionVid) -> Vec<&'a RegionVid> { debug!("non_local_upper_bound(fr={:?})", fr); - self.non_local_bound(&self.inverse_outlives, fr) + let res = self.non_local_bounds(&self.inverse_outlives, fr); + assert!(!res.is_empty(), "can't find an upper bound!?"); + res + } + + /// Returns the "postdominating" bound of the set of + /// `non_local_upper_bounds` for the given region. + crate fn non_local_upper_bound(&self, fr: RegionVid) -> RegionVid { + let upper_bounds = self.non_local_upper_bounds(&fr); + + // In case we find more than one, reduce to one for + // convenience. This is to prevent us from generating more + // complex constraints, but it will cause spurious errors. + let post_dom = self + .inverse_outlives + .mutual_immediate_postdominator(upper_bounds); + + debug!("non_local_bound: post_dom={:?}", post_dom); + + post_dom + .and_then(|&post_dom| { + // If the mutual immediate postdom is not local, then + // there is no non-local result we can return. + if !self.universal_regions.is_local_free_region(post_dom) { + Some(post_dom) + } else { + None + } + }) .unwrap_or(self.universal_regions.fr_static) } + /// Finds a "lower bound" for `fr` that is not local. In other /// words, returns the largest (*) known region `fr1` that (a) is - /// outlived by `fr` and (b) is not local. This cannot fail, - /// because we will always find `'static` at worst. + /// outlived by `fr` and (b) is not local. /// /// (*) If there are multiple competing choices, we pick the "postdominating" /// one. See `TransitiveRelation::postdom_upper_bound` for details. crate fn non_local_lower_bound(&self, fr: RegionVid) -> Option { debug!("non_local_lower_bound(fr={:?})", fr); - self.non_local_bound(&self.outlives, fr) + let lower_bounds = self.non_local_bounds(&self.outlives, &fr); + + // In case we find more than one, reduce to one for + // convenience. This is to prevent us from generating more + // complex constraints, but it will cause spurious errors. + let post_dom = self + .outlives + .mutual_immediate_postdominator(lower_bounds); + + debug!("non_local_bound: post_dom={:?}", post_dom); + + post_dom + .and_then(|&post_dom| { + // If the mutual immediate postdom is not local, then + // there is no non-local result we can return. + if !self.universal_regions.is_local_free_region(post_dom) { + Some(post_dom) + } else { + None + } + }) } - /// Helper for `non_local_upper_bound` and - /// `non_local_lower_bound`. Repeatedly invokes `postdom_parent` - /// until we find something that is not local. Returns `None` if we - /// never do so. - fn non_local_bound( + /// Helper for `non_local_upper_bounds` and `non_local_lower_bounds`. + /// Repeatedly invokes `postdom_parent` until we find something that is not + /// local. Returns `None` if we never do so. + fn non_local_bounds<'a>( &self, - relation: &TransitiveRelation, - fr0: RegionVid, - ) -> Option { + relation: &'a TransitiveRelation, + fr0: &'a RegionVid, + ) -> Vec<&'a RegionVid> { // This method assumes that `fr0` is one of the universally // quantified region variables. - assert!(self.universal_regions.is_universal_region(fr0)); + assert!(self.universal_regions.is_universal_region(*fr0)); let mut external_parents = vec![]; - let mut queue = vec![&fr0]; + let mut queue = vec![fr0]; // Keep expanding `fr` into its parents until we reach // non-local regions. @@ -157,24 +202,7 @@ impl UniversalRegionRelations<'tcx> { debug!("non_local_bound: external_parents={:?}", external_parents); - // In case we find more than one, reduce to one for - // convenience. This is to prevent us from generating more - // complex constraints, but it will cause spurious errors. - let post_dom = relation - .mutual_immediate_postdominator(external_parents) - .cloned(); - - debug!("non_local_bound: post_dom={:?}", post_dom); - - post_dom.and_then(|post_dom| { - // If the mutual immediate postdom is not local, then - // there is no non-local result we can return. - if !self.universal_regions.is_local_free_region(post_dom) { - Some(post_dom) - } else { - None - } - }) + external_parents } /// Returns `true` if fr1 is known to outlive fr2. diff --git a/src/test/ui/nll/closure-requirements/issue-58127-mutliple-requirements.rs b/src/test/ui/nll/closure-requirements/issue-58127-mutliple-requirements.rs new file mode 100644 index 0000000000000..71d5d4053ee25 --- /dev/null +++ b/src/test/ui/nll/closure-requirements/issue-58127-mutliple-requirements.rs @@ -0,0 +1,40 @@ +// revisions: migrate nll +//[migrate]compile-flags: -Z borrowck=migrate +#![cfg_attr(nll, feature(nll))] + +// compile-pass + +// Test that we propagate region relations from closures precisely when there is +// more than one non-local lower bound. + +// In this case the closure has signature +// |x: &'4 mut (&'5 (&'1 str, &'2 str), &'3 str)| -> .. +// We end up with a `'3: '5` constraint that we can propagate as +// `'3: '1`, `'3: '2`, but previously we approximated it as `'3: 'static`. + +// As an optimization, we primarily propagate bounds for the "representative" +// of each SCC. As such we have these two similar cases where hopefully one +// of them will test the case we want (case2, when this test was added). +mod case1 { + fn f(s: &str) { + g(s, |x| h(x)); + } + + fn g(_: T, _: F) + where F: Fn(&mut (&(T, T), T)) {} + + fn h(_: &mut (&(T, T), T)) {} +} + +mod case2 { + fn f(s: &str) { + g(s, |x| h(x)); + } + + fn g(_: T, _: F) + where F: Fn(&mut (T, &(T, T))) {} + + fn h(_: &mut (T, &(T, T))) {} +} + +fn main() {}