Skip to content

Commit

Permalink
Drop function parameters in expected order
Browse files Browse the repository at this point in the history
Given the function

fn foo((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {}

Prior to 1.12 we dropped both `_x` and `_y` before the rest of their
respective parameters, since then we dropped `_x` and `_y` after. The
original order appears to be the correct order, as the value created
later is dropped first, so we revert to that order and add a test for
it.
  • Loading branch information
matthewjasper committed Nov 30, 2018
1 parent 3dde9e1 commit 914515f
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 7 deletions.
14 changes: 7 additions & 7 deletions src/librustc_mir/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,13 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
let place = Place::Local(local);
let &ArgInfo(ty, opt_ty_info, pattern, ref self_binding) = arg_info;

// Make sure we drop (parts of) the argument even when not matched on.
self.schedule_drop(
pattern.as_ref().map_or(ast_body.span, |pat| pat.span),
argument_scope, &place, ty,
DropKind::Value { cached_block: CachedBlock::default() },
);

if let Some(pattern) = pattern {
let pattern = self.hir.pattern_from_hir(pattern);
let span = pattern.span;
Expand Down Expand Up @@ -946,13 +953,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
}
}
}

// Make sure we drop (parts of) the argument even when not matched on.
self.schedule_drop(
pattern.as_ref().map_or(ast_body.span, |pat| pat.span),
argument_scope, &place, ty,
DropKind::Value { cached_block: CachedBlock::default() },
);
}

// Enter the argument pattern bindings source scope, if it exists.
Expand Down
68 changes: 68 additions & 0 deletions src/test/run-pass/binding/fn-arg-incomplete-pattern-drop-order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Check that partially moved from function parameters are dropped after the
// named bindings that move from them.

// ignore-wasm32-bare compiled with panic=abort by default

use std::{panic, cell::RefCell};

struct LogDrop<'a>(i32, Context<'a>);

#[derive(Copy, Clone)]
struct Context<'a> {
panic_on: i32,
drops: &'a RefCell<Vec<i32>>,
}

impl<'a> Context<'a> {
fn record_drop(self, index: i32) {
self.drops.borrow_mut().push(index);
if index == self.panic_on {
panic!();
}
}
}

impl<'a> Drop for LogDrop<'a> {
fn drop(&mut self) {
self.1.record_drop(self.0);
}
}

fn bindings_in_params((_x, _): (LogDrop, LogDrop), (_, _y): (LogDrop, LogDrop)) {}
fn bindings_with_let(a: (LogDrop, LogDrop), b: (LogDrop, LogDrop)) {
// Drop order in foo is the same as the following bindings.
// _temp2 is declared after _x to avoid a difference between `_: T` and
// `x: T` in function parameters.
let _temp1 = a;
let (_x, _) = _temp1;

let _temp2 = b;
let (_, _y) = _temp2;
}

fn test_drop_order(panic_on: i32, fun: fn((LogDrop, LogDrop), (LogDrop, LogDrop))) {
let context = Context {
panic_on,
drops: &RefCell::new(Vec::new()),
};
let one = LogDrop(1, context);
let two = LogDrop(2, context);
let three = LogDrop(3, context);
let four = LogDrop(4, context);

let res = panic::catch_unwind(panic::AssertUnwindSafe(|| {
fun((three, four), (two, one));
}));
if panic_on == 0 {
assert!(res.is_ok(), "should not have panicked");
} else {
assert!(res.is_err(), "should have panicked");
}
assert_eq!(*context.drops.borrow(), [1, 2, 3, 4], "incorrect drop order");
}

fn main() {
(0..=4).for_each(|i| test_drop_order(i, bindings_in_params));
(0..=4).for_each(|i| test_drop_order(i, bindings_with_let));
(0..=4).for_each(|i| test_drop_order(i, |(_x, _), (_, _y)| {}));
}

0 comments on commit 914515f

Please sign in to comment.