1
Fork 0

Rollup merge of #133428 - compiler-errors:rpitit-unsound, r=lcnr

Actually use placeholder regions for trait method late bound regions in `collect_return_position_impl_trait_in_trait_tys`

So in https://github.com/rust-lang/rust/pull/113182, I introduced a "diagnostics improvement" in the form of 473c88dfb6, which changes which signature we end up instantiating with placeholder regions and which signature we end up instantiating with fresh region vars so that we have placeholders corresponding to the names of the late-bound regions coming from the *impl*.

However, this is not sound, since now we're essentially no longer proving that *all* instantiations of the trait method are compatible with an instantiation of the impl method, but vice versa (which is weaker).  Let's look at the example `tests/ui/impl-trait/in-trait/do-not-imply-from-trait-impl.rs`:

```rust
trait MkStatic {
    fn mk_static(self) -> &'static str;
}

impl MkStatic for &'static str {
    fn mk_static(self) -> &'static str { self }
}

trait Foo {
    fn foo<'a: 'static, 'late>(&'late self) -> impl MkStatic;
}

impl Foo for str {
    fn foo<'a: 'static>(&'a self) -> impl MkStatic + 'static {
        self
    }
}

fn call_foo<T: Foo + ?Sized>(t: &T) -> &'static str {
    t.foo().mk_static()
}

fn main() {
    let s = call_foo(String::from("hello, world").as_str());
    println!("> {s}");
}
```

To collect RPITITs, we were previously instantiating the trait signature with infer vars (`fn(&'?0 str) -> ?1t` where `?1t` is the variable we use to infer the RPITIT) and the impl signature with placeholders (there are no late-bound regions in that signature, so we just have `fn(&'a str) -> Opaque`).

Equating the signatures works, since all we do is unify `?1t` with `Opaque` and `'?0` with `'a`. However, conceptually it *shouldn't* hold, since this definition is not valid for *all* instantiations of the trait method but just the one where `'0` (i.e. `'late`) is equal to `'a` :(

## So what

This PR effectively reverts 473c88dfb6 to fix the unsoundness.

Fixes #133427
Also fixes #133425, which is actually coincidentally another instance of this bug (but not one that is weaponized into UB, just one that causes an ICE in refinement checking).
This commit is contained in:
Guillaume Gomez 2024-11-28 03:14:47 +01:00 committed by GitHub
commit 3e095e864a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 92 additions and 44 deletions

View file

@ -523,8 +523,9 @@ pub(super) fn collect_return_position_impl_trait_in_trait_tys<'tcx>(
let impl_sig = ocx.normalize(
&misc_cause,
param_env,
tcx.liberate_late_bound_regions(
impl_m.def_id,
infcx.instantiate_binder_with_fresh_vars(
return_span,
infer::HigherRankedType,
tcx.fn_sig(impl_m.def_id).instantiate_identity(),
),
);
@ -536,10 +537,9 @@ pub(super) fn collect_return_position_impl_trait_in_trait_tys<'tcx>(
// them with inference variables.
// We will use these inference variables to collect the hidden types of RPITITs.
let mut collector = ImplTraitInTraitCollector::new(&ocx, return_span, param_env, impl_m_def_id);
let unnormalized_trait_sig = infcx
.instantiate_binder_with_fresh_vars(
return_span,
infer::HigherRankedType,
let unnormalized_trait_sig = tcx
.liberate_late_bound_regions(
impl_m.def_id,
tcx.fn_sig(trait_m.def_id).instantiate(tcx, trait_to_impl_args),
)
.fold_with(&mut collector);
@ -702,8 +702,8 @@ pub(super) fn collect_return_position_impl_trait_in_trait_tys<'tcx>(
let mut remapped_types = DefIdMap::default();
for (def_id, (ty, args)) in collected_types {
match infcx.fully_resolve((ty, args)) {
Ok((ty, args)) => {
match infcx.fully_resolve(ty) {
Ok(ty) => {
// `ty` contains free regions that we created earlier while liberating the
// trait fn signature. However, projection normalization expects `ty` to
// contains `def_id`'s early-bound regions.
@ -883,33 +883,27 @@ impl<'tcx> ty::FallibleTypeFolder<TyCtxt<'tcx>> for RemapHiddenTyRegions<'tcx> {
self.tcx
}
fn try_fold_ty(&mut self, t: Ty<'tcx>) -> Result<Ty<'tcx>, Self::Error> {
if let ty::Alias(ty::Opaque, ty::AliasTy { args, def_id, .. }) = *t.kind() {
let mut mapped_args = Vec::with_capacity(args.len());
for (arg, v) in std::iter::zip(args, self.tcx.variances_of(def_id)) {
mapped_args.push(match (arg.unpack(), v) {
// Skip uncaptured opaque args
(ty::GenericArgKind::Lifetime(_), ty::Bivariant) => arg,
_ => arg.try_fold_with(self)?,
});
}
Ok(Ty::new_opaque(self.tcx, def_id, self.tcx.mk_args(&mapped_args)))
} else {
t.try_super_fold_with(self)
}
}
fn try_fold_region(
&mut self,
region: ty::Region<'tcx>,
) -> Result<ty::Region<'tcx>, Self::Error> {
match region.kind() {
// Remap late-bound regions from the function.
// Never remap bound regions or `'static`
ty::ReBound(..) | ty::ReStatic | ty::ReError(_) => return Ok(region),
// We always remap liberated late-bound regions from the function.
ty::ReLateParam(_) => {}
// Remap early-bound regions as long as they don't come from the `impl` itself,
// in which case we don't really need to renumber them.
ty::ReEarlyParam(ebr) if ebr.index as usize >= self.num_impl_args => {}
_ => return Ok(region),
ty::ReEarlyParam(ebr) => {
if ebr.index as usize >= self.num_impl_args {
// Remap
} else {
return Ok(region);
}
}
ty::ReVar(_) | ty::RePlaceholder(_) | ty::ReErased => unreachable!(
"should not have leaked vars or placeholders into hidden type of RPITIT"
),
}
let e = if let Some(id_region) = self.map.get(&region) {