From 24a8d3bce38607b385f19571d5120b892b18ae9a Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 16 Jul 2021 17:34:17 +0000 Subject: [PATCH 1/2] Add some more tracing instrumentation --- compiler/rustc_trait_selection/src/opaque_types.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_trait_selection/src/opaque_types.rs b/compiler/rustc_trait_selection/src/opaque_types.rs index 0061ce4ed37..d5e334b5c65 100644 --- a/compiler/rustc_trait_selection/src/opaque_types.rs +++ b/compiler/rustc_trait_selection/src/opaque_types.rs @@ -568,6 +568,7 @@ fn generate_member_constraint( /// - `substs`, the substs used to instantiate this opaque type /// - `instantiated_ty`, the inferred type C1 -- fully resolved, lifted version of /// `opaque_defn.concrete_ty` + #[instrument(skip(self))] fn infer_opaque_definition_from_instantiation( &self, opaque_type_key: OpaqueTypeKey<'tcx>, @@ -576,11 +577,6 @@ fn infer_opaque_definition_from_instantiation( ) -> Ty<'tcx> { let OpaqueTypeKey { def_id, substs } = opaque_type_key; - debug!( - "infer_opaque_definition_from_instantiation(def_id={:?}, instantiated_ty={:?})", - def_id, instantiated_ty - ); - // Use substs to build up a reverse map from regions to their // identity mappings. This is necessary because of `impl // Trait` lifetimes are computed by replacing existing @@ -588,6 +584,7 @@ fn infer_opaque_definition_from_instantiation( // `impl Trait` return type, resulting in the parameters // shifting. let id_substs = InternalSubsts::identity_for_item(self.tcx, def_id); + debug!(?id_substs); let map: FxHashMap, GenericArg<'tcx>> = substs.iter().enumerate().map(|(index, subst)| (subst, id_substs[index])).collect(); @@ -602,7 +599,7 @@ fn infer_opaque_definition_from_instantiation( instantiated_ty, span, )); - debug!("infer_opaque_definition_from_instantiation: definition_ty={:?}", definition_ty); + debug!(?definition_ty); definition_ty } @@ -857,7 +854,7 @@ fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> { self.tcx.mk_generator(def_id, substs, movability) } - ty::Param(..) => { + ty::Param(param) => { // Look it up in the substitution list. match self.map.get(&ty.into()).map(|k| k.unpack()) { // Found it in the substitution list; replace with the parameter from the @@ -865,6 +862,7 @@ fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> { Some(GenericArgKind::Type(t1)) => t1, Some(u) => panic!("type mapped to unexpected kind: {:?}", u), None => { + debug!(?param, ?self.map); self.tcx .sess .struct_span_err( @@ -931,8 +929,8 @@ struct Instantiator<'a, 'tcx> { } impl<'a, 'tcx> Instantiator<'a, 'tcx> { + #[instrument(skip(self))] fn instantiate_opaque_types_in_map>(&mut self, value: T) -> T { - debug!("instantiate_opaque_types_in_map(value={:?})", value); let tcx = self.infcx.tcx; value.fold_with(&mut BottomUpFolder { tcx, From ebe21ac23a8a259079c9c59669c1d7da1fa88d0c Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 16 Jul 2021 17:34:23 +0000 Subject: [PATCH 2/2] Infer all inference variables via InferCx The previous algorithm was correct for the example given in its documentation, but when the TAIT was declared as a free item instead of an associated item, the generic parameters were the wrong ones. --- compiler/rustc_middle/src/ty/mod.rs | 2 +- compiler/rustc_typeck/src/check/mod.rs | 117 +----------------- compiler/rustc_typeck/src/check/writeback.rs | 52 ++++---- .../ui/type-alias-impl-trait/issue-60371.rs | 2 +- .../type-alias-impl-trait/issue-60371.stderr | 7 +- 5 files changed, 36 insertions(+), 144 deletions(-) diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index 156e860e1a3..f1fa964628b 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -856,7 +856,7 @@ pub fn is_empty(&self) -> bool { } } -#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, TyEncodable, TyDecodable)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, TyEncodable, TyDecodable, TypeFoldable)] pub struct OpaqueTypeKey<'tcx> { pub def_id: DefId, pub substs: SubstsRef<'tcx>, diff --git a/compiler/rustc_typeck/src/check/mod.rs b/compiler/rustc_typeck/src/check/mod.rs index ff7d291d3c9..34d0908bcc7 100644 --- a/compiler/rustc_typeck/src/check/mod.rs +++ b/compiler/rustc_typeck/src/check/mod.rs @@ -112,11 +112,9 @@ use rustc_index::bit_set::BitSet; use rustc_index::vec::Idx; use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind}; -use rustc_middle::ty::fold::{TypeFoldable, TypeFolder}; use rustc_middle::ty::query::Providers; -use rustc_middle::ty::subst::GenericArgKind; use rustc_middle::ty::subst::{InternalSubsts, Subst, SubstsRef}; -use rustc_middle::ty::{self, RegionKind, Ty, TyCtxt, UserType}; +use rustc_middle::ty::{self, Ty, TyCtxt, UserType}; use rustc_session::config; use rustc_session::parse::feature_err; use rustc_session::Session; @@ -321,117 +319,6 @@ fn used_trait_imports(tcx: TyCtxt<'_>, def_id: LocalDefId) -> &FxHashSet Self::MyItem -/// } -/// impl MyTrait for T where T: Iterator { -/// type MyItem = impl Iterator; -/// fn use_it(self) -> Self::MyItem { -/// self -/// } -/// } -/// ``` -/// -/// When we normalize the signature of `use_it` from the impl block, -/// we will normalize `Self::MyItem` to the opaque type `impl Iterator` -/// However, this projection result may contain inference variables, due -/// to the way that projection works. We didn't have any inference variables -/// in the signature to begin with - leaving them in will cause us to incorrectly -/// conclude that we don't have a defining use of `MyItem`. By mapping inference -/// variables back to the actual generic parameters, we will correctly see that -/// we have a defining use of `MyItem` -fn fixup_opaque_types<'tcx, T>(tcx: TyCtxt<'tcx>, val: T) -> T -where - T: TypeFoldable<'tcx>, -{ - struct FixupFolder<'tcx> { - tcx: TyCtxt<'tcx>, - } - - impl<'tcx> TypeFolder<'tcx> for FixupFolder<'tcx> { - fn tcx<'a>(&'a self) -> TyCtxt<'tcx> { - self.tcx - } - - fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> { - match *ty.kind() { - ty::Opaque(def_id, substs) => { - debug!("fixup_opaque_types: found type {:?}", ty); - // Here, we replace any inference variables that occur within - // the substs of an opaque type. By definition, any type occurring - // in the substs has a corresponding generic parameter, which is what - // we replace it with. - // This replacement is only run on the function signature, so any - // inference variables that we come across must be the rust of projection - // (there's no other way for a user to get inference variables into - // a function signature). - if ty.needs_infer() { - let new_substs = InternalSubsts::for_item(self.tcx, def_id, |param, _| { - let old_param = substs[param.index as usize]; - match old_param.unpack() { - GenericArgKind::Type(old_ty) => { - if let ty::Infer(_) = old_ty.kind() { - // Replace inference type with a generic parameter - self.tcx.mk_param_from_def(param) - } else { - old_param.fold_with(self) - } - } - GenericArgKind::Const(old_const) => { - if let ty::ConstKind::Infer(_) = old_const.val { - // This should never happen - we currently do not support - // 'const projections', e.g.: - // `impl MyTrait for T where ::MyConst == 25` - // which should be the only way for us to end up with a const inference - // variable after projection. If Rust ever gains support for this kind - // of projection, this should *probably* be changed to - // `self.tcx.mk_param_from_def(param)` - bug!( - "Found infer const: `{:?}` in opaque type: {:?}", - old_const, - ty - ); - } else { - old_param.fold_with(self) - } - } - GenericArgKind::Lifetime(old_region) => { - if let RegionKind::ReVar(_) = old_region { - self.tcx.mk_param_from_def(param) - } else { - old_param.fold_with(self) - } - } - } - }); - let new_ty = self.tcx.mk_opaque(def_id, new_substs); - debug!("fixup_opaque_types: new type: {:?}", new_ty); - new_ty - } else { - ty - } - } - _ => ty.super_fold_with(self), - } - } - } - - debug!("fixup_opaque_types({:?})", val); - val.fold_with(&mut FixupFolder { tcx }) -} - fn typeck_const_arg<'tcx>( tcx: TyCtxt<'tcx>, (did, param_did): (LocalDefId, DefId), @@ -510,8 +397,6 @@ fn typeck_with_fallback<'tcx>( fn_sig, ); - let fn_sig = fixup_opaque_types(tcx, fn_sig); - let fcx = check_fn(&inh, param_env, fn_sig, decl, id, body, None).0; fcx } else { diff --git a/compiler/rustc_typeck/src/check/writeback.rs b/compiler/rustc_typeck/src/check/writeback.rs index 589570f1cb7..0aa059b7de8 100644 --- a/compiler/rustc_typeck/src/check/writeback.rs +++ b/compiler/rustc_typeck/src/check/writeback.rs @@ -496,6 +496,8 @@ fn visit_opaque_types(&mut self, span: Span) { debug_assert!(!instantiated_ty.has_escaping_bound_vars()); + let opaque_type_key = self.fcx.fully_resolve(opaque_type_key).unwrap(); + // Prevent: // * `fn foo() -> Foo` // * `fn foo() -> Foo` @@ -508,6 +510,8 @@ fn visit_opaque_types(&mut self, span: Span) { // fn foo() -> Foo { .. } // ``` // figures out the concrete type with `U`, but the stored type is with `T`. + + // FIXME: why are we calling this here? This seems too early, and duplicated. let definition_ty = self.fcx.infer_opaque_definition_from_instantiation( opaque_type_key, instantiated_ty, @@ -529,33 +533,33 @@ fn visit_opaque_types(&mut self, span: Span) { } } - if !opaque_type_key.substs.needs_infer() { - // We only want to add an entry into `concrete_opaque_types` - // if we actually found a defining usage of this opaque type. - // Otherwise, we do nothing - we'll either find a defining usage - // in some other location, or we'll end up emitting an error due - // to the lack of defining usage - if !skip_add { - let old_concrete_ty = self - .typeck_results - .concrete_opaque_types - .insert(opaque_type_key, definition_ty); - if let Some(old_concrete_ty) = old_concrete_ty { - if old_concrete_ty != definition_ty { - span_bug!( - span, - "`visit_opaque_types` tried to write different types for the same \ + if opaque_type_key.substs.needs_infer() { + span_bug!(span, "{:#?} has inference variables", opaque_type_key.substs) + } + + // We only want to add an entry into `concrete_opaque_types` + // if we actually found a defining usage of this opaque type. + // Otherwise, we do nothing - we'll either find a defining usage + // in some other location, or we'll end up emitting an error due + // to the lack of defining usage + if !skip_add { + let old_concrete_ty = self + .typeck_results + .concrete_opaque_types + .insert(opaque_type_key, definition_ty); + if let Some(old_concrete_ty) = old_concrete_ty { + if old_concrete_ty != definition_ty { + span_bug!( + span, + "`visit_opaque_types` tried to write different types for the same \ opaque type: {:?}, {:?}, {:?}, {:?}", - opaque_type_key.def_id, - definition_ty, - opaque_defn, - old_concrete_ty, - ); - } + opaque_type_key.def_id, + definition_ty, + opaque_defn, + old_concrete_ty, + ); } } - } else { - self.tcx().sess.delay_span_bug(span, "`opaque_defn` has inference variables"); } } } diff --git a/src/test/ui/type-alias-impl-trait/issue-60371.rs b/src/test/ui/type-alias-impl-trait/issue-60371.rs index 4ac7f9423ff..b7c8a58a656 100644 --- a/src/test/ui/type-alias-impl-trait/issue-60371.rs +++ b/src/test/ui/type-alias-impl-trait/issue-60371.rs @@ -9,7 +9,7 @@ trait Bug { impl Bug for &() { type Item = impl Bug; //~ ERROR `impl Trait` in type aliases is unstable //~^ ERROR the trait bound `(): Bug` is not satisfied - //~^^ ERROR could not find defining uses + //~^^ ERROR the trait bound `(): Bug` is not satisfied const FUN: fn() -> Self::Item = || (); //~^ ERROR type alias impl trait is not permitted here diff --git a/src/test/ui/type-alias-impl-trait/issue-60371.stderr b/src/test/ui/type-alias-impl-trait/issue-60371.stderr index 255d381bf06..6857d5264b6 100644 --- a/src/test/ui/type-alias-impl-trait/issue-60371.stderr +++ b/src/test/ui/type-alias-impl-trait/issue-60371.stderr @@ -25,11 +25,14 @@ LL | type Item = impl Bug; = help: the following implementations were found: <&() as Bug> -error: could not find defining uses +error[E0277]: the trait bound `(): Bug` is not satisfied --> $DIR/issue-60371.rs:10:17 | LL | type Item = impl Bug; - | ^^^^^^^^ + | ^^^^^^^^ the trait `Bug` is not implemented for `()` + | + = help: the following implementations were found: + <&() as Bug> error: aborting due to 4 previous errors