WF check lifetime bounds for locals with type params

[ShoyuVanilla]

Fixes #115175
Fixes #148854

[ShoyuVanilla]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5ce1ef4 (5ce1ef4ffbf1e6d8c09f46fa655ea765bacf4427, parent: cf8a95590a1b768b7711f2631d5b5e3ead464de7)

[ShoyuVanilla]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: f70ee28 (f70ee28831acc074c109afab32d287057c3eca46, parent: cf8a95590a1b768b7711f2631d5b5e3ead464de7)

[ShoyuVanilla]

@craterbot check

[craterbot]

👌 Experiment pr-149389 created and queued.
🤖 Automatically detected try build f70ee28
🔍 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-149389 is now running

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

[craterbot]

🎉 Experiment pr-149389 is completed!
📊 5 regressed and 3 fixed (746168 total)
📊 2051 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

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

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-149389/retry-regressed-list.txt ↩

[ShoyuVanilla]

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-149389/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-149389-1 created and queued.
🤖 Automatically detected try build f70ee28
🔍 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

[petrochenkov]

@craterbot p=11
All the small runs will complete in a few hours and won't need to wait for a few days until the long beta runs complete.

[craterbot]

📝 Configuration of the pr-149389-1 experiment changed.

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

[craterbot]

🚧 Experiment pr-149389-1 is now running

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

[craterbot]

🎉 Experiment pr-149389-1 is completed!
📊 0 regressed and 0 fixed (1832 total)
📊 118 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

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

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-149389-1/retry-regressed-list.txt ↩

[ShoyuVanilla]

I expected lots of breakages but seems not
r? lcnr

[lcnr]

can you also add a test for the unsoundness?

[ShoyuVanilla]

Added a test with the code from #148854

[lcnr]

Suggested change

	// This has to error due to an unsatisfied outlives bound on
	// `Inner<T: 'static>` as its implicit drop relies on that
	// bound.

[lcnr]

writing an FCP for this rn

View changes since this review

[lcnr]

@rfcbot fcp merge types

MIR typeck needs to check that all used types are well-formed. While one could imagine checking all locals for well-formedness, we don't. We only check types which flow into the body and assuming that most expressions and subtyping maintain well-formedness.

As part of this FCP proposal I took some time to look through everything chcked for WF in borrowck, it should be the following:

• function signatures of Call terminators, both before and after normalization
• the array type of Repeat expressions (I don't know why)
• FnDef -> FnPtr coercions to reduce impact of Implied bounds on nested references + variance = soundness hole #25860
• FnDef constants
• all type information provided by the user, tracked in multiple ways
  • user_type_annotations in the MIR body
  • the user_ty of local_decls
  • the rvalue_user_type for ADT constructors?
  • AscribeUserType statements used for a bunch of stuff otherwise not checked
  • the type of constants used in the body to handle e.g. const N: Inv<'static> and let _: Inv<'a> = N;

Notably, we only check that the type of locals is well-formed if we explicitly track the fact that the user provided type information for them. As a performance optimization, we only track that fact if checking well-formedness may result in lifetime constraints.

This previously didn't add user type annotations for types which involve type params but no regions. This meant the following incorrectly compiles

struct Static<T: 'static>(T);
fn test<T>() {
    let _ = None::<Static<T>>;
}

@theemathas was then able to weaponize this in #148854 by adding a Drop impl which relies on this bound.

This PR changes us to weaken the perf optimization so that we now also check user type annotations if they have type parameters in them. There are no crater regressions from this change.

[rust-rfcbot]

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[oli-obk]

@rust-timer build f70ee28

[rust-timer]

Finished benchmarking commit (f70ee28): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.2%, 1.0%]	2
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-0.4%, 1.0%]	3

Max RSS (memory usage)

Results (primary -5.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (primary -1.3%, secondary -1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-2.5%	[-3.0%, -2.2%]	3
Improvements ✅ (secondary)	-3.0%	[-3.8%, -2.2%]	2
All ❌✅ (primary)	-1.3%	[-3.0%, 2.2%]	4

Binary size

Results (primary -0.1%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-1.0%, -0.1%]	5
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	32
All ❌✅ (primary)	-0.1%	[-1.0%, 0.0%]	11

Bootstrap: 470.335s -> 475.664s (1.13%)
Artifact size: 386.93 MiB -> 386.89 MiB (-0.01%)

[lqd]

Is it expected that changing WF has diffs in mir-opts tests?

[oli-obk]

It looks like there are query differences in metadata. Maybe the wf checks cause some difference but I wouldn't know how or why. I'd say noise (even if reproducible, any other change could cause it, too)