Lower index bounds checking to `PtrMetadata`, this time with the right fake borrow semantics 😸 #135748

compiler-errors · 2025-01-19T22:51:52Z

Change Rvalue::RawRef to take a RawRefKind instead of just a Mutability. Then introduce RawRefKind::FakeForPtrMetadata and use that for lowering index bounds checking to a PtrMetadata. This new RawRefKind::FakeForPtrMetadata acts like a shallow fake borrow in borrowck, which mimics the semantics of the old Rvalue::Len operation we're replacing.

We can then use this RawRefKind instead of using a span desugaring hack in CTFE.

cc @scottmcm @RalfJung

rustbot · 2025-01-19T22:52:01Z

r? @lcnr

rustbot has assigned @lcnr.
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-01-19T22:52:03Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

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

The Miri subtree was changed

cc @rust-lang/miri

scottmcm · 2025-01-19T23:14:36Z

compiler/rustc_mir_build/src/builder/expr/as_place.rs

+                // though. FIXME: Do we really *need* to count this as a use?
+                // Could partial array tracking work off something else instead?
+                self.cfg.push_fake_read(block, source_info, FakeReadCause::ForIndex, place);
+                let const_ = Const::from_ty_const(*len_const, usize_ty, self.tcx);


With things like #134371 and #134352, you might want something here to handle weirdness.

Not sure what the right way to do that is, but you could consider something like

Suggested change

let const_ = Const::from_ty_const(*len_const, usize_ty, self.tcx);

let const_ =

if let Some(err) = self.infcx.is_tainted_by_errors() {

Const::new_error(self.tcx, err)

} else {

Const::from_ty_const(*len_const, usize_ty, self.tcx)

};

(or whatever context is the right place to check for problems that we've already detected -- maybe self.tcx.dcx().has_errors() is better? No idea.)

Right, this inadvertently reapplies that bug I pointed out. I can fix that too.

Also, I don't really know what this has to do with error tainting?

Lowering this directly to a mir::Const::Ty causes ICEs even on code that's expected to pass, like when doing reads on a slice of &[(); T::ASSOC_CONST] when GCE is enabled (seetests/ui/const-generics/issues/issue-89146.rs) .

Anyways, this ICE is unrelated, and I'm going to fix it separately. I'm tracking that work in #135753. Either that one lands first, which will cause a conflict on this, or this one lands first and I'll fix the approach in this PR -- I don't think it needs to block this PR in any case.

The case that has to do with error tainting is when you get a len_const that's not actually a usize, like in the repro in #134352 where the len_const is actually a u64. There's already been an error about the type being wrong, but that doesn't keep the MIR from being very unhappy about a type mismatch after building returns a u64 constant for something that's supposed to be usize.

scottmcm

Thanks for putting this together, and so quickly too! Much appreciated.

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

…ake borrow

RalfJung · 2025-01-20T06:06:36Z

compiler/rustc_middle/src/mir/syntax.rs

+            // We have no type corresponding to a shallow borrow, so use
+            // `&` as an approximation.


This comment looks like it still needs updating.

RalfJung · 2025-01-20T06:06:46Z

compiler/rustc_middle/src/mir/syntax.rs

+pub enum RawPtrKind {
+    Mut,
+    Not,
+    FakeForPtrMetadata,


Please add a doc comment explaining this variant.

RalfJung · 2025-01-20T06:07:34Z

compiler/rustc_const_eval/src/interpret/step.rs

@@ -250,8 +254,13 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                let src = self.eval_place(place)?;
                let place = self.force_allocation(&src)?;
                let mut val = ImmTy::from_immediate(place.to_ref(self), dest.layout);
-                if !place_base_raw {
+                if !place_base_raw
+                    && span.desugaring_kind() != Some(DesugaringKind::IndexBoundsCheckReborrow)


Wasn't the point to get rid of this?

You are reviewing the wrong code

That is to say, you're reviewing the un-revert commit, not the functional changes on top.

compiler/rustc_span/src/hygiene.rs

rustbot assigned lcnr Jan 19, 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 Jan 19, 2025

This comment has been minimized.

Sign in to view

scottmcm reviewed Jan 19, 2025

View reviewed changes

compiler-errors added 2 commits January 20, 2025 04:20

Reapply "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-…

2f2593f

…ptrmetadata, r=davidtwco,RalfJung" This reverts commit 122a55b.

Represent the raw pointer for a array length check as a new kind of f…

a8e871e

…ake borrow

compiler-errors force-pushed the len-2 branch from 89c30c7 to a8e871e Compare January 20, 2025 04:21

RalfJung reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower index bounds checking to `PtrMetadata`, this time with the right fake borrow semantics 😸 #135748

Lower index bounds checking to `PtrMetadata`, this time with the right fake borrow semantics 😸 #135748

compiler-errors commented Jan 19, 2025

rustbot commented Jan 19, 2025

rustbot commented Jan 19, 2025

This comment has been minimized.

scottmcm Jan 19, 2025 •

edited

Loading

compiler-errors Jan 19, 2025

compiler-errors Jan 20, 2025

scottmcm Jan 20, 2025

scottmcm left a comment

RalfJung Jan 20, 2025

RalfJung Jan 20, 2025

RalfJung Jan 20, 2025

compiler-errors Jan 20, 2025

compiler-errors Jan 20, 2025

		// We have no type corresponding to a shallow borrow, so use
		// `&` as an approximation.

Lower index bounds checking to PtrMetadata, this time with the right fake borrow semantics 😸 #135748

Are you sure you want to change the base?

Lower index bounds checking to PtrMetadata, this time with the right fake borrow semantics 😸 #135748

Conversation

compiler-errors commented Jan 19, 2025

rustbot commented Jan 19, 2025

rustbot commented Jan 19, 2025

This comment has been minimized.

scottmcm Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lower index bounds checking to `PtrMetadata`, this time with the right fake borrow semantics 😸 #135748

Lower index bounds checking to `PtrMetadata`, this time with the right fake borrow semantics 😸 #135748

scottmcm Jan 19, 2025 •

edited

Loading