Make subtyping explicit in MIR

[ouz-a]

This adds new mir-opt that pushes new ProjectionElem called ProjectionElem::Subtype(T) to Rvalue of a subtyped assignment so we can unsoundness issues like #107205

Addresses #112651

r? @lcnr

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[RalfJung]

I am quite confused by the fact that this is added as a mir-opt. A mir-opt can never change the meaning of a program. So this means that MIR is valid even without these subtyping projections. But this also means this approach is unsuited to prevent any bugs, since it would be completely okay for code to alter MIR without adding these projections.

[ouz-a]

I am quite confused by the fact that this is added as a mir-opt. A mir-opt can never change the meaning of a program. So this means that MIR is valid even without these subtyping projections. But this also means this approach is unsuited to prevent any bugs, since it would be completely okay for code to alter MIR without adding these projections.

I'm implementing this with suggestions from @lcnr, they will give the details better than I do, but as far as I understand our goal here is to make subtyping explicit and easy to see/debug, and return ICE instead of unsound code. How should we approach this problem if not with mir-opt. Some discussions regarding this mir-opt are here #107205 (comment) and here #112307 (comment) .

Thanks for the reviews!

[RalfJung]

Well the primary choice to make is whether MIR without explicit subtyping is still considered valid or not. If it is valid, then codegen must be able to handle it, and this new pass doesn't really change that. If it is not valid, then MIR building should already introduce the required subtyping projections. Ever having invalid MIR is a potential problem. After all, if codegen gets subtyping wrong, why would other MIR consumers do any better? Why only protect codegen with this new pass?

There's also the option of making this a new MIR phase, but we should have very good justification to introduce a new phase since those come with overhead in terms of specification and always having to ensure each MIR pass only runs on MIR that is in a phase it can work with.

[lcnr]

I don't think subtyping can sensibly be explicit before mir borrowck, because we'd have to add subtyping projections everywhere as we don't have borrowck information. After borrowck, we only care about subtyping which can impact trait selection/TypeId, i.e. subtyping which affects the type after erasing all free regons.

Having this subtyping be explicit during optimization and codegen is dangerous, so my goal is to make it full explicit before any optimizations are run. I believe that subtyping should always be fully explicit (and checked by the validator) in MirPhase::Runtime. MIR with implicit subtyping during this phase will be invalid.

[RalfJung]

Having this subtyping be explicit during optimization and codegen is dangerous

I assume you mean "implicit" is dangerous?
I'm fine with making this an analysis MIR vs runtime MIR thing. That should be properly reflected in validation and documentation.

[lcnr]

unsure about the ICE, probably makes sense to only enable this transformation with a flag and then enable that flag while compiling UI tests or sth

[ouz-a]

unsure about the ICE, probably makes sense to only enable this transformation with a flag and then enable that flag while compiling UI tests or sth

Problem is this also ICEs on some of the ui tests

[oli-obk]

Problem is this also ICEs on some of the ui tests

yes, but it is easier to debug then ;)

[rustbot]

This PR changes src/bootstrap/defaults/config.compiler.toml. If appropriate, please also update config.codegen.toml so the defaults are in sync.

[rust-timer]

Finished benchmarking commit (eb0f3ed): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.4%, 0.8%]	2
Regressions ❌ (secondary)	0.5%	[0.1%, 1.1%]	17
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.5%]	2
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	0.0%	[-0.6%, 0.8%]	4

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.

	mean	range	count
Regressions ❌ (primary)	3.8%	[2.7%, 4.4%]	3
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-5.2%	[-9.3%, -0.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-9.3%, 4.4%]	6

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)	-2.9%	[-3.6%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Binary size

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.1%	[0.0%, 0.5%]	17
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	12
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.3%, 0.5%]	27

Bootstrap: 623.042s -> 620.328s (-0.44%)
Artifact size: 271.97 MiB -> 272.00 MiB (0.01%)

[rylev]

@ouz-a @lcnr Perf is mostly neutral in primary benchmarks, but there do seem to be more regressions in secondary benchmarks. I imagine these might be expected if we're strictly doing more work in some of those cases?

[lcnr]

yeah, we should have probably done a perf run before merging 😅 #112651 (comment) may slightly improve it again, but we do end up simply doing more work

[Mark-Simulacrum]

Nominating for stable as it's a dependency of the approved #116415 PR.

[Mark-Simulacrum]

Already backported and included in 1.74.1, marking as such.