Rip out rustc_layout_scalar_valid_range_* attribute support by oli-obk · Pull Request #155433 · rust-lang/rust

oli-obk · 2026-04-17T11:41:04Z

View all comments

And either removes tests for it or replaces the uses with pattern types.

primarily fixes #135996

fixes #147761
fixes #133652

rustbot · 2026-04-17T11:41:08Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the CTFE machinery

cc @RalfJung, @lcnr

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

rustbot · 2026-04-17T11:41:10Z

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 72 candidates
Random selection from 18 candidates

JonathanBrouwer · 2026-04-17T11:42:37Z

Happy to review this
Love the branch name 🤣

r? me

ChayimFriedman2

Please revert the rust-analyzer changes. r-a needs to support old compilers, and also it doesn't even properly support pattern types yet.

View changes since this review

rustbot · 2026-04-17T11:45:41Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

mejrs · 2026-04-17T19:32:11Z

There is some prose in the dev guide that will no longer apply.

The other complicated safety check is for writes to fields of layout constrained structs (such as NonNull). These are found by looking for the borrow or assignment expression and then visiting the subexpression being borrowed or assigned with a separate visitor.

https://rustc-dev-guide.rust-lang.org/unsafety-checking.html?other-checks-involving-unsafe

rustbot · 2026-04-21T07:09:41Z

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @tshepang

RalfJung

Nice :)

View changes since this review

RalfJung · 2026-04-21T13:26:41Z

+        // *After* all of this, check further information stored in the layout.
+        // On leaf types like `!` or empty enums, this will raise the error.
+        // This means that for types wrapping such a type, we won't ever get here, but it's
+        // just the simplest way to check for this case.
        //
        // FIXME: We could avoid some redundant checks here. For newtypes wrapping
        // scalars, we do the same check on every "level" (e.g., first we check
-        // MyNewtype and then the scalar in there).
+        // the fields of MyNewtype, and then we check MyNewType again).


This seems mostly outdated now, and the FIXME is resolved because there are no longer any redundant checks.

The only question that remains is why we need an is_uninhabited check here. Can this even be reached? Maybe it can be turned into an assertion that just double-checks that indeed we caught all uninhabited types above?

the uninhabited check is still reachable and it's sufficiently unrelated to pattern types and layout attributes that I've decided to leave the fixme and try it in a separate PR once this one lands

RalfJung · 2026-04-21T13:27:39Z

-        match val.layout.backend_repr {
-            BackendRepr::Scalar(scalar_layout) => {
-                if !scalar_layout.is_uninit_valid() {
-                    // There is something to check here.
-                    // We read directly via `ecx` since the read cannot fail -- we already read
-                    // this field above when recursing into the field.
-                    let scalar = self.ecx.read_scalar(val)?;
-                    self.visit_scalar(scalar, scalar_layout)?;
-                }
-            }
-            BackendRepr::ScalarPair(a_layout, b_layout) => {
-                // We can only proceed if *both* scalars need to be initialized.
-                // FIXME: find a way to also check ScalarPair when one side can be uninit but
-                // the other must be init.
-                if !a_layout.is_uninit_valid() && !b_layout.is_uninit_valid() {
-                    // We read directly via `ecx` since the read cannot fail -- we already read
-                    // this field above when recursing into the field.
-                    let (a, b) = self.ecx.read_immediate(val)?.to_scalar_pair();
-                    self.visit_scalar(a, a_layout)?;
-                    self.visit_scalar(b, b_layout)?;
-                }
-            }
-            BackendRepr::SimdVector { .. } | BackendRepr::SimdScalableVector { .. } => {
-                // No checks here, we assume layout computation gets this right.
-                // (This is harder to check since Miri does not represent these as `Immediate`. We
-                // also cannot use field projections since this might be a newtype around a vector.)
-            }
-            BackendRepr::Memory { .. } => {
-                // Nothing to do.
-            }
-        }


This crucially relies on the fact that layout computation will never make a type's layout more restrictive than a field. Is there some way we can debug-assert this to avoid soundness bugs in the future where we emit range information to LLVM that Miri never checks?

I readded it as an assertion for now, there's probably something we can do nicer

RalfJung · 2026-04-21T13:29:40Z

                    // we won't see optimizations actually breaking such programs.
                    ty::PatternKind::Or(_patterns) => {}
                }
+                match val.layout.backend_repr {


Suggested change

match val.layout.backend_repr {

// FIXME(pattern_types): Instead of checking the resulting layout, we should be checking

// the pattern directly.

match val.layout.backend_repr {

Fine for now but structurally it'd make a lot more sense to check the pattern rather than the layout (and then somehow have a sanity check that the layout does not add extra restrictions not covered by the pattern).

it does, but I haven't figured out how to do that without just repeating a lot of logic. Tho I guess we can just not look at whether is_uninit_valid is set, because if your pattern type is the full range of what the base type allows, that's so rare, that the small perf hit is fine to take

JonathanBrouwer · 2026-04-21T15:55:30Z

r? RalfJung
Since you seem to be reviewing this already, and I'm a bit busy

rustbot · 2026-04-21T15:55:35Z

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

RalfJung · 2026-04-21T16:27:13Z

Sorry, that won't work. I have too many other things on my plate already. I only looked at the const-eval part and left some comments related to that.

@rustbot reroll

ShoyuVanilla

The other changes look straightforward to me, though I'm not fully confident about the const-eval part. Overall this looks good, but would it make sense to get a second look from someone more familiar with this area?

View changes since this review

oli-obk · 2026-04-22T06:24:15Z

I'm not fully confident about the const-eval part. Overall this looks good, but would it make sense to get a second look from someone more familiar with this area?

Ralf already reviewed the const eval parts and I addressed those

RalfJung · 2026-04-22T06:27:41Z

Yeah I will take a look at the const-eval parts again to review the revision.

rustbot · 2026-04-22T12:26:32Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. labels Apr 17, 2026

rustbot assigned chenyukang Apr 17, 2026

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue. labels Apr 17, 2026

rustbot assigned JonathanBrouwer and unassigned chenyukang Apr 17, 2026

ChayimFriedman2 requested changes Apr 17, 2026

View reviewed changes

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2026

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 17, 2026

This comment has been minimized.

Sign in to view

oli-obk force-pushed the bye-bye-long-attribute branch from c6053aa to cc13e2c Compare April 17, 2026 19:20

This comment has been minimized.

Sign in to view

oli-obk force-pushed the bye-bye-long-attribute branch from cc13e2c to 663f77e Compare April 21, 2026 07:08

This comment has been minimized.

Sign in to view

oli-obk force-pushed the bye-bye-long-attribute branch from 663f77e to c01a244 Compare April 21, 2026 07:09

rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Apr 21, 2026

oli-obk force-pushed the bye-bye-long-attribute branch from c01a244 to 07e6d54 Compare April 21, 2026 07:18

This comment has been minimized.

Sign in to view

oli-obk force-pushed the bye-bye-long-attribute branch from 07e6d54 to cbd6780 Compare April 21, 2026 09:42

This comment has been minimized.

Sign in to view

oli-obk force-pushed the bye-bye-long-attribute branch from cbd6780 to 4fb465f Compare April 21, 2026 12:16

This comment has been minimized.

Sign in to view

RalfJung reviewed Apr 21, 2026

View reviewed changes

oli-obk force-pushed the bye-bye-long-attribute branch from 4fb465f to 28fa32d Compare April 21, 2026 15:28

rustbot assigned RalfJung and unassigned JonathanBrouwer Apr 21, 2026

rustbot assigned ShoyuVanilla and unassigned RalfJung Apr 21, 2026

ShoyuVanilla approved these changes Apr 22, 2026

View reviewed changes

Comment thread tests/ui/consts/const-eval/ub-ref-ptr.rs

tshepang reviewed Apr 22, 2026

View reviewed changes

Comment thread src/doc/rustc-dev-guide/src/unsafety-checking.md

This comment has been minimized.

Sign in to view

oli-obk and others added 6 commits April 22, 2026 14:23

Stop using rustc_scalar_layout attr in gvn test

f9c9efb

Stop using rustc_scalar_layout attr in jump threading test

6988b2e

Stop using rustc_scalar_layout in enum test

8b9a4f0

Remove mir-opt mention of rustc_scalar_layout attr

27a32ff

Remove rustc_layout_scalar_valid_range attr from debuginfo

190314b

Rip out rustc_layout_scalar_valid_range_* attribute support

e993ad9

oli-obk force-pushed the bye-bye-long-attribute branch from 28fa32d to e993ad9 Compare April 22, 2026 12:26

Uh oh!

Conversation

oli-obk commented Apr 17, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 17, 2026

Uh oh!

rustbot commented Apr 17, 2026

Uh oh!

JonathanBrouwer commented Apr 17, 2026

Uh oh!

ChayimFriedman2 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Apr 17, 2026

Uh oh!

This comment has been minimized.

mejrs commented Apr 17, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Apr 21, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

oli-obk Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

oli-obk Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

oli-obk Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer commented Apr 21, 2026

Uh oh!

rustbot commented Apr 21, 2026

Uh oh!

RalfJung commented Apr 21, 2026

Uh oh!

ShoyuVanilla left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk commented Apr 22, 2026

Uh oh!

RalfJung commented Apr 22, 2026

Uh oh!

Uh oh!

This comment has been minimized.

rustbot commented Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

oli-obk commented Apr 17, 2026 •

edited by rustbot

Loading

ChayimFriedman2 left a comment •

edited by rustbot

Loading

RalfJung left a comment •

edited by rustbot

Loading

ShoyuVanilla left a comment •

edited by rustbot

Loading