Update `InterpCx::project_field` to take `FieldIdx` #142103

scottmcm · 2025-06-06T02:02:35Z

As suggested by Ralf in #142005 (comment)

rustbot · 2025-06-06T02:02:40Z

rustbot has assigned @oli-obk.
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

rustbot · 2025-06-06T02:02:42Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

As suggested by Ralf in 142005.

oli-obk

This PR highlights several situations where we can do improvements, but let's land this on its own and investigate from_usize and as_usize calls later

oli-obk · 2025-06-06T08:00:45Z

compiler/rustc_abi/src/layout/ty.rs

@@ -288,7 +295,7 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
                // More than one non-1-ZST field.
                return None;
            }
-            found = Some((field_idx, field));
+            found = Some((FieldIdx::from_usize(field_idx), field));


it may be worth adding an iter and an iter_enumerated method to FieldsShape

Yeah, most of the layout-related usizes in ABI are sketchy. I've tried pulling on the tendrils before, but usually ended up giving up because of just how far that goes and -- as I mentioned in the other PR -- that the ones related to arrays sometimes need more than FieldIdx, and figuring out which those are can be hard.

(Conveniently this one was obvious because a over-4-billion-length array clearly can't have exactly one non-ZST.)

oli-obk · 2025-06-06T08:06:59Z

@bors r+ rollup

bors · 2025-06-06T08:07:01Z

📌 Commit 8bce225 has been approved by oli-obk

It is now in the queue for this repository.

compiler/rustc_abi/src/layout/ty.rs

RalfJung · 2025-06-06T09:18:48Z

compiler/rustc_abi/src/layout/ty.rs

+impl FieldIdx {
+    /// The second field.
+    ///
+    /// For use alongside [`FieldIdx::ZERO`], particularly with scalar pairs.


Why is this not defined near FieldIdx::ZERO?

Because all rustc_newtype_index! types get a ZERO; it's not something special for FieldIdx.

(See for example how there's both https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.BasicBlock.html#associatedconstant.ZERO and https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/constant.START_BLOCK.html.)

RalfJung · 2025-06-06T09:19:24Z

compiler/rustc_const_eval/src/const_eval/mod.rs

@@ -60,7 +60,7 @@ pub(crate) fn try_destructure_mir_constant_for_user_output<'tcx>(

    let fields_iter = (0..field_count)
        .map(|i| {
-            let field_op = ecx.project_field(&down, i).discard_err()?;
+            let field_op = ecx.project_field(&down, FieldIdx::from_usize(i)).discard_err()?;


This could also use a typed field iterator

RalfJung · 2025-06-06T09:19:59Z

compiler/rustc_const_eval/src/const_eval/valtrees.rs

@@ -40,7 +40,7 @@ fn branches<'tcx>(
    }

    for i in 0..field_count {
-        let field = ecx.project_field(&place, i).unwrap();
+        let field = ecx.project_field(&place, FieldIdx::from_usize(i)).unwrap();


And more here

(probably not worth pointing them all out)

RalfJung · 2025-06-06T09:20:51Z

compiler/rustc_const_eval/src/interpret/projection.rs

        // Computing the layout does normalization, so we get a normalized type out of this
        // even if the field type is non-normalized (possible e.g. via associated types).
-        let field_layout = base.layout().field(self, field);
+        let field_layout = base.layout().field(self, field.as_usize());


Why does field not take FieldIdx...?

It's https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.TyAndLayout.html#method.field, I think. And that's like I mentioned in #142005 (comment) something that takes usize today, because it works on arrays https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_middle/ty/layout.rs.html#795-998.

Co-authored-by: Ralf Jung <[email protected]>

scottmcm · 2025-06-06T09:31:00Z

@bors r=oli-obk

bors · 2025-06-06T09:31:02Z

📌 Commit f9cf096 has been approved by oli-obk

It is now in the queue for this repository.

RalfJung · 2025-06-06T10:28:08Z

Yeah those should be separate methods for fields and array elements. Is this entire project tracked somewhere? (Also re: field iterators)

scottmcm · 2025-06-06T17:44:35Z

I think "project" overstates at least how I've been handling this. I had rust-lang/compiler-team#606 to add FieldIdx originally, then @WaffleLapkin's rust-lang/compiler-team#639 is explicitly about using it in more places.

There's never been a tracking issue for it, or a list of all the things that should happen, or anything. Just a bunch of "spots I saw in passing" PRs.

…-obk Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang#142005 (comment)

rustbot assigned oli-obk Jun 6, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 6, 2025

This comment has been minimized.

Sign in to view

Update InterpCx::project_field to take FieldIdx

8bce225

As suggested by Ralf in 142005.

scottmcm force-pushed the fieldidx-in-interp branch from 74c269e to 8bce225 Compare June 6, 2025 02:16

oli-obk approved these changes Jun 6, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2025

RalfJung reviewed Jun 6, 2025

View reviewed changes

compiler/rustc_abi/src/layout/ty.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jun 6, 2025

View reviewed changes

Update compiler/rustc_abi/src/layout/ty.rs

f9cf096

Co-authored-by: Ralf Jung <[email protected]>

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 6, 2025

Rollup merge of rust-lang#142103 - scottmcm:fieldidx-in-interp, r=oli…

3a6f1b0

…-obk Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang#142005 (comment)

GuillaumeGomez mentioned this pull request Jun 6, 2025

Rollup of 11 pull requests #142133

Open

Update InterpCx::project_field to take FieldIdx #142103

Are you sure you want to change the base?

Update InterpCx::project_field to take FieldIdx #142103

Conversation

scottmcm commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 6, 2025

Uh oh!

rustbot commented Jun 6, 2025

Uh oh!

This comment has been minimized.

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jun 6, 2025

Uh oh!

bors commented Jun 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm commented Jun 6, 2025

Uh oh!

bors commented Jun 6, 2025

Uh oh!

RalfJung commented Jun 6, 2025 via email

Uh oh!

scottmcm commented Jun 6, 2025

Uh oh!

Uh oh!

Update `InterpCx::project_field` to take `FieldIdx` #142103

Update `InterpCx::project_field` to take `FieldIdx` #142103

scottmcm commented Jun 6, 2025 •

edited

Loading