miri: require (almost) all 1-ZST arguments to be actually passed

[RalfJung]

View all comments

We can't ignore all of them since the compiler itself relies on non-capturing closure arguments being ignored.

Fixes rust-lang/miri#4993

Cc @folkertdev since it also changes the checks for variadics.

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @oli-obk, @lcnr

[rustbot]

r? @nikomatsakis

rustbot has assigned @nikomatsakis.
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, mir
• compiler, mir expanded to 73 candidates
• Random selection from 21 candidates

[saethlin]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[rust-bors]

⌛ Trying commit 403e9b0 with merge 583ebe0…

To cancel the try build, run the command @bors try cancel.

Workflow: https://github.com/rust-lang/rust/actions/runs/25256422338

[saethlin]

I feel like this should mention that normally ZSTs are ignored. But maybe you disagree and think that's base knowledge for Rust argument passing, which would be fine with me.

The connection to closure coercion is turning my brain into a pretzel. Does this mean that we still ignore the argument in this?

fn func<T>(_: T) {}

func(|| {});

View changes since the review

[RalfJung]

I have extended the comment, does that help?

The connection to closure coercion is turning my brain into a pretzel. Does this mean that we still ignore the argument in this?

Yes. That's unfortunate, ideally we'd still report UB if that argument gets ignored. However, I'm not sure it's worth trying to restrict this. It seems non-trivial to figure out whether the callee is actually a closure. But maybe you have a good idea for how to do that?

[saethlin]

Conceptually, I think of the closure captures being implicitly passed to closure calls, not literally self. But of course the FnOnce trait takes self so I suspect trying to implement my mental model would be really fiddly, if even possible.

[RalfJung]

In MIR closures are just normal functions -- closure conversion already happened at that point. So we can't really treat them as anything else.

The best we could do is check the DefId of the callee and figure out if it is a closure, and then treat the first parameter different.

[saethlin]

The new explanation is much easier for me to follow. Thanks.

r? saethlin
@bors r+

[rust-bors]

📌 Commit 3427f02 has been approved by saethlin

It is now in the queue for this repository.

[saethlin]

@bors try-

[rust-bors]

Unknown command "try-". Run @bors help to see available commands.

[saethlin]

@bors try cancel

[rust-bors]

Try build cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/25256422338

[rust-bors]

⚠️ A new commit ec802b2e72a59eda62cefbb9c0de81695a860de8 was pushed.

This pull request was unapproved.

[theemathas]

Miri reports no UB for this code with this PR. Is this intended?

use std::mem::transmute;

fn main() {
    let f: fn(i32) = |x: i32| {
        println!("{x}");
    };
    let g: fn((), i32) = unsafe { transmute(f) };
    g((), 123);
}

I think Miri is treating the () argument as the closure argument.

[RalfJung]

It is less intended and more unavoidable.

Miri also doesn't report UB for code that incorrectly relies on repr(Rust) field order. So I would categorize that code as relying on unstable undocumented ABI guarantees, but the code will actually compile and run correctly on today's compilers so I can live with Miri not reporting an error.

[RalfJung]

@saethlin what do you think about the latest version of this?

[RalfJung]

@theemathas thinking about your example again I think it is in fact covered by our documented ABI guarantees since we say that all 1-ZST are ABI compatible, with both () and the closure environment type are 1-ZST. It just kind of "leaks" the fact that there is actually an argument there that implicitly gets removed by the closure-to-fn-ptr coercion.

[saethlin]

@saethlin what do you think about the latest version of this?

I wish our semantics were easier to implement.

@bors r+ rollup=iffy

[rust-bors]

📌 Commit 5d2de1b has been approved by saethlin

It is now in the queue for this repository.

[RalfJung]

I wish our semantics were easier to implement.

💯

We could avoid this complication by having the closure-to-fn-ptr coercion insert a shim that does the ABI adjustment. But that's outside the parts of the compiler I am comfortable in...

[jhpratt]

@bors r-

#157225 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#157225), which was unapproved.

View changes since this unapproval

[rustbot]

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.

[rust-bors]

⚠️ A new commit a96cc82f159d32a00e6169074689d2a0f126a206 was pushed.

This PR was contained in a rollup (#157225), which was closed.

[RalfJung]

@bors r=saethlin

[rust-bors]

📌 Commit a96cc82 has been approved by saethlin

It is now in the queue for this repository.