From 5b85c8cbe70abb914f9ba66116192667b8235cfb Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Mon, 28 Jul 2014 11:33:06 -0700 Subject: [PATCH] librustc: Forbid pattern bindings after `@`s, for memory safety. 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] --- src/doc/rust.md | 7 ++- src/libcollections/vec.rs | 2 +- src/librustc/lint/context.rs | 5 +- .../borrowck/gather_loans/restrictions.rs | 49 +++++++++++-------- src/librustc/middle/check_match.rs | 46 +++++++++++++++-- .../typeck/infer/region_inference/mod.rs | 10 ++-- ...her-can-live-while-the-other-survives-1.rs | 25 ---------- .../bind-by-move-no-sub-bindings-fun-args.rs | 21 -------- ...ndings.rs => pattern-bindings-after-at.rs} | 23 ++++----- src/test/run-pass/match-pattern-bindings.rs | 17 ++----- src/test/run-pass/nested-patterns.rs | 27 ---------- 11 files changed, 103 insertions(+), 129 deletions(-) delete mode 100644 src/test/compile-fail/bind-by-move-neither-can-live-while-the-other-survives-1.rs delete mode 100644 src/test/compile-fail/bind-by-move-no-sub-bindings-fun-args.rs rename src/test/compile-fail/{bind-by-move-no-sub-bindings.rs => pattern-bindings-after-at.rs} (57%) delete mode 100644 src/test/run-pass/nested-patterns.rs diff --git a/src/doc/rust.md b/src/doc/rust.md index 3f4be7bbf31e5..7259588233ee5 100644 --- a/src/doc/rust.md +++ b/src/doc/rust.md @@ -3309,7 +3309,12 @@ enum List { Nil, Cons(uint, Box) } 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!() + } + } } } diff --git a/src/libcollections/vec.rs b/src/libcollections/vec.rs index dcee92f6dbced..a0c92887c43ed 100644 --- a/src/libcollections/vec.rs +++ b/src/libcollections/vec.rs @@ -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(), diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 7d9ec29d70144..e3829892f7bff 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -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); }, diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 48399cb0b7e02..da739faabad93 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -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, _) => { diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 4d10676a5892e..21b9a3f905391 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -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 @@ -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); } _ => () } @@ -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], witness: WitnessPreference) -> Usefulness { +fn is_useful(cx: &MatchCheckCtxt, + matrix: &Matrix, + v: &[Gc], + witness: WitnessPreference) + -> Usefulness { + let &Matrix(ref rows) = matrix; debug!("{:}", matrix); if rows.len() == 0u { return match witness { @@ -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, @@ -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); } } @@ -856,7 +866,6 @@ fn is_refutable(cx: &MatchCheckCtxt, pat: Gc) -> Option> { } // Legality of move bindings checking - fn check_legality_of_move_bindings(cx: &MatchCheckCtxt, has_guard: bool, pats: &[Gc]) { @@ -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 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), + } + } +} + diff --git a/src/librustc/middle/typeck/infer/region_inference/mod.rs b/src/librustc/middle/typeck/infer/region_inference/mod.rs index d17553e9c39b3..dc674da38af5b 100644 --- a/src/librustc/middle/typeck/infer/region_inference/mod.rs +++ b/src/librustc/middle/typeck/infer/region_inference/mod.rs @@ -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: @@ -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 diff --git a/src/test/compile-fail/bind-by-move-neither-can-live-while-the-other-survives-1.rs b/src/test/compile-fail/bind-by-move-neither-can-live-while-the-other-survives-1.rs deleted file mode 100644 index a60348c4a3a67..0000000000000 --- a/src/test/compile-fail/bind-by-move-neither-can-live-while-the-other-survives-1.rs +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2012 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// 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"); - } -} - -fn main() { - let x = Some(X { x: () }); - match x { - Some(ref _y @ _z) => { }, //~ ERROR cannot bind by-move and by-ref in the same pattern - None => fail!() - } -} diff --git a/src/test/compile-fail/bind-by-move-no-sub-bindings-fun-args.rs b/src/test/compile-fail/bind-by-move-no-sub-bindings-fun-args.rs deleted file mode 100644 index 0e5b659f125e4..0000000000000 --- a/src/test/compile-fail/bind-by-move-no-sub-bindings-fun-args.rs +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright 2012 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -// Issue #12534. - -struct A(Box); - -fn f(a @ A(u): A) -> Box { //~ ERROR cannot bind by-move with sub-bindings - drop(a); - u -} - -fn main() {} - diff --git a/src/test/compile-fail/bind-by-move-no-sub-bindings.rs b/src/test/compile-fail/pattern-bindings-after-at.rs similarity index 57% rename from src/test/compile-fail/bind-by-move-no-sub-bindings.rs rename to src/test/compile-fail/pattern-bindings-after-at.rs index 34b83af1d2ece..9cd2d0d28b165 100644 --- a/src/test/compile-fail/bind-by-move-no-sub-bindings.rs +++ b/src/test/compile-fail/pattern-bindings-after-at.rs @@ -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. // @@ -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 { + 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); + } + _ => () } } + diff --git a/src/test/run-pass/match-pattern-bindings.rs b/src/test/run-pass/match-pattern-bindings.rs index 61e905e5b17b6..e6ce94ec5d4aa 100644 --- a/src/test/run-pass/match-pattern-bindings.rs +++ b/src/test/run-pass/match-pattern-bindings.rs @@ -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)); } diff --git a/src/test/run-pass/nested-patterns.rs b/src/test/run-pass/nested-patterns.rs deleted file mode 100644 index 08816d345b400..0000000000000 --- a/src/test/run-pass/nested-patterns.rs +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2012 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -struct A { a: int, b: int } -struct B { a: int, b: C } -struct D { a: int, d: C } -struct C { c: int } - -pub fn main() { - match (A {a: 10, b: 20}) { - x@A {a, b: 20} => { assert!(x.a == 10); assert!(a == 10); } - A {b: _b, ..} => { fail!(); } - } - let mut x@B {b, ..} = B {a: 10, b: C {c: 20}}; - x.b.c = 30; - assert_eq!(b.c, 20); - let mut y@D {d, ..} = D {a: 10, d: C {c: 20}}; - y.d.c = 30; - assert_eq!(d.c, 20); -}