Ban field-projecting into [rustc_layout_scalar_valid_range_*] types

[scottmcm]

Proposal

When you look at MIR today, you'll often see things that look like missed-optimization bugs, such as this:

https://github.com/rust-lang/rust/blob/d3a4b1f46ba0fff6239e3d75abd285287ccd17f9/tests/mir-opt/pre-codegen/vec_deref.vec_deref_to_slice.PreCodegen.after.panic-abort.mir#L76-L77

    _4 = copy (((((*_1).0: alloc::raw_vec::RawVec<u8>).0: alloc::raw_vec::RawVecInner).0: std::ptr::Unique<u8>).0: std::ptr::NonNull<u8>);
    _5 = copy (_4.0: *const u8);

Why, after we successfully collapsed all those field projections in the first line into one place projection, didn't the second line also get collapsed?

Well, it's because NonNull is

#[rustc_layout_scalar_valid_range_start(1)]

And thus if we put that projection in a chain, it would be harder for the backends to notice that it needs extra !nonnull metadata, because it'd look like it's just loading a normal pointer -- after all, that's the type that we'd be getting from the load.

It would be nice, then, to just disallow putting something like this in the middle of a ProjectionElem chain, so that if future MIR building or MIR optimizations accidentally re-introduce it, it'll be caught early as an ICE, rather than a subtle perf regression bug.

What would the code and the MIR do instead? Transmute it. That code is already handling metadata for niched types, whether ones using scalar_valid_range, primitives like char & bool, or enums, so it already works.

We're already moving that way anyway for other reasons, so I think this wouldn't be much, if any, of a hardship.

Since we moved to the generic NonZero<_> type for integers, that's already using transmute instead of field projection https://github.com/rust-lang/rust/blob/d3a4b1f46ba0fff6239e3d75abd285287ccd17f9/library/core/src/num/nonzero.rs#L446-L455 and construction rust-lang/rust#120809 .

The future of custom-validity types like this is @oli-obk's pattern types, which already in rust-lang/rust#120131 require this:

The only way to create or deconstruct a pattern type is via transmute.

(After all, there's no field in a u32 is 1.. to aggregate or to which to project.)

Thus doing this will also get the user code -- like rustc_newtype_index! -- ready for when we can move to using pattern types instead of the attribute.

Mentors or Reviewers

I can take on doing the various PRs needed for this.

The basic check itself is easy, just something like this in the validator

                        if self.tcx.get_attr(adt_def.did(), sym::rustc_layout_scalar_valid_range_start).is_some() {
                            self.fail(
                                location,
                                format!("You can't project to field {f:?} of rustc_layout_scalar_valid_range_start type {adt_def:?}"),
                            );
                        }
                        if self.tcx.get_attr(adt_def.did(), sym::rustc_layout_scalar_valid_range_end).is_some() {
                            self.fail(
                                location,
                                format!("You can't project to field {f:?} of rustc_layout_scalar_valid_range_end type {adt_def:?}"),
                            );
                        }

But there will be a tail of other things that would need to happen too before that -- notably updating NonNull in core, and updating things like ElaborateBoxDerefs -- so it will need various reviews along the way from MIR reviewers and standard library reviewers.

Process

The main points of the Major Change Process are as follows:

• File an issue describing the proposal.
• A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
  • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
  • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
• Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler

[WaffleLapkin]

@rustbot second

[apiraino]

@rustbot label -final-comment-period +major-change-accepted