Prevent copies for unsized fn params

[JakobDegen]

Fixes #111175 .

Regression test added too.

r? rust-lang/mir-opt or @RalfJung

[tmiasko]

The place could be modified in between the move and the call. For example:

#![feature(unsized_fn_params)]
fn f(x: [i32], _: ()) {
    assert_eq!(&x, [1]);
}
fn main() {
    let mut x: Box<[i32]> = Box::new([1]);
    f(*x, {x = Box::new([2]) as Box<[i32]>; let _ = x;});
}

And a slightly different variant:

#![feature(unsized_fn_params)]
fn f(x: [i32], _: ()) {
    assert_eq!([1], x);
}
fn main() {
    let mut x: Box<[i32]> = Box::new([1]);
    f(*x, x[0] = 2);
}

[JakobDegen]

Hmm, I think the relevant example is this one:

#![feature(unsized_fn_params)]

fn uns(x: [i32], _: ()) {}

fn foo(mut x: [i32]) {
    uns(x, { x[0] = 1; () });
}

As far as I can tell there is no Mir that we can generate for foo that both correctly rejects that code in borrowck and doesn't have an unsized local.

I'm tempted that we should still merge this, with an appropriate test and comment added (will do that tomorrow). The feature is already unsound, and removing this bug is a pre-requisite to unimplementing unsized locals (which I do think we should do)

[RalfJung]

The feature is already unsound

You mean unsized_fn_params? Having actual semantics-changing MIR transforms strikes me as a separate kind of unsoundness from codegen being wrong.

[JakobDegen]

You mean unsized_fn_params? Having actual semantics-changing MIR transforms strikes me as a separate kind of unsoundness from codegen being wrong.

Yes. This isn't a Mir transform though - the "bug" in this PR is that the above example will incorrectly compile and uns will see the result of the mutation in the evaluation of the second param. That's a bug in Mir building though, and as I noted, fixing that bug either implies unsized locals or major refactors to Mir.

[tmiasko]

Can you describe what is the end goal here?

For the standard library, unsized locals aren't necessary except for the currently unstable forget_unsized, so one could reject them altogether. Though, that doesn't on its own solve the soundness issue, since it is also necessary to ensure that no transformation introduces unsized temporaries. Inlining for example does introduce them.

It might be easier to fix the alignment issue.

[JakobDegen]

@tmiasko I'll submit an MCP tomorrow for removing unsized locals from rustc completely (and will explain the motivation for that in there). Under the assumption that that's desirable, I feel quite strongly that we should not block that choice on this bug. The fact that unsized_fn_params cannot be implemented correctly in Mir without reliance on an extremely broken feature is unfortunate. Blocking the removal of the broken feature on that situation is imo not a reasonable response though.

[RalfJung]

For the standard library, unsized locals aren't necessary except for the currently unstable forget_unsized

They aren't necessary at all, std only uses the unsized_fn_params feature, not unsized_locals.

The end goal is to have unsized_fn_params never generate an alloca. That was the goal of that feature gate from the start. It just turns out it failed to achieve that.

Yes. This isn't a Mir transform though - the "bug" in this PR is that the above example will incorrectly compile and uns will see the result of the mutation in the evaluation of the second param. That's a bug in Mir building though, and as I noted, fixing that bug either implies unsized locals or major refactors to Mir.

That's a bug in MIR building after this PR, but done correctly (well, using unsized locals) before this PR?

[JakobDegen]

Yes, although I'd slightly object to the use of "correctly" and "unsized locals" in the same sentence :P

[tmiasko]

They aren't necessary at all, std only uses the unsized_fn_params feature, not unsized_locals.

The end goal is to have unsized_fn_params never generate an alloca. That was the goal of that feature gate from the start. It just turns out it failed to achieve that.

As of now, the forget_unsized does actually use unsized locals, but it is the only thing to do so (I checked with borrowck modified to always reject unsized temporaries). With forget_unsized gone, you can remove unsized locals or fix the feature gate without introducing new evaluation order bugs.

[tmiasko]

Note that this was previously moving the box, avoiding problems with evaluation order and unsized temporaries.

[RalfJung]

As of now, the forget_unsized does actually use unsized locals, but it is the only thing to do so (I checked with borrowck modified to always reject unsized temporaries). With forget_unsized gone, you can remove unsized locals or fix the feature gate without introducing new evaluation order bugs.

So basically you are saying, unsized_fn_params should not accept the pattern of forwarding an unsized argument to another function? I guess that would be an option, but we'd have to crater it to evaluate whether other unsized_fn_params users need this.

Also removing forget_unsized is not an option. But we could replace the wrapper function by a re-export of the intrinsic. That would regress taking a fn ptr to it, but would probably be enough to do a crater run.

[JakobDegen]

I've updated the PR - it should now not regress any cases that previously did not require unsized locals to work

[tmiasko]

In general, I would suggest fixing unsized_fn_params feature gate instead, so that it covers part that can be implemented correctly without unsized locals. Especially if the eventual goal is to remove the unsized locals. This would ensure that standard library doesn't rely on broken / incorrect features.

[JakobDegen]

@tmiasko that seems like a reasonable thing to want - is there a reason it has to happen as a part of this PR? I don't really interpret this PR as making anything worse than the status quo

[tmiasko]

What is the motivation behind this change?

If you argue that correct MIR building in this situation requires "unsized locals or major refactors", why de facto propose to accept this code under unsized_fn_params feature gate instead of rejecting it? It seems like this approach only creates risk that you end up with a feature that cannot be implemented correctly, especially that what you propose to do next is to remove unsized locals.

[JakobDegen]

The motivation for this change is to unblock the removal of unsized locals. We could also just not land this and then remove unsized locals anyway, in which case this code will ICE. That's fine by me too, maybe it's preferable.

Mostly what I'm trying to avoid is us getting into a situation where we can't remove unsized locals until someone sits down and does the work of fixing this properly

[tmiasko]

As I indicated earlier this isn't sufficient to avoid unsized locals, see for example #111355.

I can take a look into updating the feature gate.

[JakobDegen]

Sure; I'll close this then

[RalfJung]

As I indicated earlier this isn't sufficient to avoid unsized locals, see for example #111355.

That issue is closed now, but #111175 is still open. Do we need to revive this PR?