From c1aa9bf84945642c0beec4838322502b88dccf03 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 26 Sep 2022 23:13:12 +0000 Subject: [PATCH 1/4] Fix subst issues with RPITIT --- .../src/check/compare_method.rs | 37 ++++++++++++++++--- compiler/rustc_middle/src/ty/subst.rs | 19 ++++++++-- .../src/traits/project.rs | 5 ++- .../ui/impl-trait/in-trait/issue-102301.rs | 18 +++++++++ 4 files changed, 69 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/impl-trait/in-trait/issue-102301.rs diff --git a/compiler/rustc_hir_analysis/src/check/compare_method.rs b/compiler/rustc_hir_analysis/src/check/compare_method.rs index 5e5dbedb4bd7b..986d5bed39ef7 100644 --- a/compiler/rustc_hir_analysis/src/check/compare_method.rs +++ b/compiler/rustc_hir_analysis/src/check/compare_method.rs @@ -551,15 +551,40 @@ pub fn collect_trait_impl_trait_tys<'tcx>( let id_substs = InternalSubsts::identity_for_item(tcx, def_id); debug!(?id_substs, ?substs); let map: FxHashMap, ty::GenericArg<'tcx>> = - substs.iter().enumerate().map(|(index, arg)| (arg, id_substs[index])).collect(); + std::iter::zip(substs, id_substs).collect(); debug!(?map); + // NOTE(compiler-errors): RPITITs, like all other RPITs, have early-bound + // region substs that are synthesized during AST lowering. These are substs + // that are appended to the parent substs (trait and trait method). However, + // we're trying to infer the unsubstituted type value of the RPITIT inside + // the *impl*, so we can later use the impl's method substitutions to normalize + // an RPITIT to a concrete type. + // + // Due to the design of RPITITs, during AST lowering, we have no idea that + // an impl method is satisfying a trait method with RPITITs in it. Therefore, + // we don't have a list ofearly-bound region substs for the RPITIT in the impl. + // Since early region parameters are index-based, we can't just rebase these + // (trait method) early-bound region substs onto the impl, since region + // parameters are index-based, and there's no guarantee that the indices from + // the trait substs and impl substs line up -- so we subtract the number of + // trait substs and add the number of impl substs to *renumber* these early- + // bound regions to their corresponding indices in the impl's substitutions list. + // + // Also, we only need to account for a difference in trait and impl substs, + // since we previously enforce that the trait method and impl method have the + // same generics. + let num_trait_substs = trait_to_impl_substs.len(); + let num_impl_substs = tcx.generics_of(impl_m.container_id(tcx)).params.len(); let ty = tcx.fold_regions(ty, |region, _| { - if let ty::ReFree(_) = region.kind() { - map[®ion.into()].expect_region() - } else { - region - } + let ty::ReFree(_) = region.kind() else { return region; }; + let ty::ReEarlyBound(e) = map[®ion.into()].expect_region().kind() + else { bug!("expected ReFree to map to ReEarlyBound"); }; + tcx.mk_region(ty::ReEarlyBound(ty::EarlyBoundRegion { + def_id: e.def_id, + name: e.name, + index: (e.index as usize - num_trait_substs + num_impl_substs) as u32, + })) }); debug!(%ty); collected_tys.insert(def_id, ty); diff --git a/compiler/rustc_middle/src/ty/subst.rs b/compiler/rustc_middle/src/ty/subst.rs index 36eb2ab5157e5..e552d3f1cc551 100644 --- a/compiler/rustc_middle/src/ty/subst.rs +++ b/compiler/rustc_middle/src/ty/subst.rs @@ -606,9 +606,21 @@ impl<'a, 'tcx> TypeFolder<'tcx> for SubstFolder<'a, 'tcx> { fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> { #[cold] #[inline(never)] - fn region_param_out_of_range(data: ty::EarlyBoundRegion) -> ! { + fn region_param_out_of_range(data: ty::EarlyBoundRegion, substs: &[GenericArg<'_>]) -> ! { bug!( - "Region parameter out of range when substituting in region {} (index={})", + "Region parameter out of range when substituting in region {} (index={}, substs = {:?})", + data.name, + data.index, + substs, + ) + } + + #[cold] + #[inline(never)] + fn region_param_invalid(data: ty::EarlyBoundRegion, other: GenericArgKind<'_>) -> ! { + bug!( + "Unexpected parameter {:?} when substituting in region {} (index={})", + other, data.name, data.index ) @@ -624,7 +636,8 @@ impl<'a, 'tcx> TypeFolder<'tcx> for SubstFolder<'a, 'tcx> { let rk = self.substs.get(data.index as usize).map(|k| k.unpack()); match rk { Some(GenericArgKind::Lifetime(lt)) => self.shift_region_through_binders(lt), - _ => region_param_out_of_range(data), + Some(other) => region_param_invalid(data, other), + None => region_param_out_of_range(data, self.substs), } } _ => r, diff --git a/compiler/rustc_trait_selection/src/traits/project.rs b/compiler/rustc_trait_selection/src/traits/project.rs index 1ca9a1c1994f8..693c172893154 100644 --- a/compiler/rustc_trait_selection/src/traits/project.rs +++ b/compiler/rustc_trait_selection/src/traits/project.rs @@ -2254,7 +2254,10 @@ fn confirm_impl_trait_in_trait_candidate<'tcx>( } let impl_fn_def_id = leaf_def.item.def_id; - let impl_fn_substs = obligation.predicate.substs.rebase_onto(tcx, trait_fn_def_id, data.substs); + // Rebase from {trait}::{fn}::{opaque} to {impl}::{fn}::{opaque}, + // since `data.substs` are the impl substs. + let impl_fn_substs = + obligation.predicate.substs.rebase_onto(tcx, tcx.parent(trait_fn_def_id), data.substs); let cause = ObligationCause::new( obligation.cause.span, diff --git a/src/test/ui/impl-trait/in-trait/issue-102301.rs b/src/test/ui/impl-trait/in-trait/issue-102301.rs new file mode 100644 index 0000000000000..a93714a658ef0 --- /dev/null +++ b/src/test/ui/impl-trait/in-trait/issue-102301.rs @@ -0,0 +1,18 @@ +// check-pass + +#![feature(return_position_impl_trait_in_trait)] +#![allow(incomplete_features)] + +trait Foo { + fn foo>(self) -> impl Foo; +} + +struct Bar; + +impl Foo for Bar { + fn foo>(self) -> impl Foo { + self + } +} + +fn main() {} From cb20758257a5efe790e27460df53c12bf1c90403 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 27 Sep 2022 00:54:02 +0000 Subject: [PATCH 2/4] Add test --- src/test/ui/async-await/in-trait/issue-102310.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 src/test/ui/async-await/in-trait/issue-102310.rs diff --git a/src/test/ui/async-await/in-trait/issue-102310.rs b/src/test/ui/async-await/in-trait/issue-102310.rs new file mode 100644 index 0000000000000..49c3e9feeb4c4 --- /dev/null +++ b/src/test/ui/async-await/in-trait/issue-102310.rs @@ -0,0 +1,15 @@ +// check-pass +// edition:2021 + +#![feature(async_fn_in_trait)] +#![allow(incomplete_features)] + +pub trait SpiDevice { + async fn transaction(&mut self); +} + +impl SpiDevice for () { + async fn transaction(&mut self) {} +} + +fn main() {} From e994de803df1f1a9f2bbd4da1258d03ea05b4231 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 10 Oct 2022 23:35:51 +0000 Subject: [PATCH 3/4] Equate full fn signatures to infer all region variables --- .../src/check/compare_method.rs | 46 +++++++++++++------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/compare_method.rs b/compiler/rustc_hir_analysis/src/check/compare_method.rs index 986d5bed39ef7..6ee534363855a 100644 --- a/compiler/rustc_hir_analysis/src/check/compare_method.rs +++ b/compiler/rustc_hir_analysis/src/check/compare_method.rs @@ -465,30 +465,30 @@ pub fn collect_trait_impl_trait_tys<'tcx>( let ocx = ObligationCtxt::new(infcx); let norm_cause = ObligationCause::misc(return_span, impl_m_hir_id); - let impl_return_ty = ocx.normalize( + let impl_sig = ocx.normalize( norm_cause.clone(), param_env, - infcx - .replace_bound_vars_with_fresh_vars( - return_span, - infer::HigherRankedType, - tcx.fn_sig(impl_m.def_id), - ) - .output(), + infcx.replace_bound_vars_with_fresh_vars( + return_span, + infer::HigherRankedType, + tcx.fn_sig(impl_m.def_id), + ), ); + let impl_return_ty = impl_sig.output(); let mut collector = ImplTraitInTraitCollector::new(&ocx, return_span, param_env, impl_m_hir_id); - let unnormalized_trait_return_ty = tcx + let unnormalized_trait_sig = tcx .liberate_late_bound_regions( impl_m.def_id, tcx.bound_fn_sig(trait_m.def_id).subst(tcx, trait_to_placeholder_substs), ) - .output() .fold_with(&mut collector); - let trait_return_ty = - ocx.normalize(norm_cause.clone(), param_env, unnormalized_trait_return_ty); + let trait_sig = ocx.normalize(norm_cause.clone(), param_env, unnormalized_trait_sig); + let trait_return_ty = trait_sig.output(); - let wf_tys = FxHashSet::from_iter([unnormalized_trait_return_ty, trait_return_ty]); + let wf_tys = FxHashSet::from_iter( + unnormalized_trait_sig.inputs_and_output.iter().chain(trait_sig.inputs_and_output.iter()), + ); match infcx.at(&cause, param_env).eq(trait_return_ty, impl_return_ty) { Ok(infer::InferOk { value: (), obligations }) => { @@ -521,6 +521,26 @@ pub fn collect_trait_impl_trait_tys<'tcx>( } } + // Unify the whole function signature. We need to do this to fully infer + // the lifetimes of the return type, but do this after unifying just the + // return types, since we want to avoid duplicating errors from + // `compare_predicate_entailment`. + match infcx + .at(&cause, param_env) + .eq(tcx.mk_fn_ptr(ty::Binder::dummy(trait_sig)), tcx.mk_fn_ptr(ty::Binder::dummy(impl_sig))) + { + Ok(infer::InferOk { value: (), obligations }) => { + ocx.register_obligations(obligations); + } + Err(terr) => { + let guar = tcx.sess.delay_span_bug( + return_span, + format!("could not unify `{trait_sig}` and `{impl_sig}`: {terr:?}"), + ); + return Err(guar); + } + } + // Check that all obligations are satisfied by the implementation's // RPITs. let errors = ocx.select_all_or_error(); From 4259f333054033b603d9ba80962bd28fa1eee3af Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 15 Oct 2022 17:49:32 +0000 Subject: [PATCH 4/4] typos --- .../src/check/compare_method.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/compare_method.rs b/compiler/rustc_hir_analysis/src/check/compare_method.rs index 6ee534363855a..60eaad9b498fc 100644 --- a/compiler/rustc_hir_analysis/src/check/compare_method.rs +++ b/compiler/rustc_hir_analysis/src/check/compare_method.rs @@ -578,18 +578,18 @@ pub fn collect_trait_impl_trait_tys<'tcx>( // region substs that are synthesized during AST lowering. These are substs // that are appended to the parent substs (trait and trait method). However, // we're trying to infer the unsubstituted type value of the RPITIT inside - // the *impl*, so we can later use the impl's method substitutions to normalize - // an RPITIT to a concrete type. + // the *impl*, so we can later use the impl's method substs to normalize + // an RPITIT to a concrete type (`confirm_impl_trait_in_trait_candidate`). // // Due to the design of RPITITs, during AST lowering, we have no idea that - // an impl method is satisfying a trait method with RPITITs in it. Therefore, - // we don't have a list ofearly-bound region substs for the RPITIT in the impl. + // an impl method corresponds to a trait method with RPITITs in it. Therefore, + // we don't have a list of early-bound region substs for the RPITIT in the impl. // Since early region parameters are index-based, we can't just rebase these - // (trait method) early-bound region substs onto the impl, since region - // parameters are index-based, and there's no guarantee that the indices from - // the trait substs and impl substs line up -- so we subtract the number of - // trait substs and add the number of impl substs to *renumber* these early- - // bound regions to their corresponding indices in the impl's substitutions list. + // (trait method) early-bound region substs onto the impl, and there's no + // guarantee that the indices from the trait substs and impl substs line up. + // So to fix this, we subtract the number of trait substs and add the number of + // impl substs to *renumber* these early-bound regions to their corresponding + // indices in the impl's substitutions list. // // Also, we only need to account for a difference in trait and impl substs, // since we previously enforce that the trait method and impl method have the