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

[compiler-errors]

So in #113182, I introduced a "diagnostics improvement" in the form of 473c88d, 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:

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 473c88d 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).

[rustbot]

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[compiler-errors]

r? types

Or @cjgillot could review this if they want, since they were involved in some of this impl -> trait RPITIT remapping in the history of RPITITs and so may be able to page in some context.

[compiler-errors]

You may think, wow this diagnostic sucks, since it's mentioning a lifetime that isn't even in the signature.

Well, it does suck, but it doesn't suck any more than the error you would get from the code if it had no RPITITs. So I don't think it needs fixing here.

[compiler-errors]

We don't need this.

In fact, it has the potential to cause ICEs since us not remapping bivariant lifetimes on opaques may lead us to leaking infer vars into query responses. This does not happen in practice though.

[BoxyUwU]

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

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

Fails to compile on nightly, why does adding a RPITIT make this start compiling. Even if the logic for RPITITs is wrong I wouldn't have expected this to affect impl checks in general

[compiler-errors]

Because the bad RPITIT adds implied bounds due to the bidirectional outlives we install in: https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_analysis/src/collect/predicates_of.rs#L329

[compiler-errors]

It doesn't pass on stable because #129383 changed the ordering we duplicate lifetimes (soundly, that PR did not do anything's wrong) but that affects what regions we end up mapping the hidden type back to.

We end up remapping an opaque hidden type to Opaque<'a, 'late> which has that bad implied bound.

[BoxyUwU]

r? @lcnr

seems reasonable to me but I don't have a very rigorous understanding of opaques

[lcnr]

r=me after nit

[compiler-errors]

@bors r=lcnr rollup

[bors]

📌 Commit 871cfc9 has been approved by lcnr

It is now in the queue for this repository.