Use placeholders to prevent using inferred RPITIT types to imply their own well-formedness

[compiler-errors]

The issue here is that we use the same signature to do RPITIT inference as we do to compute implied bounds. To fix this, when gathering the assumed wf types for the method, we replace all of the infer vars (that will be eventually used to infer RPITIT types) with type placeholders, which imply nothing about lifetime bounds.

This solution kind of sucks, but I'm not certain there's another feasible way to fix this. If anyone has a better solution, I'd be glad to hear it.

My naive first solution was, instead of using placeholders, to replace the signature with the RPITIT projections that it originally started out with. But turns out that we can't just use the unnormalized signature of the trait method in implied_outlives_bounds since we normalize during WF computation -- that would cause a query cycle in collect_return_position_impl_trait_in_trait_tys.

idk who to request review...
r? @lcnr or @aliemjay i guess.

Fixes #116060

[aliemjay]

a couple problems with this implementation:

• implied bounds from the impl header are not used

#![feature(return_position_impl_trait_in_trait)]
trait Trait {
    fn foo() -> impl Sized;
}
impl<T> Trait for &'static T {
    fn foo() -> &'static T { loop {} } //~ERROR
}

• we should probably map the placeholder types back to the inferred hidden types in the result of infcx.implied_bounds_tys. Otherwise this breaks:

trait Trait {
    fn foo<T>() -> (&'static impl Sized, impl Sized);
}
impl Trait for () {
    fn foo<T>() -> (&'static T, &'static T) { loop {} }
}

Both cases seem rather rare and can hopefully be fixed later in a back-compat way, so I'm fine landing this PR as it is.

r? @aliemjay
@bors r+

[bors]

📌 Commit 8568121 has been approved by aliemjay

It is now in the queue for this repository.

[compiler-errors]

@aliemjay: Good point. I can fix both of these in a quick follow-up after this PR (or just r- if you want me to fix them now, they both seem easy to do).

[aliemjay]

Well, better to fix them here if you like so.
@bors r-
you can r=me if you changed your mind.

[aliemjay]

• we should probably map the placeholder types back to the inferred hidden types in the result of infcx.implied_bounds_tys. Otherwise this breaks:

In retrospect, this was a stupid idea :)
It is actually unsound:

#![feature(return_position_impl_trait_in_trait)]
use core::fmt::Display;
trait Trait {
    fn foo(self) -> (Option<&'static (impl Sized + 'static)>, Box<dyn Display>);
}
impl<T: Display> Trait for T {
    fn foo(self) -> (Option<&'static T>, Box<dyn Display>) {
        (None, Box::new(self))
    }
}
fn extend(arg: impl Trait) -> Box<dyn Display> {
    arg.foo().1
}
fn main() {
    let use_after_free = extend(&String::from("temporary"));
    println!("{}", use_after_free);
}

[compiler-errors]

Actually yeah I'll do that implied-bounds-from-the-impl-header one in a separate pr

@bors r=aliemjay

[bors]

📌 Commit 8568121 has been approved by aliemjay

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8568121 with merge 7bdbfb8...

[bors]

💔 Test failed - checks-actions

[compiler-errors]

?? not related

@bors retry

[rust-log-analyzer]

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling addr2line v0.21.0
[RUSTC-TIMING] addr2line test:false 0.535
[RUSTC-TIMING] gimli test:false 5.040
[RUSTC-TIMING] object test:false 5.691
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
  local time: Tue Sep 26 00:01:51 UTC 2023
  network time: Tue, 26 Sep 2023 00:01:51 GMT
##[endgroup]
##[endgroup]
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.

[bors]

⌛ Testing commit 8568121 with merge a61f6f3...

[bors]

☀️ Test successful - checks-actions
Approved by: aliemjay
Pushing a61f6f3 to master...

[rust-timer]

Finished benchmarking commit (a61f6f3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.966s -> 630.902s (-0.17%)
Artifact size: 317.23 MiB -> 317.18 MiB (-0.02%)

[lcnr]

@compiler-errors do you think it would be useful to extend the rustc-dev-guide with the changes from this PR?

It feels like there's a subtle soundness issue here so it would be good to document this as part of the "RPITIT overview".