From 18fcb3f591b9a0bac969ace7727e153f61236305 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Sun, 7 May 2023 19:25:45 -0700 Subject: [PATCH] Prevent copies for unsized fn params --- .../src/build/expr/as_operand.rs | 7 ++- ...zed_arg_forwarding.forward.built.after.mir | 47 +++++++++++++++++++ .../building/unsized_arg_forwarding.rs | 19 ++++++++ .../unsized-locals/borrow-after-move.stderr | 26 ++++------ tests/ui/unsized-locals/double-move.stderr | 24 ++-------- 5 files changed, 84 insertions(+), 39 deletions(-) create mode 100644 tests/mir-opt/building/unsized_arg_forwarding.forward.built.after.mir create mode 100644 tests/mir-opt/building/unsized_arg_forwarding.rs diff --git a/compiler/rustc_mir_build/src/build/expr/as_operand.rs b/compiler/rustc_mir_build/src/build/expr/as_operand.rs index 6941da331fc58..25237d9b62844 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_operand.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_operand.rs @@ -163,8 +163,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // !sized means !copy, so this is an unsized move assert!(!ty.is_copy_modulo_regions(tcx, param_env)); - // As described above, detect the case where we are passing a value of unsized - // type, and that value is coming from the deref of a box. if let ExprKind::Deref { arg } = expr.kind { // Generate let tmp0 = arg0 let operand = unpack!( @@ -177,6 +175,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { projection: tcx.mk_place_elems(&[PlaceElem::Deref]), }; + return block.and(Operand::Move(place)); + } else { + // FIXME: This does not prevent the place from being modified before being used, + // see `tests/mir-opt/unsized_arg_forwarding.rs`. + let place = unpack!(block = this.as_place(block, expr)); return block.and(Operand::Move(place)); } } diff --git a/tests/mir-opt/building/unsized_arg_forwarding.forward.built.after.mir b/tests/mir-opt/building/unsized_arg_forwarding.forward.built.after.mir new file mode 100644 index 0000000000000..52b548e1d1c84 --- /dev/null +++ b/tests/mir-opt/building/unsized_arg_forwarding.forward.built.after.mir @@ -0,0 +1,47 @@ +// MIR for `forward` after built + +fn forward(_1: [usize], _2: usize) -> () { + debug x => _1; // in scope 0 at $DIR/unsized_arg_forwarding.rs:+0:12: +0:17 + debug i => _2; // in scope 0 at $DIR/unsized_arg_forwarding.rs:+0:28: +0:29 + let mut _0: (); // return place in scope 0 at $DIR/unsized_arg_forwarding.rs:+0:38: +0:38 + let _3: (); // in scope 0 at $DIR/unsized_arg_forwarding.rs:+4:5: +7:7 + let mut _4: (); // in scope 0 at $DIR/unsized_arg_forwarding.rs:+4:12: +7:6 + let mut _5: usize; // in scope 0 at $DIR/unsized_arg_forwarding.rs:+5:16: +5:17 + let _6: usize; // in scope 0 at $DIR/unsized_arg_forwarding.rs:+5:11: +5:12 + let mut _7: usize; // in scope 0 at $DIR/unsized_arg_forwarding.rs:+5:9: +5:13 + let mut _8: bool; // in scope 0 at $DIR/unsized_arg_forwarding.rs:+5:9: +5:13 + + bb0: { + StorageLive(_3); // scope 0 at $DIR/unsized_arg_forwarding.rs:+4:5: +7:7 + StorageLive(_4); // scope 0 at $DIR/unsized_arg_forwarding.rs:+4:12: +7:6 + StorageLive(_5); // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:16: +5:17 + _5 = _2; // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:16: +5:17 + StorageLive(_6); // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:11: +5:12 + _6 = _2; // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:11: +5:12 + _7 = Len(_1); // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:9: +5:13 + _8 = Lt(_6, _7); // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:9: +5:13 + assert(move _8, "index out of bounds: the length is {} but the index is {}", move _7, _6) -> [success: bb1, unwind: bb3]; // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:9: +5:13 + } + + bb1: { + _1[_6] = move _5; // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:9: +5:17 + StorageDead(_5); // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:16: +5:17 + StorageDead(_6); // scope 0 at $DIR/unsized_arg_forwarding.rs:+5:17: +5:18 + _4 = (); // scope 0 at $DIR/unsized_arg_forwarding.rs:+6:9: +6:11 + _3 = nop(move _1, move _4) -> [return: bb2, unwind: bb3]; // scope 0 at $DIR/unsized_arg_forwarding.rs:+4:5: +7:7 + // mir::Constant + // + span: $DIR/unsized_arg_forwarding.rs:10:5: 10:8 + // + literal: Const { ty: fn([usize], ()) {nop}, val: Value() } + } + + bb2: { + StorageDead(_4); // scope 0 at $DIR/unsized_arg_forwarding.rs:+7:6: +7:7 + StorageDead(_3); // scope 0 at $DIR/unsized_arg_forwarding.rs:+7:7: +7:8 + _0 = const (); // scope 0 at $DIR/unsized_arg_forwarding.rs:+0:38: +8:2 + return; // scope 0 at $DIR/unsized_arg_forwarding.rs:+8:2: +8:2 + } + + bb3 (cleanup): { + resume; // scope 0 at $DIR/unsized_arg_forwarding.rs:+0:1: +8:2 + } +} diff --git a/tests/mir-opt/building/unsized_arg_forwarding.rs b/tests/mir-opt/building/unsized_arg_forwarding.rs new file mode 100644 index 0000000000000..4a711b0a5a2fe --- /dev/null +++ b/tests/mir-opt/building/unsized_arg_forwarding.rs @@ -0,0 +1,19 @@ +#![feature(unsized_fn_params)] + +fn nop(_: [usize], _: ()) {} + +// EMIT_MIR unsized_arg_forwarding.forward.built.after.mir +fn forward(mut x: [usize], i: usize) { + // Check that we do not copy `x` into a temporary. Unfortunately this means that the write to + // `x[0]` does not result in a borrowck error. There is currently no way to generate Mir that + // does not exhibit this bug. + nop(x, { + x[i] = i; + () + }); +} + +fn main() { + let x: Box<[usize]> = Box::new([1]); + forward(*x, 0); +} diff --git a/tests/ui/unsized-locals/borrow-after-move.stderr b/tests/ui/unsized-locals/borrow-after-move.stderr index 9e3c345dd8001..5c62333164b2b 100644 --- a/tests/ui/unsized-locals/borrow-after-move.stderr +++ b/tests/ui/unsized-locals/borrow-after-move.stderr @@ -24,28 +24,24 @@ error[E0382]: borrow of moved value: `y` LL | let y = *x; | - move occurs because `y` has type `str`, which does not implement the `Copy` trait LL | drop_unsized(y); - | - value moved here + | --------------- value moved here ... LL | println!("{}", &y); | ^^ value borrowed here after move - | -note: consider changing this parameter type in function `drop_unsized` to borrow instead if owning the value isn't necessary - --> $DIR/borrow-after-move.rs:14:31 - | -LL | fn drop_unsized(_: T) {} - | ------------ ^ this parameter takes ownership of the value - | | - | in this function error[E0382]: borrow of moved value: `x` --> $DIR/borrow-after-move.rs:31:24 | -LL | let y = *x; - | -- value moved here LL | y.foo(); + | ----- `*x` moved due to this method call LL | println!("{}", &x); | ^^ value borrowed here after move | +note: `Foo::foo` takes ownership of the receiver `self`, which moves `*x` + --> $DIR/borrow-after-move.rs:5:12 + | +LL | fn foo(self) -> String; + | ^^^^ = note: move occurs because `*x` has type `str`, which does not implement the `Copy` trait error[E0382]: borrow of moved value: `y` @@ -54,16 +50,10 @@ error[E0382]: borrow of moved value: `y` LL | let y = *x; | - move occurs because `y` has type `str`, which does not implement the `Copy` trait LL | y.foo(); - | ----- `y` moved due to this method call + | ------- value moved here ... LL | println!("{}", &y); | ^^ value borrowed here after move - | -note: `Foo::foo` takes ownership of the receiver `self`, which moves `y` - --> $DIR/borrow-after-move.rs:5:12 - | -LL | fn foo(self) -> String; - | ^^^^ error[E0382]: borrow of moved value: `x` --> $DIR/borrow-after-move.rs:40:24 diff --git a/tests/ui/unsized-locals/double-move.stderr b/tests/ui/unsized-locals/double-move.stderr index 49b906bbe02b7..e9a05198c242f 100644 --- a/tests/ui/unsized-locals/double-move.stderr +++ b/tests/ui/unsized-locals/double-move.stderr @@ -8,22 +8,14 @@ LL | #![feature(unsized_locals, unsized_fn_params)] = note: `#[warn(incomplete_features)]` on by default error[E0382]: use of moved value: `y` - --> $DIR/double-move.rs:21:22 + --> $DIR/double-move.rs:21:9 | LL | let y = *x; | - move occurs because `y` has type `str`, which does not implement the `Copy` trait LL | drop_unsized(y); - | - value moved here + | --------------- value moved here LL | drop_unsized(y); - | ^ value used here after move - | -note: consider changing this parameter type in function `drop_unsized` to borrow instead if owning the value isn't necessary - --> $DIR/double-move.rs:14:31 - | -LL | fn drop_unsized(_: T) {} - | ------------ ^ this parameter takes ownership of the value - | | - | in this function + | ^^^^^^^^^^^^^^^ value used here after move error[E0382]: use of moved value: `x` --> $DIR/double-move.rs:27:22 @@ -51,15 +43,9 @@ error[E0382]: use of moved value: `y` LL | let y = *x; | - move occurs because `y` has type `str`, which does not implement the `Copy` trait LL | y.foo(); - | ----- `y` moved due to this method call + | ------- value moved here LL | y.foo(); - | ^ value used here after move - | -note: `Foo::foo` takes ownership of the receiver `self`, which moves `y` - --> $DIR/double-move.rs:5:12 - | -LL | fn foo(self) -> String; - | ^^^^ + | ^^^^^^^ value used here after move error[E0382]: use of moved value: `x` --> $DIR/double-move.rs:46:9