From d26bfca8f83d8a931197970de9d3e4b1afff9e6f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 30 Oct 2019 20:47:40 -0400 Subject: [PATCH 1/4] Normalize all opaque types when converting ParamEnv to Reveal::All When we normalize a type using a ParamEnv with a reveal mode of RevealMode::All, we will normalize opaque types to their underlying types (e.g. `type MyOpaque = impl Foo` -> `StructThatImplsFoo`). However, the ParamEnv may still have predicates referring to the un-normalized opaque type (e.g. `>`). This can cause trait projection to fail, since a type containing normalized opaque types will not match up with the un-normalized type in the `ParamEnv`. To fix this, we now explicitly normalize all opaque types in caller_bounds of a `ParamEnv` when changing its mode to `RevealMode::All`. This ensures that all predicatse will refer to the underlying types of any opaque types involved, allowing them to be matched up properly during projection. To reflect the fact that normalization is occuring, `ParamEnv::with_reveal_all` is renamed to `ParamEnv::with_reveal_all_normalized` Fixes #65918 --- src/librustc/traits/specialize/mod.rs | 2 +- src/librustc/ty/layout.rs | 4 +- src/librustc/ty/mod.rs | 12 +- src/librustc/ty/sty.rs | 4 +- src/librustc/ty/util.rs | 151 ++++++++++-------- src/librustc_codegen_utils/symbol_names/v0.rs | 2 +- src/librustc_mir/shim.rs | 2 +- src/librustc_mir/transform/elaborate_drops.rs | 2 +- .../opaque-type-in-predicate.rs | 52 ++++++ 9 files changed, 155 insertions(+), 76 deletions(-) create mode 100644 src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs diff --git a/src/librustc/traits/specialize/mod.rs b/src/librustc/traits/specialize/mod.rs index 88a2db3dc6223..2a7aadf27f21d 100644 --- a/src/librustc/traits/specialize/mod.rs +++ b/src/librustc/traits/specialize/mod.rs @@ -130,7 +130,7 @@ pub fn find_associated_item<'tcx>( match ancestors.leaf_def(tcx, item.ident, item.kind) { Some(node_item) => { let substs = tcx.infer_ctxt().enter(|infcx| { - let param_env = param_env.with_reveal_all(); + let param_env = param_env.with_reveal_all_normalized(tcx); let substs = substs.rebase_onto(tcx, trait_def_id, impl_data.substs); let substs = translate_substs(&infcx, param_env, impl_data.impl_def_id, substs, node_item.node); diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index c67e6a0a13e63..d3fb7b9d8e3db 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -1949,7 +1949,7 @@ impl<'tcx> LayoutOf for LayoutCx<'tcx, TyCtxt<'tcx>> { /// Computes the layout of a type. Note that this implicitly /// executes in "reveal all" mode. fn layout_of(&self, ty: Ty<'tcx>) -> Self::TyLayout { - let param_env = self.param_env.with_reveal_all(); + let param_env = self.param_env.with_reveal_all_normalized(self.tcx); let ty = self.tcx.normalize_erasing_regions(param_env, ty); let details = self.tcx.layout_raw(param_env.and(ty))?; let layout = TyLayout { @@ -1976,7 +1976,7 @@ impl LayoutOf for LayoutCx<'tcx, ty::query::TyCtxtAt<'tcx>> { /// Computes the layout of a type. Note that this implicitly /// executes in "reveal all" mode. fn layout_of(&self, ty: Ty<'tcx>) -> Self::TyLayout { - let param_env = self.param_env.with_reveal_all(); + let param_env = self.param_env.with_reveal_all_normalized(*self.tcx); let ty = self.tcx.normalize_erasing_regions(param_env, ty); let details = self.tcx.layout_raw(param_env.and(ty))?; let layout = TyLayout { diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 8ccfc467f4a0b..87ec404d96202 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -1666,7 +1666,7 @@ impl<'tcx> ParamEnv<'tcx> { /// environments like codegen or doing optimizations. /// /// N.B., if you want to have predicates in scope, use `ParamEnv::new`, - /// or invoke `param_env.with_reveal_all()`. + /// or invoke `param_env.with_reveal_all_normalized()`. #[inline] pub fn reveal_all() -> Self { Self::new(List::empty(), Reveal::All, None) @@ -1688,8 +1688,14 @@ impl<'tcx> ParamEnv<'tcx> { /// the desired behavior during codegen and certain other special /// contexts; normally though we want to use `Reveal::UserFacing`, /// which is the default. - pub fn with_reveal_all(self) -> Self { - ty::ParamEnv { reveal: Reveal::All, ..self } + /// + /// All opaque types in the caller_bounds of the `ParamEnv` + /// will be normalized to their underlying types + pub fn with_reveal_all_normalized(self, tcx: TyCtxt<'tcx>) -> Self { + let caller_bounds = tcx.normalize_impl_trait_types( + &self.caller_bounds + ); + ty::ParamEnv { reveal: Reveal::All, caller_bounds, ..self } } /// Returns this same environment but with no caller bounds. diff --git a/src/librustc/ty/sty.rs b/src/librustc/ty/sty.rs index 6cb0d1e9946b5..d1180ddff4e02 100644 --- a/src/librustc/ty/sty.rs +++ b/src/librustc/ty/sty.rs @@ -2319,7 +2319,7 @@ impl<'tcx> Const<'tcx> { ty: Ty<'tcx>, ) -> Option { assert_eq!(self.ty, ty); - let size = tcx.layout_of(param_env.with_reveal_all().and(ty)).ok()?.size; + let size = tcx.layout_of(param_env.with_reveal_all_normalized(tcx).and(ty)).ok()?.size; // if `ty` does not depend on generic parameters, use an empty param_env self.eval(tcx, param_env).val.try_to_bits(size) } @@ -2331,7 +2331,7 @@ impl<'tcx> Const<'tcx> { param_env: ParamEnv<'tcx>, ) -> &Const<'tcx> { let try_const_eval = |did, param_env: ParamEnv<'tcx>, substs| { - let param_env_and_substs = param_env.with_reveal_all().and(substs); + let param_env_and_substs = param_env.with_reveal_all_normalized().and(substs); // Avoid querying `tcx.const_eval(...)` with any e.g. inference vars. if param_env_and_substs.has_local_value() { diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index 0b11a9efd81d7..e5af753a4f667 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -8,6 +8,7 @@ use crate::mir::interpret::{sign_extend, truncate}; use crate::ich::NodeIdHashingMode; use crate::traits::{self, ObligationCause}; use crate::ty::{self, DefIdTree, Ty, TyCtxt, GenericParamDefKind, TypeFoldable}; +use crate::ty::fold::TypeFolder; use crate::ty::subst::{Subst, InternalSubsts, SubstsRef, GenericArgKind}; use crate::ty::query::TyCtxtAt; use crate::ty::TyKind::*; @@ -699,81 +700,32 @@ impl<'tcx> TyCtxt<'tcx> { } } + /// Normalizes all opaque types in the given value, replacing them + /// with their underlying types. + pub fn normalize_impl_trait_types>(self, val: &T) -> T { + let mut visitor = OpaqueTypeExpander { + seen_opaque_tys: FxHashSet::default(), + expanded_cache: FxHashMap::default(), + primary_def_id: None, + found_recursion: false, + check_recursion: false, + tcx: self, + }; + val.fold_with(&mut visitor) + } + /// Expands the given impl trait type, stopping if the type is recursive. pub fn try_expand_impl_trait_type( self, def_id: DefId, substs: SubstsRef<'tcx>, ) -> Result, Ty<'tcx>> { - use crate::ty::fold::TypeFolder; - - struct OpaqueTypeExpander<'tcx> { - // Contains the DefIds of the opaque types that are currently being - // expanded. When we expand an opaque type we insert the DefId of - // that type, and when we finish expanding that type we remove the - // its DefId. - seen_opaque_tys: FxHashSet, - // Cache of all expansions we've seen so far. This is a critical - // optimization for some large types produced by async fn trees. - expanded_cache: FxHashMap<(DefId, SubstsRef<'tcx>), Ty<'tcx>>, - primary_def_id: DefId, - found_recursion: bool, - tcx: TyCtxt<'tcx>, - } - - impl<'tcx> OpaqueTypeExpander<'tcx> { - fn expand_opaque_ty( - &mut self, - def_id: DefId, - substs: SubstsRef<'tcx>, - ) -> Option> { - if self.found_recursion { - return None; - } - let substs = substs.fold_with(self); - if self.seen_opaque_tys.insert(def_id) { - let expanded_ty = match self.expanded_cache.get(&(def_id, substs)) { - Some(expanded_ty) => expanded_ty, - None => { - let generic_ty = self.tcx.type_of(def_id); - let concrete_ty = generic_ty.subst(self.tcx, substs); - let expanded_ty = self.fold_ty(concrete_ty); - self.expanded_cache.insert((def_id, substs), expanded_ty); - expanded_ty - } - }; - self.seen_opaque_tys.remove(&def_id); - Some(expanded_ty) - } else { - // If another opaque type that we contain is recursive, then it - // will report the error, so we don't have to. - self.found_recursion = def_id == self.primary_def_id; - None - } - } - } - - impl<'tcx> TypeFolder<'tcx> for OpaqueTypeExpander<'tcx> { - fn tcx(&self) -> TyCtxt<'tcx> { - self.tcx - } - - fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { - if let ty::Opaque(def_id, substs) = t.kind { - self.expand_opaque_ty(def_id, substs).unwrap_or(t) - } else if t.has_projections() { - t.super_fold_with(self) - } else { - t - } - } - } - let mut visitor = OpaqueTypeExpander { seen_opaque_tys: FxHashSet::default(), expanded_cache: FxHashMap::default(), - primary_def_id: def_id, + primary_def_id: Some(def_id), found_recursion: false, + check_recursion: true, tcx: self, }; let expanded_type = visitor.expand_opaque_ty(def_id, substs).unwrap(); @@ -785,6 +737,75 @@ impl<'tcx> TyCtxt<'tcx> { } } +struct OpaqueTypeExpander<'tcx> { + // Contains the DefIds of the opaque types that are currently being + // expanded. When we expand an opaque type we insert the DefId of + // that type, and when we finish expanding that type we remove the + // its DefId. + seen_opaque_tys: FxHashSet, + // Cache of all expansions we've seen so far. This is a critical + // optimization for some large types produced by async fn trees. + expanded_cache: FxHashMap<(DefId, SubstsRef<'tcx>), Ty<'tcx>>, + primary_def_id: Option, + found_recursion: bool, + // Whether or not to check for recursive opaque types. + // This is 'true' when we're explicitly checking for opaque type + // recursion, and 'false' otherwise to avoid unecessary work. + check_recursion: bool, + tcx: TyCtxt<'tcx>, +} + +impl<'tcx> OpaqueTypeExpander<'tcx> { + fn expand_opaque_ty( + &mut self, + def_id: DefId, + substs: SubstsRef<'tcx>, + ) -> Option> { + if self.found_recursion { + return None; + } + let substs = substs.fold_with(self); + if !self.check_recursion || self.seen_opaque_tys.insert(def_id) { + let expanded_ty = match self.expanded_cache.get(&(def_id, substs)) { + Some(expanded_ty) => expanded_ty, + None => { + let generic_ty = self.tcx.type_of(def_id); + let concrete_ty = generic_ty.subst(self.tcx, substs); + let expanded_ty = self.fold_ty(concrete_ty); + self.expanded_cache.insert((def_id, substs), expanded_ty); + expanded_ty + } + }; + if self.check_recursion { + self.seen_opaque_tys.remove(&def_id); + } + Some(expanded_ty) + } else { + // If another opaque type that we contain is recursive, then it + // will report the error, so we don't have to. + self.found_recursion = def_id == *self.primary_def_id.as_ref().unwrap(); + None + } + } +} + +impl<'tcx> TypeFolder<'tcx> for OpaqueTypeExpander<'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { + if let ty::Opaque(def_id, substs) = t.kind { + self.expand_opaque_ty(def_id, substs).unwrap_or(t) + } else if t.has_projections() { + t.super_fold_with(self) + } else { + t + } + } +} + + impl<'tcx> ty::TyS<'tcx> { /// Checks whether values of this type `T` are *moved* or *copied* /// when referenced -- this amounts to a check for whether `T: diff --git a/src/librustc_codegen_utils/symbol_names/v0.rs b/src/librustc_codegen_utils/symbol_names/v0.rs index 1dfcc21f3903d..0cf58f259c7a4 100644 --- a/src/librustc_codegen_utils/symbol_names/v0.rs +++ b/src/librustc_codegen_utils/symbol_names/v0.rs @@ -264,7 +264,7 @@ impl Printer<'tcx> for SymbolMangler<'tcx> { let parent_def_id = DefId { index: key.parent.unwrap(), ..impl_def_id }; let mut param_env = self.tcx.param_env(impl_def_id) - .with_reveal_all(); + .with_reveal_all_normalized(self.tcx); if !substs.is_empty() { param_env = param_env.subst(self.tcx, substs); } diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 708686fdcf9f1..de92324d6f3f2 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -213,7 +213,7 @@ fn build_drop_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, ty: Option>) }); } let patch = { - let param_env = tcx.param_env(def_id).with_reveal_all(); + let param_env = tcx.param_env(def_id).with_reveal_all_normalized(tcx); let mut elaborator = DropShimElaborator { body: &body, patch: MirPatch::new(&body), diff --git a/src/librustc_mir/transform/elaborate_drops.rs b/src/librustc_mir/transform/elaborate_drops.rs index f91a08bcd9aa6..3b852c633d62a 100644 --- a/src/librustc_mir/transform/elaborate_drops.rs +++ b/src/librustc_mir/transform/elaborate_drops.rs @@ -25,7 +25,7 @@ impl<'tcx> MirPass<'tcx> for ElaborateDrops { debug!("elaborate_drops({:?} @ {:?})", src, body.span); let def_id = src.def_id(); - let param_env = tcx.param_env(src.def_id()).with_reveal_all(); + let param_env = tcx.param_env(src.def_id()).with_reveal_all_normalized(tcx); let move_data = match MoveData::gather_moves(body, tcx) { Ok(move_data) => move_data, Err(_) => bug!("No `move_errors` should be allowed in MIR borrowck"), diff --git a/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs b/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs new file mode 100644 index 0000000000000..e09fe9d037c41 --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs @@ -0,0 +1,52 @@ +// run-pass +// Regression test for issue #65918 - checks that we do not +// ICE when attempting to normalize a predicate containting +// opaque types + +#![feature(type_alias_impl_trait)] + +use std::marker::PhantomData; + +/* copied Index and TryFrom for convinience (and simplicity) */ +trait MyIndex { + type O; + fn my_index(self) -> Self::O; +} +trait MyFrom: Sized { + type Error; + fn my_from(value: T) -> Result; +} + +/* MCVE starts here */ +trait F {} +impl F for () {} +type DummyT = impl F; +fn _dummy_t() -> DummyT {} + +struct Phantom1(PhantomData); +struct Phantom2(PhantomData); +struct Scope(Phantom2>); + +impl Scope { + fn new() -> Self { + Scope(Phantom2(PhantomData)) + } +} + +impl MyFrom> for Phantom1 { + type Error = (); + fn my_from(_: Phantom2) -> Result { + Ok(Phantom1(PhantomData)) + } +} + +impl>>, U> MyIndex> for Scope { + type O = T; + fn my_index(self) -> Self::O { + MyFrom::my_from(self.0).ok().unwrap() + } +} + +fn main() { + let _pos: Phantom1> = Scope::new().my_index(); +} From db8e92a929fd6454e84e54ad425d8b96e9a23caa Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 30 Oct 2019 22:22:05 -0400 Subject: [PATCH 2/4] Fix nits Co-Authored-By: Mazdak Farrokhzad --- src/librustc/ty/mod.rs | 2 +- src/librustc/ty/util.rs | 10 +++++----- .../type-alias-impl-trait/opaque-type-in-predicate.rs | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 87ec404d96202..4a9c599214774 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -1690,7 +1690,7 @@ impl<'tcx> ParamEnv<'tcx> { /// which is the default. /// /// All opaque types in the caller_bounds of the `ParamEnv` - /// will be normalized to their underlying types + /// will be normalized to their underlying types. pub fn with_reveal_all_normalized(self, tcx: TyCtxt<'tcx>) -> Self { let caller_bounds = tcx.normalize_impl_trait_types( &self.caller_bounds diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index e5af753a4f667..b04cde5c29687 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -738,18 +738,18 @@ impl<'tcx> TyCtxt<'tcx> { } struct OpaqueTypeExpander<'tcx> { - // Contains the DefIds of the opaque types that are currently being - // expanded. When we expand an opaque type we insert the DefId of + // Contains the `DefId`s of the opaque types that are currently being + // expanded. When we expand an opaque type we insert the `DefId` of // that type, and when we finish expanding that type we remove the - // its DefId. + // its `DefId`. seen_opaque_tys: FxHashSet, // Cache of all expansions we've seen so far. This is a critical - // optimization for some large types produced by async fn trees. + // optimization for some large types produced by `async fn` trees. expanded_cache: FxHashMap<(DefId, SubstsRef<'tcx>), Ty<'tcx>>, primary_def_id: Option, found_recursion: bool, // Whether or not to check for recursive opaque types. - // This is 'true' when we're explicitly checking for opaque type + // This is `true` when we're explicitly checking for opaque type // recursion, and 'false' otherwise to avoid unecessary work. check_recursion: bool, tcx: TyCtxt<'tcx>, diff --git a/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs b/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs index e09fe9d037c41..36a287e032ca3 100644 --- a/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs +++ b/src/test/ui/type-alias-impl-trait/opaque-type-in-predicate.rs @@ -1,4 +1,5 @@ // run-pass + // Regression test for issue #65918 - checks that we do not // ICE when attempting to normalize a predicate containting // opaque types From 33e196df1ab95c1a0df43130895c6b9571f09ac1 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 30 Oct 2019 22:23:46 -0400 Subject: [PATCH 3/4] Use doc comments instead of regular comments --- src/librustc/ty/util.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs index b04cde5c29687..6e5d3dc716dc4 100644 --- a/src/librustc/ty/util.rs +++ b/src/librustc/ty/util.rs @@ -737,20 +737,22 @@ impl<'tcx> TyCtxt<'tcx> { } } +/// Expands any nested opaque types, caching the expansion +/// of each (DefId, SubstsRef) pair struct OpaqueTypeExpander<'tcx> { - // Contains the `DefId`s of the opaque types that are currently being - // expanded. When we expand an opaque type we insert the `DefId` of - // that type, and when we finish expanding that type we remove the - // its `DefId`. + /// Contains the `DefId`s of the opaque types that are currently being + /// expanded. When we expand an opaque type we insert the `DefId` of + /// that type, and when we finish expanding that type we remove the + /// its `DefId`. seen_opaque_tys: FxHashSet, - // Cache of all expansions we've seen so far. This is a critical - // optimization for some large types produced by `async fn` trees. + /// Cache of all expansions we've seen so far. This is a critical + /// optimization for some large types produced by `async fn` trees. expanded_cache: FxHashMap<(DefId, SubstsRef<'tcx>), Ty<'tcx>>, primary_def_id: Option, found_recursion: bool, - // Whether or not to check for recursive opaque types. - // This is `true` when we're explicitly checking for opaque type - // recursion, and 'false' otherwise to avoid unecessary work. + /// Whether or not to check for recursive opaque types. + /// This is `true` when we're explicitly checking for opaque type + /// recursion, and 'false' otherwise to avoid unecessary work. check_recursion: bool, tcx: TyCtxt<'tcx>, } From 10a5c93b332d2c377dae0107ef2a3874ccc214f3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 2 Dec 2019 13:00:41 -0500 Subject: [PATCH 4/4] Fix rebase fallout --- src/librustc/ty/sty.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/ty/sty.rs b/src/librustc/ty/sty.rs index d1180ddff4e02..e6fe52b0758db 100644 --- a/src/librustc/ty/sty.rs +++ b/src/librustc/ty/sty.rs @@ -2331,7 +2331,7 @@ impl<'tcx> Const<'tcx> { param_env: ParamEnv<'tcx>, ) -> &Const<'tcx> { let try_const_eval = |did, param_env: ParamEnv<'tcx>, substs| { - let param_env_and_substs = param_env.with_reveal_all_normalized().and(substs); + let param_env_and_substs = param_env.with_reveal_all_normalized(tcx).and(substs); // Avoid querying `tcx.const_eval(...)` with any e.g. inference vars. if param_env_and_substs.has_local_value() {