From 697739d10961399b47ea5f4526c5c9cdb8a12e61 Mon Sep 17 00:00:00 2001 From: Alexander Regueiro Date: Sun, 24 Mar 2019 19:32:59 +0000 Subject: [PATCH 1/3] Aggregation of drive-by cosmetic changes. --- src/librustc/hir/lowering.rs | 7 +- src/librustc/infer/opaque_types/mod.rs | 81 ++++++++++--------- src/librustc/infer/outlives/env.rs | 4 +- .../infer/outlives/free_region_map.rs | 5 +- src/librustc/traits/auto_trait.rs | 4 +- src/librustc/traits/query/outlives_bounds.rs | 8 +- src/librustc/ty/mod.rs | 2 +- src/librustc/ty/outlives.rs | 2 +- src/librustc/ty/query/mod.rs | 10 +-- .../transitive_relation.rs | 9 +-- src/librustc_typeck/check/regionck.rs | 4 +- 11 files changed, 73 insertions(+), 63 deletions(-) diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs index 2251e67233c5f..7e27d1789bbef 100644 --- a/src/librustc/hir/lowering.rs +++ b/src/librustc/hir/lowering.rs @@ -37,8 +37,9 @@ use crate::hir::map::{DefKey, DefPathData, Definitions}; use crate::hir::def_id::{DefId, DefIndex, DefIndexAddressSpace, CRATE_DEF_INDEX}; use crate::hir::def::{Def, PathResolution, PerNS}; use crate::hir::{GenericArg, ConstArg}; -use crate::lint::builtin::{self, PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES, - ELIDED_LIFETIMES_IN_PATHS}; +use crate::lint::builtin::{ + self, PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES, ELIDED_LIFETIMES_IN_PATHS +}; use crate::middle::cstore::CrateStore; use crate::session::Session; use crate::session::config::nightly_options; @@ -5145,7 +5146,7 @@ impl<'a> LoweringContext<'a> { /// `Box`. In those cases, `lower_lifetime` is invoked. fn elided_dyn_bound(&mut self, span: Span) -> hir::Lifetime { match self.anonymous_lifetime_mode { - // NB. We intentionally ignore the create-parameter mode here. + // N.B., We intentionally ignore the create-parameter mode here. // and instead "pass through" to resolve-lifetimes, which will apply // the object-lifetime-defaulting rules. Elided object lifetime defaults // do not act like other elided lifetimes. In other words, given this: diff --git a/src/librustc/infer/opaque_types/mod.rs b/src/librustc/infer/opaque_types/mod.rs index 8bd2084316320..93e806f728847 100644 --- a/src/librustc/infer/opaque_types/mod.rs +++ b/src/librustc/infer/opaque_types/mod.rs @@ -13,27 +13,27 @@ use crate::util::nodemap::DefIdMap; pub type OpaqueTypeMap<'tcx> = DefIdMap>; -/// Information about the opaque, abstract types whose values we +/// Information about the opaque, existential types whose values we /// are inferring in this function (these are the `impl Trait` that /// appear in the return type). #[derive(Copy, Clone, Debug)] pub struct OpaqueTypeDecl<'tcx> { - /// The substitutions that we apply to the abstract that this + /// The substitutions that we apply to the existential type that this /// `impl Trait` desugars to. e.g., if: /// /// fn foo<'a, 'b, T>() -> impl Trait<'a> /// /// winds up desugared to: /// - /// abstract type Foo<'x, X>: Trait<'x> + /// existential type Foo<'x, X>: Trait<'x> /// fn foo<'a, 'b, T>() -> Foo<'a, T> /// /// then `substs` would be `['a, T]`. pub substs: SubstsRef<'tcx>, - /// The type variable that represents the value of the abstract type + /// The type variable that represents the value of the existential type /// that we require. In other words, after we compile this function, - /// we will be created a constraint like: + /// we will have created a constraint like: /// /// Foo<'a, T> = ?C /// @@ -141,12 +141,12 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { /// Here, we have two `impl Trait` types whose values are being /// inferred (the `impl Bar<'a>` and the `impl /// Bar<'b>`). Conceptually, this is sugar for a setup where we - /// define underlying abstract types (`Foo1`, `Foo2`) and then, in + /// define underlying existential types (`Foo1`, `Foo2`) and then, in /// the return type of `foo`, we *reference* those definitions: /// /// ```text - /// abstract type Foo1<'x>: Bar<'x>; - /// abstract type Foo2<'x>: Bar<'x>; + /// existential type Foo1<'x>: Bar<'x>; + /// existential type Foo2<'x>: Bar<'x>; /// fn foo<'a, 'b>(..) -> (Foo1<'a>, Foo2<'b>) { .. } /// // ^^^^ ^^ /// // | | @@ -205,7 +205,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { /// /// This is actually a bit of a tricky constraint in general. We /// want to say that each variable (e.g., `'0`) can only take on - /// values that were supplied as arguments to the abstract type + /// values that were supplied as arguments to the existential type /// (e.g., `'a` for `Foo1<'a>`) or `'static`, which is always in /// scope. We don't have a constraint quite of this kind in the current /// region checker. @@ -232,10 +232,10 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { /// # The `free_region_relations` parameter /// /// The `free_region_relations` argument is used to find the - /// "minimum" of the regions supplied to a given abstract type. + /// "minimum" of the regions supplied to a given existential type. /// It must be a relation that can answer whether `'a <= 'b`, /// where `'a` and `'b` are regions that appear in the "substs" - /// for the abstract type references (the `<'a>` in `Foo1<'a>`). + /// for the existential type references (the `<'a>` in `Foo1<'a>`). /// /// Note that we do not impose the constraints based on the /// generic regions from the `Foo1` definition (e.g., `'x`). This @@ -251,7 +251,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { /// /// Here, the fact that `'b: 'a` is known only because of the /// implied bounds from the `&'a &'b u32` parameter, and is not - /// "inherent" to the abstract type definition. + /// "inherent" to the existential type definition. /// /// # Parameters /// @@ -364,16 +364,16 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { } Component::Param(_) => { - // ignore type parameters like `T`, they are captured - // implicitly by the `impl Trait` + // Ignore type parameters like `T`, since they are captured + // implicitly by the `impl Trait`. } Component::UnresolvedInferenceVariable(_) => { - // we should get an error that more type - // annotations are needed in this case + // We should get an error that more type + // annotations are needed in this case. self.tcx .sess - .delay_span_bug(span, "unresolved inf var in opaque"); + .delay_span_bug(span, "unresolved inf var in opaque type"); } Component::Projection(ty::ProjectionTy { @@ -397,20 +397,32 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { } } } + + // For soundness, we need to ensure that every region that is captured by the opaque type + // but does not explicitly appear in the opaque type outlives the actual (concrete) type. + // This allows for invariant lifetimes to be captured by opaque types as long as + // short-lived lifetimes are not permitted to escape and cause UB. + let opaque_substs_regions = opaque_defn.substs.regions().collect(); + let captured_regions = concrete_ty_regions.difference(&opaque_substs_regions); + for captured_region in captured_regions { + for concrete_ty_region in &concrete_ty_regions { + self.sub_regions(infer::CallReturn(span), *concrete_ty_region, captured_region) + } + } } /// Given the fully resolved, instantiated type for an opaque /// type, i.e., the value of an inference variable like C1 or C2 - /// (*), computes the "definition type" for an abstract type + /// (*), computes the "definition type" for an existential type /// definition -- that is, the inferred value of `Foo1<'x>` or /// `Foo2<'x>` that we would conceptually use in its definition: /// - /// abstract type Foo1<'x>: Bar<'x> = AAA; <-- this type AAA - /// abstract type Foo2<'x>: Bar<'x> = BBB; <-- or this type BBB + /// existential type Foo1<'x>: Bar<'x> = AAA; <-- this type AAA + /// existential type Foo2<'x>: Bar<'x> = BBB; <-- or this type BBB /// fn foo<'a, 'b>(..) -> (Foo1<'a>, Foo2<'b>) { .. } /// - /// Note that these values are defined in terms of a distinct set of - /// generic parameters (`'x` instead of `'a`) from C1 or C2. The main + /// Note that these values are defined in terms of a set of generic + /// parameters (`'x` instead of `'a`) distinct from C1 and C2. The main /// purpose of this function is to do that translation. /// /// (*) C1 and C2 were introduced in the comments on @@ -450,23 +462,20 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { .collect(); // Convert the type from the function into a type valid outside - // the function, by replacing invalid regions with 'static, + // the function, by replacing invalid regions with `'static`, // after producing an error for each of them. let definition_ty = - instantiated_ty.fold_with(&mut ReverseMapper::new( - self.tcx, - self.is_tainted_by_errors(), - def_id, + instantiated_ty.fold_with(&mut ReverseMapper { + tcx: self.tcx, map, - instantiated_ty, - )); + }); debug!( "infer_opaque_definition_from_instantiation: definition_ty={:?}", definition_ty ); // We can unwrap here because our reverse mapper always - // produces things with 'gcx lifetime, though the type folder + // produces things with `'gcx` lifetime, though the type folder // obscures that. let definition_ty = gcx.lift(&definition_ty).unwrap(); @@ -656,20 +665,20 @@ impl<'a, 'gcx, 'tcx> Instantiator<'a, 'gcx, 'tcx> { // value we are inferring. At present, this is // always true during the first phase of // type-check, but not always true later on during - // NLL. Once we support named abstract types more fully, + // NLL. Once we support named existential types more fully, // this same scenario will be able to arise during all phases. // - // Here is an example using `abstract type` that indicates + // Here is an example using `existential type` that indicates // the distinction we are checking for: // // ```rust // mod a { - // pub abstract type Foo: Iterator; - // pub fn make_foo() -> Foo { .. } + // pub existential type Foo: Iterator; + // pub fn make_foo() -> Foo { .. } // } // // mod b { - // fn foo() -> a::Foo { a::make_foo() } + // fn foo() -> a::Foo { a::make_foo() } // } // ``` // @@ -777,7 +786,7 @@ impl<'a, 'gcx, 'tcx> Instantiator<'a, 'gcx, 'tcx> { required_region_bounds ); - // make sure that we are in fact defining the *entire* type + // Make sure that we are in fact defining the *entire* type // e.g., `existential type Foo: Bar;` needs to be // defined by a function like `fn foo() -> Foo`. debug!( diff --git a/src/librustc/infer/outlives/env.rs b/src/librustc/infer/outlives/env.rs index 39aa51a95f793..4ae186532f715 100644 --- a/src/librustc/infer/outlives/env.rs +++ b/src/librustc/infer/outlives/env.rs @@ -197,8 +197,8 @@ impl<'a, 'gcx: 'tcx, 'tcx: 'a> OutlivesEnvironment<'tcx> { ) where I: IntoIterator>, { - // Record relationships such as `T:'x` that don't go into the - // free-region-map but which we use here. + // Record relationships such as `T: 'x`, which don't go into the + // free-region-map, but which we use here. for outlives_bound in outlives_bounds { debug!("add_outlives_bounds: outlives_bound={:?}", outlives_bound); match outlives_bound { diff --git a/src/librustc/infer/outlives/free_region_map.rs b/src/librustc/infer/outlives/free_region_map.rs index 5349e990a7761..f19752dfe2282 100644 --- a/src/librustc/infer/outlives/free_region_map.rs +++ b/src/librustc/infer/outlives/free_region_map.rs @@ -15,7 +15,7 @@ impl<'tcx> FreeRegionMap<'tcx> { self.relation.is_empty() } - // Record that `'sup:'sub`. Or, put another way, `'sub <= 'sup`. + // Record that `'sup: 'sub`. Or, put another way, `'sub <= 'sup`. // (with the exception that `'static: 'x` is not notable) pub fn relate_regions(&mut self, sub: Region<'tcx>, sup: Region<'tcx>) { debug!("relate_regions(sub={:?}, sup={:?})", sub, sup); @@ -24,7 +24,7 @@ impl<'tcx> FreeRegionMap<'tcx> { } } - /// Computes the least-upper-bound of two free regions. In some + /// Computes the least upper bound of two free regions. In some /// cases, this is more conservative than necessary, in order to /// avoid making arbitrary choices. See /// `TransitiveRelation::postdom_upper_bound` for more details. @@ -90,6 +90,7 @@ impl_stable_hash_for!(struct FreeRegionMap<'tcx> { impl<'a, 'tcx> Lift<'tcx> for FreeRegionMap<'a> { type Lifted = FreeRegionMap<'tcx>; + fn lift_to_tcx<'b, 'gcx>(&self, tcx: TyCtxt<'b, 'gcx, 'tcx>) -> Option> { self.relation.maybe_map(|&fr| tcx.lift(&fr)) .map(|relation| FreeRegionMap { relation }) diff --git a/src/librustc/traits/auto_trait.rs b/src/librustc/traits/auto_trait.rs index e93351197fe47..9139953b6c656 100644 --- a/src/librustc/traits/auto_trait.rs +++ b/src/librustc/traits/auto_trait.rs @@ -141,9 +141,9 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> { // selection and projection: // // * We can always cache the result of a particular trait selection for the lifetime of - // an InfCtxt + // an `InferCtxt`. // * Given a projection bound such as '::SomeItem = K', if 'T: - // SomeTrait' doesn't hold, then we don't need to care about the 'SomeItem = K' + // SomeTrait' doesn't hold, then we don't need to care about the 'SomeItem = K'. // // We fix the first assumption by manually clearing out all of the InferCtxt's caches // in between calls to SelectionContext.select. This allows us to keep all of the diff --git a/src/librustc/traits/query/outlives_bounds.rs b/src/librustc/traits/query/outlives_bounds.rs index 954de15905fb7..05cf07afad2a1 100644 --- a/src/librustc/traits/query/outlives_bounds.rs +++ b/src/librustc/traits/query/outlives_bounds.rs @@ -72,7 +72,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { /// argument types are well-formed. This may imply certain relationships /// between generic parameters. For example: /// - /// fn foo<'a,T>(x: &'a T) + /// fn foo<'a, T>(x: &'a T) /// /// can only be called with a `'a` and `T` such that `&'a T` is WF. /// For `&'a T` to be WF, `T: 'a` must hold. So we can assume `T: 'a`. @@ -102,7 +102,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { Err(NoSolution) => { self.tcx.sess.delay_span_bug( span, - "implied_outlives_bounds failed to solve all obligations" + "implied_outlives_bounds: failed to solve all obligations", ); return vec![]; } @@ -117,7 +117,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { Err(_) => { self.tcx.sess.delay_span_bug( span, - "implied_outlives_bounds failed to instantiate" + "implied_outlives_bounds: failed to instantiate", ); return vec![]; } @@ -130,7 +130,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> { if fulfill_cx.select_all_or_error(self).is_err() { self.tcx.sess.delay_span_bug( span, - "implied_outlives_bounds failed to solve obligations from instantiation" + "implied_outlives_bounds: failed to solve obligations from instantiation", ); } diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 91b84b68802b1..8c570b08338fa 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -1643,7 +1643,7 @@ pub struct ParamEnv<'tcx> { pub reveal: traits::Reveal, /// If this `ParamEnv` comes from a call to `tcx.param_env(def_id)`, - /// register that `def_id` (useful for transitioning to the chalk trait + /// register that `def_id` (useful for transitioning to the Chalk trait /// solver). pub def_id: Option, } diff --git a/src/librustc/ty/outlives.rs b/src/librustc/ty/outlives.rs index 5b21ed5abd77b..6107e8e1e713c 100644 --- a/src/librustc/ty/outlives.rs +++ b/src/librustc/ty/outlives.rs @@ -1,4 +1,4 @@ -// The outlines relation `T: 'a` or `'a: 'b`. This code frequently +// The outlives relation; `T: 'a` or `'a: 'b`. This code frequently // refers to rules defined in RFC 1214 (`OutlivesFooBar`), so see that // RFC for reference. diff --git a/src/librustc/ty/query/mod.rs b/src/librustc/ty/query/mod.rs index a2d7815920e0b..185ae313679ce 100644 --- a/src/librustc/ty/query/mod.rs +++ b/src/librustc/ty/query/mod.rs @@ -158,7 +158,7 @@ rustc_query_append! { [define_queries!][ <'tcx> [] fn is_const_fn_raw: IsConstFn(DefId) -> bool, - /// Returns true if calls to the function may be promoted + /// Returns `true` if calls to the function may be promoted. /// /// This is either because the function is e.g., a tuple-struct or tuple-variant /// constructor, or because it has the `#[rustc_promotable]` attribute. The attribute should @@ -174,22 +174,22 @@ rustc_query_append! { [define_queries!][ <'tcx> /// instead. [] fn crate_variances: crate_variances(CrateNum) -> Lrc, - /// Maps from def-id of a type or region parameter to its + /// Maps from def-ID of a type or region parameter to its /// (inferred) variance. [] fn variances_of: ItemVariances(DefId) -> Lrc>, }, TypeChecking { - /// Maps from def-id of a type to its (inferred) outlives. + /// Maps from def-ID of a type to its (inferred) outlives. [] fn inferred_outlives_crate: InferredOutlivesCrate(CrateNum) -> Lrc>, }, Other { - /// Maps from an impl/trait def-id to a list of the def-ids of its items + /// Maps from an impl/trait def-ID to a list of the def-IDs of its items. [] fn associated_item_def_ids: AssociatedItemDefIds(DefId) -> Lrc>, - /// Maps from a trait item to the trait item "descriptor" + /// Maps from a trait item to the trait item "descriptor". [] fn associated_item: AssociatedItems(DefId) -> ty::AssociatedItem, [] fn impl_trait_ref: ImplTraitRef(DefId) -> Option>, diff --git a/src/librustc_data_structures/transitive_relation.rs b/src/librustc_data_structures/transitive_relation.rs index 0974607fabea8..e13f5f72f55c1 100644 --- a/src/librustc_data_structures/transitive_relation.rs +++ b/src/librustc_data_structures/transitive_relation.rs @@ -7,10 +7,9 @@ use std::fmt::Debug; use std::hash::Hash; use std::mem; - #[derive(Clone, Debug)] pub struct TransitiveRelation { - // List of elements. This is used to map from a T to a usize. + // List of elements. This is used to map from a `T` to a `usize`. elements: Vec, // Maps each element to an index. @@ -22,7 +21,7 @@ pub struct TransitiveRelation { // This is a cached transitive closure derived from the edges. // Currently, we build it lazilly and just throw out any existing - // copy whenever a new edge is added. (The Lock is to permit + // copy whenever a new edge is added. (The `Lock` is to permit // the lazy computation.) This is kind of silly, except for the // fact its size is tied to `self.elements.len()`, so I wanted to // wait before building it up to avoid reallocating as new edges @@ -209,10 +208,10 @@ impl TransitiveRelation { } }; - // in some cases, there are some arbitrary choices to be made; + // In some cases, there are some arbitrary choices to be made; // it doesn't really matter what we pick, as long as we pick // the same thing consistently when queried, so ensure that - // (a, b) are in a consistent relative order + // (a, b) are in a consistent relative order. if a > b { mem::swap(&mut a, &mut b); } diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs index a03d33a3ef5bc..1a12f22fd32a3 100644 --- a/src/librustc_typeck/check/regionck.rs +++ b/src/librustc_typeck/check/regionck.rs @@ -374,7 +374,7 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> { fn visit_region_obligations(&mut self, hir_id: hir::HirId) { debug!("visit_region_obligations: hir_id={:?}", hir_id); - // region checking can introduce new pending obligations + // Region checking can introduce new pending obligations, // which, when processed, might generate new region // obligations. So make sure we process those. self.select_all_obligations_or_error(); @@ -402,7 +402,7 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> { // data will be accessible from anywhere that the variable is // accessed. We must be wary of loops like this: // - // // from src/test/compile-fail/borrowck-lend-flow.rs + // // from `src/test/compile-fail/borrowck-lend-flow.rs` // let mut v = box 3, w = box 4; // let mut x = &mut w; // loop { From 3be94173981c685bce18809e8107b4899201a0e1 Mon Sep 17 00:00:00 2001 From: Alexander Regueiro Date: Sun, 24 Mar 2019 19:33:04 +0000 Subject: [PATCH 2/3] Allow opaque types to hide lifetimes in concrete type so long as they outlive concrete type. --- src/librustc/infer/opaque_types/mod.rs | 281 ++++++------------ .../infer/outlives/free_region_map.rs | 11 +- .../transitive_relation.rs | 4 + .../nll/type_check/free_region_relations.rs | 5 + .../borrow_check/nll/universal_regions.rs | 4 + 5 files changed, 115 insertions(+), 190 deletions(-) diff --git a/src/librustc/infer/opaque_types/mod.rs b/src/librustc/infer/opaque_types/mod.rs index 93e806f728847..b7862691b05f3 100644 --- a/src/librustc/infer/opaque_types/mod.rs +++ b/src/librustc/infer/opaque_types/mod.rs @@ -3,7 +3,7 @@ use crate::hir; use crate::hir::Node; use crate::infer::{self, InferCtxt, InferOk, TypeVariableOrigin}; use crate::infer::outlives::free_region_map::FreeRegionRelations; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use crate::traits::{self, PredicateObligation}; use crate::ty::{self, Ty, TyCtxt, GenericParamDefKind}; use crate::ty::fold::{BottomUpFolder, TypeFoldable, TypeFolder}; @@ -276,84 +276,87 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { opaque_defn: &OpaqueTypeDecl<'tcx>, free_region_relations: &FRR, ) { - debug!("constrain_opaque_type()"); - debug!("constrain_opaque_type: def_id={:?}", def_id); - debug!("constrain_opaque_type: opaque_defn={:#?}", opaque_defn); + debug!("constrain_opaque_type(def_id={:?}, opaque_defn={:#?})", def_id, opaque_defn); let concrete_ty = self.resolve_type_vars_if_possible(&opaque_defn.concrete_ty); debug!("constrain_opaque_type: concrete_ty={:?}", concrete_ty); - let abstract_type_generics = self.tcx.generics_of(def_id); + let opaque_type_generics = self.tcx.generics_of(def_id); let span = self.tcx.def_span(def_id); // If there are required region bounds, we can just skip - // ahead. There will already be a registered region - // obligation related `concrete_ty` to those regions. - if opaque_defn.has_required_region_bounds { - return; - } - - // There were no `required_region_bounds`, - // so we have to search for a `least_region`. - // Go through all the regions used as arguments to the - // abstract type. These are the parameters to the abstract - // type; so in our example above, `substs` would contain - // `['a]` for the first impl trait and `'b` for the - // second. - let mut least_region = None; - for param in &abstract_type_generics.params { - match param.kind { - GenericParamDefKind::Lifetime => {} - _ => continue - } - // Get the value supplied for this region from the substs. - let subst_arg = opaque_defn.substs.region_at(param.index as usize); - - // Compute the least upper bound of it with the other regions. - debug!("constrain_opaque_types: least_region={:?}", least_region); - debug!("constrain_opaque_types: subst_arg={:?}", subst_arg); - match least_region { - None => least_region = Some(subst_arg), - Some(lr) => { - if free_region_relations.sub_free_regions(lr, subst_arg) { - // keep the current least region - } else if free_region_relations.sub_free_regions(subst_arg, lr) { - // switch to `subst_arg` - least_region = Some(subst_arg); - } else { - // There are two regions (`lr` and - // `subst_arg`) which are not relatable. We can't - // find a best choice. - self.tcx - .sess - .struct_span_err(span, "ambiguous lifetime bound in `impl Trait`") - .span_label( - span, - format!("neither `{}` nor `{}` outlives the other", lr, subst_arg), - ) - .emit(); - - least_region = Some(self.tcx.mk_region(ty::ReEmpty)); - break; + // ahead. There will already be a registered region + // obligation relating `concrete_ty` to those regions. + let least_region = if opaque_defn.has_required_region_bounds { + None + } else { + // There were no `required_region_bounds`, + // so we have to search for a `least_region`. + // Go through all the regions used as arguments to the + // existential type. These are the parameters to the existential + // type; so in our example above, `substs` would contain + // `['a]` for the first impl trait and `'b` for the + // second. + let mut least_region = None; + for param in &opaque_type_generics.params { + match param.kind { + GenericParamDefKind::Lifetime => {} + _ => continue + } + // Get the value supplied for this region from the substs. + let subst_arg = opaque_defn.substs.region_at(param.index as usize); + + // Compute the least upper bound of it with the other regions. + debug!("constrain_opaque_type: least_region={:?}", least_region); + debug!("constrain_opaque_type: subst_arg={:?}", subst_arg); + match least_region { + None => least_region = Some(subst_arg), + Some(lr) => { + if free_region_relations.sub_free_regions(lr, subst_arg) { + // Keep the current least region. + } else if free_region_relations.sub_free_regions(subst_arg, lr) { + // Switch to `subst_arg`. + least_region = Some(subst_arg); + } else { + // There are two regions (`lr` and `subst_arg`) that are not relatable. + // We can't find a best choice. + self.tcx + .sess + .struct_span_err(span, "ambiguous lifetime bound in `impl Trait`") + .span_label( + span, + format!("neither `{}` nor `{}` outlives the other", + lr, subst_arg), + ) + .emit(); + + least_region = Some(self.tcx.mk_region(ty::ReEmpty)); + break; + } } } } - } - - let least_region = least_region.unwrap_or(self.tcx.types.re_static); - debug!("constrain_opaque_types: least_region={:?}", least_region); + least_region + }.unwrap_or(self.tcx.types.re_static); + debug!("constrain_opaque_type: least_region={:?}", least_region); // Require that the type `concrete_ty` outlives // `least_region`, modulo any type parameters that appear - // in the type, which we ignore. This is because impl - // trait values are assumed to capture all the in-scope + // in the type, which we ignore. This is because `impl + // Trait` values are assumed to capture all the in-scope // type parameters. This little loop here just invokes // `outlives` repeatedly, draining all the nested // obligations that result. + let mut concrete_ty_regions = FxHashSet::default(); let mut types = vec![concrete_ty]; - let bound_region = |r| self.sub_regions(infer::CallReturn(span), least_region, r); + let mut bound_region = |r| { + concrete_ty_regions.insert(r); + if !opaque_defn.has_required_region_bounds { + self.sub_regions(infer::CallReturn(span), least_region, r) + } + }; while let Some(ty) = types.pop() { let mut components = smallvec![]; self.tcx.push_outlives_components(ty, &mut components); @@ -485,49 +488,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { struct ReverseMapper<'cx, 'gcx: 'tcx, 'tcx: 'cx> { tcx: TyCtxt<'cx, 'gcx, 'tcx>, - - /// If errors have already been reported in this fn, we suppress - /// our own errors because they are sometimes derivative. - tainted_by_errors: bool, - - opaque_type_def_id: DefId, map: FxHashMap, Kind<'gcx>>, - map_missing_regions_to_empty: bool, - - /// initially `Some`, set to `None` once error has been reported - hidden_ty: Option>, -} - -impl<'cx, 'gcx, 'tcx> ReverseMapper<'cx, 'gcx, 'tcx> { - fn new( - tcx: TyCtxt<'cx, 'gcx, 'tcx>, - tainted_by_errors: bool, - opaque_type_def_id: DefId, - map: FxHashMap, Kind<'gcx>>, - hidden_ty: Ty<'tcx>, - ) -> Self { - Self { - tcx, - tainted_by_errors, - opaque_type_def_id, - map, - map_missing_regions_to_empty: false, - hidden_ty: Some(hidden_ty), - } - } - - fn fold_kind_mapping_missing_regions_to_empty(&mut self, kind: Kind<'tcx>) -> Kind<'tcx> { - assert!(!self.map_missing_regions_to_empty); - self.map_missing_regions_to_empty = true; - let kind = kind.fold_with(self); - self.map_missing_regions_to_empty = false; - kind - } - - fn fold_kind_normally(&mut self, kind: Kind<'tcx>) -> Kind<'tcx> { - assert!(!self.map_missing_regions_to_empty); - kind.fold_with(self) - } } impl<'cx, 'gcx, 'tcx> TypeFolder<'gcx, 'tcx> for ReverseMapper<'cx, 'gcx, 'tcx> { @@ -537,109 +498,53 @@ impl<'cx, 'gcx, 'tcx> TypeFolder<'gcx, 'tcx> for ReverseMapper<'cx, 'gcx, 'tcx> fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> { match r { - // ignore bound regions that appear in the type (e.g., this + // Ignore bound regions that appear in the type (e.g., this // would ignore `'r` in a type like `for<'r> fn(&'r u32)`. ty::ReLateBound(..) | - // ignore `'static`, as that can appear anywhere + // Ignore `'static`, as that can appear anywhere. ty::ReStatic | - // ignore `ReScope`, as that can appear anywhere - // See `src/test/run-pass/issue-49556.rs` for example. + // Ignore `ReScope`, as that can appear anywhere. + // See `src/test/run-pass/issue-49556.rs`, for example. ty::ReScope(..) => return r, - _ => { } + _ => {} } match self.map.get(&r.into()).map(|k| k.unpack()) { Some(UnpackedKind::Lifetime(r1)) => r1, Some(u) => panic!("region mapped to unexpected kind: {:?}", u), None => { - if !self.map_missing_regions_to_empty && !self.tainted_by_errors { - if let Some(hidden_ty) = self.hidden_ty.take() { - let span = self.tcx.def_span(self.opaque_type_def_id); - let mut err = struct_span_err!( - self.tcx.sess, - span, - E0700, - "hidden type for `impl Trait` captures lifetime that \ - does not appear in bounds", - ); - - // Assuming regionck succeeded, then we must - // be capturing *some* region from the fn - // header, and hence it must be free, so it's - // ok to invoke this fn (which doesn't accept - // all regions, and would ICE if an - // inappropriate region is given). We check - // `is_tainted_by_errors` by errors above, so - // we don't get in here unless regionck - // succeeded. (Note also that if regionck - // failed, then the regions we are attempting - // to map here may well be giving errors - // *because* the constraints were not - // satisfiable.) - self.tcx.note_and_explain_free_region( - &mut err, - &format!("hidden type `{}` captures ", hidden_ty), - r, - "" - ); - - err.emit(); - } - } + // No mapping was found. This means that it is either a + // disallowed lifetime, which will be caught by regionck, + // a region in a non-upvar closure generic, which is + // explicitly allowed (see below for more on this), + // or a lifetime that does not explicitly appear in + // `impl Trait` bounds but which outlives the lifetimes + // or types which *do* appear in the `impl Trait` bounds. + // + // These lifetimes are explicitly allowed to prevent + // the user from having to add dummy references to the + // unnamed lifetimes in their `impl Trait` bounds + // (e.g., `+ DummyTraitWithALifetimeArg<'a>`). Adding + // the lifetimes via `+` doesn't work because the type + // doesn't outlive those lifetimes, it just contains them. + // + // On closures: there is a somewhat subtle (read: hacky) + // consideration. The problem is that our closure types + // currently include all the lifetime parameters declared + // on the enclosing function, even if they are unused by + // the closure itself. We can't readily filter them out, + // so we replace them with `empty`. This can't really make + // a diference to the rest of hte compiler; those regions + // are ignored for the outlives relation, and hence don't + // affect trait selection or auto traits, and they are + // erased during trans. self.tcx.types.re_empty }, } } - - fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> { - match ty.sty { - ty::Closure(def_id, substs) => { - // I am a horrible monster and I pray for death. When - // we encounter a closure here, it is always a closure - // from within the function that we are currently - // type-checking -- one that is now being encapsulated - // in an existential abstract type. Ideally, we would - // go through the types/lifetimes that it references - // and treat them just like we would any other type, - // which means we would error out if we find any - // reference to a type/region that is not in the - // "reverse map". - // - // **However,** in the case of closures, there is a - // somewhat subtle (read: hacky) consideration. The - // problem is that our closure types currently include - // all the lifetime parameters declared on the - // enclosing function, even if they are unused by the - // closure itself. We can't readily filter them out, - // so here we replace those values with `'empty`. This - // can't really make a difference to the rest of the - // compiler; those regions are ignored for the - // outlives relation, and hence don't affect trait - // selection or auto traits, and they are erased - // during codegen. - - let generics = self.tcx.generics_of(def_id); - let substs = self.tcx.mk_substs(substs.substs.iter().enumerate().map( - |(index, &kind)| { - if index < generics.parent_count { - // Accommodate missing regions in the parent kinds... - self.fold_kind_mapping_missing_regions_to_empty(kind) - } else { - // ...but not elsewhere. - self.fold_kind_normally(kind) - } - }, - )); - - self.tcx.mk_closure(def_id, ty::ClosureSubsts { substs }) - } - - _ => ty.super_fold_with(self), - } - } } struct Instantiator<'a, 'gcx: 'tcx, 'tcx: 'a> { diff --git a/src/librustc/infer/outlives/free_region_map.rs b/src/librustc/infer/outlives/free_region_map.rs index f19752dfe2282..4888cd2d73e6f 100644 --- a/src/librustc/infer/outlives/free_region_map.rs +++ b/src/librustc/infer/outlives/free_region_map.rs @@ -1,4 +1,5 @@ use crate::ty::{self, Lift, TyCtxt, Region}; +use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::transitive_relation::TransitiveRelation; #[derive(Clone, RustcEncodable, RustcDecodable, Debug, Default)] @@ -51,12 +52,18 @@ impl<'tcx> FreeRegionMap<'tcx> { /// slightly different way; this trait allows functions to be abstract /// over which version is in use. pub trait FreeRegionRelations<'tcx> { - /// Tests whether `r_a <= r_b`. Both must be free regions or - /// `'static`. + /// Gets all the free regions in the set of relations. + fn all_regions(&self) -> FxHashSet>; + + /// Tests whether `r_a <= r_b`. Both must be free regions or `'static`. fn sub_free_regions(&self, shorter: ty::Region<'tcx>, longer: ty::Region<'tcx>) -> bool; } impl<'tcx> FreeRegionRelations<'tcx> for FreeRegionMap<'tcx> { + fn all_regions(&self) -> FxHashSet> { + self.relation.elements().map(|r| *r).collect() + } + fn sub_free_regions(&self, r_a: Region<'tcx>, r_b: Region<'tcx>) diff --git a/src/librustc_data_structures/transitive_relation.rs b/src/librustc_data_structures/transitive_relation.rs index e13f5f72f55c1..33f2696053a53 100644 --- a/src/librustc_data_structures/transitive_relation.rs +++ b/src/librustc_data_structures/transitive_relation.rs @@ -53,6 +53,10 @@ struct Edge { } impl TransitiveRelation { + pub fn elements(&self) -> impl Iterator { + self.elements.iter() + } + pub fn is_empty(&self) -> bool { self.edges.is_empty() } 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 3b663ef6dad61..693f2c9207ef0 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 @@ -10,6 +10,7 @@ use rustc::mir::ConstraintCategory; use rustc::traits::query::outlives_bounds::{self, OutlivesBound}; use rustc::traits::query::type_op::{self, TypeOp}; use rustc::ty::{self, RegionVid, Ty}; +use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::transitive_relation::TransitiveRelation; use std::rc::Rc; use syntax_pos::DUMMY_SP; @@ -358,6 +359,10 @@ impl UniversalRegionRelationsBuilder<'cx, 'gcx, 'tcx> { /// over the `FreeRegionMap` from lexical regions and /// `UniversalRegions` (from NLL)`. impl<'tcx> FreeRegionRelations<'tcx> for UniversalRegionRelations<'tcx> { + fn all_regions(&self) -> FxHashSet> { + self.universal_regions.regions().map(|r| *r).collect() + } + fn sub_free_regions(&self, shorter: ty::Region<'tcx>, longer: ty::Region<'tcx>) -> bool { let shorter = shorter.to_region_vid(); assert!(self.universal_regions.is_universal_region(shorter)); diff --git a/src/librustc_mir/borrow_check/nll/universal_regions.rs b/src/librustc_mir/borrow_check/nll/universal_regions.rs index ae8dfa8144fd9..c39d0f1f8e9f4 100644 --- a/src/librustc_mir/borrow_check/nll/universal_regions.rs +++ b/src/librustc_mir/borrow_check/nll/universal_regions.rs @@ -208,6 +208,10 @@ impl<'tcx> UniversalRegions<'tcx> { }.build() } + pub fn regions(&self) -> impl Iterator> { + self.indices.indices.keys() + } + /// Given a reference to a closure type, extracts all the values /// from its free regions and returns a vector with them. This is /// used when the closure's creator checks that the From 74a623f64f1f3c7111272359e449efcaedd28a38 Mon Sep 17 00:00:00 2001 From: Alexander Regueiro Date: Sun, 24 Mar 2019 19:30:09 +0000 Subject: [PATCH 3/3] Updated tests. --- src/librustc/infer/opaque_types/mod.rs | 2 +- .../issue-55608-captures-empty-region.rs | 6 ++-- .../region-escape-via-bound-contravariant.rs | 2 +- .../ui/impl-trait/region-escape-via-bound.rs | 14 ++++++--- .../ui/impl-trait/region-escape-via-swap.rs | 29 +++++++++++++++++++ .../impl-trait/region-escape-via-swap.stderr | 12 ++++++++ 6 files changed, 56 insertions(+), 9 deletions(-) create mode 100644 src/test/ui/impl-trait/region-escape-via-swap.rs create mode 100644 src/test/ui/impl-trait/region-escape-via-swap.stderr diff --git a/src/librustc/infer/opaque_types/mod.rs b/src/librustc/infer/opaque_types/mod.rs index b7862691b05f3..8fba8b493bd6b 100644 --- a/src/librustc/infer/opaque_types/mod.rs +++ b/src/librustc/infer/opaque_types/mod.rs @@ -404,7 +404,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { // For soundness, we need to ensure that every region that is captured by the opaque type // but does not explicitly appear in the opaque type outlives the actual (concrete) type. // This allows for invariant lifetimes to be captured by opaque types as long as - // short-lived lifetimes are not permitted to escape and cause UB. + // short-lived lifetimes are not permitted to escape and cause UB. let opaque_substs_regions = opaque_defn.substs.regions().collect(); let captured_regions = concrete_ty_regions.difference(&opaque_substs_regions); for captured_region in captured_regions { diff --git a/src/test/ui/impl-trait/issue-55608-captures-empty-region.rs b/src/test/ui/impl-trait/issue-55608-captures-empty-region.rs index 7ebc348996f5e..68ebaa5b46491 100644 --- a/src/test/ui/impl-trait/issue-55608-captures-empty-region.rs +++ b/src/test/ui/impl-trait/issue-55608-captures-empty-region.rs @@ -1,9 +1,9 @@ +// run-pass + // This used to ICE because it creates an `impl Trait` that captures a // hidden empty region. -#![feature(conservative_impl_trait)] - -fn server() -> impl FilterBase2 { //~ ERROR [E0700] +fn server() -> impl FilterBase2 { segment2(|| { loop { } }).map2(|| "") } diff --git a/src/test/ui/impl-trait/region-escape-via-bound-contravariant.rs b/src/test/ui/impl-trait/region-escape-via-bound-contravariant.rs index e2310a3907f7e..d7f114f823003 100644 --- a/src/test/ui/impl-trait/region-escape-via-bound-contravariant.rs +++ b/src/test/ui/impl-trait/region-escape-via-bound-contravariant.rs @@ -1,4 +1,4 @@ -// In contrast to `region-escape-via-bound-invariant`, in this case we +// In contrast to `region-escape-via-bound.rs`, in this case we // *can* return a value of type `&'x u32`, even though `'x` does not // appear in the bounds. This is because `&` is contravariant, and so // we are *actually* returning a `&'y u32`. diff --git a/src/test/ui/impl-trait/region-escape-via-bound.rs b/src/test/ui/impl-trait/region-escape-via-bound.rs index d62aec800e8ce..6c3e7c292e67e 100644 --- a/src/test/ui/impl-trait/region-escape-via-bound.rs +++ b/src/test/ui/impl-trait/region-escape-via-bound.rs @@ -1,5 +1,7 @@ -// Test that we do not allow the region `'x` to escape in the impl -// trait **even though** `'y` escapes, which outlives `'x`. +// run-pass + +// Test that we allow the region `'x` to escape in the impl +// because 'y` escapes, which outlives `'x`. // // See https://github.com/rust-lang/rust/issues/46541 for more details. @@ -14,10 +16,14 @@ trait Trait<'a> { } impl Trait<'b> for Cell<&'a u32> { } fn foo(x: Cell<&'x u32>) -> impl Trait<'y> - //~^ ERROR hidden type for `impl Trait` captures lifetime that does not appear in bounds [E0700] + // ^ hidden type for `impl Trait` captures lifetime that does not appear in bounds + // because it outlives the lifetime that *does* appear in the bounds, `'y` where 'x: 'y { x } -fn main() { } +fn main() { + let x = 123; + let _ = foo(Cell::new(&x)); +} diff --git a/src/test/ui/impl-trait/region-escape-via-swap.rs b/src/test/ui/impl-trait/region-escape-via-swap.rs new file mode 100644 index 0000000000000..f31440bc99c5e --- /dev/null +++ b/src/test/ui/impl-trait/region-escape-via-swap.rs @@ -0,0 +1,29 @@ +// compile-fail + +// See https://github.com/rust-lang/rust/pull/57870#issuecomment-457333709 for more details. + +trait Swap: Sized { + fn swap(self, other: Self); +} + +impl Swap for &mut T { + fn swap(self, other: Self) { + std::mem::swap(self, other); + } +} + +// The hidden relifetimegion `'b` should not be allowed to escape this function, since it may not +// outlive '`a`, in which case we have a dangling reference. +fn hide<'a, 'b, T: 'static>(x: &'a mut &'b T) -> impl Swap + 'a { +//~^ lifetime mismatch [E0623] + x +} + +fn dangle() -> &'static [i32; 3] { + let mut res = &[4, 5, 6]; + let x = [1, 2, 3]; + hide(&mut res).swap(hide(&mut &x)); + res +} + +fn main() {} diff --git a/src/test/ui/impl-trait/region-escape-via-swap.stderr b/src/test/ui/impl-trait/region-escape-via-swap.stderr new file mode 100644 index 0000000000000..ad5418a6e1b2f --- /dev/null +++ b/src/test/ui/impl-trait/region-escape-via-swap.stderr @@ -0,0 +1,12 @@ +error[E0623]: lifetime mismatch + --> $DIR/region-escape-via-swap.rs:17:50 + | +LL | fn hide<'a, 'b, T: 'static>(x: &'a mut &'b T) -> impl Swap + 'a { + | ----- ^^^^^^^^^^^^^^ + | | | + | | ...but data from `x` is returned here + | this parameter and the return type are declared with different lifetimes... + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0623`.