Skip to content

Commit

Permalink
auto merge of #16053 : pcwalton/rust/at-pattern-bindings, r=pnkfelix
Browse files Browse the repository at this point in the history
This is an alternative to upgrading the way rvalues are handled in the
borrow check. Making rvalues handled more like lvalues in the borrow
check caused numerous problems related to double mutable borrows and
rvalue scopes. Rather than come up with more borrow check rules to try
to solve these problems, I decided to just forbid pattern bindings after
`@`. This affected fewer than 10 lines of code in the compiler and
libraries.

This breaks code like:

    match x {
        y @ z => { ... }
    }

    match a {
        b @ Some(c) => { ... }
    }

Change this code to use nested `match` or `let` expressions. For
example:

    match x {
        y => {
            let z = y;
            ...
        }
    }

    match a {
        Some(c) => {
            let b = Some(c);
            ...
        }
    }

Closes #14587.

[breaking-change]

May need discussion at the meeting, but r? @nikomatsakis anyway
  • Loading branch information
bors committed Aug 1, 2014
2 parents 6248858 + 5b85c8c commit f2fa559
Show file tree
Hide file tree
Showing 11 changed files with 103 additions and 129 deletions.
7 changes: 6 additions & 1 deletion src/doc/rust.md
Original file line number Diff line number Diff line change
Expand Up @@ -3309,7 +3309,12 @@ enum List { Nil, Cons(uint, Box<List>) }
fn is_sorted(list: &List) -> bool {
match *list {
Nil | Cons(_, box Nil) => true,
Cons(x, ref r @ box Cons(y, _)) => (x <= y) && is_sorted(&**r)
Cons(x, ref r @ box Cons(_, _)) => {
match *r {
box Cons(y, _) => (x <= y) && is_sorted(&**r),
_ => fail!()
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/libcollections/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1724,7 +1724,7 @@ mod tests {
}
}

let mut count_x @ mut count_y = 0;
let (mut count_x, mut count_y) = (0, 0);
{
let mut tv = TwoVec {
x: Vec::new(),
Expand Down
5 changes: 3 additions & 2 deletions src/librustc/lint/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,8 +630,9 @@ impl LintPass for GatherNodeLevels {
match it.node {
ast::ItemEnum(..) => {
let lint_id = LintId::of(builtin::VARIANT_SIZE_DIFFERENCE);
match cx.lints.get_level_source(lint_id) {
lvlsrc @ (lvl, _) if lvl != Allow => {
let lvlsrc = cx.lints.get_level_source(lint_id);
match lvlsrc {
(lvl, _) if lvl != Allow => {
cx.node_levels.borrow_mut()
.insert((it.id, lint_id), lvlsrc);
},
Expand Down
49 changes: 29 additions & 20 deletions src/librustc/middle/borrowck/gather_loans/restrictions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,27 +139,36 @@ impl<'a> RestrictionsContext<'a> {
Safe
}

mc::cat_deref(cmt_base, _, pk @ mc::BorrowedPtr(ty::MutBorrow, lt)) |
mc::cat_deref(cmt_base, _, pk @ mc::Implicit(ty::MutBorrow, lt)) => {
// R-Deref-Mut-Borrowed
if !self.bccx.is_subregion_of(self.loan_region, lt) {
self.bccx.report(
BckError {
span: self.span,
cause: self.cause,
cmt: cmt_base,
code: err_borrowed_pointer_too_short(
self.loan_region, lt)});
return Safe;
mc::cat_deref(cmt_base, _, pk) => {
match pk {
mc::BorrowedPtr(ty::MutBorrow, lt) |
mc::Implicit(ty::MutBorrow, lt) => {
// R-Deref-Mut-Borrowed
if !self.bccx.is_subregion_of(self.loan_region, lt) {
self.bccx.report(
BckError {
span: self.span,
cause: self.cause,
cmt: cmt_base,
code: err_borrowed_pointer_too_short(
self.loan_region, lt)});
return Safe;
}

let result = self.restrict(cmt_base);
self.extend(result, cmt.mutbl, LpDeref(pk))
}
mc::UnsafePtr(..) => {
// We are very trusting when working with unsafe
// pointers.
Safe
}
_ => {
self.bccx.tcx.sess.span_bug(self.span,
"unhandled memcat in \
cat_deref")
}
}

let result = self.restrict(cmt_base);
self.extend(result, cmt.mutbl, LpDeref(pk))
}

mc::cat_deref(_, _, mc::UnsafePtr(..)) => {
// We are very trusting when working with unsafe pointers.
Safe
}

mc::cat_discr(cmt_base, _) => {
Expand Down
46 changes: 42 additions & 4 deletions src/librustc/middle/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ fn check_expr(cx: &mut MatchCheckCtxt, ex: &Expr) {
check_legality_of_move_bindings(cx,
arm.guard.is_some(),
arm.pats.as_slice());
for pat in arm.pats.iter() {
check_legality_of_bindings_in_at_patterns(cx, &**pat);
}
}

// Second, if there is a guard on each arm, make sure it isn't
Expand Down Expand Up @@ -200,6 +203,7 @@ fn check_expr(cx: &mut MatchCheckCtxt, ex: &Expr) {

// Check legality of move bindings.
check_legality_of_move_bindings(cx, false, [ *pat ]);
check_legality_of_bindings_in_at_patterns(cx, &**pat);
}
_ => ()
}
Expand Down Expand Up @@ -455,8 +459,12 @@ fn all_constructors(cx: &MatchCheckCtxt, left_ty: ty::t,

// Note: is_useful doesn't work on empty types, as the paper notes.
// So it assumes that v is non-empty.
fn is_useful(cx: &MatchCheckCtxt, matrix @ &Matrix(ref rows): &Matrix,
v: &[Gc<Pat>], witness: WitnessPreference) -> Usefulness {
fn is_useful(cx: &MatchCheckCtxt,
matrix: &Matrix,
v: &[Gc<Pat>],
witness: WitnessPreference)
-> Usefulness {
let &Matrix(ref rows) = matrix;
debug!("{:}", matrix);
if rows.len() == 0u {
return match witness {
Expand Down Expand Up @@ -819,8 +827,9 @@ fn check_local(cx: &mut MatchCheckCtxt, loc: &Local) {
None => ()
}

// Check legality of move bindings.
// Check legality of move bindings and `@` patterns.
check_legality_of_move_bindings(cx, false, [ loc.pat ]);
check_legality_of_bindings_in_at_patterns(cx, &*loc.pat);
}

fn check_fn(cx: &mut MatchCheckCtxt,
Expand All @@ -840,6 +849,7 @@ fn check_fn(cx: &mut MatchCheckCtxt,
None => ()
}
check_legality_of_move_bindings(cx, false, [input.pat]);
check_legality_of_bindings_in_at_patterns(cx, &*input.pat);
}
}

Expand All @@ -856,7 +866,6 @@ fn is_refutable(cx: &MatchCheckCtxt, pat: Gc<Pat>) -> Option<Gc<Pat>> {
}

// Legality of move bindings checking

fn check_legality_of_move_bindings(cx: &MatchCheckCtxt,
has_guard: bool,
pats: &[Gc<Pat>]) {
Expand Down Expand Up @@ -966,3 +975,32 @@ impl<'a> Delegate for MutationChecker<'a> {
}
}

/// Forbids bindings in `@` patterns. This is necessary for memory safety,
/// because of the way rvalues are handled in the borrow check. (See issue
/// #14587.)
fn check_legality_of_bindings_in_at_patterns(cx: &MatchCheckCtxt, pat: &Pat) {
let mut visitor = AtBindingPatternVisitor {
cx: cx,
};
visitor.visit_pat(pat, true);
}

struct AtBindingPatternVisitor<'a,'b> {
cx: &'a MatchCheckCtxt<'b>,
}

impl<'a,'b> Visitor<bool> for AtBindingPatternVisitor<'a,'b> {
fn visit_pat(&mut self, pat: &Pat, bindings_allowed: bool) {
if !bindings_allowed && pat_is_binding(&self.cx.tcx.def_map, pat) {
self.cx.tcx.sess.span_err(pat.span,
"pattern bindings are not allowed \
after an `@`");
}

match pat.node {
PatIdent(_, _, Some(_)) => visit::walk_pat(self, pat, false),
_ => visit::walk_pat(self, pat, bindings_allowed),
}
}
}

10 changes: 6 additions & 4 deletions src/librustc/middle/typeck/infer/region_inference/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,9 @@ impl<'a> RegionVarBindings<'a> {
b).as_slice());
}

(f @ ReFree(ref fr), ReScope(s_id)) |
(ReScope(s_id), f @ ReFree(ref fr)) => {
(ReFree(ref fr), ReScope(s_id)) |
(ReScope(s_id), ReFree(ref fr)) => {
let f = ReFree(*fr);
// A "free" region can be interpreted as "some region
// at least as big as the block fr.scope_id". So, we can
// reasonably compare free regions and scopes:
Expand Down Expand Up @@ -706,8 +707,9 @@ impl<'a> RegionVarBindings<'a> {
b).as_slice());
}

(ReFree(ref fr), s @ ReScope(s_id)) |
(s @ ReScope(s_id), ReFree(ref fr)) => {
(ReFree(ref fr), ReScope(s_id)) |
(ReScope(s_id), ReFree(ref fr)) => {
let s = ReScope(s_id);
// Free region is something "at least as big as
// `fr.scope_id`." If we find that the scope `fr.scope_id` is bigger
// than the scope `s_id`, then we can say that the GLB
Expand Down

This file was deleted.

21 changes: 0 additions & 21 deletions src/test/compile-fail/bind-by-move-no-sub-bindings-fun-args.rs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2012 The Rust Project Developers. See the COPYRIGHT
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
Expand All @@ -8,18 +8,19 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct X { x: (), }

impl Drop for X {
fn drop(&mut self) {
println!("destructor runs");
}
enum Option<T> {
None,
Some(T),
}

fn main() {
let x = Some(X { x: () });
match x {
Some(_y @ ref _z) => { }, //~ ERROR cannot bind by-move with sub-bindings
None => fail!()
match &mut Some(1i) {
ref mut z @ &Some(ref a) => {
//~^ ERROR pattern bindings are not allowed after an `@`
**z = None;
println!("{}", *a);
}
_ => ()
}
}

17 changes: 4 additions & 13 deletions src/test/run-pass/match-pattern-bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,15 @@ fn main() {
ref b @ None => b
}, &Some(1i));
assert_eq!(match value {
ref a @ ref _c @ Some(_) => a,
ref b @ None => b
}, &Some(1i));
assert_eq!(match value {
_a @ ref c @ Some(_) => c,
ref c @ Some(_) => c,
ref b @ None => b
}, &Some(1i));
assert_eq!(match "foobarbaz" {
_a @ b @ _ => b
b @ _ => b
}, "foobarbaz");

let a @ b @ c = "foobarbaz";
let a @ _ = "foobarbaz";
assert_eq!(a, "foobarbaz");
assert_eq!(b, "foobarbaz");
assert_eq!(c, "foobarbaz");
let value = Some(true);
let ref a @ b @ ref c = value;
let ref a @ _ = value;
assert_eq!(a, &Some(true));
assert_eq!(b, Some(true));
assert_eq!(c, &Some(true));
}
27 changes: 0 additions & 27 deletions src/test/run-pass/nested-patterns.rs

This file was deleted.

0 comments on commit f2fa559

Please sign in to comment.