Neither require nor imply lifetime bounds on opaque type for well formedness

[oli-obk]

The actual hidden type can live arbitrarily longer than any individual lifetime and arbitrarily shorter than all but one of the lifetimes.

fixes #86218
fixes #84305

This is a breaking change but it is a necessary soundness fix

[rust-highfive]

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

[oli-obk]

r? @jackh726

[oli-obk]

When the compiler encounters a &'b OpaqueType<'a> it assumes that 'a: 'b. This is not correct though, as the hidden type of the opaque type may be (), so one can produce an &'b OpaqueType<'a> simply by providing any &'b (). Most of the time this is not an issue as creating a &'b OpaqueType<'a> constrains everything else with the same lifetimes, so one can indeed rely on 'a: 'b. An example where the check works is

    type OpaqueType<'a> = impl Send;
    fn convert<'a, 'b, T: ?Sized>(x: &'a T, _proof: &'b OpaqueType<'a>) -> &'b T {
        x
    }
    let r;
    {
        let x = String::from("Hello World?");
        r = convert(&x, &()); // error `x` must live for `'static`
    }
    println!("{}", r);

The more complex example in #84305 (comment) demonstrates that with sufficient associated type indirection this error can be avoided.

[oli-obk]

@bors try

preparing for crater check run, as this is a breaking change

[bors]

⌛ Trying commit bbceabb934a5864cfb93326ba887cda4df563997 with merge 42d1facfffa1a5357d6d62815f7be91f8ad69ed8...

[jackh726]

Can you also move this into the parent directory? The intention was for this to be "known-bugs", but I guess I could have named better.

[jackh726]

When the compiler encounters ...

This comment is helpful, thanks! Will think a bit about this when I get some time.

[b-naber]

Does this also fix #42940?

[bors]

☀️ Try build successful - checks-actions
Build commit: 42d1facfffa1a5357d6d62815f7be91f8ad69ed8 (42d1facfffa1a5357d6d62815f7be91f8ad69ed8)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-95474 created and queued.
🤖 Automatically detected try build 42d1facfffa1a5357d6d62815f7be91f8ad69ed8
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-95474 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-95474 is completed!
📊 11 regressed and 2 fixed (223147 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[oli-obk]

None of the failures look related. There is a spurious "overflow evaluating requirement" in lu_packets that we should look into separately, but I don't see how this PR could have caused it.

My reasoning for it being spurious is that it manifests with different types:

• Vec: https://crater-reports.s3.amazonaws.com/pr-95474/try%2342d1facfffa1a5357d6d62815f7be91f8ad69ed8/gh/lcdr.world_server/log.txt
• Option: https://crater-reports.s3.amazonaws.com/pr-95474/try%2342d1facfffa1a5357d6d62815f7be91f8ad69ed8/gh/lcdr.lu_packets/log.txt

[oli-obk]

This is a breaking change, fixing a soundness bug. Crater found no regressions.

TLDR: &'b OpaqueType<'a> currently implies 'a: 'b, even though that isn't necessarily true if the opaque type's hidden type does not have any lifetimes, thus causing unsoundness. After this PR, we don't imply that this outlives bound holds anymore.

A moderately longer explanation of what is going on can be found in this earlier comment on this PR. An example of code that will now fail to compile is the newly added test. Note that it will also break cases that accidentally use this soundly.

[jackh726]

Let's wait for lang team decision until I review.

[aliemjay]

👍

[jackh726]

@bors r+

[bors]

📌 Commit 59e285f has been approved by jackh726

It is now in the queue for this repository.

[bors]

⌛ Testing commit 59e285f with merge f5193a9...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing f5193a9 to master...

[rust-timer]

Finished benchmarking commit (f5193a9): 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)

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[4.3%, 4.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.

	mean1	range	count2
Regressions ❌ (primary)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[1.8%, 1.8%]	1

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²