Skip to content

Commit

Permalink
Rollup merge of #95953 - JakobDegen:repeat-leak, r=oli-obk
Browse files Browse the repository at this point in the history
Modify MIR building to drop repeat expressions with length zero

Closes #74836 .

Previously, when a user wrote `[foo; 0]` we used to simply leak `foo`. The goal is to fix that. This PR changes MIR building to make `[foo; 0]` equivalent to `{ drop(foo); [] }` in all cases. Of course, this is a breaking change (see below). A crater run did not indicate any regressions though, and given that the previous behavior was almost definitely not what any user wanted, it seems unlikely that anyone was relying on this.

Note that const generics are in general unaffected by this. Inserting the extra `drop` is only meaningful/necessary when `foo` is of a non-`Copy` type, and array repeat expressions with const generic repetition count must always be `Copy`.

Besides the obvious change to behavior associated with the additional drop, there are three categories of examples where this also changes observable behavior. In all of these cases, the new behavior is consistent with what you would get by replacing `[foo; 0]` with `{ drop(foo); [] }`. As such, none of these give the user new powers to express more things.

**No longer allowed in const (breaking)**:

```rust
const _: [String; 0] = [String::new(); 0];
```

This compiles on stable today. Because we now introduce the drop of `String`, this no longer compiles as `String` may not be dropped in a const context.

**Reduced dataflow (non-breaking)**:

```rust
let mut x: i32 = 0;
let r = &x;
let a = [r; 0];
x = 5;
let _b = a;
```

Borrowck rejects this code on stable because it believes there is dataflow between `a` and `r`, and so the lifetime of `r` has to extend to the last statement. This change removes the dataflow and the above code is allowed to compile.

**More const promotion (non-breaking)**:

```rust
let _v: &'static [String; 0] = &[String::new(); 0];
```

This does not compile today because `String` having drop glue keeps it from being const promoted (despite that drop glue never being executed). After this change, this is allowed to compile.

### Alternatives

A previous attempt at this tried to reduce breakage by various tricks. This is still a possibility, but given that crater showed no regressions it seems unclear why we would want to introduce this complexity.

Disallowing `[foo; 0]` completely is also an option, but obviously this is more of a breaking change. I do not know how often this is actually used though.

r? `@oli-obk`
  • Loading branch information
Dylan-DPC authored May 25, 2022
2 parents cbdce42 + 0f65bcd commit 11faf2e
Show file tree
Hide file tree
Showing 5 changed files with 210 additions and 5 deletions.
50 changes: 45 additions & 5 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
})
}
ExprKind::Repeat { value, count } => {
let value_operand = unpack!(
block =
this.as_operand(block, scope, &this.thir[value], None, NeedsTemporary::No)
);
block.and(Rvalue::Repeat(value_operand, count))
if Some(0) == count.try_eval_usize(this.tcx, this.param_env) {
this.build_zero_repeat(block, value, scope, source_info)
} else {
let value_operand = unpack!(
block = this.as_operand(
block,
scope,
&this.thir[value],
None,
NeedsTemporary::No
)
);
block.and(Rvalue::Repeat(value_operand, count))
}
}
ExprKind::Binary { op, lhs, rhs } => {
let lhs = unpack!(
Expand Down Expand Up @@ -516,6 +525,37 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

fn build_zero_repeat(
&mut self,
mut block: BasicBlock,
value: ExprId,
scope: Option<region::Scope>,
outer_source_info: SourceInfo,
) -> BlockAnd<Rvalue<'tcx>> {
let this = self;
let value = &this.thir[value];
let elem_ty = value.ty;
if let Some(Category::Constant) = Category::of(&value.kind) {
// Repeating a const does nothing
} else {
// For a non-const, we may need to generate an appropriate `Drop`
let value_operand =
unpack!(block = this.as_operand(block, scope, value, None, NeedsTemporary::No));
if let Operand::Move(to_drop) = value_operand {
let success = this.cfg.start_new_block();
this.cfg.terminate(
block,
outer_source_info,
TerminatorKind::Drop { place: to_drop, target: success, unwind: None },
);
this.diverge_from(block);
block = success;
}
this.record_operands_moved(&[value_operand]);
}
block.and(Rvalue::Aggregate(Box::new(AggregateKind::Array(elem_ty)), Vec::new()))
}

fn limit_capture_mutability(
&mut self,
upvar_span: Span,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir_build/src/build/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.cfg.block_data(start).terminator().kind,
TerminatorKind::Assert { .. }
| TerminatorKind::Call { .. }
| TerminatorKind::Drop { .. }
| TerminatorKind::DropAndReplace { .. }
| TerminatorKind::FalseUnwind { .. }
| TerminatorKind::InlineAsm { .. }
Expand Down
15 changes: 15 additions & 0 deletions src/test/ui/drop/repeat-drop-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
fn borrowck_catch() {
let foo = String::new();
let _bar = foo;
let _baz = [foo; 0]; //~ ERROR use of moved value: `foo` [E0382]
}

const _: [String; 0] = [String::new(); 0];
//~^ ERROR destructors cannot be evaluated at compile-time [E0493]

fn must_be_init() {
let x: u8;
let _ = [x; 0]; //~ ERROR: use of possibly-uninitialized variable: `x`
}

fn main() {}
29 changes: 29 additions & 0 deletions src/test/ui/drop/repeat-drop-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
error[E0382]: use of moved value: `foo`
--> $DIR/repeat-drop-2.rs:4:17
|
LL | let foo = String::new();
| --- move occurs because `foo` has type `String`, which does not implement the `Copy` trait
LL | let _bar = foo;
| --- value moved here
LL | let _baz = [foo; 0];
| ^^^ value used here after move

error[E0493]: destructors cannot be evaluated at compile-time
--> $DIR/repeat-drop-2.rs:7:25
|
LL | const _: [String; 0] = [String::new(); 0];
| -^^^^^^^^^^^^^----
| ||
| |constants cannot evaluate destructors
| value is dropped here

error[E0381]: use of possibly-uninitialized variable: `x`
--> $DIR/repeat-drop-2.rs:12:14
|
LL | let _ = [x; 0];
| ^ use of possibly-uninitialized `x`

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0381, E0382, E0493.
For more information about an error, try `rustc --explain E0381`.
120 changes: 120 additions & 0 deletions src/test/ui/drop/repeat-drop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
// run-pass
// ignore-wasm32-bare no unwinding panic
// ignore-avr no unwinding panic
// ignore-nvptx64 no unwinding panic

static mut CHECK: usize = 0;

struct DropChecker(usize);

impl Drop for DropChecker {
fn drop(&mut self) {
unsafe {
if CHECK != self.0 - 1 {
panic!("Found {}, should have found {}", CHECK, self.0 - 1);
}
CHECK = self.0;
}
}
}

macro_rules! check_drops {
($l:literal) => {
unsafe { assert_eq!(CHECK, $l) }
};
}

struct DropPanic;

impl Drop for DropPanic {
fn drop(&mut self) {
panic!()
}
}

fn value_zero() {
unsafe { CHECK = 0 };
let foo = DropChecker(1);
let v: [DropChecker; 0] = [foo; 0];
check_drops!(1);
std::mem::drop(v);
check_drops!(1);
}

fn value_one() {
unsafe { CHECK = 0 };
let foo = DropChecker(1);
let v: [DropChecker; 1] = [foo; 1];
check_drops!(0);
std::mem::drop(v);
check_drops!(1);
}

const DROP_CHECKER: DropChecker = DropChecker(1);

fn const_zero() {
unsafe { CHECK = 0 };
let v: [DropChecker; 0] = [DROP_CHECKER; 0];
check_drops!(0);
std::mem::drop(v);
check_drops!(0);
}

fn const_one() {
unsafe { CHECK = 0 };
let v: [DropChecker; 1] = [DROP_CHECKER; 1];
check_drops!(0);
std::mem::drop(v);
check_drops!(1);
}

fn const_generic_zero<const N: usize>() {
unsafe { CHECK = 0 };
let v: [DropChecker; N] = [DROP_CHECKER; N];
check_drops!(0);
std::mem::drop(v);
check_drops!(0);
}

fn const_generic_one<const N: usize>() {
unsafe { CHECK = 0 };
let v: [DropChecker; N] = [DROP_CHECKER; N];
check_drops!(0);
std::mem::drop(v);
check_drops!(1);
}

// Make sure that things are allowed to promote as expected

fn allow_promote() {
unsafe { CHECK = 0 };
let foo = DropChecker(1);
let v: &'static [DropChecker; 0] = &[foo; 0];
check_drops!(1);
std::mem::drop(v);
check_drops!(1);
}

// Verify that unwinding in the drop causes the right things to drop in the right order
fn on_unwind() {
unsafe { CHECK = 0 };
std::panic::catch_unwind(|| {
let panic = DropPanic;
let _local = DropChecker(2);
let _v = (DropChecker(1), [panic; 0]);
std::process::abort();
})
.unwrap_err();
check_drops!(2);
}

fn main() {
value_zero();
value_one();
const_zero();
const_one();
const_generic_zero::<0>();
const_generic_one::<1>();
allow_promote();
on_unwind();
}

0 comments on commit 11faf2e

Please sign in to comment.