Enforce supertrait outlives obligations hold when confirming impl

[compiler-errors]

TL;DR: We elaborate super-predicates and apply any outlives obligations when proving an impl holds to fix a mismatch between implied bounds.

Bugs in implied bounds (and implied well-formedness) occur whenever there is a mismatch between the assumptions that some code can assume to hold, and the obligations that a caller/user of that code must prove. If the former is stronger than the latter, then unsoundness occurs.

Take a look at the example unsoundness:

use std::fmt::Display;

trait Static: 'static {}
impl<T> Static for &'static T {}
fn foo<S: Display>(x: S) -> Box<dyn Display>
where
    &'static S: Static,
{
    Box::new(x)
}

fn main() {
    let s = foo(&String::from("blah blah blah"));
    println!("{}", s);
}

This specific example occurs because we elaborate obligations in fn foo:

• &'static S: Static
  • &'static S: 'static <- super predicate
    • S: 'static <- elaborating outlives bounds

However, when calling foo, we only need to prove the direct set of where clauses. So at the call site for some substitution S = &'not_static str, that means only proving &'static &'not_static str: Static. To prove this, we apply the impl, which itself holds trivially since it has no where clauses.

This is the mismatch -- foo is allowed to assume that S: 'static via elaborating supertraits, but callers of foo never need to prove that S: 'static.

There are several approaches to fixing this, all of which have problems due to current limitations in our type system:

1. proving the elaborated set of predicates always - This leads to issues since we don't have coinductive trait semantics, so we easily hit new cycles.
   • This would fix our issue, since callers of foo would have to both prove &'static &'not_static str: Static and its elaborated bounds, which would surface the problematic 'not_static: 'static outlives obligation.
   • However, proving supertraits when proving impls leads to inductive cycles which can't be fixed until we get coinductive trait semantics.
2. Proving that an impl header is WF when applying that impl:
   • This would fix our issue, since when we try to prove &'static &'not_static str: Static, we'd need to prove WF(&'static &'not_static str), which would surface the problematic 'not_static: 'static outlives obligation.
   • However, this leads to issues since we don't have higher-ranked implied bounds. This breaks things when trying to apply impls to higher-ranked trait goals.

To get around these limitations, we apply a subset of (1.), which is to elaborate the supertrait obligations of the impl but filter only the (region/type) outlives out of that set, since those can never participate in an inductive cycle. This is likely not sufficient to fix a pathological example of this issue, but it does clearly fill in a major gap that we're currently overlooking.

This can also result in 'unintended' errors due to missing implied-bounds on binders. We did not encounter this in the crater run and don't expect people to rely on this code in practice:

trait Outlives<'b>: 'b {}
impl<'b, T> Outlives<'b> for &'b T {}
fn foo<'b>()
where
    // This bound will break due to this PR as we end up proving
    // `&'b &'!a (): 'b` without the implied `'!a: 'b`
    // bound.
    for<'a> &'b &'a (): Outlives<'b>, 
{}

Fixes #98117

---

Crater: #124336 (comment)
Triaged: #124336 (comment)

All of the fallout is due to generic const exprs, and can be ignored.

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 26513eb with merge 311f8b6...

[compiler-errors]

TODO: also do this in the new solver

[compiler-errors]

@craterbot check

[craterbot]

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

@craterbot run mode=check-only start=master#5557f8c9d08d7f3f680943dcf97b6d4a7eb13aea end=try#311f8b6ca41358db8ae08490a9ca0916f11e9de3

[craterbot]

👌 Experiment pr-124336 created and queued.
🔍 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-124336 is now running

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

[craterbot]

🎉 Experiment pr-124336 is completed!
📊 16 regressed and 3 fixed (442198 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

[apiraino]

seems the crater run wasnt too happy

@rustbot author

[compiler-errors]

@craterbot run mode=check-only start=master#5557f8c9d08d7f3f680943dcf97b6d4a7eb13aea end=try#311f8b6ca41358db8ae08490a9ca0916f11e9de3 crates=https://crater-reports.s3.amazonaws.com/pr-124336/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-124336-1 created and queued.
🔍 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-124336-1 is now running

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

[craterbot]

🎉 Experiment pr-124336-1 is completed!
📊 10 regressed and 0 fixed (16 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

[apiraino]

better, but still some regressions (if I read the report correctly)

@rustbot author

[lcnr]

Suggested change

	if matches!(self.tcx().def_kind(def_id), DefKind::Impl { of_trait: true })
	// Register any outlives obligations from the trait here, cc #124336.
	if matches!(self.tcx().def_kind(def_id), DefKind::Impl { of_trait: true })

[lcnr]

this comment doesn't feel right?

[compiler-errors]

I guess it should be "actually do coinduction correctly and just register supertrait obligations always". The impl header WF check is a totally different approach, yeah.

[lcnr]

We also don't elaborate all supertrait obligations 😅 we only elaborate region obligations as elaborating all of them causes inductive solver cycles

[compiler-errors]

Well I think the point here is that we do deep elaboration elaborate(...) rather than shallow elaboration tcx.explicit_supertraits(...). I'll reword the comment.

[lcnr]

r=me after comments

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

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

[compiler-errors]

2024-08-05T14:06:42.4927490Z #1 32.95 error: failed to list workers: Unavailable: connection error: desc = "transport: Error while dialing: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory"

lol

@bors r=lcnr

[bors]

📌 Commit fa9ae7b has been approved by lcnr

It is now in the queue for this repository.

[compiler-errors]

@bors rollup=never since i guess we're elaborating more here

[bors]

⌛ Testing commit fa9ae7b with merge 2b78d92...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 2b78d92 to master...

[rust-timer]

Finished benchmarking commit (2b78d92): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.2%, 1.5%]	95
Regressions ❌ (secondary)	1.8%	[0.2%, 4.2%]	38
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 1.5%]	95

Max RSS (memory usage)

Results (primary 1.9%, secondary 1.5%)

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)	1.9%	[1.1%, 2.7%]	2
Regressions ❌ (secondary)	3.9%	[2.7%, 5.2%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-5.1%, -0.8%]	3
All ❌✅ (primary)	1.9%	[1.1%, 2.7%]	2

Cycles

Results (primary 1.4%, secondary 4.0%)

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)	1.4%	[0.9%, 2.2%]	14
Regressions ❌ (secondary)	4.0%	[2.4%, 5.5%]	22
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[0.9%, 2.2%]	14

Binary size

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

Bootstrap: 758.182s -> 759.596s (0.19%)
Artifact size: 336.79 MiB -> 336.92 MiB (0.04%)

[compiler-errors]

Ruh roh!

[compiler-errors]

Let me see if I can queryify the transitive supertrait outlives here...

[Kobzol]

Regression resolved in #128746.

@rustbot label: +perf-regression-triaged