From 5851d3242cce2a53fc25df21ab5ad20dc1fd6a62 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 22 May 2013 06:54:35 -0400 Subject: [PATCH 1/5] Move checking for moves and initialization of local variables and patterns into borrow checker and generalize what moves are allowed. Fixes a nasty bug or two in the pattern move checking code. Unifies dataflow code used for initialization and other things. First step towards once fns. Everybody wins. Fixes #4384. Fixes #4715. cc once fns (#2202), optimizing local moves (#5016). --- src/librust/rust.rc | 12 +- src/librustc/driver/driver.rs | 5 +- src/librustc/middle/borrowck/check_loans.rs | 229 +++---- .../borrowck/gather_loans/gather_moves.rs | 164 +++++ .../middle/borrowck/gather_loans/mod.rs | 77 ++- src/librustc/middle/borrowck/mod.rs | 115 +++- src/librustc/middle/borrowck/move_data.rs | 568 ++++++++++++++++++ src/librustc/middle/check_match.rs | 63 +- src/librustc/middle/dataflow.rs | 4 +- src/librustc/middle/liveness.rs | 182 +----- src/librustc/middle/mem_categorization.rs | 41 +- src/librustc/middle/moves.rs | 184 ++---- src/librustc/middle/pat_util.rs | 4 +- src/librustc/middle/resolve.rs | 2 +- src/librustc/middle/typeck/check/_match.rs | 8 +- src/librustc/util/ppaux.rs | 6 + src/librustdoc/markdown_index_pass.rs | 18 +- src/librustdoc/markdown_pass.rs | 30 +- src/librustdoc/markdown_writer.rs | 22 +- src/librustdoc/sectionalize_pass.rs | 2 +- src/librustdoc/tystr_pass.rs | 4 +- src/libsyntax/ast_util.rs | 31 +- .../compile-fail/borrowck-move-by-capture.rs | 3 +- .../borrowck-move-out-of-vec-tail.rs | 31 + src/test/compile-fail/borrowck-unary-move.rs | 2 +- .../compile-fail/by-move-pattern-binding.rs | 2 +- ...deconstructing-destructing-struct-match.rs | 2 +- src/test/compile-fail/issue-2590.rs | 2 +- .../compile-fail/liveness-move-in-loop.rs | 3 - .../compile-fail/liveness-move-in-while.rs | 4 +- .../moves-based-on-type-access-to-field.rs | 4 +- .../moves-based-on-type-block-bad.rs | 4 +- .../compile-fail/moves-based-on-type-exprs.rs | 2 +- .../moves-based-on-type-match-bindings.rs | 19 + src/test/compile-fail/no-reuse-move-arc.rs | 4 +- .../use-after-move-self-based-on-type.rs | 2 +- src/test/compile-fail/use-after-move-self.rs | 2 +- .../borrowck-unary-move-2.rs | 2 +- src/test/run-pass/move-out-of-field.rs | 23 + src/test/run-pass/vec-matching-fold.rs | 8 +- src/test/run-pass/vec-tail-matching.rs | 6 +- 41 files changed, 1282 insertions(+), 614 deletions(-) create mode 100644 src/librustc/middle/borrowck/gather_loans/gather_moves.rs create mode 100644 src/librustc/middle/borrowck/move_data.rs create mode 100644 src/test/compile-fail/borrowck-move-out-of-vec-tail.rs create mode 100644 src/test/compile-fail/moves-based-on-type-match-bindings.rs rename src/test/{compile-fail => run-pass}/borrowck-unary-move-2.rs (94%) create mode 100644 src/test/run-pass/move-out-of-field.rs diff --git a/src/librust/rust.rc b/src/librust/rust.rc index 36246b7a9a14c..00925a5700ada 100644 --- a/src/librust/rust.rc +++ b/src/librust/rust.rc @@ -152,15 +152,15 @@ fn cmd_help(args: &[~str]) -> ValidUsage { } match args { - [command_string] => print_usage(command_string), - _ => Invalid + [ref command_string] => print_usage(copy *command_string), + _ => Invalid } } fn cmd_test(args: &[~str]) -> ValidUsage { match args { - [filename] => { - let test_exec = Path(filename).filestem().unwrap() + "test~"; + [ref filename] => { + let test_exec = Path(*filename).filestem().unwrap() + "test~"; invoke("rustc", &[~"--test", filename.to_owned(), ~"-o", test_exec.to_owned()], rustc::main); let exit_code = run::process_status(~"./" + test_exec, []); @@ -172,8 +172,8 @@ fn cmd_test(args: &[~str]) -> ValidUsage { fn cmd_run(args: &[~str]) -> ValidUsage { match args { - [filename, ..prog_args] => { - let exec = Path(filename).filestem().unwrap() + "~"; + [ref filename, ..prog_args] => { + let exec = Path(*filename).filestem().unwrap() + "~"; invoke("rustc", &[filename.to_owned(), ~"-o", exec.to_owned()], rustc::main); let exit_code = run::process_status(~"./"+exec, prog_args); diff --git a/src/librustc/driver/driver.rs b/src/librustc/driver/driver.rs index 64d3b0e373cfd..65838f62498dc 100644 --- a/src/librustc/driver/driver.rs +++ b/src/librustc/driver/driver.rs @@ -263,8 +263,8 @@ pub fn compile_rest(sess: Session, time(time_passes, ~"loop checking", || middle::check_loop::check_crate(ty_cx, crate)); - let middle::moves::MoveMaps {moves_map, variable_moves_map, - moved_variables_set, capture_map} = + let middle::moves::MoveMaps {moves_map, moved_variables_set, + capture_map} = time(time_passes, ~"compute moves", || middle::moves::compute_moves(ty_cx, method_map, crate)); @@ -274,7 +274,6 @@ pub fn compile_rest(sess: Session, time(time_passes, ~"liveness checking", || middle::liveness::check_crate(ty_cx, method_map, - variable_moves_map, capture_map, crate)); let (root_map, write_guard_map) = diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 237a464dc9e96..f2bba4f694a90 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -33,20 +33,23 @@ use syntax::codemap::span; struct CheckLoanCtxt<'self> { bccx: @BorrowckCtxt, - dfcx: &'self LoanDataFlow, + dfcx_loans: &'self LoanDataFlow, + move_data: move_data::FlowedMoveData, all_loans: &'self [Loan], reported: @mut HashSet, } pub fn check_loans(bccx: @BorrowckCtxt, - dfcx: &LoanDataFlow, + dfcx_loans: &LoanDataFlow, + move_data: move_data::FlowedMoveData, all_loans: &[Loan], body: &ast::blk) { debug!("check_loans(body id=%?)", body.node.id); let clcx = @mut CheckLoanCtxt { bccx: bccx, - dfcx: dfcx, + dfcx_loans: dfcx_loans, + move_data: move_data, all_loans: all_loans, reported: @mut HashSet::new(), }; @@ -62,7 +65,6 @@ pub fn check_loans(bccx: @BorrowckCtxt, enum MoveError { MoveOk, - MoveFromIllegalCmt(mc::cmt), MoveWhileBorrowed(/*loan*/@LoanPath, /*loan*/span) } @@ -79,7 +81,7 @@ pub impl<'self> CheckLoanCtxt<'self> { //! are issued for future scopes and thus they may have been //! *issued* but not yet be in effect. - for self.dfcx.each_bit_on_entry(scope_id) |loan_index| { + for self.dfcx_loans.each_bit_on_entry(scope_id) |loan_index| { let loan = &self.all_loans[loan_index]; if !op(loan) { return false; @@ -131,7 +133,7 @@ pub impl<'self> CheckLoanCtxt<'self> { //! we encounter `scope_id`. let mut result = ~[]; - for self.dfcx.each_gen_bit(scope_id) |loan_index| { + for self.dfcx_loans.each_gen_bit(scope_id) |loan_index| { result.push(loan_index); } return result; @@ -251,6 +253,29 @@ pub impl<'self> CheckLoanCtxt<'self> { } } + fn check_if_path_is_moved(&self, + id: ast::node_id, + span: span, + use_kind: MovedValueUseKind, + lp: @LoanPath) { + /*! + * Reports an error if `expr` (which should be a path) + * is using a moved/uninitialized value + */ + + debug!("check_if_path_is_moved(id=%?, use_kind=%?, lp=%s)", + id, use_kind, lp.repr(self.bccx.tcx)); + for self.move_data.each_move_of(id, lp) |move, moved_lp| { + self.bccx.report_use_of_moved_value( + span, + use_kind, + lp, + move, + moved_lp); + return; + } + } + fn check_assignment(&self, expr: @ast::expr) { // We don't use cat_expr() here because we don't want to treat // auto-ref'd parameters in overloaded operators as rvalues. @@ -261,48 +286,42 @@ pub impl<'self> CheckLoanCtxt<'self> { debug!("check_assignment(cmt=%s)", cmt.repr(self.tcx())); - // check that the value being assigned is declared as mutable - // and report an error otherwise. - match cmt.mutbl { - mc::McDeclared => { - // OK, but we have to mark arguments as requiring mut - // if they are assigned (other cases are handled by liveness, - // since we need to distinguish local variables assigned - // once vs those assigned multiple times) - match cmt.cat { - mc::cat_self(*) | - mc::cat_arg(*) => { - mark_variable_as_used_mut(self, cmt); - } - _ => {} + // Mutable values can be assigned, as long as they obey loans + // and aliasing restrictions: + if cmt.mutbl.is_mutable() { + if check_for_aliasable_mutable_writes(self, expr, cmt) { + if check_for_assignment_to_restricted_or_frozen_location( + self, expr, cmt) + { + // Safe, but record for lint pass later: + mark_variable_as_used_mut(self, cmt); } } - mc::McInherited => { - // OK, but we may have to add an entry to `used_mut_nodes` - mark_variable_as_used_mut(self, cmt); - } - mc::McReadOnly | mc::McImmutable => { - // Subtle: liveness guarantees that immutable local - // variables are only assigned once, so no need to - // report an error for an assignment to a local - // variable (note also that it is not legal to borrow - // for a local variable before it has been assigned - // for the first time). - if !self.is_local_variable(cmt) { - self.bccx.span_err( - expr.span, - fmt!("cannot assign to %s %s" - cmt.mutbl.to_user_str(), - self.bccx.cmt_to_str(cmt))); - } + return; + } + + // For immutable local variables, assignments are legal + // if they cannot already have been assigned + if self.is_local_variable(cmt) { + assert!(cmt.mutbl.is_immutable()); // no "const" locals + let lp = opt_loan_path(cmt).get(); + for self.move_data.each_assignment_of(expr.id, lp) |assign| { + self.bccx.report_reassigned_immutable_variable( + expr.span, + lp, + assign); return; } + return; } - if check_for_aliasable_mutable_writes(self, expr, cmt) { - check_for_assignment_to_restricted_or_frozen_location( - self, expr, cmt); - } + // Otherwise, just a plain error. + self.bccx.span_err( + expr.span, + fmt!("cannot assign to %s %s" + cmt.mutbl.to_user_str(), + self.bccx.cmt_to_str(cmt))); + return; fn mark_variable_as_used_mut(this: &CheckLoanCtxt, cmt: mc::cmt) { @@ -538,12 +557,6 @@ pub impl<'self> CheckLoanCtxt<'self> { let cmt = self.bccx.cat_expr(ex); match self.analyze_move_out_from_cmt(cmt) { MoveOk => {} - MoveFromIllegalCmt(_) => { - self.bccx.span_err( - cmt.span, - fmt!("cannot move out of %s", - self.bccx.cmt_to_str(cmt))); - } MoveWhileBorrowed(loan_path, loan_span) => { self.bccx.span_err( cmt.span, @@ -561,29 +574,7 @@ pub impl<'self> CheckLoanCtxt<'self> { } fn analyze_move_out_from_cmt(&self, cmt: mc::cmt) -> MoveError { - debug!("check_move_out_from_cmt(cmt=%s)", cmt.repr(self.tcx())); - - match cmt.cat { - // Rvalues, locals, and arguments can be moved: - mc::cat_rvalue | mc::cat_local(_) | - mc::cat_arg(_) | mc::cat_self(_) => {} - - // It seems strange to allow a move out of a static item, - // but what happens in practice is that you have a - // reference to a constant with a type that should be - // moved, like `None::<~int>`. The type of this constant - // is technically `Option<~int>`, which moves, but we know - // that the content of static items will never actually - // contain allocated pointers, so we can just memcpy it. - mc::cat_static_item => {} - - mc::cat_deref(_, _, mc::unsafe_ptr(*)) => {} - - // Nothing else. - _ => { - return MoveFromIllegalCmt(cmt); - } - } + debug!("analyze_move_out_from_cmt(cmt=%s)", cmt.repr(self.tcx())); // FIXME(#4384) inadequare if/when we permit `move a.b` @@ -631,54 +622,53 @@ fn check_loans_in_fn<'a>(fk: &visit::fn_kind, visit::fk_anon(*) | visit::fk_fn_block(*) => { - let fty = ty::node_id_to_type(this.tcx(), id); - let fty_sigil = ty::ty_closure_sigil(fty); - check_moves_from_captured_variables(this, id, fty_sigil); + check_captured_variables(this, id, sp); } } visit::visit_fn(fk, decl, body, sp, id, this, visitor); - fn check_moves_from_captured_variables(this: @mut CheckLoanCtxt, - id: ast::node_id, - fty_sigil: ast::Sigil) { - match fty_sigil { - ast::ManagedSigil | ast::OwnedSigil => { - let cap_vars = this.bccx.capture_map.get(&id); - for cap_vars.each |cap_var| { - match cap_var.mode { - moves::CapRef | moves::CapCopy => { loop; } - moves::CapMove => { } - } - let def_id = ast_util::def_id_of_def(cap_var.def).node; - let ty = ty::node_id_to_type(this.tcx(), def_id); - let cmt = this.bccx.cat_def(id, cap_var.span, - ty, cap_var.def); - let move_err = this.analyze_move_out_from_cmt(cmt); - match move_err { - MoveOk => {} - MoveFromIllegalCmt(move_cmt) => { - this.bccx.span_err( - cap_var.span, - fmt!("illegal by-move capture of %s", - this.bccx.cmt_to_str(move_cmt))); - } - MoveWhileBorrowed(loan_path, loan_span) => { - this.bccx.span_err( - cap_var.span, - fmt!("cannot move `%s` into closure \ - because it is borrowed", - this.bccx.loan_path_to_str(loan_path))); - this.bccx.span_note( - loan_span, - fmt!("borrow of `%s` occurs here", - this.bccx.loan_path_to_str(loan_path))); - } - } + fn check_captured_variables(this: @mut CheckLoanCtxt, + closure_id: ast::node_id, + span: span) { + let cap_vars = this.bccx.capture_map.get(&closure_id); + for cap_vars.each |cap_var| { + match cap_var.mode { + moves::CapRef | moves::CapCopy => { + let var_id = ast_util::def_id_of_def(cap_var.def).node; + let lp = @LpVar(var_id); + this.check_if_path_is_moved(closure_id, span, + MovedInCapture, lp); + } + moves::CapMove => { + check_by_move_capture(this, closure_id, cap_var); + } + } + } + return; + + fn check_by_move_capture(this: @mut CheckLoanCtxt, + closure_id: ast::node_id, + cap_var: &moves::CaptureVar) { + let var_id = ast_util::def_id_of_def(cap_var.def).node; + let ty = ty::node_id_to_type(this.tcx(), var_id); + let cmt = this.bccx.cat_def(closure_id, cap_var.span, + ty, cap_var.def); + let move_err = this.analyze_move_out_from_cmt(cmt); + match move_err { + MoveOk => {} + MoveWhileBorrowed(loan_path, loan_span) => { + this.bccx.span_err( + cap_var.span, + fmt!("cannot move `%s` into closure \ + because it is borrowed", + this.bccx.loan_path_to_str(loan_path))); + this.bccx.span_note( + loan_span, + fmt!("borrow of `%s` occurs here", + this.bccx.loan_path_to_str(loan_path))); } } - - ast::BorrowedSigil => {} } } } @@ -692,11 +682,11 @@ fn check_loans_in_local<'a>(local: @ast::local, fn check_loans_in_expr<'a>(expr: @ast::expr, this: @mut CheckLoanCtxt<'a>, vt: visit::vt<@mut CheckLoanCtxt<'a>>) { + visit::visit_expr(expr, this, vt); + debug!("check_loans_in_expr(expr=%s)", expr.repr(this.tcx())); - visit::visit_expr(expr, this, vt); - this.check_for_conflicting_loans(expr.id); if this.bccx.moves_map.contains(&expr.id) { @@ -704,6 +694,17 @@ fn check_loans_in_expr<'a>(expr: @ast::expr, } match expr.node { + ast::expr_self | + ast::expr_path(*) => { + if !this.move_data.is_assignee(expr.id) { + let cmt = this.bccx.cat_expr_unadjusted(expr); + debug!("path cmt=%s", cmt.repr(this.tcx())); + for opt_loan_path(cmt).each |&lp| { + this.check_if_path_is_moved(expr.id, expr.span, + MovedInUse, lp); + } + } + } ast::expr_assign(dest, _) | ast::expr_assign_op(_, dest, _) => { this.check_assignment(dest); diff --git a/src/librustc/middle/borrowck/gather_loans/gather_moves.rs b/src/librustc/middle/borrowck/gather_loans/gather_moves.rs new file mode 100644 index 0000000000000..d32c1873ba053 --- /dev/null +++ b/src/librustc/middle/borrowck/gather_loans/gather_moves.rs @@ -0,0 +1,164 @@ +// 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. + +/*! + * Computes moves. + */ + +use core::prelude::*; +use mc = middle::mem_categorization; +use middle::borrowck::*; +use middle::borrowck::move_data::*; +use middle::moves; +use middle::ty; +use syntax::ast; +use syntax::ast_util; +use syntax::codemap::span; +use util::ppaux::{UserString}; + +pub fn gather_decl(bccx: @BorrowckCtxt, + move_data: &mut MoveData, + decl_id: ast::node_id, + _decl_span: span, + var_id: ast::node_id) { + let loan_path = @LpVar(var_id); + move_data.add_move(bccx.tcx, loan_path, decl_id, Declared); +} + +pub fn gather_move_from_expr(bccx: @BorrowckCtxt, + move_data: &mut MoveData, + move_expr: @ast::expr, + cmt: mc::cmt) { + gather_move_from_expr_or_pat(bccx, move_data, move_expr.id, + MoveExpr(move_expr), cmt); +} + +pub fn gather_move_from_pat(bccx: @BorrowckCtxt, + move_data: &mut MoveData, + move_pat: @ast::pat, + cmt: mc::cmt) { + gather_move_from_expr_or_pat(bccx, move_data, move_pat.id, + MovePat(move_pat), cmt); +} + +fn gather_move_from_expr_or_pat(bccx: @BorrowckCtxt, + move_data: &mut MoveData, + move_id: ast::node_id, + move_kind: MoveKind, + cmt: mc::cmt) { + if !check_is_legal_to_move_from(bccx, cmt, cmt) { + return; + } + + match opt_loan_path(cmt) { + Some(loan_path) => { + move_data.add_move(bccx.tcx, loan_path, move_id, move_kind); + } + None => { + // move from rvalue or unsafe pointer, hence ok + } + } +} + +pub fn gather_captures(bccx: @BorrowckCtxt, + move_data: &mut MoveData, + closure_expr: @ast::expr) { + let captured_vars = bccx.capture_map.get(&closure_expr.id); + for captured_vars.each |captured_var| { + match captured_var.mode { + moves::CapMove => { + let fvar_id = ast_util::def_id_of_def(captured_var.def).node; + let loan_path = @LpVar(fvar_id); + move_data.add_move(bccx.tcx, loan_path, closure_expr.id, + Captured(closure_expr)); + } + moves::CapCopy | moves::CapRef => {} + } + } +} + +pub fn gather_assignment(bccx: @BorrowckCtxt, + move_data: &mut MoveData, + assignment_id: ast::node_id, + assignment_span: span, + assignee_loan_path: @LoanPath, + assignee_id: ast::node_id) { + move_data.add_assignment(bccx.tcx, + assignee_loan_path, + assignment_id, + assignment_span, + assignee_id); +} + +fn check_is_legal_to_move_from(bccx: @BorrowckCtxt, + cmt0: mc::cmt, + cmt: mc::cmt) -> bool { + match cmt.cat { + mc::cat_stack_upvar(*) | + mc::cat_implicit_self(*) | + mc::cat_copied_upvar(*) | + mc::cat_deref(_, _, mc::region_ptr(*)) | + mc::cat_deref(_, _, mc::gc_ptr(*)) => { + bccx.span_err( + cmt0.span, + fmt!("cannot move out of %s", + bccx.cmt_to_str(cmt))); + false + } + + // It seems strange to allow a move out of a static item, + // but what happens in practice is that you have a + // reference to a constant with a type that should be + // moved, like `None::<~int>`. The type of this constant + // is technically `Option<~int>`, which moves, but we know + // that the content of static items will never actually + // contain allocated pointers, so we can just memcpy it. + // Since static items can never have allocated memory, + // this is ok. For now anyhow. + mc::cat_static_item => { + true + } + + mc::cat_rvalue(*) | + mc::cat_local(*) | + mc::cat_arg(*) | + mc::cat_self(*) | + mc::cat_deref(_, _, mc::unsafe_ptr(*)) => { + true + } + + mc::cat_downcast(b) | + mc::cat_interior(b, _) => { + match ty::get(b.ty).sty { + ty::ty_struct(did, _) | ty::ty_enum(did, _) => { + if ty::has_dtor(bccx.tcx, did) { + bccx.span_err( + cmt0.span, + fmt!("cannot move out of type `%s`, \ + which defines the `Drop` trait", + b.ty.user_string(bccx.tcx))); + false + } else { + check_is_legal_to_move_from(bccx, cmt0, b) + } + } + _ => { + check_is_legal_to_move_from(bccx, cmt0, b) + } + } + } + + mc::cat_deref(b, _, mc::uniq_ptr(*)) | + mc::cat_discr(b, _) => { + check_is_legal_to_move_from(bccx, cmt0, b) + } + } +} + diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index a422d99b6f5cf..893365bdec195 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -19,6 +19,7 @@ use core::prelude::*; use middle::borrowck::*; +use middle::borrowck::move_data::MoveData; use mc = middle::mem_categorization; use middle::pat_util; use middle::ty::{ty_region}; @@ -35,6 +36,7 @@ use syntax::visit; mod lifetime; mod restrictions; +mod gather_moves; /// Context used while gathering loans: /// @@ -65,28 +67,32 @@ mod restrictions; struct GatherLoanCtxt { bccx: @BorrowckCtxt, id_range: id_range, + move_data: @mut move_data::MoveData, all_loans: @mut ~[Loan], item_ub: ast::node_id, repeating_ids: ~[ast::node_id] } pub fn gather_loans(bccx: @BorrowckCtxt, - body: &ast::blk) -> (id_range, @mut ~[Loan]) { + body: &ast::blk) + -> (id_range, @mut ~[Loan], @mut move_data::MoveData) { let glcx = @mut GatherLoanCtxt { bccx: bccx, id_range: id_range::max(), all_loans: @mut ~[], item_ub: body.node.id, - repeating_ids: ~[body.node.id] + repeating_ids: ~[body.node.id], + move_data: @mut MoveData::new() }; let v = visit::mk_vt(@visit::Visitor {visit_expr: gather_loans_in_expr, visit_block: gather_loans_in_block, visit_fn: gather_loans_in_fn, visit_stmt: add_stmt_to_map, visit_pat: add_pat_to_id_range, + visit_local: gather_loans_in_local, .. *visit::default_visitor()}); (v.visit_block)(body, glcx, v); - return (glcx.id_range, glcx.all_loans); + return (glcx.id_range, glcx.all_loans, glcx.move_data); } fn add_pat_to_id_range(p: @ast::pat, @@ -130,6 +136,35 @@ fn gather_loans_in_block(blk: &ast::blk, visit::visit_block(blk, this, vt); } +fn gather_loans_in_local(local: @ast::local, + this: @mut GatherLoanCtxt, + vt: visit::vt<@mut GatherLoanCtxt>) { + if local.node.init.is_none() { + // Variable declarations without initializers are considered "moves": + let tcx = this.bccx.tcx; + do pat_util::pat_bindings(tcx.def_map, local.node.pat) |_, id, span, _| { + gather_moves::gather_decl(this.bccx, + this.move_data, + id, + span, + id); + } + } else { + // Variable declarations with initializers are considered "assigns": + let tcx = this.bccx.tcx; + do pat_util::pat_bindings(tcx.def_map, local.node.pat) |_, id, span, _| { + gather_moves::gather_assignment(this.bccx, + this.move_data, + id, + span, + @LpVar(id), + id); + } + } + + visit::visit_local(local, this, vt); +} + fn gather_loans_in_expr(ex: @ast::expr, this: @mut GatherLoanCtxt, vt: visit::vt<@mut GatherLoanCtxt>) { @@ -147,6 +182,13 @@ fn gather_loans_in_expr(ex: @ast::expr, this.guarantee_adjustments(ex, *adjustments); } + // If this expression is a move, gather it: + if this.bccx.is_move(ex.id) { + let cmt = this.bccx.cat_expr(ex); + gather_moves::gather_move_from_expr( + this.bccx, this.move_data, ex, cmt); + } + // Special checks for various kinds of expressions: match ex.node { ast::expr_addr_of(mutbl, base) => { @@ -159,6 +201,23 @@ fn gather_loans_in_expr(ex: @ast::expr, visit::visit_expr(ex, this, vt); } + ast::expr_assign(l, _) | ast::expr_assign_op(_, l, _) => { + let l_cmt = this.bccx.cat_expr(l); + match opt_loan_path(l_cmt) { + Some(l_lp) => { + gather_moves::gather_assignment(this.bccx, this.move_data, + ex.id, ex.span, + l_lp, l.id); + } + None => { + // This can occur with e.g. `*foo() = 5`. In such + // cases, there is no need to check for conflicts + // with moves etc, just ignore. + } + } + visit::visit_expr(ex, this, vt); + } + ast::expr_match(ex_v, ref arms) => { let cmt = this.bccx.cat_expr(ex_v); for arms.each |arm| { @@ -203,6 +262,11 @@ fn gather_loans_in_expr(ex: @ast::expr, this.pop_repeating_id(body.node.id); } + ast::expr_fn_block(*) => { + gather_moves::gather_captures(this.bccx, this.move_data, ex); + visit::visit_expr(ex, this, vt); + } + _ => { visit::visit_expr(ex, this, vt); } @@ -558,8 +622,11 @@ pub impl GatherLoanCtxt { } } ast::bind_by_copy | ast::bind_infer => { - // Nothing to do here; neither copies nor moves induce - // borrows. + // No borrows here, but there may be moves + if self.bccx.is_move(pat.id) { + gather_moves::gather_move_from_pat( + self.bccx, self.move_data, pat, cmt); + } } } } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 39479e726f8b6..4554fde15fad5 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -19,7 +19,7 @@ use middle::moves; use middle::dataflow::DataFlowContext; use middle::dataflow::DataFlowOperator; use util::common::stmt_set; -use util::ppaux::{note_and_explain_region, Repr}; +use util::ppaux::{note_and_explain_region, Repr, UserString}; use core::hashmap::{HashSet, HashMap}; use core::io; @@ -46,6 +46,8 @@ pub mod check_loans; #[path="gather_loans/mod.rs"] pub mod gather_loans; +pub mod move_data; + pub struct LoanDataFlowOperator; pub type LoanDataFlow = DataFlowContext; @@ -121,21 +123,28 @@ fn borrowck_fn(fk: &visit::fn_kind, debug!("borrowck_fn(id=%?)", id); // Check the body of fn items. - let (id_range, all_loans) = + let (id_range, all_loans, move_data) = gather_loans::gather_loans(this, body); - let all_loans: &~[Loan] = &*all_loans; // FIXME(#5074) - let mut dfcx = + let mut loan_dfcx = DataFlowContext::new(this.tcx, this.method_map, LoanDataFlowOperator, id_range, all_loans.len()); for all_loans.eachi |loan_idx, loan| { - dfcx.add_gen(loan.gen_scope, loan_idx); - dfcx.add_kill(loan.kill_scope, loan_idx); + loan_dfcx.add_gen(loan.gen_scope, loan_idx); + loan_dfcx.add_kill(loan.kill_scope, loan_idx); } - dfcx.propagate(body); - check_loans::check_loans(this, &dfcx, *all_loans, body); + loan_dfcx.propagate(body); + + let flowed_moves = move_data::FlowedMoveData::new(move_data, + this.tcx, + this.method_map, + id_range, + body); + + check_loans::check_loans(this, &loan_dfcx, flowed_moves, + *all_loans, body); } } @@ -226,13 +235,13 @@ pub struct Loan { span: span, } -#[deriving(Eq)] +#[deriving(Eq, IterBytes)] pub enum LoanPath { LpVar(ast::node_id), // `x` in doc.rs LpExtend(@LoanPath, mc::MutabilityCategory, LoanPathElem) } -#[deriving(Eq)] +#[deriving(Eq, IterBytes)] pub enum LoanPathElem { LpDeref, // `*LV` in doc.rs LpInterior(mc::InteriorKind) // `LV.f` in doc.rs @@ -407,6 +416,11 @@ pub enum AliasableViolationKind { BorrowViolation } +pub enum MovedValueUseKind { + MovedInUse, + MovedInCapture, +} + /////////////////////////////////////////////////////////////////////////// // Misc @@ -419,6 +433,10 @@ pub impl BorrowckCtxt { self.tcx.region_maps.is_subscope_of(r_sub, r_sup) } + fn is_move(&self, id: ast::node_id) -> bool { + self.moves_map.contains(&id) + } + fn cat_expr(&self, expr: @ast::expr) -> mc::cmt { mc::cat_expr(self.tcx, self.method_map, expr) } @@ -478,6 +496,83 @@ pub impl BorrowckCtxt { self.note_and_explain_bckerr(err); } + fn report_use_of_moved_value(&self, + use_span: span, + use_kind: MovedValueUseKind, + lp: @LoanPath, + move: &move_data::Move, + moved_lp: @LoanPath) { + let verb = match use_kind { + MovedInUse => "use", + MovedInCapture => "capture", + }; + + match move.kind { + move_data::Declared => { + self.tcx.sess.span_err( + use_span, + fmt!("%s of possibly uninitialized value: `%s`", + verb, + self.loan_path_to_str(lp))); + } + _ => { + let partially = if lp == moved_lp {""} else {"partially "}; + self.tcx.sess.span_err( + use_span, + fmt!("%s of %smoved value: `%s`", + verb, + partially, + self.loan_path_to_str(lp))); + } + } + + match move.kind { + move_data::Declared => {} + + move_data::MoveExpr(expr) => { + let expr_ty = ty::expr_ty_adjusted(self.tcx, expr); + self.tcx.sess.span_note( + expr.span, + fmt!("`%s` moved here because it has type `%s`, \ + which is moved by default (use `copy` to override)", + self.loan_path_to_str(moved_lp), + expr_ty.user_string(self.tcx))); + } + + move_data::MovePat(pat) => { + let pat_ty = ty::node_id_to_type(self.tcx, pat.id); + self.tcx.sess.span_note( + pat.span, + fmt!("`%s` moved here because it has type `%s`, \ + which is moved by default (use `ref` to override)", + self.loan_path_to_str(moved_lp), + pat_ty.user_string(self.tcx))); + } + + move_data::Captured(expr) => { + self.tcx.sess.span_note( + expr.span, + fmt!("`%s` moved into closure environment here \ + because its type is moved by default \ + (make a copy and capture that instead to override)", + self.loan_path_to_str(moved_lp))); + } + } + } + + fn report_reassigned_immutable_variable(&self, + span: span, + lp: @LoanPath, + assign: &move_data::Assignment) { + self.tcx.sess.span_err( + span, + fmt!("re-assignment of immutable variable `%s`", + self.loan_path_to_str(lp))); + self.tcx.sess.span_note( + assign.span, + fmt!("prior assignment occurs here")); + } + fn span_err(&self, s: span, m: &str) { self.tcx.sess.span_err(s, m); } diff --git a/src/librustc/middle/borrowck/move_data.rs b/src/librustc/middle/borrowck/move_data.rs new file mode 100644 index 0000000000000..84bd7ecb1a167 --- /dev/null +++ b/src/librustc/middle/borrowck/move_data.rs @@ -0,0 +1,568 @@ +// 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. + +/*! + +Data structures used for tracking moves. + +*/ + +use core::prelude::*; +use core::hashmap::{HashMap, HashSet}; +use middle::borrowck::*; +use middle::dataflow::DataFlowContext; +use middle::dataflow::DataFlowOperator; +use middle::ty; +use middle::typeck; +use syntax::ast; +use syntax::ast_util; +use syntax::codemap::span; +use syntax::opt_vec::OptVec; +use syntax::opt_vec; +use util::ppaux::Repr; + +pub struct MoveData { + paths: ~[MovePath], + path_map: HashMap<@LoanPath, MovePathIndex>, + moves: ~[Move], + path_assignments: ~[Assignment], + var_assignments: ~[Assignment], + assignee_ids: HashSet, +} + +pub struct FlowedMoveData { + move_data: @mut MoveData, + // ^~~~~~~~~~~~~ + // It makes me sad to use @mut here, except that due to + // the visitor design, this is what gather_loans + // must produce. + dfcx_moves: MoveDataFlow, + dfcx_assign: AssignDataFlow +} + +#[deriving(Eq)] +pub struct MovePathIndex(uint); + +static InvalidMovePathIndex: MovePathIndex = + MovePathIndex(uint::max_value); + +#[deriving(Eq)] +pub struct MoveIndex(uint); + +static InvalidMoveIndex: MoveIndex = + MoveIndex(uint::max_value); + +#[deriving(Eq)] +pub struct VarAssignmentIndex(uint); + +static InvalidVarAssignmentIndex: VarAssignmentIndex = + VarAssignmentIndex(uint::max_value); + +pub struct MovePath { + index: MovePathIndex, + loan_path: @LoanPath, + parent: MovePathIndex, + first_move: MoveIndex, + first_child: MovePathIndex, + next_sibling: MovePathIndex, +} + +pub enum MoveKind { + Declared, // When declared, variables start out "moved". + MoveExpr(@ast::expr), // Expression or binding that moves a variable + MovePat(@ast::pat), // By-move binding + Captured(@ast::expr), // Closure creation that moves a value +} + +pub struct Move { + path: MovePathIndex, + id: ast::node_id, + kind: MoveKind, + next_move: MoveIndex, +} + +pub struct Assignment { + path: MovePathIndex, + id: ast::node_id, + span: span, +} + +pub struct MoveDataFlowOperator; +pub type MoveDataFlow = DataFlowContext; + +pub struct AssignDataFlowOperator; +pub type AssignDataFlow = DataFlowContext; + +impl MoveData { + pub fn new() -> MoveData { + MoveData { + paths: ~[], + path_map: HashMap::new(), + moves: ~[], + path_assignments: ~[], + var_assignments: ~[], + assignee_ids: HashSet::new(), + } + } + + fn path<'a>(&'a self, index: MovePathIndex) -> &'a MovePath { + //! Type safe indexing operator + &self.paths[*index] + } + + fn mut_path<'a>(&'a mut self, index: MovePathIndex) -> &'a mut MovePath { + //! Type safe indexing operator + &mut self.paths[*index] + } + + fn move<'a>(&'a self, index: MoveIndex) -> &'a Move { + //! Type safe indexing operator + &self.moves[*index] + } + + fn is_var_path(&self, index: MovePathIndex) -> bool { + //! True if `index` refers to a variable + self.path(index).parent == InvalidMovePathIndex + } + + pub fn move_path(&mut self, + tcx: ty::ctxt, + lp: @LoanPath) -> MovePathIndex { + /*! + * Returns the existing move path index for `lp`, if any, + * and otherwise adds a new index for `lp` and any of its + * base paths that do not yet have an index. + */ + + match self.path_map.find(&lp) { + Some(&index) => { + return index; + } + None => {} + } + + let index = match *lp { + LpVar(*) => { + let index = MovePathIndex(self.paths.len()); + + self.paths.push(MovePath { + index: index, + loan_path: lp, + parent: InvalidMovePathIndex, + first_move: InvalidMoveIndex, + first_child: InvalidMovePathIndex, + next_sibling: InvalidMovePathIndex, + }); + + index + } + + LpExtend(base, _, _) => { + let parent_index = self.move_path(tcx, base); + let index = MovePathIndex(self.paths.len()); + + let next_sibling = self.path(parent_index).first_child; + self.mut_path(parent_index).first_child = index; + + self.paths.push(MovePath { + index: index, + loan_path: lp, + parent: parent_index, + first_move: InvalidMoveIndex, + first_child: InvalidMovePathIndex, + next_sibling: next_sibling, + }); + + index + } + }; + + debug!("move_path(lp=%s, index=%?)", + lp.repr(tcx), + index); + + assert_eq!(*index, self.paths.len() - 1); + self.path_map.insert(lp, index); + return index; + } + + fn existing_move_path(&self, + lp: @LoanPath) + -> Option { + self.path_map.find_copy(&lp) + } + + fn existing_base_paths(&self, + lp: @LoanPath) + -> OptVec { + let mut result = opt_vec::Empty; + self.add_existing_base_paths(lp, &mut result); + result + } + + fn add_existing_base_paths(&self, + lp: @LoanPath, + result: &mut OptVec) { + /*! + * Adds any existing move path indices for `lp` and any base + * paths of `lp` to `result`, but does not add new move paths + */ + + match self.path_map.find_copy(&lp) { + Some(index) => { + for self.each_base_path(index) |p| { + result.push(p); + } + } + None => { + match *lp { + LpVar(*) => { } + LpExtend(b, _, _) => { + self.add_existing_base_paths(b, result); + } + } + } + } + + } + + pub fn add_move(&mut self, + tcx: ty::ctxt, + lp: @LoanPath, + id: ast::node_id, + kind: MoveKind) { + /*! + * Adds a new move entry for a move of `lp` that occurs at + * location `id` with kind `kind`. + */ + + debug!("add_move(lp=%s, id=%?, kind=%?)", + lp.repr(tcx), + id, + kind); + + let path_index = self.move_path(tcx, lp); + let move_index = MoveIndex(self.moves.len()); + + let next_move = self.path(path_index).first_move; + self.mut_path(path_index).first_move = move_index; + + self.moves.push(Move { + path: path_index, + id: id, + kind: kind, + next_move: next_move + }); + } + + pub fn add_assignment(&mut self, + tcx: ty::ctxt, + lp: @LoanPath, + assign_id: ast::node_id, + span: span, + assignee_id: ast::node_id) { + /*! + * Adds a new record for an assignment to `lp` that occurs at + * location `id` with the given `span`. + */ + + debug!("add_assignment(lp=%s, assign_id=%?, assignee_id=%?", + lp.repr(tcx), assign_id, assignee_id); + + let path_index = self.move_path(tcx, lp); + + self.assignee_ids.insert(assignee_id); + + let assignment = Assignment { + path: path_index, + id: assign_id, + span: span, + }; + + if self.is_var_path(path_index) { + debug!("add_assignment[var](lp=%s, assignment=%u, path_index=%?)", + lp.repr(tcx), self.var_assignments.len(), path_index); + + self.var_assignments.push(assignment); + } else { + debug!("add_assignment[path](lp=%s, path_index=%?)", + lp.repr(tcx), path_index); + + self.path_assignments.push(assignment); + } + } + + fn add_gen_kills(&self, + tcx: ty::ctxt, + dfcx_moves: &mut MoveDataFlow, + dfcx_assign: &mut AssignDataFlow) { + /*! + * Adds the gen/kills for the various moves and + * assignments into the provided data flow contexts. + * Moves are generated by moves and killed by assignments and + * scoping. Assignments are generated by assignment to variables and + * killed by scoping. See `doc.rs` for more details. + */ + + for self.moves.eachi |i, move| { + dfcx_moves.add_gen(move.id, i); + } + + for self.var_assignments.eachi |i, assignment| { + dfcx_assign.add_gen(assignment.id, i); + self.kill_moves(assignment.path, assignment.id, dfcx_moves); + } + + for self.path_assignments.each |assignment| { + self.kill_moves(assignment.path, assignment.id, dfcx_moves); + } + + // Kill all moves related to a variable `x` when it goes out + // of scope: + for self.paths.each |path| { + match *path.loan_path { + LpVar(id) => { + let kill_id = tcx.region_maps.encl_scope(id); + let path = *self.path_map.get(&path.loan_path); + self.kill_moves(path, kill_id, dfcx_moves); + } + LpExtend(*) => {} + } + } + + // Kill all assignments when the variable goes out of scope: + for self.var_assignments.eachi |assignment_index, assignment| { + match *self.path(assignment.path).loan_path { + LpVar(id) => { + let kill_id = tcx.region_maps.encl_scope(id); + dfcx_assign.add_kill(kill_id, assignment_index); + } + LpExtend(*) => { + tcx.sess.bug("Var assignment for non var path"); + } + } + } + } + + fn each_base_path(&self, + index: MovePathIndex, + f: &fn(MovePathIndex) -> bool) + -> bool { + let mut p = index; + while p != InvalidMovePathIndex { + if !f(p) { + return false; + } + p = self.path(p).parent; + } + return true; + } + + fn each_extending_path(&self, + index: MovePathIndex, + f: &fn(MovePathIndex) -> bool) -> bool { + if !f(index) { + return false; + } + + let mut p = self.path(index).first_child; + while p != InvalidMovePathIndex { + if !self.each_extending_path(p, f) { + return false; + } + p = self.path(p).next_sibling; + } + + return true; + } + + fn each_applicable_move(&self, + index0: MovePathIndex, + f: &fn(MoveIndex) -> bool) -> bool { + for self.each_extending_path(index0) |index| { + let mut p = self.path(index).first_move; + while p != InvalidMoveIndex { + if !f(p) { + return false; + } + p = self.move(p).next_move; + } + } + return true; + } + + fn kill_moves(&self, + path: MovePathIndex, + kill_id: ast::node_id, + dfcx_moves: &mut MoveDataFlow) { + for self.each_applicable_move(path) |move_index| { + dfcx_moves.add_kill(kill_id, *move_index); + } + } +} + +impl FlowedMoveData { + pub fn new(move_data: @mut MoveData, + tcx: ty::ctxt, + method_map: typeck::method_map, + id_range: ast_util::id_range, + body: &ast::blk) + -> FlowedMoveData + { + let mut dfcx_moves = + DataFlowContext::new(tcx, + method_map, + MoveDataFlowOperator, + id_range, + move_data.moves.len()); + let mut dfcx_assign = + DataFlowContext::new(tcx, + method_map, + AssignDataFlowOperator, + id_range, + move_data.var_assignments.len()); + move_data.add_gen_kills(tcx, &mut dfcx_moves, &mut dfcx_assign); + dfcx_moves.propagate(body); + dfcx_assign.propagate(body); + FlowedMoveData { + move_data: move_data, + dfcx_moves: dfcx_moves, + dfcx_assign: dfcx_assign, + } + } + + pub fn each_move_of(&self, + id: ast::node_id, + loan_path: @LoanPath, + f: &fn(&Move, @LoanPath) -> bool) + -> bool { + /*! + * Iterates through each move of `loan_path` (or some base path + * of `loan_path`) that *may* have occurred on entry to `id` without + * an intervening assignment. In other words, any moves that + * would invalidate a reference to `loan_path` at location `id`. + */ + + // Bad scenarios: + // + // 1. Move of `a.b.c`, use of `a.b.c` + // 2. Move of `a.b.c`, use of `a.b.c.d` + // 3. Move of `a.b.c`, use of `a` or `a.b` + // + // OK scenario: + // + // 4. move of `a.b.c`, use of `a.b.d` + + let base_indices = self.move_data.existing_base_paths(loan_path); + if base_indices.is_empty() { + return true; + } + + let opt_loan_path_index = self.move_data.existing_move_path(loan_path); + + for self.dfcx_moves.each_bit_on_entry(id) |index| { + let move = &self.move_data.moves[index]; + let moved_path = move.path; + if base_indices.contains(&moved_path) { + // Scenario 1 or 2: `loan_path` or some base path of + // `loan_path` was moved. + if !f(move, self.move_data.path(moved_path).loan_path) { + return false; + } + loop; + } + + for opt_loan_path_index.each |&loan_path_index| { + for self.move_data.each_base_path(moved_path) |p| { + if p == loan_path_index { + // Scenario 3: some extension of `loan_path` + // was moved + if !f(move, self.move_data.path(moved_path).loan_path) { + return false; + } + } + } + } + } + return true; + } + + pub fn is_assignee(&self, + id: ast::node_id) + -> bool { + //! True if `id` is the id of the LHS of an assignment + + self.move_data.assignee_ids.contains(&id) + } + + pub fn each_assignment_of(&self, + id: ast::node_id, + loan_path: @LoanPath, + f: &fn(&Assignment) -> bool) + -> bool { + /*! + * Iterates through every assignment to `loan_path` that + * may have occurred on entry to `id`. `loan_path` must be + * a single variable. + */ + + let loan_path_index = { + match self.move_data.existing_move_path(loan_path) { + Some(i) => i, + None => { + // if there were any assignments, it'd have an index + return true; + } + } + }; + + for self.dfcx_assign.each_bit_on_entry(id) |index| { + let assignment = &self.move_data.var_assignments[index]; + if assignment.path == loan_path_index && !f(assignment) { + return false; + } + } + return true; + } +} + +impl DataFlowOperator for MoveDataFlowOperator { + #[inline(always)] + fn initial_value(&self) -> bool { + false // no loans in scope by default + } + + #[inline(always)] + fn join(&self, succ: uint, pred: uint) -> uint { + succ | pred // moves from both preds are in scope + } + + #[inline(always)] + fn walk_closures(&self) -> bool { + true + } +} + +impl DataFlowOperator for AssignDataFlowOperator { + #[inline(always)] + fn initial_value(&self) -> bool { + false // no assignments in scope by default + } + + #[inline(always)] + fn join(&self, succ: uint, pred: uint) -> uint { + succ | pred // moves from both preds are in scope + } + + #[inline(always)] + fn walk_closures(&self) -> bool { + true + } +} diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 803fdc4ed5d70..748ae83a60c57 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -866,7 +866,7 @@ pub fn check_legality_of_move_bindings(cx: @MatchCheckCtxt, if !any_by_move { return; } // pointless micro-optimization for pats.each |pat| { - do walk_pat(*pat) |p| { + for walk_pat(*pat) |p| { if pat_is_binding(def_map, p) { match p.node { pat_ident(_, _, sub) => { @@ -884,66 +884,5 @@ pub fn check_legality_of_move_bindings(cx: @MatchCheckCtxt, } } } - - // Now check to ensure that any move binding is not behind an - // @ or &, or within a struct with a destructor. This is - // always illegal. - let vt = visit::mk_vt(@visit::Visitor { - visit_pat: |pat, (behind_bad_pointer, behind_dtor_struct): (bool, bool), v| { - match pat.node { - pat_ident(_, _, sub) => { - debug!("(check legality of move) checking pat \ - ident with behind_bad_pointer %? and behind_dtor_struct %?", - behind_bad_pointer, behind_dtor_struct); - - if behind_bad_pointer || behind_dtor_struct && - cx.moves_map.contains(&pat.id) - { - let msg = if behind_bad_pointer { - "by-move pattern bindings may not occur behind @ or & bindings" - } else { - "cannot bind by-move within struct (it has a destructor)" - }; - cx.tcx.sess.span_err(pat.span, msg); - } - - match sub { - None => {} - Some(subpat) => { - (v.visit_pat)(subpat, - (behind_bad_pointer, behind_dtor_struct), - v); - } - } - } - - pat_box(subpat) | pat_region(subpat) => { - (v.visit_pat)(subpat, (true, behind_dtor_struct), v); - } - - pat_struct(_, ref fields, _) => { - let behind_dtor_struct = behind_dtor_struct || - (match cx.tcx.def_map.find(&pat.id) { - Some(&def_struct(id)) => { - ty::has_dtor(cx.tcx, id) - } - _ => false - }); - debug!("(check legality of move) checking pat \ - struct with behind_bad_pointer %? and behind_dtor_struct %?", - behind_bad_pointer, behind_dtor_struct); - - for fields.each |fld| { - (v.visit_pat)(fld.pat, (behind_bad_pointer, - behind_dtor_struct), v) - } - } - - _ => visit::visit_pat(pat, (behind_bad_pointer, behind_dtor_struct), v) - } - }, - .. *visit::default_visitor::<(bool, bool)>() - }); - (vt.visit_pat)(*pat, (false, false), vt); } } diff --git a/src/librustc/middle/dataflow.rs b/src/librustc/middle/dataflow.rs index 5898b6a5e4d15..e0806359c5d09 100644 --- a/src/librustc/middle/dataflow.rs +++ b/src/librustc/middle/dataflow.rs @@ -651,10 +651,10 @@ impl<'self, O:DataFlowOperator> PropagationContext<'self, O> { } ast::expr_struct(_, ref fields, with_expr) => { - self.walk_opt_expr(with_expr, in_out, loop_scopes); for fields.each |field| { self.walk_expr(field.node.expr, in_out, loop_scopes); } + self.walk_opt_expr(with_expr, in_out, loop_scopes); } ast::expr_call(f, ref args, _) => { @@ -826,7 +826,7 @@ impl<'self, O:DataFlowOperator> PropagationContext<'self, O> { debug!("DataFlowContext::walk_pat(pat=%s, in_out=%s)", pat.repr(self.dfcx.tcx), bits_to_str(reslice(in_out))); - do ast_util::walk_pat(pat) |p| { + for ast_util::walk_pat(pat) |p| { debug!(" p.id=%? in_out=%s", p.id, bits_to_str(reslice(in_out))); self.merge_with_entry_set(p.id, in_out); self.dfcx.apply_gen_kill(p.id, in_out); diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs index e4b93468c2938..4897d6c87dec1 100644 --- a/src/librustc/middle/liveness.rs +++ b/src/librustc/middle/liveness.rs @@ -109,7 +109,6 @@ use middle::pat_util; use middle::ty; use middle::typeck; use middle::moves; -use util::ppaux::ty_to_str; use core::cast::transmute; use core::hashmap::HashMap; @@ -146,7 +145,6 @@ fn live_node_kind_to_str(lnk: LiveNodeKind, cx: ty::ctxt) -> ~str { pub fn check_crate(tcx: ty::ctxt, method_map: typeck::method_map, - variable_moves_map: moves::VariableMovesMap, capture_map: moves::CaptureMap, crate: @crate) { let visitor = visit::mk_vt(@visit::Visitor { @@ -159,7 +157,6 @@ pub fn check_crate(tcx: ty::ctxt, let initial_maps = @mut IrMaps(tcx, method_map, - variable_moves_map, capture_map); visit::visit_crate(crate, initial_maps, visitor); tcx.sess.abort_if_errors(); @@ -229,7 +226,6 @@ enum VarKind { struct IrMaps { tcx: ty::ctxt, method_map: typeck::method_map, - variable_moves_map: moves::VariableMovesMap, capture_map: moves::CaptureMap, num_live_nodes: uint, @@ -243,13 +239,11 @@ struct IrMaps { fn IrMaps(tcx: ty::ctxt, method_map: typeck::method_map, - variable_moves_map: moves::VariableMovesMap, capture_map: moves::CaptureMap) -> IrMaps { IrMaps { tcx: tcx, method_map: method_map, - variable_moves_map: variable_moves_map, capture_map: capture_map, num_live_nodes: 0, num_vars: 0, @@ -349,7 +343,6 @@ fn visit_fn(fk: &visit::fn_kind, // swap in a new set of IR maps for this function body: let fn_maps = @mut IrMaps(this.tcx, this.method_map, - this.variable_moves_map, this.capture_map); unsafe { @@ -1399,11 +1392,7 @@ pub impl Liveness { fn check_local(local: @local, this: @Liveness, vt: vt<@Liveness>) { match local.node.init { Some(_) => { - - // Initializer: this.warn_about_unused_or_dead_vars_in_pat(local.node.pat); - this.check_for_reassignments_in_pat(local.node.pat, - local.node.is_mutbl); } None => { @@ -1438,35 +1427,6 @@ fn check_arm(arm: &arm, this: @Liveness, vt: vt<@Liveness>) { fn check_expr(expr: @expr, this: @Liveness, vt: vt<@Liveness>) { match expr.node { - expr_path(_) | expr_self => { - for this.variable_from_def_map(expr.id, expr.span).each |var| { - let ln = this.live_node(expr.id, expr.span); - - match this.ir.variable_moves_map.find(&expr.id) { - None => {} - Some(&entire_expr) => { - debug!("(checking expr) is a move: `%s`", - expr_to_str(expr, this.tcx.sess.intr())); - this.check_move_from_var(ln, *var, entire_expr); - } - } - } - - visit::visit_expr(expr, this, vt); - } - - expr_fn_block(*) => { - let caps = this.ir.captures(expr); - for caps.each |cap| { - let var = this.variable(cap.var_nid, expr.span); - if cap.is_move { - this.check_move_from_var(cap.ln, var, expr); - } - } - - visit::visit_expr(expr, this, vt); - } - expr_assign(l, r) => { this.check_lvalue(l, vt); (vt.visit_expr)(r, this, vt); @@ -1507,7 +1467,7 @@ fn check_expr(expr: @expr, this: @Liveness, vt: vt<@Liveness>) { expr_cast(*) | expr_unary(*) | expr_ret(*) | expr_break(*) | expr_again(*) | expr_lit(_) | expr_block(*) | expr_mac(*) | expr_addr_of(*) | expr_struct(*) | expr_repeat(*) | - expr_paren(*) => { + expr_paren(*) | expr_fn_block(*) | expr_path(*) | expr_self(*) => { visit::visit_expr(expr, this, vt); } } @@ -1547,43 +1507,17 @@ pub impl Liveness { } } - fn check_move_from_var(&self, - ln: LiveNode, - var: Variable, - move_expr: @expr) { - /*! - * Checks whether `var` is live on entry to any of the - * successors of `ln`. If it is, report an error. - * `move_expr` is the expression which caused the variable - * to be moved. - * - * Note that `move_expr` is not necessarily a reference to the - * variable. It might be an expression like `x.f` which could - * cause a move of the variable `x`, or a closure creation. - */ - - debug!("check_move_from_var(%s, %s)", - ln.to_str(), var.to_str()); - - match self.live_on_exit(ln, var) { - None => {} - Some(lnk) => self.report_illegal_move(lnk, var, move_expr) - } - } - fn check_lvalue(@self, expr: @expr, vt: vt<@Liveness>) { match expr.node { expr_path(_) => { match self.tcx.def_map.get_copy(&expr.id) { - def_local(nid, mutbl) => { + def_local(nid, _) => { // Assignment to an immutable variable or argument: only legal // if there is no later assignment. If this local is actually // mutable, then check for a reassignment to flag the mutability // as being used. let ln = self.live_node(expr.id, expr.span); let var = self.variable(nid, expr.span); - self.check_for_reassignment(ln, var, expr.span, - if mutbl {Some(nid)} else {None}); self.warn_about_dead_assign(expr.span, expr.id, ln, var); } def => { @@ -1607,118 +1541,6 @@ pub impl Liveness { } } - fn check_for_reassignments_in_pat(&self, pat: @pat, mutbl: bool) { - do self.pat_bindings(pat) |ln, var, sp, id| { - self.check_for_reassignment(ln, var, sp, - if mutbl {Some(id)} else {None}); - } - } - - fn check_for_reassignment(&self, ln: LiveNode, var: Variable, - orig_span: span, mutbl: Option) { - match self.assigned_on_exit(ln, var) { - Some(ExprNode(span)) => { - match mutbl { - Some(id) => { self.tcx.used_mut_nodes.insert(id); } - None => { - self.tcx.sess.span_err( - span, - "re-assignment of immutable variable"); - self.tcx.sess.span_note( - orig_span, - "prior assignment occurs here"); - } - } - } - Some(lnk) => { - self.tcx.sess.span_bug( - orig_span, - fmt!("illegal writer: %?", lnk)); - } - None => {} - } - } - - fn report_illegal_move(&self, lnk: LiveNodeKind, - var: Variable, - move_expr: @expr) { - // the only time that it is possible to have a moved variable - // used by ExitNode would be arguments or fields in a ctor. - // we give a slightly different error message in those cases. - if lnk == ExitNode { - // FIXME #4715: this seems like it should be reported in the - // borrow checker - let vk = self.ir.var_kinds[*var]; - match vk { - Arg(_, name) => { - self.tcx.sess.span_err( - move_expr.span, - fmt!("illegal move from argument `%s`, which is not \ - copy or move mode", *self.tcx.sess.str_of(name))); - return; - } - Local(*) | ImplicitRet => { - self.tcx.sess.span_bug( - move_expr.span, - fmt!("illegal reader (%?) for `%?`", - lnk, vk)); - } - } - } - - match move_expr.node { - expr_fn_block(*) => { - self.report_illegal_read( - move_expr.span, lnk, var, MovedValue); - let name = self.ir.variable_name(var); - self.tcx.sess.span_note( - move_expr.span, - fmt!("`%s` moved into closure environment here \ - because its type is moved by default", - *name)); - } - expr_path(*) => { - self.report_illegal_read( - move_expr.span, lnk, var, MovedValue); - self.report_move_location( - move_expr, var, "", "it"); - } - expr_field(*) => { - self.report_illegal_read( - move_expr.span, lnk, var, PartiallyMovedValue); - self.report_move_location( - move_expr, var, "field of ", "the field"); - } - expr_index(*) => { - self.report_illegal_read( - move_expr.span, lnk, var, PartiallyMovedValue); - self.report_move_location( - move_expr, var, "element of ", "the element"); - } - _ => { - self.report_illegal_read( - move_expr.span, lnk, var, PartiallyMovedValue); - self.report_move_location( - move_expr, var, "subcomponent of ", "the subcomponent"); - } - }; - } - - fn report_move_location(&self, - move_expr: @expr, - var: Variable, - expr_descr: &str, - pronoun: &str) { - let move_expr_ty = ty::expr_ty(self.tcx, move_expr); - let name = self.ir.variable_name(var); - self.tcx.sess.span_note( - move_expr.span, - fmt!("%s`%s` moved here because %s has type %s, \ - which is moved by default (use `copy` to override)", - expr_descr, *name, pronoun, - ty_to_str(self.tcx, move_expr_ty))); - } - fn report_illegal_read(&self, chk_span: span, lnk: LiveNodeKind, diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 52c7bf0a21e7d..0d33555974724 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -93,19 +93,26 @@ pub enum ptr_kind { // We use the term "interior" to mean "something reachable from the // base without a pointer dereference", e.g. a field -#[deriving(Eq)] +#[deriving(Eq, IterBytes)] pub enum InteriorKind { InteriorField(FieldName), - InteriorElement(ty::t), // ty::t is the type of the vec/str + InteriorElement(ElementKind), } -#[deriving(Eq)] +#[deriving(Eq, IterBytes)] pub enum FieldName { NamedField(ast::ident), PositionalField(uint) } -#[deriving(Eq)] +#[deriving(Eq, IterBytes)] +pub enum ElementKind { + VecElement, + StrElement, + OtherElement, +} + +#[deriving(Eq, IterBytes)] pub enum MutabilityCategory { McImmutable, // Immutable. McReadOnly, // Read-only (`const`) @@ -192,7 +199,7 @@ pub fn opt_deref_kind(t: ty::t) -> Option { ty::ty_evec(_, ty::vstore_fixed(_)) | ty::ty_estr(ty::vstore_fixed(_)) => { - Some(deref_interior(InteriorElement(t))) + Some(deref_interior(InteriorElement(element_kind(t)))) } _ => None @@ -749,7 +756,7 @@ pub impl mem_categorization_ctxt { @cmt_ { id:elt.id(), span:elt.span(), - cat:cat_interior(of_cmt, InteriorElement(vec_ty)), + cat:cat_interior(of_cmt, InteriorElement(element_kind(vec_ty))), mutbl:mutbl, ty:mt.ty } @@ -993,12 +1000,14 @@ pub impl mem_categorization_ctxt { cat_interior(_, InteriorField(PositionalField(_))) => { ~"anonymous field" } - cat_interior(_, InteriorElement(t)) => { - match ty::get(t).sty { - ty::ty_evec(*) => ~"vec content", - ty::ty_estr(*) => ~"str content", - _ => ~"indexed content" - } + cat_interior(_, InteriorElement(VecElement)) => { + ~"vec content" + } + cat_interior(_, InteriorElement(StrElement)) => { + ~"str content" + } + cat_interior(_, InteriorElement(OtherElement)) => { + ~"indexed content" } cat_stack_upvar(_) => { ~"captured outer variable" @@ -1193,3 +1202,11 @@ impl Repr for InteriorKind { } } } + +fn element_kind(t: ty::t) -> ElementKind { + match ty::get(t).sty { + ty::ty_evec(*) => VecElement, + ty::ty_estr(*) => StrElement, + _ => OtherElement + } +} diff --git a/src/librustc/middle/moves.rs b/src/librustc/middle/moves.rs index 3b20344b3ead3..159f7707dd383 100644 --- a/src/librustc/middle/moves.rs +++ b/src/librustc/middle/moves.rs @@ -88,32 +88,32 @@ Similar reasoning can be applied to `let` expressions: ## Output -The pass results in the struct `MoveMaps` which contains two sets, -`moves_map` and `variable_moves_map`, and one map, `capture_map`. - -`moves_map` is a set containing the id of every *outermost -expression* or *binding* that is moved. Note that `moves_map` only -contains the *outermost expressions* that are moved. Therefore, if -you have a use of `x.b`, as in the example `y` above, the -expression `x.b` would be in the `moves_map` but not `x`. The -reason for this is that, for most purposes, it's only the outermost -expression that is needed. The borrow checker and trans, for -example, only care about the outermost expressions that are moved. -It is more efficient therefore just to store those entries. - -In the case of the liveness pass, however, we need to know which -*variable references* are moved (see the Enforcement of Moves -section below for more details). That is, for the `x.b` -expression, liveness only cares about the `x`. For this purpose, -we have a second map, `variable_moves_map`, that contains the ids -of all variable references which is moved. - -The `capture_map` maps from the node_id of a closure expression to an -array of `CaptureVar` structs detailing which variables are captured -and how (by ref, by copy, by move). +The pass results in the struct `MoveMaps` which contains several +maps: + +`moves_map` is a set containing the id of every *outermost expression* or +*binding* that causes a move. Note that `moves_map` only contains the *outermost +expressions* that are moved. Therefore, if you have a use of `x.b`, +as in the example `y` above, the expression `x.b` would be in the +`moves_map` but not `x`. The reason for this is that, for most +purposes, it's only the outermost expression that is needed. The +borrow checker and trans, for example, only care about the outermost +expressions that are moved. It is more efficient therefore just to +store those entries. + +Sometimes though we want to know the variables that are moved (in +particular in the borrow checker). For these cases, the set +`moved_variables_set` just collects the ids of variables that are +moved. + +Finally, the `capture_map` maps from the node_id of a closure +expression to an array of `CaptureVar` structs detailing which +variables are captured and how (by ref, by copy, by move). ## Enforcement of Moves +FIXME out of date + The enforcement of moves is somewhat complicated because it is divided amongst the liveness and borrowck modules. In general, the borrow checker is responsible for guaranteeing that *only owned data is @@ -136,12 +136,8 @@ invalidated. In more concrete terms, the `moves_map` generated from this example would contain both the expression `x.b` (1) and the expression `x` (2). Note that it would not contain `x` (1), because `moves_map` only -contains the outermost expressions that are moved. However, -`moves_map` is not used by liveness. It uses the -`variable_moves_map`, which would contain both references to `x`: (1) -and (2). Therefore, after computing which variables are live where, -liveness will see that the reference (1) to `x` is both present in -`variable_moves_map` and that `x` is live and report an error. +contains the outermost expressions that are moved. However, the id of +`x` would be present in the `moved_variables_set`. Now let's look at another illegal example, but one where liveness would not catch the error: @@ -213,6 +209,7 @@ use middle::freevars; use middle::ty; use middle::typeck::{method_map}; use util::ppaux; +use util::ppaux::Repr; use util::common::indenter; use core::hashmap::{HashSet, HashMap}; @@ -220,7 +217,6 @@ use syntax::ast::*; use syntax::ast_util; use syntax::visit; use syntax::visit::vt; -use syntax::print::pprust; use syntax::codemap::span; #[deriving(Encodable, Decodable)] @@ -241,11 +237,6 @@ pub type CaptureMap = @mut HashMap; pub type MovesMap = @mut HashSet; -/** - * For each variable which will be moved, links to the - * expression */ -pub type VariableMovesMap = @mut HashMap; - /** * Set of variable node-ids that are moved. * @@ -257,7 +248,6 @@ pub type MovedVariablesSet = @mut HashSet; /** See the section Output on the module comment for explanation. */ pub struct MoveMaps { moves_map: MovesMap, - variable_moves_map: VariableMovesMap, moved_variables_set: MovedVariablesSet, capture_map: CaptureMap } @@ -269,9 +259,8 @@ struct VisitContext { } enum UseMode { - MoveInWhole, // Move the entire value. - MoveInPart(@expr), // Some subcomponent will be moved - Read // Read no matter what the type. + Move, // This value or something owned by it is moved. + Read // Read no matter what the type. } pub fn compute_moves(tcx: ty::ctxt, @@ -287,7 +276,6 @@ pub fn compute_moves(tcx: ty::ctxt, method_map: method_map, move_maps: MoveMaps { moves_map: @mut HashSet::new(), - variable_moves_map: @mut HashMap::new(), capture_map: @mut HashMap::new(), moved_variables_set: @mut HashSet::new() } @@ -317,21 +305,6 @@ fn compute_modes_for_expr(expr: @expr, cx.consume_expr(expr, v); } -pub impl UseMode { - fn component_mode(&self, expr: @expr) -> UseMode { - /*! - * - * Assuming that `self` is the mode for an expression E, - * returns the appropriate mode to use for a subexpression of E. - */ - - match *self { - Read | MoveInPart(_) => *self, - MoveInWhole => MoveInPart(expr) - } - } -} - pub impl VisitContext { fn consume_exprs(&self, exprs: &[@expr], @@ -347,18 +320,20 @@ pub impl VisitContext { visitor: vt) { /*! - * * Indicates that the value of `expr` will be consumed, * meaning either copied or moved depending on its type. */ - debug!("consume_expr(expr=%?/%s)", - expr.id, - pprust::expr_to_str(expr, self.tcx.sess.intr())); + debug!("consume_expr(expr=%s)", + expr.repr(self.tcx)); let expr_ty = ty::expr_ty_adjusted(self.tcx, expr); - let mode = self.consume_mode_for_ty(expr_ty); - self.use_expr(expr, mode, visitor); + if ty::type_moves_by_default(self.tcx, expr_ty) { + self.move_maps.moves_map.insert(expr.id); + self.use_expr(expr, Move, visitor); + } else { + self.use_expr(expr, Read, visitor); + }; } fn consume_block(&self, @@ -366,7 +341,6 @@ pub impl VisitContext { visitor: vt) { /*! - * * Indicates that the value of `blk` will be consumed, * meaning either copied or moved depending on its type. */ @@ -382,46 +356,20 @@ pub impl VisitContext { } } - fn consume_mode_for_ty(&self, ty: ty::t) -> UseMode { - /*! - * - * Selects the appropriate `UseMode` to consume a value with - * the type `ty`. This will be `MoveEntireMode` if `ty` is - * not implicitly copyable. - */ - - let result = if ty::type_moves_by_default(self.tcx, ty) { - MoveInWhole - } else { - Read - }; - - debug!("consume_mode_for_ty(ty=%s) = %?", - ppaux::ty_to_str(self.tcx, ty), result); - - return result; - } - fn use_expr(&self, expr: @expr, expr_mode: UseMode, visitor: vt) { /*! - * * Indicates that `expr` is used with a given mode. This will * in turn trigger calls to the subcomponents of `expr`. */ - debug!("use_expr(expr=%?/%s, mode=%?)", - expr.id, pprust::expr_to_str(expr, self.tcx.sess.intr()), + debug!("use_expr(expr=%s, mode=%?)", + expr.repr(self.tcx), expr_mode); - match expr_mode { - MoveInWhole => { self.move_maps.moves_map.insert(expr.id); } - MoveInPart(_) | Read => {} - } - // `expr_mode` refers to the post-adjustment value. If one of // those adjustments is to take a reference, then it's only // reading the underlying expression, not moving it. @@ -429,7 +377,7 @@ pub impl VisitContext { Some(&@ty::AutoDerefRef( ty::AutoDerefRef { autoref: Some(_), _})) => Read, - _ => expr_mode.component_mode(expr) + _ => expr_mode }; debug!("comp_mode = %?", comp_mode); @@ -437,21 +385,13 @@ pub impl VisitContext { match expr.node { expr_path(*) | expr_self => { match comp_mode { - MoveInPart(entire_expr) => { - self.move_maps.variable_moves_map.insert( - expr.id, entire_expr); - + Move => { let def = self.tcx.def_map.get_copy(&expr.id); for moved_variable_node_id_from_def(def).each |&id| { self.move_maps.moved_variables_set.insert(id); } } Read => {} - MoveInWhole => { - self.tcx.sess.span_bug( - expr.span, - "Component mode can never be MoveInWhole"); - } } } @@ -546,19 +486,10 @@ pub impl VisitContext { self.consume_arm(arm, visitor); } - let by_move_bindings_present = - self.arms_have_by_move_bindings( - self.move_maps.moves_map, *arms); - - if by_move_bindings_present { - // If one of the arms moves a value out of the - // discriminant, then the discriminant itself is - // moved. - self.consume_expr(discr, visitor); - } else { - // Otherwise, the discriminant is merely read. - self.use_expr(discr, Read, visitor); - } + // The discriminant may, in fact, be partially moved + // if there are by-move bindings, but borrowck deals + // with that itself. + self.use_expr(discr, Read, visitor); } expr_copy(base) => { @@ -719,18 +650,17 @@ pub impl VisitContext { */ do pat_bindings(self.tcx.def_map, pat) |bm, id, _span, _path| { - let mode = match bm { - bind_by_copy => Read, - bind_by_ref(_) => Read, + let binding_moves = match bm { + bind_by_copy => false, + bind_by_ref(_) => false, bind_infer => { let pat_ty = ty::node_id_to_type(self.tcx, id); - self.consume_mode_for_ty(pat_ty) + ty::type_moves_by_default(self.tcx, pat_ty) } }; - match mode { - MoveInWhole => { self.move_maps.moves_map.insert(id); } - MoveInPart(_) | Read => {} + if binding_moves { + self.move_maps.moves_map.insert(id); } } } @@ -759,20 +689,18 @@ pub impl VisitContext { fn arms_have_by_move_bindings(&self, moves_map: MovesMap, - arms: &[arm]) -> bool + arms: &[arm]) -> Option<@pat> { for arms.each |arm| { - for arm.pats.each |pat| { - let mut found = false; - do pat_bindings(self.tcx.def_map, *pat) |_, node_id, _, _| { - if moves_map.contains(&node_id) { - found = true; + for arm.pats.each |&pat| { + for ast_util::walk_pat(pat) |p| { + if moves_map.contains(&p.id) { + return Some(p); } } - if found { return true; } } } - return false; + return None; } fn compute_captures(&self, fn_expr_id: node_id) -> @[CaptureVar] { diff --git a/src/librustc/middle/pat_util.rs b/src/librustc/middle/pat_util.rs index 1fd6012143b57..1237e9fb4a26a 100644 --- a/src/librustc/middle/pat_util.rs +++ b/src/librustc/middle/pat_util.rs @@ -72,8 +72,8 @@ pub fn pat_is_binding_or_wild(dm: resolve::DefMap, pat: @pat) -> bool { } pub fn pat_bindings(dm: resolve::DefMap, pat: @pat, - it: &fn(binding_mode, node_id, span, @Path)) { - do walk_pat(pat) |p| { + it: &fn(binding_mode, node_id, span, @Path)) { + for walk_pat(pat) |p| { match p.node { pat_ident(binding_mode, pth, _) if pat_is_binding(dm, p) => { it(binding_mode, p.id, p.span, pth); diff --git a/src/librustc/middle/resolve.rs b/src/librustc/middle/resolve.rs index cda0dfd12a35e..979d04e18c340 100644 --- a/src/librustc/middle/resolve.rs +++ b/src/librustc/middle/resolve.rs @@ -4151,7 +4151,7 @@ pub impl Resolver { bindings_list: Option<@mut HashMap>, visitor: ResolveVisitor) { let pat_id = pattern.id; - do walk_pat(pattern) |pattern| { + for walk_pat(pattern) |pattern| { match pattern.node { pat_ident(binding_mode, path, _) if !path.global && path.idents.len() == 1 => { diff --git a/src/librustc/middle/typeck/check/_match.rs b/src/librustc/middle/typeck/check/_match.rs index 77b10663ac790..8edae63cea926 100644 --- a/src/librustc/middle/typeck/check/_match.rs +++ b/src/librustc/middle/typeck/check/_match.rs @@ -157,8 +157,8 @@ pub fn check_pat_variant(pcx: &pat_ctxt, pat: @ast::pat, path: @ast::Path, None); fcx.write_error(pat.id); kind_name = "[error]"; - arg_types = (copy subpats).get_or_default(~[]).map(|_| - ty::mk_err()); + arg_types = (copy *subpats).get_or_default(~[]).map(|_| + ty::mk_err()); } } } @@ -199,8 +199,8 @@ pub fn check_pat_variant(pcx: &pat_ctxt, pat: @ast::pat, path: @ast::Path, None); fcx.write_error(pat.id); kind_name = "[error]"; - arg_types = (copy subpats).get_or_default(~[]).map(|_| - ty::mk_err()); + arg_types = (copy *subpats).get_or_default(~[]).map(|_| + ty::mk_err()); } } diff --git a/src/librustc/util/ppaux.rs b/src/librustc/util/ppaux.rs index 9d74f6c7b0ed7..8694c0e356eb1 100644 --- a/src/librustc/util/ppaux.rs +++ b/src/librustc/util/ppaux.rs @@ -801,3 +801,9 @@ impl UserString for ty::TraitRef { } } } + +impl UserString for ty::t { + fn user_string(&self, tcx: ctxt) -> ~str { + ty_to_str(tcx, *self) + } +} diff --git a/src/librustdoc/markdown_index_pass.rs b/src/librustdoc/markdown_index_pass.rs index 8ff0aa2314647..289aa33f67c37 100644 --- a/src/librustdoc/markdown_index_pass.rs +++ b/src/librustdoc/markdown_index_pass.rs @@ -74,7 +74,7 @@ fn build_mod_index( ) -> doc::Index { doc::Index { entries: doc.items.map(|doc| { - item_to_entry(copy *doc, copy config) + item_to_entry(copy *doc, &config) }) } } @@ -85,14 +85,14 @@ fn build_nmod_index( ) -> doc::Index { doc::Index { entries: doc.fns.map(|doc| { - item_to_entry(doc::FnTag(copy *doc), copy config) + item_to_entry(doc::FnTag(copy *doc), &config) }) } } fn item_to_entry( doc: doc::ItemTag, - config: config::Config + config: &config::Config ) -> doc::IndexEntry { let link = match doc { doc::ModTag(_) | doc::NmodTag(_) @@ -222,13 +222,13 @@ mod test { config::DocPerCrate, ~"mod a { } fn b() { }" ); - assert!((&doc.cratemod().index).get().entries[0] == doc::IndexEntry { + assert!(doc.cratemod().index.get().entries[0] == doc::IndexEntry { kind: ~"Module", name: ~"a", brief: None, link: ~"#module-a" }); - assert!((&doc.cratemod().index).get().entries[1] == doc::IndexEntry { + assert!(doc.cratemod().index.get().entries[1] == doc::IndexEntry { kind: ~"Function", name: ~"b", brief: None, @@ -242,13 +242,13 @@ mod test { config::DocPerMod, ~"mod a { } fn b() { }" ); - assert!((&doc.cratemod().index).get().entries[0] == doc::IndexEntry { + assert!(doc.cratemod().index.get().entries[0] == doc::IndexEntry { kind: ~"Module", name: ~"a", brief: None, link: ~"a.html" }); - assert!((&doc.cratemod().index).get().entries[1] == doc::IndexEntry { + assert!(doc.cratemod().index.get().entries[1] == doc::IndexEntry { kind: ~"Function", name: ~"b", brief: None, @@ -262,7 +262,7 @@ mod test { config::DocPerMod, ~"#[doc = \"test\"] mod a { }" ); - assert!((&doc.cratemod().index).get().entries[0].brief + assert!(doc.cratemod().index.get().entries[0].brief == Some(~"test")); } @@ -272,7 +272,7 @@ mod test { config::DocPerCrate, ~"extern { fn b(); }" ); - assert!((&doc.cratemod().nmods()[0].index).get().entries[0] + assert!(doc.cratemod().nmods()[0].index.get().entries[0] == doc::IndexEntry { kind: ~"Function", name: ~"b", diff --git a/src/librustdoc/markdown_pass.rs b/src/librustdoc/markdown_pass.rs index ed882bc3434b7..a3ad8d8d04de3 100644 --- a/src/librustdoc/markdown_pass.rs +++ b/src/librustdoc/markdown_pass.rs @@ -181,12 +181,12 @@ pub fn header_name(doc: doc::ItemTag) -> ~str { } &doc::ImplTag(ref doc) => { assert!(doc.self_ty.is_some()); - let bounds = if (&doc.bounds_str).is_some() { - fmt!(" where %s", (&doc.bounds_str).get()) + let bounds = if doc.bounds_str.is_some() { + fmt!(" where %s", *doc.bounds_str.get_ref()) } else { ~"" }; - let self_ty = (&doc.self_ty).get(); + let self_ty = doc.self_ty.get_ref(); let mut trait_part = ~""; for doc.trait_types.eachi |i, trait_type| { if i == 0 { @@ -196,7 +196,7 @@ pub fn header_name(doc: doc::ItemTag) -> ~str { } trait_part += *trait_type; } - fmt!("%s for %s%s", trait_part, self_ty, bounds) + fmt!("%s for %s%s", trait_part, *self_ty, bounds) } _ => { doc.name() @@ -208,17 +208,17 @@ pub fn header_text(doc: doc::ItemTag) -> ~str { match &doc { &doc::ImplTag(ref ImplDoc) => { let header_kind = header_kind(copy doc); - let bounds = if (&ImplDoc.bounds_str).is_some() { - fmt!(" where `%s`", (&ImplDoc.bounds_str).get()) + let bounds = if ImplDoc.bounds_str.is_some() { + fmt!(" where `%s`", *ImplDoc.bounds_str.get_ref()) } else { ~"" }; let desc = if ImplDoc.trait_types.is_empty() { - fmt!("for `%s`%s", (&ImplDoc.self_ty).get(), bounds) + fmt!("for `%s`%s", *ImplDoc.self_ty.get_ref(), bounds) } else { fmt!("of `%s` for `%s`%s", ImplDoc.trait_types[0], - (&ImplDoc.self_ty).get(), + *ImplDoc.self_ty.get_ref(), bounds) }; return fmt!("%s %s", header_kind, desc); @@ -295,7 +295,7 @@ fn write_mod_contents( ) { write_common(ctxt, doc.desc(), doc.sections()); if doc.index.is_some() { - write_index(ctxt, (&doc.index).get()); + write_index(ctxt, doc.index.get_ref()); } for doc.items.each |itemTag| { @@ -340,7 +340,7 @@ fn item_header_lvl(doc: &doc::ItemTag) -> Hlvl { } } -fn write_index(ctxt: &Ctxt, index: doc::Index) { +fn write_index(ctxt: &Ctxt, index: &doc::Index) { if vec::is_empty(index.entries) { return; } @@ -353,7 +353,7 @@ fn write_index(ctxt: &Ctxt, index: doc::Index) { let id = copy entry.link; if entry.brief.is_some() { ctxt.w.put_line(fmt!("* [%s](%s) - %s", - header, id, (&entry.brief).get())); + header, id, *entry.brief.get_ref())); } else { ctxt.w.put_line(fmt!("* [%s](%s)", header, id)); } @@ -366,7 +366,7 @@ fn write_index(ctxt: &Ctxt, index: doc::Index) { fn write_nmod(ctxt: &Ctxt, doc: doc::NmodDoc) { write_common(ctxt, doc.desc(), doc.sections()); if doc.index.is_some() { - write_index(ctxt, (&doc.index).get()); + write_index(ctxt, doc.index.get_ref()); } for doc.fns.each |FnDoc| { @@ -450,17 +450,17 @@ fn write_variants( fn write_variant(ctxt: &Ctxt, doc: doc::VariantDoc) { assert!(doc.sig.is_some()); - let sig = (&doc.sig).get(); + let sig = doc.sig.get_ref(); // space out list items so they all end up within paragraph elements ctxt.w.put_line(~""); match copy doc.desc { Some(desc) => { - ctxt.w.put_line(list_item_indent(fmt!("* `%s` - %s", sig, desc))); + ctxt.w.put_line(list_item_indent(fmt!("* `%s` - %s", *sig, desc))); } None => { - ctxt.w.put_line(fmt!("* `%s`", sig)); + ctxt.w.put_line(fmt!("* `%s`", *sig)); } } } diff --git a/src/librustdoc/markdown_writer.rs b/src/librustdoc/markdown_writer.rs index b537dfdbd0bc1..973326b10dce8 100644 --- a/src/librustdoc/markdown_writer.rs +++ b/src/librustdoc/markdown_writer.rs @@ -59,20 +59,20 @@ pub fn make_writer_factory(config: config::Config) -> WriterFactory { fn markdown_writer_factory(config: config::Config) -> WriterFactory { let result: ~fn(page: doc::Page) -> Writer = |page| { - markdown_writer(copy config, page) + markdown_writer(&config, page) }; result } fn pandoc_writer_factory(config: config::Config) -> WriterFactory { let result: ~fn(doc::Page) -> Writer = |page| { - pandoc_writer(copy config, page) + pandoc_writer(&config, page) }; result } fn markdown_writer( - config: config::Config, + config: &config::Config, page: doc::Page ) -> Writer { let filename = make_local_filename(config, page); @@ -82,11 +82,11 @@ fn markdown_writer( } fn pandoc_writer( - config: config::Config, + config: &config::Config, page: doc::Page ) -> Writer { assert!(config.pandoc_cmd.is_some()); - let pandoc_cmd = (&config.pandoc_cmd).get(); + let pandoc_cmd = copy *config.pandoc_cmd.get_ref(); let filename = make_local_filename(config, page); let pandoc_args = ~[ @@ -136,15 +136,15 @@ fn generic_writer(process: ~fn(markdown: ~str)) -> Writer { } pub fn make_local_filename( - config: config::Config, + config: &config::Config, page: doc::Page ) -> Path { - let filename = make_filename(copy config, page); + let filename = make_filename(config, page); config.output_dir.push_rel(&filename) } pub fn make_filename( - config: config::Config, + config: &config::Config, page: doc::Page ) -> Path { let filename = { @@ -247,7 +247,7 @@ mod test { }; let doc = mk_doc(~"test", ~""); let page = doc::CratePage(doc.CrateDoc()); - let filename = make_local_filename(config, page); + let filename = make_local_filename(&config, page); assert_eq!(filename.to_str(), ~"output/dir/test.md"); } @@ -261,7 +261,7 @@ mod test { }; let doc = mk_doc(~"", ~""); let page = doc::CratePage(doc.CrateDoc()); - let filename = make_local_filename(config, page); + let filename = make_local_filename(&config, page); assert_eq!(filename.to_str(), ~"output/dir/index.html"); } @@ -276,7 +276,7 @@ mod test { let doc = mk_doc(~"", ~"mod a { mod b { } }"); let modb = copy doc.cratemod().mods()[0].mods()[0]; let page = doc::ItemPage(doc::ModTag(modb)); - let filename = make_local_filename(config, page); + let filename = make_local_filename(&config, page); assert_eq!(filename, Path("output/dir/a_b.html")); } } diff --git a/src/librustdoc/sectionalize_pass.rs b/src/librustdoc/sectionalize_pass.rs index 89aa09b42d510..ed069b5ed5603 100644 --- a/src/librustdoc/sectionalize_pass.rs +++ b/src/librustdoc/sectionalize_pass.rs @@ -113,7 +113,7 @@ fn sectionalize(desc: Option<~str>) -> (Option<~str>, ~[doc::Section]) { match parse_header(copy *line) { Some(header) => { if current_section.is_some() { - sections += [(¤t_section).get()]; + sections += [copy *current_section.get_ref()]; } current_section = Some(doc::Section { header: header, diff --git a/src/librustdoc/tystr_pass.rs b/src/librustdoc/tystr_pass.rs index 051825d46e1b3..716784c51c513 100644 --- a/src/librustdoc/tystr_pass.rs +++ b/src/librustdoc/tystr_pass.rs @@ -434,7 +434,7 @@ mod test { #[test] fn should_add_struct_defs() { let doc = mk_doc(~"struct S { field: () }"); - assert!((&doc.cratemod().structs()[0].sig).get().contains( + assert!(doc.cratemod().structs()[0].sig.get().contains( "struct S {")); } @@ -442,6 +442,6 @@ mod test { fn should_not_serialize_struct_attrs() { // All we care about are the fields let doc = mk_doc(~"#[wut] struct S { field: () }"); - assert!(!(&doc.cratemod().structs()[0].sig).get().contains("wut")); + assert!(!doc.cratemod().structs()[0].sig.get().contains("wut")); } } diff --git a/src/libsyntax/ast_util.rs b/src/libsyntax/ast_util.rs index 8d5af682d6205..ba56d54488068 100644 --- a/src/libsyntax/ast_util.rs +++ b/src/libsyntax/ast_util.rs @@ -527,36 +527,31 @@ pub fn is_item_impl(item: @ast::item) -> bool { } } -pub fn walk_pat(pat: @pat, it: &fn(@pat)) { - it(pat); +pub fn walk_pat(pat: @pat, it: &fn(@pat) -> bool) -> bool { + if !it(pat) { + return false; + } + match pat.node { pat_ident(_, _, Some(p)) => walk_pat(p, it), pat_struct(_, ref fields, _) => { - for fields.each |f| { - walk_pat(f.pat, it) - } + fields.each(|f| walk_pat(f.pat, it)) } pat_enum(_, Some(ref s)) | pat_tup(ref s) => { - for s.each |p| { - walk_pat(*p, it) - } + s.each(|&p| walk_pat(p, it)) } pat_box(s) | pat_uniq(s) | pat_region(s) => { walk_pat(s, it) } pat_vec(ref before, ref slice, ref after) => { - for before.each |p| { - walk_pat(*p, it) - } - for slice.each |p| { - walk_pat(*p, it) - } - for after.each |p| { - walk_pat(*p, it) - } + before.each(|&p| walk_pat(p, it)) && + slice.each(|&p| walk_pat(p, it)) && + after.each(|&p| walk_pat(p, it)) } pat_wild | pat_lit(_) | pat_range(_, _) | pat_ident(_, _, _) | - pat_enum(_, _) => { } + pat_enum(_, _) => { + true + } } } diff --git a/src/test/compile-fail/borrowck-move-by-capture.rs b/src/test/compile-fail/borrowck-move-by-capture.rs index 4e977442e1586..0efde1df6c22b 100644 --- a/src/test/compile-fail/borrowck-move-by-capture.rs +++ b/src/test/compile-fail/borrowck-move-by-capture.rs @@ -7,8 +7,7 @@ fn main() { //~^ ERROR cannot move `foo` let bar = ~3; - let _g = || { + let _g = || { //~ ERROR capture of moved value let _h: @fn() -> int = || *bar; - //~^ ERROR illegal by-move capture }; } diff --git a/src/test/compile-fail/borrowck-move-out-of-vec-tail.rs b/src/test/compile-fail/borrowck-move-out-of-vec-tail.rs new file mode 100644 index 0000000000000..dec976e0a6068 --- /dev/null +++ b/src/test/compile-fail/borrowck-move-out-of-vec-tail.rs @@ -0,0 +1,31 @@ +// Test that we do not permit moves from &[] matched by a vec pattern. + +struct Foo { + string: ~str +} + +pub fn main() { + let x = [ + Foo { string: ~"foo" }, + Foo { string: ~"bar" }, + Foo { string: ~"baz" } + ]; + match x { + [first, ..tail] => { + match tail { + [Foo { string: a }, Foo { string: b }] => { + //~^ ERROR cannot move out of dereference of & pointer + //~^^ ERROR cannot move out of dereference of & pointer + } + _ => { + ::std::util::unreachable(); + } + } + let z = copy tail[0]; + debug!(fmt!("%?", z)); + } + _ => { + ::std::util::unreachable(); + } + } +} diff --git a/src/test/compile-fail/borrowck-unary-move.rs b/src/test/compile-fail/borrowck-unary-move.rs index cf7529865118a..a67a12f9d0f73 100644 --- a/src/test/compile-fail/borrowck-unary-move.rs +++ b/src/test/compile-fail/borrowck-unary-move.rs @@ -10,7 +10,7 @@ fn foo(x: ~int) -> int { let y = &*x; - free(x); //~ ERROR cannot move out of `*x` because it is borrowed + free(x); //~ ERROR cannot move out of `x` because it is borrowed *y } diff --git a/src/test/compile-fail/by-move-pattern-binding.rs b/src/test/compile-fail/by-move-pattern-binding.rs index 1efed154286ec..dc42e28ec2523 100644 --- a/src/test/compile-fail/by-move-pattern-binding.rs +++ b/src/test/compile-fail/by-move-pattern-binding.rs @@ -13,7 +13,7 @@ fn main() { let s = S { x: Bar(~"hello") }; match &s.x { &Foo => {} - &Bar(identifier) => f(copy identifier) //~ ERROR by-move pattern bindings may not occur + &Bar(identifier) => f(copy identifier) //~ ERROR cannot move }; match &s.x { &Foo => {} diff --git a/src/test/compile-fail/disallowed-deconstructing-destructing-struct-match.rs b/src/test/compile-fail/disallowed-deconstructing-destructing-struct-match.rs index 40305ba8b95c9..478a56c03010a 100644 --- a/src/test/compile-fail/disallowed-deconstructing-destructing-struct-match.rs +++ b/src/test/compile-fail/disallowed-deconstructing-destructing-struct-match.rs @@ -23,6 +23,6 @@ fn main() { match x { X { x: y } => error!("contents: %s", y) - //~^ ERROR cannot bind by-move within struct + //~^ ERROR cannot move out of type `X`, which defines the `Drop` trait } } diff --git a/src/test/compile-fail/issue-2590.rs b/src/test/compile-fail/issue-2590.rs index a0b967d59593a..92f2e5ea689c1 100644 --- a/src/test/compile-fail/issue-2590.rs +++ b/src/test/compile-fail/issue-2590.rs @@ -18,7 +18,7 @@ trait parse { impl parse for parser { fn parse(&self) -> ~[int] { - self.tokens //~ ERROR cannot move out of field + self.tokens //~ ERROR cannot move out of dereference of & pointer } } diff --git a/src/test/compile-fail/liveness-move-in-loop.rs b/src/test/compile-fail/liveness-move-in-loop.rs index d1663bc356b18..6fe59f0ca52d1 100644 --- a/src/test/compile-fail/liveness-move-in-loop.rs +++ b/src/test/compile-fail/liveness-move-in-loop.rs @@ -16,10 +16,7 @@ fn main() { loop { loop { loop { -// tjc: Not sure why it prints the same error twice x = y; //~ ERROR use of moved value - //~^ ERROR use of moved value - copy x; } } diff --git a/src/test/compile-fail/liveness-move-in-while.rs b/src/test/compile-fail/liveness-move-in-while.rs index 6b4147242d19b..26e82dd367343 100644 --- a/src/test/compile-fail/liveness-move-in-while.rs +++ b/src/test/compile-fail/liveness-move-in-while.rs @@ -13,10 +13,8 @@ fn main() { let y: ~int = ~42; let mut x: ~int; loop { - debug!(y); -// tjc: not sure why it prints the same error twice + debug!(y); //~ ERROR use of moved value: `y` while true { while true { while true { x = y; copy x; } } } //~^ ERROR use of moved value: `y` - //~^^ ERROR use of moved value: `y` } } diff --git a/src/test/compile-fail/moves-based-on-type-access-to-field.rs b/src/test/compile-fail/moves-based-on-type-access-to-field.rs index 6cc19b18c20a6..1a2beedff9306 100644 --- a/src/test/compile-fail/moves-based-on-type-access-to-field.rs +++ b/src/test/compile-fail/moves-based-on-type-access-to-field.rs @@ -7,13 +7,13 @@ fn touch(_a: &A) {} fn f10() { let x = Foo { f: ~"hi", y: 3 }; - consume(x.f); //~ NOTE field of `x` moved here + consume(x.f); //~ NOTE `x.f` moved here touch(&x.y); //~ ERROR use of partially moved value: `x` } fn f20() { let x = ~[~"hi"]; - consume(x[0]); //~ NOTE element of `x` moved here + consume(x[0]); //~ NOTE `(*x)[]` moved here touch(&x[0]); //~ ERROR use of partially moved value: `x` } diff --git a/src/test/compile-fail/moves-based-on-type-block-bad.rs b/src/test/compile-fail/moves-based-on-type-block-bad.rs index 76d50710bb8c1..ca58097b555e1 100644 --- a/src/test/compile-fail/moves-based-on-type-block-bad.rs +++ b/src/test/compile-fail/moves-based-on-type-block-bad.rs @@ -16,9 +16,9 @@ fn main() { let s = S { x: ~Bar(~42) }; loop { do f(&s) |hellothere| { - match hellothere.x { //~ ERROR cannot move out + match hellothere.x { ~Foo(_) => {} - ~Bar(x) => io::println(x.to_str()), + ~Bar(x) => io::println(x.to_str()), //~ ERROR cannot move out ~Baz => {} } } diff --git a/src/test/compile-fail/moves-based-on-type-exprs.rs b/src/test/compile-fail/moves-based-on-type-exprs.rs index 5b733129ee5dc..40ee37fae78a8 100644 --- a/src/test/compile-fail/moves-based-on-type-exprs.rs +++ b/src/test/compile-fail/moves-based-on-type-exprs.rs @@ -86,7 +86,7 @@ fn f110() { } fn f120() { - let x = ~[~"hi", ~"ho"]; + let mut x = ~[~"hi", ~"ho"]; vec::swap(x, 0, 1); touch(&x[0]); touch(&x[1]); diff --git a/src/test/compile-fail/moves-based-on-type-match-bindings.rs b/src/test/compile-fail/moves-based-on-type-match-bindings.rs new file mode 100644 index 0000000000000..42944a206b360 --- /dev/null +++ b/src/test/compile-fail/moves-based-on-type-match-bindings.rs @@ -0,0 +1,19 @@ +// Tests that bindings to move-by-default values trigger moves of the +// discriminant. Also tests that the compiler explains the move in +// terms of the binding, not the discriminant. + +struct Foo { f: A } +fn guard(_s: ~str) -> bool {fail!()} +fn touch(_a: &A) {} + +fn f10() { + let x = Foo {f: ~"hi"}; + + let y = match x { + Foo {f} => {} //~ NOTE moved here + }; + + touch(&x); //~ ERROR use of partially moved value: `x` +} + +fn main() {} diff --git a/src/test/compile-fail/no-reuse-move-arc.rs b/src/test/compile-fail/no-reuse-move-arc.rs index 607127f2fee74..c9e5144557acc 100644 --- a/src/test/compile-fail/no-reuse-move-arc.rs +++ b/src/test/compile-fail/no-reuse-move-arc.rs @@ -15,12 +15,12 @@ fn main() { let v = ~[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]; let arc_v = arc::ARC(v); - do task::spawn() { //~ NOTE `arc_v` moved into closure environment here + do task::spawn() { let v = arc_v.get(); assert_eq!(v[3], 4); }; assert_eq!((arc_v.get())[2], 3); //~ ERROR use of moved value: `arc_v` - info!(arc_v); + info!(arc_v); //~ ERROR use of moved value: `arc_v` } diff --git a/src/test/compile-fail/use-after-move-self-based-on-type.rs b/src/test/compile-fail/use-after-move-self-based-on-type.rs index d19b4dfbd57d0..da8e0c9f2b697 100644 --- a/src/test/compile-fail/use-after-move-self-based-on-type.rs +++ b/src/test/compile-fail/use-after-move-self-based-on-type.rs @@ -9,7 +9,7 @@ impl Drop for S { pub impl S { fn foo(self) -> int { self.bar(); - return self.x; //~ ERROR use of partially moved value + return self.x; //~ ERROR use of moved value: `self` } fn bar(self) {} diff --git a/src/test/compile-fail/use-after-move-self.rs b/src/test/compile-fail/use-after-move-self.rs index b2eaffdd06605..37db40d14365e 100644 --- a/src/test/compile-fail/use-after-move-self.rs +++ b/src/test/compile-fail/use-after-move-self.rs @@ -5,7 +5,7 @@ struct S { pub impl S { fn foo(self) -> int { self.bar(); - return *self.x; //~ ERROR use of partially moved value + return *self.x; //~ ERROR use of moved value: `self` } fn bar(self) {} diff --git a/src/test/compile-fail/borrowck-unary-move-2.rs b/src/test/run-pass/borrowck-unary-move-2.rs similarity index 94% rename from src/test/compile-fail/borrowck-unary-move-2.rs rename to src/test/run-pass/borrowck-unary-move-2.rs index 898830bbe55ba..c74fd4a68e719 100644 --- a/src/test/compile-fail/borrowck-unary-move-2.rs +++ b/src/test/run-pass/borrowck-unary-move-2.rs @@ -28,5 +28,5 @@ struct wrapper(noncopyable); fn main() { let x1 = wrapper(noncopyable()); - let _x2 = *x1; //~ ERROR cannot move out + let _x2 = *x1; } diff --git a/src/test/run-pass/move-out-of-field.rs b/src/test/run-pass/move-out-of-field.rs new file mode 100644 index 0000000000000..b6485348a5184 --- /dev/null +++ b/src/test/run-pass/move-out-of-field.rs @@ -0,0 +1,23 @@ +use std::str; + +struct StringBuffer { + s: ~str +} + +impl StringBuffer { + pub fn append(&mut self, v: &str) { + str::push_str(&mut self.s, v); + } +} + +fn to_str(sb: StringBuffer) -> ~str { + sb.s +} + +fn main() { + let mut sb = StringBuffer {s: ~""}; + sb.append("Hello, "); + sb.append("World!"); + let str = to_str(sb); + assert_eq!(str, ~"Hello, World!"); +} \ No newline at end of file diff --git a/src/test/run-pass/vec-matching-fold.rs b/src/test/run-pass/vec-matching-fold.rs index 7dcea2d30b7df..05a6dee06cc87 100644 --- a/src/test/run-pass/vec-matching-fold.rs +++ b/src/test/run-pass/vec-matching-fold.rs @@ -4,8 +4,8 @@ fn foldl( function: &fn(partial: U, element: &T) -> U ) -> U { match values { - [head, ..tail] => - foldl(tail, function(initial, &head), function), + [ref head, ..tail] => + foldl(tail, function(initial, head), function), [] => initial.clone() } } @@ -16,8 +16,8 @@ fn foldr( function: &fn(element: &T, partial: U) -> U ) -> U { match values { - [..head, tail] => - foldr(head, function(&tail, initial), function), + [..head, ref tail] => + foldr(head, function(tail, initial), function), [] => initial.clone() } } diff --git a/src/test/run-pass/vec-tail-matching.rs b/src/test/run-pass/vec-tail-matching.rs index cf4aebbd08270..6e1a47ad2dfd3 100644 --- a/src/test/run-pass/vec-tail-matching.rs +++ b/src/test/run-pass/vec-tail-matching.rs @@ -19,9 +19,9 @@ pub fn main() { [Foo { _ }, _, Foo { _ }, ..tail] => { ::std::util::unreachable(); } - [Foo { string: a }, Foo { string: b }] => { - assert_eq!(a, ~"bar"); - assert_eq!(b, ~"baz"); + [Foo { string: ref a }, Foo { string: ref b }] => { + assert_eq!("bar", a.slice(0, a.len())); + assert_eq!("baz", b.slice(0, b.len())); } _ => { ::std::util::unreachable(); From 329f7a17e2162807d62b5f1f4c3d97da1317d192 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 26 May 2013 05:11:35 -0400 Subject: [PATCH 2/5] Update docs on the formal basis of the borrow checker. --- src/librustc/middle/borrowck/doc.rs | 1129 ++++++++--------- .../middle/borrowck/gather_loans/lifetime.rs | 20 +- .../middle/borrowck/gather_loans/mod.rs | 2 + .../borrowck/gather_loans/restrictions.rs | 11 + 4 files changed, 527 insertions(+), 635 deletions(-) diff --git a/src/librustc/middle/borrowck/doc.rs b/src/librustc/middle/borrowck/doc.rs index 1e09fbe71843c..2151c86519a09 100644 --- a/src/librustc/middle/borrowck/doc.rs +++ b/src/librustc/middle/borrowck/doc.rs @@ -13,8 +13,31 @@ # The Borrow Checker This pass has the job of enforcing memory safety. This is a subtle -topic. The only way I know how to explain it is terms of a formal -model, so that's what I'll do. +topic. This docs aim to explain both the practice and the theory +behind the borrow checker. They start with a high-level overview of +how it works, and then proceed to dive into the theoretical +background. Finally, they go into detail on some of the more subtle +aspects. + +# Overview + +The borrow checker checks one function at a time. It operates in two +passes. The first pass, called `gather_loans`, walks over the function +and identifies all of the places where borrows occur (e.g., `&` +expressions and `ref` bindings). For each borrow, we check various +basic safety conditions at this time (for example, that the lifetime +of the borrow doesn't exceed the lifetime of the value being +borrowed). + +It then uses the dataflow module to propagate which of those borrows +may be in scope at each point in the procedure. A loan is considered +to come into scope at the expression that caused it and to go out of +scope when the lifetime of the resulting borrowed pointer expires. + +Once the in-scope loans are known for each point in the program, the +borrow checker walks the IR again in a second pass called +`check_loans`. This pass examines each statement and makes sure that +it is safe with respect to the in-scope loans. # Formal model @@ -60,691 +83,543 @@ This is of course dangerous because mutating `x` will free the old value and hence invalidate `y`. The borrow checker aims to prevent this sort of thing. -### Loans +### Loans and restrictions The way the borrow checker works is that it analyzes each borrow expression (in our simple model, that's stuff like `&LV`, though in real life there are a few other cases to consider). For each borrow -expression, it computes a vector of loans: +expression, it computes a `Loan`, which is a data structure that +records (1) the value being borrowed, (2) the mutability and scope of +the borrow, and (3) a set of restrictions. In the code, `Loan` is a +struct defined in `middle::borrowck`. Formally, we define `LOAN` as +follows: + + LOAN = (LV, LT, MQ, RESTRICTION*) + RESTRICTION = (LV, ACTION*) + ACTION = MUTATE | CLAIM | FREEZE | ALIAS + +Here the `LOAN` tuple defines the lvalue `LV` being borrowed; the +lifetime `LT` of that borrow; the mutability `MQ` of the borrow; and a +list of restrictions. The restrictions indicate actions which, if +taken, could invalidate the loan and lead to type safety violations. + +Each `RESTRICTION` is a pair of a restrictive lvalue `LV` (which will +either be the path that was borrowed or some prefix of the path that +was borrowed) and a set of restricted actions. There are three kinds +of actions that may be restricted for the path `LV`: + +- `MUTATE` means that `LV` cannot be assigned to; +- `CLAIM` means that the `LV` cannot be borrowed mutably; +- `FREEZE` means that the `LV` cannot be borrowed immutably; +- `ALIAS` means that `LV` cannot be aliased in any way (not even `&const`). + +Finally, it is never possible to move from an lvalue that appears in a +restriction. This implies that the "empty restriction" `(LV, [])`, +which contains an empty set of actions, still has a purpose---it +prevents moves from `LV`. I chose not to make `MOVE` a fourth kind of +action because that would imply that sometimes moves are permitted +from restrictived values, which is not the case. - LOAN = (LV, LT, PT, LK) - PT = Partial | Total - LK = MQ | RESERVE +### Example -Each `LOAN` tuple indicates some sort of restriction on what can be -done to the lvalue `LV`; `LV` will always be a path owned by the -current stack frame. These restrictions are called "loans" because -they are always the result of a borrow expression. +To give you a better feeling for what kind of restrictions derived +from a loan, let's look at the loan `L` that would be issued as a +result of the borrow `&mut (*x).f` in the example above: + + L = ((*x).f, 'a, mut, RS) where + RS = [((*x).f, [MUTATE, CLAIM, FREEZE]), + (*x, [MUTATE, CLAIM, FREEZE]), + (x, [MUTATE, CLAIM, FREEZE])] + +The loan states that the expression `(*x).f` has been loaned as +mutable for the lifetime `'a`. Because the loan is mutable, that means +that the value `(*x).f` may be mutated via the newly created borrowed +pointer (and *only* via that pointer). This is reflected in the +restrictions `RS` that accompany the loan. + +The first restriction `((*x).f, [MUTATE, CLAIM, FREEZE])` states that +the lender may not mutate nor freeze `(*x).f`. Mutation is illegal +because `(*x).f` is only supposed to be mutated via the new borrowed +pointer, not by mutating the original path `(*x).f`. Freezing is +illegal because the path now has an `&mut` alias; so even if we the +lender were to consider `(*x).f` to be immutable, it might be mutated +via this alias. Both of these restrictions are temporary. They will be +enforced for the lifetime `'a` of the loan. After the loan expires, +the restrictions no longer apply. + +The second restriction on `*x` is interesting because it does not +apply to the path that was lent (`(*x).f`) but rather to a prefix of +the borrowed path. This is due to the rules of inherited mutability: +if the user were to assign to (or freeze) `*x`, they would indirectly +overwrite (or freeze) `(*x).f`, and thus invalidate the borrowed +pointer that was created. In general it holds that when a path is +lent, restrictions are issued for all the owning prefixes of that +path. In this case, the path `*x` owns the path `(*x).f` and, +because `x` is an owned pointer, the path `x` owns the path `*x`. +Therefore, borrowing `(*x).f` yields restrictions on both +`*x` and `x`. -Every loan has a lifetime `LT` during which those restrictions are in -effect. The indicator `PT` distinguishes between *total* loans, in -which the LV itself was borrowed, and *partial* loans, which means -that some content ownwed by LV was borrowed. +## Checking for illegal assignments, moves, and reborrows -The final element in the loan tuple is the *loan kind* `LK`. There -are four kinds: mutable, immutable, const, and reserve: +Once we have computed the loans introduced by each borrow, the borrow +checker uses a data flow propagation to compute the full set of loans +in scope at each expression and then uses that set to decide whether +that expression is legal. Remember that the scope of loan is defined +by its lifetime LT. We sometimes say that a loan which is in-scope at +a particular point is an "outstanding loan", aand the set of +restrictions included in those loans as the "outstanding +restrictions". + +The kinds of expressions which in-scope loans can render illegal are: +- *assignments* (`lv = v`): illegal if there is an in-scope restriction + against mutating `lv`; +- *moves*: illegal if there is any in-scope restriction on `lv` at all; +- *mutable borrows* (`&mut lv`): illegal there is an in-scope restriction + against mutating `lv` or aliasing `lv`; +- *immutable borrows* (`&lv`): illegal there is an in-scope restriction + against freezing `lv` or aliasing `lv`; +- *read-only borrows* (`&const lv`): illegal there is an in-scope restriction + against aliasing `lv`. -- A "mutable" loan means that LV may be written to through an alias, and - thus LV cannot be written to directly or immutably aliased (remember - that we preserve the invariant that any given value can only be - written to through one path at a time; hence if there is a mutable - alias to LV, then LV cannot be written directly until this alias is - out of scope). +# Formal rules -- An "immutable" loan means that LV must remain immutable. Hence it - cannot be written, but other immutable aliases are permitted. +Now that we hopefully have some kind of intuitive feeling for how the +borrow checker works, let's look a bit more closely now at the precise +conditions that it uses. For simplicity I will ignore const loans. -- A "const" loan means that an alias to LV exists. LV may still be - written or frozen. +I will present the rules in a modified form of standard inference +rules, which looks as as follows: -- A "reserve" loan is the strongest case. It prevents both mutation - and aliasing of any kind, including `&const` loans. Reserve loans - are a side-effect of borrowing an `&mut` loan. + PREDICATE(X, Y, Z) // Rule-Name + Condition 1 + Condition 2 + Condition 3 + +The initial line states the predicate that is to be satisfied. The +indented lines indicate the conditions that must be met for the +predicate to be satisfied. The right-justified comment states the name +of this rule: there are comments in the borrowck source referencing +these names, so that you can cross reference to find the actual code +that corresponds to the formal rule. + +## The `gather_loans` pass -In addition to affecting mutability, a loan of any kind implies that -LV cannot be moved. +We start with the `gather_loans` pass, which walks the AST looking for +borrows. For each borrow, there are three bits of information: the +lvalue `LV` being borrowed and the mutability `MQ` and lifetime `LT` +of the resulting pointer. Given those, `gather_loans` applies three +validity tests: -### Example +1. `MUTABILITY(LV, MQ)`: The mutability of the borrowed pointer is +compatible with the mutability of `LV` (i.e., not borrowing immutable +data as mutable). -To give you a better feeling for what a loan is, let's look at three -loans that would be issued as a result of the borrow `&(*x).f` in the -example above: - - ((*x).f, Total, mut, 'a) - (*x, Partial, mut, 'a) - (x, Partial, mut, 'a) - -The first loan states that the expression `(*x).f` has been loaned -totally as mutable for the lifetime `'a`. This first loan would -prevent an assignment `(*x).f = ...` from occurring during the -lifetime `'a`. - -Now let's look at the second loan. You may have expected that each -borrow would result in only one loan. But this is not the case. -Instead, there will be loans for every path where mutation might -affect the validity of the borrowed pointer that is created (in some -cases, there can even be multiple loans per path, see the section on -"Borrowing in Calls" below for the gory details). The reason for this -is to prevent actions that would indirectly affect the borrowed path. -In this case, we wish to ensure that `(*x).f` is not mutated except -through the mutable alias `y`. Therefore, we must not only prevent an -assignment to `(*x).f` but also an assignment like `*x = Foo {...}`, -as this would also mutate the field `f`. To do so, we issue a -*partial* mutable loan for `*x` (the loan is partial because `*x` -itself was not borrowed). This partial loan will cause any attempt to -assign to `*x` to be flagged as an error. - -Because both partial and total loans prevent assignments, you may -wonder why we bother to distinguish between them. The reason for this -distinction has to do with preventing double borrows. In particular, -it is legal to borrow both `&mut x.f` and `&mut x.g` simultaneously, -but it is not legal to borrow `&mut x.f` twice. In the borrow checker, -the first case would result in two *partial* mutable loans of `x` -(along with one total mutable loan of `x.f` and one of `x.g) whereas -the second would result in two *total* mutable loans of `x.f` (along -with two partial mutable loans of `x`). Multiple *total mutable* loan -for the same path are not permitted, but multiple *partial* loans (of -any mutability) are permitted. - -Finally, we come to the third loan. This loan is a partial mutable -loan of `x`. This loan prevents us from reassigning `x`, which would -be bad for two reasons. First, it would change the value of `(*x).f` -but, even worse, it would cause the pointer `y` to become a dangling -pointer. Bad all around. +2. `LIFETIME(LV, LT, MQ)`: The lifetime of the borrow does not exceed +the lifetime of the value being borrowed. This pass is also +responsible for inserting root annotations to keep managed values +alive and for dynamically freezing `@mut` boxes. -## Checking for illegal assignments, moves, and reborrows +3. `RESTRICTIONS(LV, ACTIONS) = RS`: This pass checks and computes the +restrictions to maintain memory safety. These are the restrictions +that will go into the final loan. We'll discuss in more detail below. -Once we have computed the loans introduced by each borrow, the borrow -checker will determine the full set of loans in scope at each -expression and use that to decide whether that expression is legal. -Remember that the scope of loan is defined by its lifetime LT. We -sometimes say that a loan which is in-scope at a particular point is -an "outstanding loan". - -The kinds of expressions which in-scope loans can render illegal are -*assignments*, *moves*, and *borrows*. - -An assignments to an lvalue LV is illegal if there is in-scope mutable -or immutable loan for LV. Assignment with an outstanding mutable loan -is illegal because then the `&mut` pointer is supposed to be the only -way to mutate the value. Assignment with an outstanding immutable -loan is illegal because the value is supposed to be immutable at that -point. - -A move from an lvalue LV is illegal if there is any sort of -outstanding loan. - -A borrow expression may be illegal if any of the loans which it -produces conflict with other outstanding loans. Two loans are -considered compatible if one of the following conditions holds: - -- At least one loan is a const loan. -- Both loans are partial loans. -- Both loans are immutable. - -Any other combination of loans is illegal. - -# The set of loans that results from a borrow expression - -Here we'll define four functions---MUTATE, FREEZE, ALIAS, and -TAKE---which are all used to compute the set of LOANs that result -from a borrow expression. The first three functions each have -a similar type signature: - - MUTATE(LV, LT, PT) -> LOANS - FREEZE(LV, LT, PT) -> LOANS - ALIAS(LV, LT, PT) -> LOANS - -MUTATE, FREEZE, and ALIAS are used when computing the loans result -from mutable, immutable, and const loans respectively. For example, -the loans resulting from an expression like `&mut (*x).f` would be -computed by `MUTATE((*x).f, LT, Total)`, where `LT` is the lifetime of -the resulting pointer. Similarly the loans for `&(*x).f` and `&const -(*x).f` would be computed by `FREEZE((*x).f, LT, Total)` and -`ALIAS((*x).f, LT, Total)` respectively. (Actually this is a slight -simplification; see the section below on Borrows in Calls for the full -gory details) - -The names MUTATE, FREEZE, and ALIAS are intended to suggest the -semantics of `&mut`, `&`, and `&const` borrows respectively. `&mut`, -for example, creates a mutable alias of LV. `&` causes the borrowed -value to be frozen (immutable). `&const` does neither but does -introduce an alias to be the borrowed value. - -Each of these three functions is only defined for some inputs. That -is, it may occur that some particular borrow is not legal. For -example, it is illegal to make an `&mut` loan of immutable data. In -that case, the MUTATE() function is simply not defined (in the code, -it returns a Result<> condition to indicate when a loan would be -illegal). - -The final function, RESERVE, is used as part of borrowing an `&mut` -pointer. Due to the fact that it is used for one very particular -purpose, it has a rather simpler signature than the others: - - RESERVE(LV, LT) -> LOANS - -It is explained when we come to that case. - -## The function MUTATE() - -Here we use [inference rules][ir] to define the MUTATE() function. -We will go case by case for the various kinds of lvalues that -can be borrowed. - -[ir]: http://en.wikipedia.org/wiki/Rule_of_inference - -### Mutating local variables - -The rule for mutating local variables is as follows: - - Mutate-Variable: - LT <= Scope(x) - Mut(x) = Mut - -------------------------------------------------- - MUTATE(x, LT, PT) = (x, LT, PT, mut) - -Here `Scope(x)` is the lifetime of the block in which `x` was declared -and `Mut(x)` indicates the mutability with which `x` was declared. -This rule simply states that you can only create a mutable alias -to a variable if it is mutable, and that alias cannot outlive the -stack frame in which the variable is declared. - -### Mutating fields and owned pointers - -As it turns out, the rules for mutating fields and mutating owned -pointers turn out to be quite similar. The reason is that the -expressions `LV.f` and `*LV` are both owned by their base expression -`LV`. So basically the result of mutating `LV.f` or `*LV` is computed -by adding a loan for `LV.f` or `*LV` and then the loans for a partial -take of `LV`: - - Mutate-Field: - MUTATE(LV, LT, Partial) = LOANS - ------------------------------------------------------------ - MUTATE(LV.f, LT, PT) = LOANS, (LV.F, LT, PT, mut) - - Mutate-Owned-Ptr: - Type(LV) = ~Ty - MUTATE(LV, LT, Partial) = LOANS - ------------------------------------------------------------ - MUTATE(*LV, LT, PT) = LOANS, (*LV, LT, PT, mut) - -Note that while our micro-language only has fields, the slight -variations on the `Mutate-Field` rule are used for any interior content -that appears in the full Rust language, such as the contents of a -tuple, fields in a struct, or elements of a fixed-length vector. - -### Mutating dereferenced borrowed pointers - -The rule for borrowed pointers is by far the most complicated: - - Mutate-Mut-Borrowed-Ptr: - Type(LV) = <_P mut Ty // (1) - LT <= LT_P // (2) - RESERVE(LV, LT) = LOANS // (3) - ------------------------------------------------------------ - MUTATE(*LV, LT, PT) = LOANS, (*LV, LT, PT, Mut) - -Condition (1) states that only a mutable borrowed pointer can be -taken. Condition (2) states that the lifetime of the alias must be -less than the lifetime of the borrowed pointer being taken. - -Conditions (3) and (4) are where things get interesting. The intended -semantics of the borrow is that the new `&mut` pointer is the only one -which has the right to modify the data; the original `&mut` pointer -must not be used for mutation. Because borrowed pointers do not own -their content nor inherit mutability, we must be particularly cautious -of aliases, which could permit the original borrowed pointer to be -reached from another path and thus circumvent our loans. - -Here is one example of what could go wrong if we ignore clause (4): - - let x: &mut T; - ... - let y = &mut *x; // Only *y should be able to mutate... - let z = &const x; - **z = ...; // ...but here **z is still able to mutate! +# Checking mutability -Another possible error could occur with moves: +Checking mutability is fairly straightforward. We just want to prevent +immutable data from being borrowed as mutable. Note that it is ok to +borrow mutable data as immutable, since that is simply a +freeze. Formally we define a predicate `MUTABLE(LV, MQ)` which, if +defined, means that "borrowing `LV` with mutability `MQ` is ok. The +Rust code corresponding to this predicate is the function +`check_mutability` in `middle::borrowck::gather_loans`. - let x: &mut T; - ... - let y = &mut *x; // Issues loan: (*x, LT, Total, Mut) - let z = x; // moves from x - *z = ...; // Mutates *y indirectly! Bad. - -In both of these cases, the problem is that when creating the alias -`y` we would only issue a loan preventing assignment through `*x`. -But this loan can be easily circumvented by moving from `x` or -aliasing it. Note that, in the first example, the alias of `x` was -created using `&const`, which is a particularly weak form of alias. - -The danger of aliases can also occur when the `&mut` pointer itself -is already located in an alias location, as here: - - let x: @mut &mut T; // or &mut &mut T, &&mut T, - ... // &const &mut T, @&mut T, etc - let y = &mut **x; // Only *y should be able to mutate... - let z = x; - **z = ...; // ...but here **z is still able to mutate! - -When we cover the rules for RESERVE, we will see that it would -disallow this case, because MUTATE can only be applied to canonical -lvalues which are owned by the current stack frame. - -It might be the case that if `&const` and `@const` pointers were -removed, we could do away with RESERVE and simply use MUTATE instead. -But we have to be careful about the final example in particular, since -dynamic freezing would not be sufficient to prevent this example. -Perhaps a combination of MUTATE with a predicate OWNED(LV). - -One final detail: unlike every other case, when we calculate the loans -using RESERVE we do not use the original lifetime `LT` but rather -`GLB(Scope(LV), LT)`. What this says is: - -### Mutating dereferenced managed pointers - -Because the correctness of managed pointer loans is checked dynamically, -the rule is quite simple: - - Mutate-Mut-Managed-Ptr: - Type(LV) = @mut Ty - Add ROOT-FREEZE annotation for *LV with lifetime LT - ------------------------------------------------------------ - MUTATE(*LV, LT, Total) = [] - -No loans are issued. Instead, we add a side annotation that causes -`*LV` to be rooted and frozen on entry to LV. You could rephrase -these rules as having multiple returns values, or rephrase this as a -kind of loan, but whatever. - -One interesting point is that *partial takes* of `@mut` are forbidden. -This is not for any soundness reason but just because it is clearer -for users when `@mut` values are either lent completely or not at all. - -## The function FREEZE - -The rules for FREEZE are pretty similar to MUTATE. The first four -cases I'll just present without discussion, as the reasoning is -quite analogous to the MUTATE case: - - Freeze-Variable: - LT <= Scope(x) - -------------------------------------------------- - FREEZE(x, LT, PT) = (x, LT, PT, imm) - - Freeze-Field: - FREEZE(LV, LT, Partial) = LOANS - ------------------------------------------------------------ - FREEZE(LV.f, LT, PT) = LOANS, (LV.F, LT, PT, imm) - - Freeze-Owned-Ptr: - Type(LV) = ~Ty - FREEZE(LV, LT, Partial) = LOANS - ------------------------------------------------------------ - FREEZE(*LV, LT, PT) = LOANS, (*LV, LT, PT, imm) - - Freeze-Mut-Borrowed-Ptr: - Type(LV) = <_P mut Ty - LT <= LT_P - RESERVE(LV, LT) = LOANS - ------------------------------------------------------------ - FREEZE(*LV, LT, PT) = LOANS, (*LV, LT, PT, Imm) - - Freeze-Mut-Managed-Ptr: - Type(LV) = @mut Ty - Add ROOT-FREEZE annotation for *LV with lifetime LT - ------------------------------------------------------------ - Freeze(*LV, LT, Total) = [] - -The rule to "freeze" an immutable borrowed pointer is quite -simple, since the content is already immutable: - - Freeze-Imm-Borrowed-Ptr: - Type(LV) = <_P Ty // (1) - LT <= LT_P // (2) - ------------------------------------------------------------ - FREEZE(*LV, LT, PT) = LOANS, (*LV, LT, PT, Mut) - -The final two rules pertain to borrows of `@Ty`. There is a bit of -subtlety here. The main problem is that we must guarantee that the -managed box remains live for the entire borrow. We can either do this -dynamically, by rooting it, or (better) statically, and hence there -are two rules: - - Freeze-Imm-Managed-Ptr-1: - Type(LV) = @Ty - Add ROOT annotation for *LV - ------------------------------------------------------------ - FREEZE(*LV, LT, PT) = [] - - Freeze-Imm-Managed-Ptr-2: - Type(LV) = @Ty - LT <= Scope(LV) - Mut(LV) = imm - LV is not moved - ------------------------------------------------------------ - FREEZE(*LV, LT, PT) = [] - -The intention of the second rule is to avoid an extra root if LV -serves as a root. In that case, LV must (1) outlive the borrow; (2) -be immutable; and (3) not be moved. - -## The ALIAS function - -The function ALIAS is used for `&const` loans but also to handle one -corner case concerning function arguments (covered in the section -"Borrows in Calls" below). It computes the loans that result from -observing that there is a pointer to `LV` and thus that pointer must -remain valid. - -The first two rules are simple: - - Alias-Variable: - LT <= Scope(x) - -------------------------------------------------- - ALIAS(x, LT, PT) = (x, LT, PT, Const) - - Alias-Field: - ALIAS(LV, LT, Partial) = LOANS - ------------------------------------------------------------ - ALIAS(LV.f, LT, PT) = LOANS, (LV.F, LT, PT, Const) - -### Aliasing owned pointers - -The rule for owned pointers is somewhat interesting: - - Alias-Owned-Ptr: - Type(LV) = ~Ty - FREEZE(LV, LT, Partial) = LOANS - ------------------------------------------------------------ - ALIAS(*LV, LT, PT) = LOANS, (*LV, LT, PT, Const) - -Here we *freeze* the base `LV`. The reason is that if an owned -pointer is mutated it frees its content, which means that the alias to -`*LV` would become a dangling pointer. - -### Aliasing borrowed pointers - -The rule for borrowed pointers is quite simple, because borrowed -pointers do not own their content and thus do not play a role in -keeping it live: - - Alias-Borrowed-Ptr: - Type(LV) = <_P MQ Ty - LT <= LT_P - ------------------------------------------------------------ - ALIAS(*LV, LT, PT) = [] - -Basically, the existence of a borrowed pointer to some memory with -lifetime LT_P is proof that the memory can safely be aliased for any -lifetime LT <= LT_P. - -### Aliasing managed pointers - -The rules for aliasing managed pointers are similar to those -used with FREEZE, except that they apply to all manager pointers -regardles of mutability: - - Alias-Managed-Ptr-1: - Type(LV) = @MQ Ty - Add ROOT annotation for *LV - ------------------------------------------------------------ - ALIAS(*LV, LT, PT) = [] - - Alias-Managed-Ptr-2: - Type(LV) = @MQ Ty - LT <= Scope(LV) - Mut(LV) = imm - LV is not moved - ------------------------------------------------------------ - ALIAS(*LV, LT, PT) = [] - -## The RESERVE function - -The final function, RESERVE, is used for loans of `&mut` pointers. As -discussed in the section on the function MUTATE, we must be quite -careful when "re-borrowing" an `&mut` pointer to ensure that the original -`&mut` pointer can no longer be used to mutate. - -There are a couple of dangers to be aware of: - -- `&mut` pointers do not inherit mutability. Therefore, if you have - an lvalue LV with type `&mut T` and you freeze `LV`, you do *not* - freeze `*LV`. This is quite different from an `LV` with type `~T`. - -- Also, because they do not inherit mutability, if the `&mut` pointer - lives in an aliased location, then *any alias* can be used to write! - -As a consequence of these two rules, RESERVE can only be successfully -invoked on an lvalue LV that is *owned by the current stack frame*. -This ensures that there are no aliases that are not visible from the -outside. Moreover, Reserve loans are incompatible with all other -loans, even Const loans. This prevents any aliases from being created -within the current function. +## Checking mutability of variables -### Reserving local variables +*Code pointer:* Function `check_mutability()` in `gather_loans/mod.rs`, +but also the code in `mem_categorization`. -The rule for reserving a variable is generally straightforward but -with one interesting twist: +Let's begin with the rules for variables, which state that if a +variable is declared as mutable, it may be borrowed any which way, but +otherwise the variable must be borrowed as immutable or const: - Reserve-Variable: - -------------------------------------------------- - RESERVE(x, LT) = (x, LT, Total, Reserve) + MUTABILITY(X, MQ) // M-Var-Mut + DECL(X) = mut -The twist here is that the incoming lifetime is not required to -be a subset of the incoming variable, unlike every other case. To -see the reason for this, imagine the following function: + MUTABILITY(X, MQ) // M-Var-Imm + DECL(X) = imm + MQ = imm | const - struct Foo { count: uint } - fn count_field(x: &'a mut Foo) -> &'a mut count { - &mut (*x).count - } +## Checking mutability of owned content -This function consumes one `&mut` pointer and returns another with the -same lifetime pointing at a particular field. The borrow for the -`&mut` expression will result in a call to `RESERVE(x, 'a)`, which is -intended to guarantee that `*x` is not later aliased or used to -mutate. But the lifetime of `x` is limited to the current function, -which is a sublifetime of the parameter `'a`, so the rules used for -MUTATE, FREEZE, and ALIAS (which require that the lifetime of the loan -not exceed the lifetime of the variable) would result in an error. +Fields and owned pointers inherit their mutability from +their base expressions, so both of their rules basically +delegate the check to the base expression `LV`: -Nonetheless this function is perfectly legitimate. After all, the -caller has moved in an `&mut` pointer with lifetime `'a`, and thus has -given up their right to mutate the value for the remainder of `'a`. -So it is fine for us to return a pointer with the same lifetime. + MUTABILITY(LV.f, MQ) // M-Field + MUTABILITY(LV, MQ) -The reason that RESERVE differs from the other functions is that -RESERVE is not responsible for guaranteeing that the pointed-to data -will outlive the borrowed pointer being created. After all, `&mut` -values do not own the data they point at. + MUTABILITY(*LV, MQ) // M-Deref-Unique + TYPE(LV) = ~Ty + MUTABILITY(LV, MQ) -### Reserving owned content +## Checking mutability of immutable pointer types -The rules for fields and owned pointers are very straightforward: +Immutable pointer types like `&T` and `@T` can only +be borrowed if MQ is immutable or const: - Reserve-Field: - RESERVE(LV, LT) = LOANS - ------------------------------------------------------------ - RESERVE(LV.f, LT) = LOANS, (LV.F, LT, Total, Reserve) + MUTABILITY(*LV, MQ) // M-Deref-Borrowed-Imm + TYPE(LV) = &Ty + MQ == imm | const - Reserve-Owned-Ptr: - Type(LV) = ~Ty - RESERVE(LV, LT) = LOANS - ------------------------------------------------------------ - RESERVE(*LV, LT) = LOANS, (*LV, LT, Total, Reserve) + MUTABILITY(*LV, MQ) // M-Deref-Managed-Imm + TYPE(LV) = @Ty + MQ == imm | const -### Reserving `&mut` borrowed pointers +## Checking mutability of mutable pointer types -Unlike other borrowed pointers, `&mut` pointers are unaliasable, -so we can reserve them like everything else: +`&mut T` and `@mut T` can be frozen, so it is acceptable to borrow +them as either imm or mut: - Reserve-Mut-Borrowed-Ptr: - Type(LV) = <_P mut Ty - RESERVE(LV, LT) = LOANS - ------------------------------------------------------------ - RESERVE(*LV, LT) = LOANS, (*LV, LT, Total, Reserve) + MUTABILITY(*LV, MQ) // M-Deref-Borrowed-Mut + TYPE(LV) = &mut Ty -## Borrows in calls + MUTABILITY(*LV, MQ) // M-Deref-Managed-Mut + TYPE(LV) = @mut Ty -Earlier we said that the MUTATE, FREEZE, and ALIAS functions were used -to compute the loans resulting from a borrow expression. But this is -not strictly correct, there is a slight complication that occurs with -calls by which additional loans may be necessary. We will explain -that here and give the full details. +# Checking lifetime -Imagine a call expression `'a: E1(E2, E3)`, where `Ei` are some -expressions. If we break this down to something a bit lower-level, it -is kind of short for: +These rules aim to ensure that no data is borrowed for a scope that +exceeds its lifetime. In addition, these rules manage the rooting and +dynamic freezing of `@` and `@mut` values. These two computations wind +up being intimately related. Formally, we define a predicate +`LIFETIME(LV, LT, MQ)`, which states that "the lvalue `LV` can be +safely borrowed for the lifetime `LT` with mutability `MQ`". The Rust +code corresponding to this predicate is the module +`middle::borrowck::gather_loans::lifetime`. - 'a: { - 'a_arg1: let temp1: ... = E1; - 'a_arg2: let temp2: ... = E2; - 'a_arg3: let temp3: ... = E3; - 'a_call: temp1(temp2, temp3) - } +## The Scope function -Here the lifetime labels indicate the various lifetimes. As you can -see there are in fact four relevant lifetimes (only one of which was -named by the user): `'a` corresponds to the expression `E1(E2, E3)` as -a whole. `'a_arg1`, `'a_arg2`, and `'a_arg3` correspond to the -evaluations of `E1`, `E2`, and `E3` respectively. Finally, `'a_call` -corresponds to the *actual call*, which is the point where the values -of the parameters will be used. +Several of the rules refer to a helper function `SCOPE(LV)=LT`. The +`SCOPE(LV)` yields the lifetime `LT` for which the lvalue `LV` is +guaranteed to exist, presuming that no mutations occur. -Now, let's look at a (contrived, but representative) example to see -why all this matters: +The scope of a local variable is the block where it is declared: - struct Foo { f: uint, g: uint } - ... - fn add(p: &mut uint, v: uint) { - *p += v; - } - ... - fn inc(p: &mut uint) -> uint { - *p += 1; *p - } - fn weird() { - let mut x: ~Foo = ~Foo { ... }; - 'a: add(&mut (*x).f, - 'b: inc(&mut (*x).f)) // (*) - } + SCOPE(X) = block where X is declared -The important part is the line marked `(*)` which contains a call to -`add()`. The first argument is a mutable borrow of the field `f`. -The second argument *always borrows* the field `f`. Now, if these two -borrows overlapped in time, this would be illegal, because there would -be two `&mut` pointers pointing at `f`. And, in a way, they *do* -overlap in time, since the first argument will be evaluated first, -meaning that the pointer will exist when the second argument executes. -But in another important way they do not overlap in time. Let's -expand out that final call to `add()` as we did before: +The scope of a field is the scope of the struct: - 'a: { - 'a_arg1: let a_temp1: ... = add; - 'a_arg2: let a_temp2: &'a_call mut uint = &'a_call mut (*x).f; - 'a_arg3_: let a_temp3: uint = { - let b_temp1: ... = inc; - let b_temp2: &'b_call = &'b_call mut (*x).f; - 'b_call: b_temp1(b_temp2) - }; - 'a_call: a_temp1(a_temp2, a_temp3) - } + SCOPE(LV.f) = SCOPE(LV) -When it's written this way, we can see that although there are two -borrows, the first has lifetime `'a_call` and the second has lifetime -`'b_call` and in fact these lifetimes do not overlap. So everything -is fine. +The scope of a unique pointee is the scope of the pointer, since +(barring mutation or moves) the pointer will not be freed until +the pointer itself `LV` goes out of scope: -But this does not mean that there isn't reason for caution! Imagine a -devious program like *this* one: + SCOPE(*LV) = SCOPE(LV) if LV has type ~T - struct Foo { f: uint, g: uint } - ... - fn add(p: &mut uint, v: uint) { - *p += v; - } - ... - fn consume(x: ~Foo) -> uint { - x.f + x.g - } - fn weird() { - let mut x: ~Foo = ~Foo { ... }; - 'a: add(&mut (*x).f, consume(x)) // (*) - } +The scope of a managed pointee is also the scope of the pointer. This +is a conservative approximation, since there may be other aliases fo +that same managed box that would cause it to live longer: -In this case, there is only one borrow, but the second argument is -`consume(x)` instead of a second borrow. Because `consume()` is -declared to take a `~Foo`, it will in fact free the pointer `x` when -it has finished executing. If it is not obvious why this is -troublesome, consider this expanded version of that call: + SCOPE(*LV) = SCOPE(LV) if LV has type @T or @mut T - 'a: { - 'a_arg1: let a_temp1: ... = add; - 'a_arg2: let a_temp2: &'a_call mut uint = &'a_call mut (*x).f; - 'a_arg3_: let a_temp3: uint = { - let b_temp1: ... = consume; - let b_temp2: ~Foo = x; - 'b_call: b_temp1(x) - }; - 'a_call: a_temp1(a_temp2, a_temp3) - } +The scope of a borrowed pointee is the scope associated with the +pointer. This is a conservative approximation, since the data that +the pointer points at may actually live longer: + + SCOPE(*LV) = LT if LV has type &'LT T or &'LT mut T + +## Checking lifetime of variables + +The rule for variables states that a variable can only be borrowed a +lifetime `LT` that is a subregion of the variable's scope: + + LIFETIME(X, LT, MQ) // L-Local + LT <= SCOPE(X) + +## Checking lifetime for owned content + +The lifetime of a field or owned pointer is the same as the lifetime +of its owner: -In this example, we will have borrowed the first argument before `x` -is freed and then free `x` during evaluation of the second -argument. This causes `a_temp2` to be invalidated. + LIFETIME(LV.f, LT, MQ) // L-Field + LIFETIME(LV, LT, MQ) -Of course the loans computed from the borrow expression are supposed -to prevent this situation. But if we just considered the loans from -`MUTATE((*x).f, 'a_call, Total)`, the resulting loans would be: + LIFETIME(*LV, LT, MQ) // L-Deref-Owned + TYPE(LV) = ~Ty + LIFETIME(LV, LT, MQ) - ((*x).f, 'a_call, Total, Mut) - (*x, 'a_call, Partial, Mut) - (x, 'a_call, Partial, Mut) +## Checking lifetime for derefs of borrowed pointers -Because these loans are only in scope for `'a_call`, they do nothing -to prevent the move that occurs evaluating the second argument. +Borrowed pointers have a lifetime `LT'` associated with them. The +data they point at has been guaranteed to be valid for at least this +lifetime. Therefore, the borrow is valid so long as the lifetime `LT` +of the borrow is shorter than the lifetime `LT'` of the pointer +itself: -The way that we solve this is to say that if you have a borrow -expression `&'LT_P mut LV` which itself occurs in the lifetime -`'LT_B`, then the resulting loans are: + LIFETIME(*LV, LT, MQ) // L-Deref-Borrowed + TYPE(LV) = <' Ty OR <' mut Ty + LT <= LT' - MUTATE(LV, LT_P, Total) + ALIAS(LV, LUB(LT_P, LT_B), Total) +## Checking lifetime for derefs of managed, immutable pointers -The call to MUTATE is what we've seen so far. The second part -expresses the idea that the expression LV will be evaluated starting -at LT_B until the end of LT_P. Now, in the normal case, LT_P >= LT_B, -and so the second set of loans that result from a ALIAS are basically -a no-op. However, in the case of an argument where the evaluation of -the borrow occurs before the interval where the resulting pointer will -be used, this ALIAS is important. +Managed pointers are valid so long as the data within them is +*rooted*. There are two ways that this can be achieved. The first is +when the user guarantees such a root will exist. For this to be true, +three conditions must be met: + + LIFETIME(*LV, LT, MQ) // L-Deref-Managed-Imm-User-Root + TYPE(LV) = @Ty + LT <= SCOPE(LV) // (1) + LV is immutable // (2) + LV is not moved or not movable // (3) + +Condition (1) guarantees that the managed box will be rooted for at +least the lifetime `LT` of the borrow, presuming that no mutation or +moves occur. Conditions (2) and (3) then serve to guarantee that the +value is not mutated or moved. Note that lvalues are either +(ultimately) owned by a local variable, in which case we can check +whether that local variable is ever moved in its scope, or they are +owned by the pointee of an (immutable, due to condition 2) managed or +borrowed pointer, in which case moves are not permitted because the +location is aliasable. + +If the conditions of `L-Deref-Managed-Imm-User-Root` are not met, then +there is a second alternative. The compiler can attempt to root the +managed pointer itself. This permits great flexibility, because the +location `LV` where the managed pointer is found does not matter, but +there are some limitations. The lifetime of the borrow can only extend +to the innermost enclosing loop or function body. This guarantees that +the compiler never requires an unbounded amount of stack space to +perform the rooting; if this condition were violated, the compiler +might have to accumulate a list of rooted objects, for example if the +borrow occurred inside the body of a loop but the scope of the borrow +extended outside the loop. More formally, the requirement is that +there is no path starting from the borrow that leads back to the +borrow without crossing the exit from the scope `LT`. + +The rule for compiler rooting is as follows: + + LIFETIME(*LV, LT, MQ) // L-Deref-Managed-Imm-Compiler-Root + TYPE(LV) = @Ty + LT <= innermost enclosing loop/func + ROOT LV at *LV for LT + +Here I have written `ROOT LV at *LV FOR LT` to indicate that the code +makes a note in a side-table that the box `LV` must be rooted into the +stack when `*LV` is evaluated, and that this root can be released when +the scope `LT` exits. + +## Checking lifetime for derefs of managed, mutable pointers + +Loans of the contents of mutable managed pointers are simpler in some +ways that loans of immutable managed pointers, because we can never +rely on the user to root them (since the contents are, after all, +mutable). This means that the burden always falls to the compiler, so +there is only one rule: + + LIFETIME(*LV, LT, MQ) // L-Deref-Managed-Mut-Compiler-Root + TYPE(LV) = @mut Ty + LT <= innermost enclosing loop/func + ROOT LV at *LV for LT + LOCK LV at *LV as MQ for LT + +Note that there is an additional clause this time `LOCK LV at *LV as +MQ for LT`. This clause states that in addition to rooting `LV`, the +compiler should also "lock" the box dynamically, meaning that we +register that the box has been borrowed as mutable or immutable, +depending on `MQ`. This lock will fail if the box has already been +borrowed and either the old loan or the new loan is a mutable loan +(multiple immutable loans are okay). The lock is released as we exit +the scope `LT`. + +# Computing the restrictions + +The final rules govern the computation of *restrictions*, meaning that +we compute the set of actions that will be illegal for the life of the +loan. The predicate is written `RESTRICTIONS(LV, ACTIONS) = +RESTRICTION*`, which can be read "in order to prevent `ACTIONS` from +occuring on `LV`, the restrictions `RESTRICTION*` must be respected +for the lifetime of the loan". + +Note that there is an initial set of restrictions: these restrictions +are computed based on the kind of borrow: + + &mut LV => RESTRICTIONS(LV, MUTATE|CLAIM|FREEZE) + &LV => RESTRICTIONS(LV, MUTATE|CLAIM) + &const LV => RESTRICTIONS(LV, []) + +The reasoning here is that a mutable borrow must be the only writer, +therefore it prevents other writes (`MUTATE`), mutable borrows +(`CLAIM`), and immutable borrows (`FREEZE`). An immutable borrow +permits other immutable borows but forbids writes and mutable borows. +Finally, a const borrow just wants to be sure that the value is not +moved out from under it, so no actions are forbidden. + +## Restrictions for loans of a local variable + +The simplest case is a borrow of a local variable `X`: + + RESTRICTIONS(X, ACTIONS) = (X, ACTIONS) // R-Variable + +In such cases we just record the actions that are not permitted. + +## Restrictions for loans of fields + +Restricting a field is the same as restricting the owner of that +field: + + RESTRICTIONS(LV.f, ACTIONS) = RS, (LV.f, ACTIONS) // R-Field + RESTRICTIONS(LV, ACTIONS) = RS + +The reasoning here is as follows. If the field must not be mutated, +then you must not mutate the owner of the field either, since that +would indirectly modify the field. Similarly, if the field cannot be +frozen or aliased, we cannot allow the owner to be frozen or aliased, +since doing so indirectly freezes/aliases the field. This is the +origin of inherited mutability. + +## Restrictions for loans of owned pointees + +Because the mutability of owned pointees is inherited, restricting an +owned pointee is similar to restricting a field, in that it implies +restrictions on the pointer. However, owned pointers have an important +twist: if the owner `LV` is mutated, that causes the owned pointee +`*LV` to be freed! So whenever an owned pointee `*LV` is borrowed, we +must prevent the owned pointer `LV` from being mutated, which means +that we always add `MUTATE` and `CLAIM` to the restriction set imposed +on `LV`: + + RESTRICTIONS(*LV, ACTIONS) = RS, (*LV, ACTIONS) // R-Deref-Owned-Pointer + TYPE(LV) = ~Ty + RESTRICTIONS(LV, ACTIONS|MUTATE|CLAIM) = RS + +## Restrictions for loans of immutable managed/borrowed pointees + +Immutable managed/borrowed pointees are freely aliasable, meaning that +the compiler does not prevent you from copying the pointer. This +implies that issuing restrictions is useless. We might prevent the +user from acting on `*LV` itself, but there could be another path +`*LV1` that refers to the exact same memory, and we would not be +restricting that path. Therefore, the rule for `&Ty` and `@Ty` +pointers always returns an empty set of restrictions, and it only +permits restricting `MUTATE` and `CLAIM` actions: + + RESTRICTIONS(*LV, ACTIONS) = [] // R-Deref-Imm-Borrowed + TYPE(LV) = &Ty or @Ty + ACTIONS subset of [MUTATE, CLAIM] + +The reason that we can restrict `MUTATE` and `CLAIM` actions even +without a restrictions list is that it is never legal to mutate nor to +borrow mutably the contents of a `&Ty` or `@Ty` pointer. In other +words, those restrictions are already inherent in the type. + +Typically, this limitation is not an issue, because restrictions other +than `MUTATE` or `CLAIM` typically arise due to `&mut` borrow, and as +we said, that is already illegal for `*LV`. However, there is one case +where we can be asked to enforce an `ALIAS` restriction on `*LV`, +which is when you have a type like `&&mut T`. In such cases we will +report an error because we cannot enforce a lack of aliases on a `&Ty` +or `@Ty` type. That case is described in more detail in the section on +mutable borrowed pointers. + +## Restrictions for loans of const aliasable pointees + +Const pointers are read-only. There may be `&mut` or `&` aliases, and +we can not prevent *anything* but moves in that case. So the +`RESTRICTIONS` function is only defined if `ACTIONS` is the empty set. +Because moves from a `&const` or `@const` lvalue are never legal, it +is not necessary to add any restrictions at all to the final +result. + + RESTRICTIONS(*LV, []) = [] // R-Deref-Const-Borrowed + TYPE(LV) = &const Ty or @const Ty + +## Restrictions for loans of mutable borrowed pointees + +Borrowing mutable borrowed pointees is a bit subtle because we permit +users to freeze or claim `&mut` pointees. To see what I mean, consider this +(perfectly safe) code example: + + fn foo(t0: &mut T, op: fn(&T)) { + let t1: &T = &*t0; // (1) + op(t1); + } + +In the borrow marked `(1)`, the data at `*t0` is *frozen* as part of a +re-borrow. Therefore, for the lifetime of `t1`, `*t0` must not be +mutated. This is the same basic idea as when we freeze a mutable local +variable, but unlike in that case `t0` is a *pointer* to the data, and +thus we must enforce some subtle restrictions in order to guarantee +soundness. + +Intuitively, we must ensure that `*t0` is the only *mutable* path to +reach the memory that was frozen. The reason that we are so concerned +with *mutable* paths is that those are the paths through which the +user could mutate the data that was frozen and hence invalidate the +`t1` pointer. Note that const aliases to `*t0` are acceptable (and in +fact we can't prevent them without unacceptable performance cost, more +on that later) because + +There are two rules governing `&mut` pointers, but we'll begin with +the first. This rule governs cases where we are attempting to prevent +an `&mut` pointee from being mutated, claimed, or frozen, as occurs +whenever the `&mut` pointee `*LV` is reborrowed as mutable or +immutable: + + RESTRICTIONS(*LV, ACTIONS) = RS, (*LV, ACTIONS) // R-Deref-Mut-Borrowed-1 + TYPE(LV) = &mut Ty + RESTRICTIONS(LV, MUTATE|CLAIM|ALIAS) = RS + +The main interesting part of the rule is the final line, which +requires that the `&mut` *pointer* `LV` be restricted from being +mutated, claimed, or aliased. The goal of these restrictions is to +ensure that, not considering the pointer that will result from this +borrow, `LV` remains the *sole pointer with mutable access* to `*LV`. + +Restrictions against mutations and claims are necessary because if the +pointer in `LV` were to be somehow copied or moved to a different +location, then the restriction issued for `*LV` would not apply to the +new location. Consider this example, where `*t0` is frozen, but then +`t0` and `t1` are swapped, so by mutating `*t1` the user can mutate +the frozen memory that was originally found at `*t0`: + + fn foo(t0: &mut T, + t1: &mut T) { + let p: &T = &*t0; // Freezes `*t0` + t0 <-> t1; + *t1 = ...; // Invalidates `p` + } + +The restriction against *aliasing* (and, in turn, freezing) is +necessary because, if an alias were of `LV` were to be produced, then +`LV` would no longer be the sole path to access the `&mut` +pointee. Since we are only issuing restrictions against `*LV`, these +other aliases would be unrestricted, and the result would be +unsound. For example: + + fn foo(t0: &mut T) { + let p: &T = &*t0; // Freezes *t0 + let q: &&mut T = &t0; + **q = ...; // Invalidates `p` + } -In the case of our example, it would produce a set of loans like: +The second rule for `&mut` handles the case where we are not adding +any restrictions (beyond the default of "no move"): - ((*x).f, 'a, Total, Const) - (*x, 'a, Total, Const) - (x, 'a, Total, Imm) + RESTRICTIONS(*LV, []) = [] // R-Deref-Mut-Borrowed-2 + TYPE(LV) = &mut Ty -The scope of these loans is `'a = LUB('a_arg2, 'a_call)`, and so they -encompass all subsequent arguments. The first set of loans are Const -loans, which basically just prevent moves. However, when we cross -over the dereference of the owned pointer `x`, the rule for ALIAS -specifies that `x` must be frozen, and hence the final loan is an Imm -loan. In any case the troublesome second argument would be flagged -as an error. +Moving from an `&mut` pointee is never legal, so no special +restrictions are needed. -# Maps that are created +## Restrictions for loans of mutable managed pointees -Borrowck results in two maps. +With `@mut` pointees, we don't make any static guarantees. But as a +convenience, we still register a restriction against `*LV`, because +that way if we *can* find a simple static error, we will: -- `root_map`: identifies those expressions or patterns whose result - needs to be rooted. Conceptually the root_map maps from an - expression or pattern node to a `node_id` identifying the scope for - which the expression must be rooted (this `node_id` should identify - a block or call). The actual key to the map is not an expression id, - however, but a `root_map_key`, which combines an expression id with a - deref count and is used to cope with auto-deref. + RESTRICTIONS(*LV, ACTIONS) = [*LV, ACTIONS] // R-Deref-Managed-Borrowed + TYPE(LV) = @mut Ty */ diff --git a/src/librustc/middle/borrowck/gather_loans/lifetime.rs b/src/librustc/middle/borrowck/gather_loans/lifetime.rs index f7b30e22f0110..9455340268eff 100644 --- a/src/librustc/middle/borrowck/gather_loans/lifetime.rs +++ b/src/librustc/middle/borrowck/gather_loans/lifetime.rs @@ -70,11 +70,11 @@ impl GuaranteeLifetimeContext { match cmt.cat { mc::cat_rvalue | mc::cat_implicit_self | - mc::cat_copied_upvar(*) | - mc::cat_local(*) | - mc::cat_arg(*) | - mc::cat_self(*) | - mc::cat_deref(_, _, mc::region_ptr(*)) | + mc::cat_copied_upvar(*) | // L-Local + mc::cat_local(*) | // L-Local + mc::cat_arg(*) | // L-Local + mc::cat_self(*) | // L-Local + mc::cat_deref(_, _, mc::region_ptr(*)) | // L-Deref-Borrowed mc::cat_deref(_, _, mc::unsafe_ptr) => { let scope = self.scope(cmt); self.check_scope(scope) @@ -90,7 +90,7 @@ impl GuaranteeLifetimeContext { mc::cat_deref(base, derefs, mc::gc_ptr(ptr_mutbl)) => { let base_scope = self.scope(base); - // See rule Freeze-Imm-Managed-Ptr-2 in doc.rs + // L-Deref-Managed-Imm-User-Root let omit_root = ( ptr_mutbl == m_imm && self.bccx.is_subregion_of(self.loan_region, base_scope) && @@ -99,6 +99,8 @@ impl GuaranteeLifetimeContext { ); if !omit_root { + // L-Deref-Managed-Imm-Compiler-Root + // L-Deref-Managed-Mut-Compiler-Root self.check_root(cmt, base, derefs, ptr_mutbl, discr_scope); } else { debug!("omitting root, base=%s, base_scope=%?", @@ -107,8 +109,8 @@ impl GuaranteeLifetimeContext { } mc::cat_downcast(base) | - mc::cat_deref(base, _, mc::uniq_ptr(*)) | - mc::cat_interior(base, _) => { + mc::cat_deref(base, _, mc::uniq_ptr(*)) | // L-Deref-Owned + mc::cat_interior(base, _) => { // L-Field self.check(base, discr_scope) } @@ -321,6 +323,8 @@ impl GuaranteeLifetimeContext { //! lvalue `cmt` is guaranteed to be valid without any //! rooting etc, and presuming `cmt` is not mutated. + // See the SCOPE(LV) function in doc.rs + match cmt.cat { mc::cat_rvalue => { ty::re_scope(self.bccx.tcx.region_maps.cleanup_scope(cmt.id)) diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index 893365bdec195..f733d57573c1a 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -481,6 +481,8 @@ pub impl GatherLoanCtxt { borrow_span: span, cmt: mc::cmt, req_mutbl: ast::mutability) { + //! Implements the M-* rules in doc.rs. + match req_mutbl { m_const => { // Data of any mutability can be lent as const. diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 42b1c40a4b3c4..bf054b72ecf21 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -76,6 +76,7 @@ impl RestrictionsContext { mc::cat_local(local_id) | mc::cat_arg(local_id) | mc::cat_self(local_id) => { + // R-Variable let lp = @LpVar(local_id); SafeIf(lp, ~[Restriction {loan_path: lp, set: restrictions}]) @@ -89,6 +90,8 @@ impl RestrictionsContext { } mc::cat_interior(cmt_base, i) => { + // R-Field + // // Overwriting the base would not change the type of // the memory, so no additional restrictions are // needed. @@ -97,6 +100,8 @@ impl RestrictionsContext { } mc::cat_deref(cmt_base, _, mc::uniq_ptr(*)) => { + // R-Deref-Owned-Pointer + // // When we borrow the interior of an owned pointer, we // cannot permit the base to be mutated, because that // would cause the unique pointer to be freed. @@ -109,16 +114,20 @@ impl RestrictionsContext { mc::cat_implicit_self(*) | mc::cat_deref(_, _, mc::region_ptr(m_imm, _)) | mc::cat_deref(_, _, mc::gc_ptr(m_imm)) => { + // R-Deref-Imm-Borrowed Safe } mc::cat_deref(_, _, mc::region_ptr(m_const, _)) | mc::cat_deref(_, _, mc::gc_ptr(m_const)) => { + // R-Deref-Const-Borrowed self.check_no_mutability_control(cmt, restrictions); Safe } mc::cat_deref(cmt_base, _, mc::gc_ptr(m_mutbl)) => { + // R-Deref-Managed-Borrowed + // // Technically, no restrictions are *necessary* here. // The validity of the borrow is guaranteed // dynamically. However, nonetheless we add a @@ -170,9 +179,11 @@ impl RestrictionsContext { // freezing if it is not aliased. Therefore, in such // cases we restrict aliasing on `cmt_base`. if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) { + // R-Deref-Mut-Borrowed-1 let result = self.compute(cmt_base, restrictions | RESTR_ALIAS); self.extend(result, cmt.mutbl, LpDeref, restrictions) } else { + // R-Deref-Mut-Borrowed-2 let result = self.compute(cmt_base, restrictions); self.extend(result, cmt.mutbl, LpDeref, restrictions) } From bf1647c92a868d49709e3bbbe66d4f2a46e97595 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 26 May 2013 05:48:04 -0400 Subject: [PATCH 3/5] Reconcile docs and code, adding examples and adding RESTR_CLAIM --- src/librustc/middle/borrowck/check_loans.rs | 12 +-- src/librustc/middle/borrowck/doc.rs | 76 +++++++++++++++---- .../middle/borrowck/gather_loans/mod.rs | 4 +- .../borrowck/gather_loans/restrictions.rs | 12 ++- src/librustc/middle/borrowck/mod.rs | 17 +++-- .../borrowck-alias-mut-base-ptr.rs | 15 ++++ ...ck-borrow-mut-base-ptr-in-aliasable-loc.rs | 31 ++++++++ .../borrowck-move-mut-base-ptr.rs | 15 ++++ .../borrowck-swap-mut-base-ptr.rs | 16 ++++ 9 files changed, 165 insertions(+), 33 deletions(-) create mode 100644 src/test/compile-fail/borrowck-alias-mut-base-ptr.rs create mode 100644 src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs create mode 100644 src/test/compile-fail/borrowck-move-mut-base-ptr.rs create mode 100644 src/test/compile-fail/borrowck-swap-mut-base-ptr.rs diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index f2bba4f694a90..9395c3f2a40d3 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -65,7 +65,7 @@ pub fn check_loans(bccx: @BorrowckCtxt, enum MoveError { MoveOk, - MoveWhileBorrowed(/*loan*/@LoanPath, /*loan*/span) + MoveWhileBorrowed(/*move*/@LoanPath, /*loan*/@LoanPath, /*loan*/span) } pub impl<'self> CheckLoanCtxt<'self> { @@ -200,9 +200,9 @@ pub impl<'self> CheckLoanCtxt<'self> { loan1.repr(self.tcx()), loan2.repr(self.tcx())); - // Restrictions that would cause the new loan to be immutable: + // Restrictions that would cause the new loan to be illegal: let illegal_if = match loan2.mutbl { - m_mutbl => RESTR_ALIAS | RESTR_FREEZE | RESTR_MUTATE, + m_mutbl => RESTR_ALIAS | RESTR_FREEZE | RESTR_CLAIM, m_imm => RESTR_ALIAS | RESTR_FREEZE, m_const => RESTR_ALIAS, }; @@ -557,12 +557,12 @@ pub impl<'self> CheckLoanCtxt<'self> { let cmt = self.bccx.cat_expr(ex); match self.analyze_move_out_from_cmt(cmt) { MoveOk => {} - MoveWhileBorrowed(loan_path, loan_span) => { + MoveWhileBorrowed(move_path, loan_path, loan_span) => { self.bccx.span_err( cmt.span, fmt!("cannot move out of `%s` \ because it is borrowed", - self.bccx.loan_path_to_str(loan_path))); + self.bccx.loan_path_to_str(move_path))); self.bccx.span_note( loan_span, fmt!("borrow of `%s` occurs here", @@ -582,7 +582,7 @@ pub impl<'self> CheckLoanCtxt<'self> { for opt_loan_path(cmt).each |&lp| { for self.each_in_scope_restriction(cmt.id, lp) |loan, _| { // Any restriction prevents moves. - return MoveWhileBorrowed(loan.loan_path, loan.span); + return MoveWhileBorrowed(lp, loan.loan_path, loan.span); } } diff --git a/src/librustc/middle/borrowck/doc.rs b/src/librustc/middle/borrowck/doc.rs index 2151c86519a09..5e96e14dbc2cf 100644 --- a/src/librustc/middle/borrowck/doc.rs +++ b/src/librustc/middle/borrowck/doc.rs @@ -580,15 +580,27 @@ borrow, `LV` remains the *sole pointer with mutable access* to `*LV`. Restrictions against mutations and claims are necessary because if the pointer in `LV` were to be somehow copied or moved to a different location, then the restriction issued for `*LV` would not apply to the -new location. Consider this example, where `*t0` is frozen, but then -`t0` and `t1` are swapped, so by mutating `*t1` the user can mutate -the frozen memory that was originally found at `*t0`: - - fn foo(t0: &mut T, - t1: &mut T) { - let p: &T = &*t0; // Freezes `*t0` - t0 <-> t1; - *t1 = ...; // Invalidates `p` +new location. Note that because `&mut` values are non-copyable, a +simple attempt to move the base pointer will fail due to the +(implicit) restriction against moves: + + // src/test/compile-fail/borrowck-move-mut-base-ptr.rs + fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let t1 = t0; //~ ERROR cannot move out of `t0` + *t1 = 22; + } + +However, the additional restrictions against mutation mean that even a +clever attempt to use a swap to circumvent the type system will +encounter an error: + + // src/test/compile-fail/borrowck-swap-mut-base-ptr.rs + fn foo<'a>(mut t0: &'a mut int, + mut t1: &'a mut int) { + let p: &int = &*t0; // Freezes `*t0` + swap(&mut t0, &mut t1); //~ ERROR cannot borrow `t0` + *t1 = 22; } The restriction against *aliasing* (and, in turn, freezing) is @@ -598,12 +610,32 @@ pointee. Since we are only issuing restrictions against `*LV`, these other aliases would be unrestricted, and the result would be unsound. For example: - fn foo(t0: &mut T) { - let p: &T = &*t0; // Freezes *t0 - let q: &&mut T = &t0; - **q = ...; // Invalidates `p` + // src/test/compile-fail/borrowck-alias-mut-base-ptr.rs + fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let q: &const &mut int = &const t0; //~ ERROR cannot borrow `t0` + **q = 22; // (*) } +Note that the current rules also report an error at the assignment in +`(*)`, because we only permit `&mut` poiners to be assigned if they +are located in a non-aliasable location. However, I do not believe +this restriction is strictly necessary. It was added, I believe, to +discourage `&mut` from being placed in aliasable locations in the +first place. One (desirable) side-effect of restricting aliasing on +`LV` is that borrowing an `&mut` pointee found inside an aliasable +pointee yields an error: + + // src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc: + fn foo(t0: & &mut int) { + let t1 = t0; + let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&` pointer + **t1 = 22; // (*) + } + +Here at the line `(*)` you will also see the error I referred to +above, which I do not believe is strictly necessary. + The second rule for `&mut` handles the case where we are not adding any restrictions (beyond the default of "no move"): @@ -622,4 +654,22 @@ that way if we *can* find a simple static error, we will: RESTRICTIONS(*LV, ACTIONS) = [*LV, ACTIONS] // R-Deref-Managed-Borrowed TYPE(LV) = @mut Ty +# Some notes for future work + +While writing up these docs, I encountered some rules I believe to be +stricter than necessary: + +- I think the restriction against mutating `&mut` pointers found in an + aliasable location is unnecessary. They cannot be reborrowed, to be sure, + so it should be safe to mutate them. Lifting this might cause some common + cases (`&mut int`) to work just fine, but might lead to further confusion + in other cases, so maybe it's best to leave it as is. +- I think restricting the `&mut` LV against moves and `ALIAS` is sufficient, + `MUTATE` and `CLAIM` are overkill. `MUTATE` was necessary when swap was + a built-in operator, but as it is not, it is implied by `CLAIM`, + and `CLAIM` is implied by `ALIAS`. The only net effect of this is an + extra error message in some cases, though. +- I have not described how closures interact. Current code is unsound. + I am working on describing and implementing the fix. + */ diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index f733d57573c1a..c2ae364e54ce8 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -517,8 +517,8 @@ pub impl GatherLoanCtxt { fn restriction_set(&self, req_mutbl: ast::mutability) -> RestrictionSet { match req_mutbl { m_const => RESTR_EMPTY, - m_imm => RESTR_EMPTY | RESTR_MUTATE, - m_mutbl => RESTR_EMPTY | RESTR_MUTATE | RESTR_FREEZE + m_imm => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM, + m_mutbl => RESTR_EMPTY | RESTR_MUTATE | RESTR_CLAIM | RESTR_FREEZE } } diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index bf054b72ecf21..4527cdec782da 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -86,7 +86,7 @@ impl RestrictionsContext { // When we borrow the interior of an enum, we have to // ensure the enum itself is not mutated, because that // could cause the type of the memory to change. - self.compute(cmt_base, restrictions | RESTR_MUTATE) + self.compute(cmt_base, restrictions | RESTR_MUTATE | RESTR_CLAIM) } mc::cat_interior(cmt_base, i) => { @@ -105,7 +105,9 @@ impl RestrictionsContext { // When we borrow the interior of an owned pointer, we // cannot permit the base to be mutated, because that // would cause the unique pointer to be freed. - let result = self.compute(cmt_base, restrictions | RESTR_MUTATE); + let result = self.compute( + cmt_base, + restrictions | RESTR_MUTATE | RESTR_CLAIM); self.extend(result, cmt.mutbl, LpDeref, restrictions) } @@ -178,7 +180,9 @@ impl RestrictionsContext { // mutability, we can only prevent mutation or prevent // freezing if it is not aliased. Therefore, in such // cases we restrict aliasing on `cmt_base`. - if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) { + if restrictions.intersects(RESTR_MUTATE | + RESTR_CLAIM | + RESTR_FREEZE) { // R-Deref-Mut-Borrowed-1 let result = self.compute(cmt_base, restrictions | RESTR_ALIAS); self.extend(result, cmt.mutbl, LpDeref, restrictions) @@ -244,7 +248,7 @@ impl RestrictionsContext { fn check_no_mutability_control(&self, cmt: mc::cmt, restrictions: RestrictionSet) { - if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE) { + if restrictions.intersects(RESTR_MUTATE | RESTR_FREEZE | RESTR_CLAIM) { self.bccx.report(BckError {span: self.span, cmt: cmt, code: err_freeze_aliasable_const}); diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 4554fde15fad5..b8c1504dbd075 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -301,10 +301,10 @@ pub fn opt_loan_path(cmt: mc::cmt) -> Option<@LoanPath> { // Borrowing an lvalue often results in *restrictions* that limit what // can be done with this lvalue during the scope of the loan: // -// - `RESTR_MUTATE`: The lvalue may not be modified and mutable pointers to -// the value cannot be created. -// - `RESTR_FREEZE`: Immutable pointers to the value cannot be created. -// - `RESTR_ALIAS`: The lvalue may not be aliased in any way. +// - `RESTR_MUTATE`: The lvalue may not be modified. +// - `RESTR_CLAIM`: `&mut` borrows of the lvalue are forbidden. +// - `RESTR_FREEZE`: `&` borrows of the lvalue are forbidden. +// - `RESTR_ALIAS`: All borrows of the lvalue are forbidden. // // In addition, no value which is restricted may be moved. Therefore, // restrictions are meaningful even if the RestrictionSet is empty, @@ -319,10 +319,11 @@ pub struct RestrictionSet { bits: u32 } -pub static RESTR_EMPTY: RestrictionSet = RestrictionSet {bits: 0b000}; -pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b001}; -pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b010}; -pub static RESTR_ALIAS: RestrictionSet = RestrictionSet {bits: 0b100}; +pub static RESTR_EMPTY: RestrictionSet = RestrictionSet {bits: 0b0000}; +pub static RESTR_MUTATE: RestrictionSet = RestrictionSet {bits: 0b0001}; +pub static RESTR_CLAIM: RestrictionSet = RestrictionSet {bits: 0b0010}; +pub static RESTR_FREEZE: RestrictionSet = RestrictionSet {bits: 0b0100}; +pub static RESTR_ALIAS: RestrictionSet = RestrictionSet {bits: 0b1000}; pub impl RestrictionSet { fn intersects(&self, restr: RestrictionSet) -> bool { diff --git a/src/test/compile-fail/borrowck-alias-mut-base-ptr.rs b/src/test/compile-fail/borrowck-alias-mut-base-ptr.rs new file mode 100644 index 0000000000000..c51cf5b9538d9 --- /dev/null +++ b/src/test/compile-fail/borrowck-alias-mut-base-ptr.rs @@ -0,0 +1,15 @@ +// Test that attempt to alias `&mut` pointer while pointee is borrowed +// yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let q: &const &mut int = &const t0; //~ ERROR cannot borrow `t0` + **q = 22; //~ ERROR cannot assign to an `&mut` in a `&const` pointer +} + +fn main() { +} \ No newline at end of file diff --git a/src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs b/src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs new file mode 100644 index 0000000000000..7e9c298ba4732 --- /dev/null +++ b/src/test/compile-fail/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs @@ -0,0 +1,31 @@ +// Test that attempt to reborrow an `&mut` pointer in an aliasable +// location yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo(t0: & &mut int) { + let t1 = t0; + let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&` pointer + **t1 = 22; //~ ERROR cannot assign +} + +fn foo2(t0: &const &mut int) { + // Note: reborrowing from an &const actually yields two errors, since it + // is unsafe in two ways: we can't control the aliasing, and we can't + // control the mutation. + let t1 = t0; + let p: &int = &**t0; //~ ERROR cannot borrow an `&mut` in a `&const` pointer + //~^ ERROR unsafe borrow of aliasable, const value + **t1 = 22; //~ ERROR cannot assign +} + +fn foo3(t0: &mut &mut int) { + let t1 = &mut *t0; + let p: &int = &**t0; //~ ERROR cannot borrow + **t1 = 22; +} + +fn main() { +} \ No newline at end of file diff --git a/src/test/compile-fail/borrowck-move-mut-base-ptr.rs b/src/test/compile-fail/borrowck-move-mut-base-ptr.rs new file mode 100644 index 0000000000000..6a3832d2304cf --- /dev/null +++ b/src/test/compile-fail/borrowck-move-mut-base-ptr.rs @@ -0,0 +1,15 @@ +// Test that attempt to move `&mut` pointer while pointee is borrowed +// yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo(t0: &mut int) { + let p: &int = &*t0; // Freezes `*t0` + let t1 = t0; //~ ERROR cannot move out of `t0` + *t1 = 22; +} + +fn main() { +} \ No newline at end of file diff --git a/src/test/compile-fail/borrowck-swap-mut-base-ptr.rs b/src/test/compile-fail/borrowck-swap-mut-base-ptr.rs new file mode 100644 index 0000000000000..bea5f1f6ea765 --- /dev/null +++ b/src/test/compile-fail/borrowck-swap-mut-base-ptr.rs @@ -0,0 +1,16 @@ +// Test that attempt to swap `&mut` pointer while pointee is borrowed +// yields an error. +// +// Example from src/middle/borrowck/doc.rs + +use std::util::swap; + +fn foo<'a>(mut t0: &'a mut int, + mut t1: &'a mut int) { + let p: &int = &*t0; // Freezes `*t0` + swap(&mut t0, &mut t1); //~ ERROR cannot borrow `t0` + *t1 = 22; +} + +fn main() { +} \ No newline at end of file From e35db1ab35caf9318080ef293135546af5661172 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 27 May 2013 05:43:56 -0400 Subject: [PATCH 4/5] Reconcile treatment of &mut with the docs --- src/librustc/middle/borrowck/check_loans.rs | 4 +-- .../borrowck/gather_loans/restrictions.rs | 29 ++++++++++--------- src/librustc/middle/borrowck/mod.rs | 1 + 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 9395c3f2a40d3..183771956eae0 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -657,12 +657,12 @@ fn check_loans_in_fn<'a>(fk: &visit::fn_kind, let move_err = this.analyze_move_out_from_cmt(cmt); match move_err { MoveOk => {} - MoveWhileBorrowed(loan_path, loan_span) => { + MoveWhileBorrowed(move_path, loan_path, loan_span) => { this.bccx.span_err( cap_var.span, fmt!("cannot move `%s` into closure \ because it is borrowed", - this.bccx.loan_path_to_str(loan_path))); + this.bccx.loan_path_to_str(move_path))); this.bccx.span_note( loan_span, fmt!("borrow of `%s` occurs here", diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 4527cdec782da..82a3b145e1d5d 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -33,7 +33,7 @@ pub fn compute_restrictions(bccx: @BorrowckCtxt, cmt_original: cmt }; - ctxt.compute(cmt, restr) + ctxt.restrict(cmt, restr) } /////////////////////////////////////////////////////////////////////////// @@ -50,9 +50,9 @@ impl RestrictionsContext { self.bccx.tcx } - fn compute(&self, - cmt: mc::cmt, - restrictions: RestrictionSet) -> RestrictionResult { + fn restrict(&self, + cmt: mc::cmt, + restrictions: RestrictionSet) -> RestrictionResult { // Check for those cases where we cannot control the aliasing // and make sure that we are not being asked to. @@ -86,7 +86,9 @@ impl RestrictionsContext { // When we borrow the interior of an enum, we have to // ensure the enum itself is not mutated, because that // could cause the type of the memory to change. - self.compute(cmt_base, restrictions | RESTR_MUTATE | RESTR_CLAIM) + self.restrict( + cmt_base, + restrictions | RESTR_MUTATE | RESTR_CLAIM) } mc::cat_interior(cmt_base, i) => { @@ -95,7 +97,7 @@ impl RestrictionsContext { // Overwriting the base would not change the type of // the memory, so no additional restrictions are // needed. - let result = self.compute(cmt_base, restrictions); + let result = self.restrict(cmt_base, restrictions); self.extend(result, cmt.mutbl, LpInterior(i), restrictions) } @@ -105,7 +107,7 @@ impl RestrictionsContext { // When we borrow the interior of an owned pointer, we // cannot permit the base to be mutated, because that // would cause the unique pointer to be freed. - let result = self.compute( + let result = self.restrict( cmt_base, restrictions | RESTR_MUTATE | RESTR_CLAIM); self.extend(result, cmt.mutbl, LpDeref, restrictions) @@ -180,16 +182,15 @@ impl RestrictionsContext { // mutability, we can only prevent mutation or prevent // freezing if it is not aliased. Therefore, in such // cases we restrict aliasing on `cmt_base`. - if restrictions.intersects(RESTR_MUTATE | - RESTR_CLAIM | - RESTR_FREEZE) { + if restrictions != RESTR_EMPTY { // R-Deref-Mut-Borrowed-1 - let result = self.compute(cmt_base, restrictions | RESTR_ALIAS); + let result = self.restrict( + cmt_base, + RESTR_ALIAS | RESTR_MUTATE | RESTR_CLAIM); self.extend(result, cmt.mutbl, LpDeref, restrictions) } else { // R-Deref-Mut-Borrowed-2 - let result = self.compute(cmt_base, restrictions); - self.extend(result, cmt.mutbl, LpDeref, restrictions) + Safe } } @@ -200,7 +201,7 @@ impl RestrictionsContext { mc::cat_stack_upvar(cmt_base) | mc::cat_discr(cmt_base, _) => { - self.compute(cmt_base, restrictions) + self.restrict(cmt_base, restrictions) } } } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index b8c1504dbd075..1babf08aa705c 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -315,6 +315,7 @@ pub struct Restriction { set: RestrictionSet } +#[deriving(Eq)] pub struct RestrictionSet { bits: u32 } From f30b53892962ebe0511821c1a9d20b644ecb0e02 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 28 May 2013 09:33:31 -0400 Subject: [PATCH 5/5] Update documentation for moves --- src/librustc/middle/borrowck/doc.rs | 257 +++++++++++++++++++--- src/librustc/middle/borrowck/move_data.rs | 57 ++++- src/librustc/middle/moves.rs | 80 +------ 3 files changed, 270 insertions(+), 124 deletions(-) diff --git a/src/librustc/middle/borrowck/doc.rs b/src/librustc/middle/borrowck/doc.rs index 5e96e14dbc2cf..cb3983117e97c 100644 --- a/src/librustc/middle/borrowck/doc.rs +++ b/src/librustc/middle/borrowck/doc.rs @@ -19,15 +19,27 @@ how it works, and then proceed to dive into the theoretical background. Finally, they go into detail on some of the more subtle aspects. +# Table of contents + +These docs are long. Search for the section you are interested in. + +- Overview +- Formal model +- Borrowing and loans +- Moves and initialization +- Future work + # Overview The borrow checker checks one function at a time. It operates in two passes. The first pass, called `gather_loans`, walks over the function -and identifies all of the places where borrows occur (e.g., `&` -expressions and `ref` bindings). For each borrow, we check various -basic safety conditions at this time (for example, that the lifetime -of the borrow doesn't exceed the lifetime of the value being -borrowed). +and identifies all of the places where borrows (e.g., `&` expressions +and `ref` bindings) and moves (copies or captures of a linear value) +occur. It also tracks initialization sites. For each borrow and move, +it checks various basic safety conditions at this time (for example, +that the lifetime of the borrow doesn't exceed the lifetime of the +value being borrowed, or that there is no move out of an `&T` +pointee). It then uses the dataflow module to propagate which of those borrows may be in scope at each point in the procedure. A loan is considered @@ -41,8 +53,8 @@ it is safe with respect to the in-scope loans. # Formal model -Let's consider a simple subset of Rust in which you can only borrow -from lvalues like so: +Throughout the docs we'll consider a simple subset of Rust in which +you can only borrow from lvalues, defined like so: LV = x | LV.f | *LV @@ -65,9 +77,11 @@ struct name and we assume structs are declared like so: SD = struct S<'LT...> { (f: TY)... } -# An intuitive explanation +# Borrowing and loans -## Issuing loans +## An intuitive explanation + +### Issuing loans Now, imagine we had a program like this: @@ -83,7 +97,7 @@ This is of course dangerous because mutating `x` will free the old value and hence invalidate `y`. The borrow checker aims to prevent this sort of thing. -### Loans and restrictions +#### Loans and restrictions The way the borrow checker works is that it analyzes each borrow expression (in our simple model, that's stuff like `&LV`, though in @@ -120,7 +134,7 @@ prevents moves from `LV`. I chose not to make `MOVE` a fourth kind of action because that would imply that sometimes moves are permitted from restrictived values, which is not the case. -### Example +#### Example To give you a better feeling for what kind of restrictions derived from a loan, let's look at the loan `L` that would be issued as a @@ -159,7 +173,7 @@ because `x` is an owned pointer, the path `x` owns the path `*x`. Therefore, borrowing `(*x).f` yields restrictions on both `*x` and `x`. -## Checking for illegal assignments, moves, and reborrows +### Checking for illegal assignments, moves, and reborrows Once we have computed the loans introduced by each borrow, the borrow checker uses a data flow propagation to compute the full set of loans @@ -181,7 +195,7 @@ The kinds of expressions which in-scope loans can render illegal are: - *read-only borrows* (`&const lv`): illegal there is an in-scope restriction against aliasing `lv`. -# Formal rules +## Formal rules Now that we hopefully have some kind of intuitive feeling for how the borrow checker works, let's look a bit more closely now at the precise @@ -202,7 +216,7 @@ of this rule: there are comments in the borrowck source referencing these names, so that you can cross reference to find the actual code that corresponds to the formal rule. -## The `gather_loans` pass +### The `gather_loans` pass We start with the `gather_loans` pass, which walks the AST looking for borrows. For each borrow, there are three bits of information: the @@ -223,7 +237,7 @@ alive and for dynamically freezing `@mut` boxes. restrictions to maintain memory safety. These are the restrictions that will go into the final loan. We'll discuss in more detail below. -# Checking mutability +## Checking mutability Checking mutability is fairly straightforward. We just want to prevent immutable data from being borrowed as mutable. Note that it is ok to @@ -233,7 +247,7 @@ defined, means that "borrowing `LV` with mutability `MQ` is ok. The Rust code corresponding to this predicate is the function `check_mutability` in `middle::borrowck::gather_loans`. -## Checking mutability of variables +### Checking mutability of variables *Code pointer:* Function `check_mutability()` in `gather_loans/mod.rs`, but also the code in `mem_categorization`. @@ -249,7 +263,7 @@ otherwise the variable must be borrowed as immutable or const: DECL(X) = imm MQ = imm | const -## Checking mutability of owned content +### Checking mutability of owned content Fields and owned pointers inherit their mutability from their base expressions, so both of their rules basically @@ -262,7 +276,7 @@ delegate the check to the base expression `LV`: TYPE(LV) = ~Ty MUTABILITY(LV, MQ) -## Checking mutability of immutable pointer types +### Checking mutability of immutable pointer types Immutable pointer types like `&T` and `@T` can only be borrowed if MQ is immutable or const: @@ -275,7 +289,7 @@ be borrowed if MQ is immutable or const: TYPE(LV) = @Ty MQ == imm | const -## Checking mutability of mutable pointer types +### Checking mutability of mutable pointer types `&mut T` and `@mut T` can be frozen, so it is acceptable to borrow them as either imm or mut: @@ -286,7 +300,7 @@ them as either imm or mut: MUTABILITY(*LV, MQ) // M-Deref-Managed-Mut TYPE(LV) = @mut Ty -# Checking lifetime +## Checking lifetime These rules aim to ensure that no data is borrowed for a scope that exceeds its lifetime. In addition, these rules manage the rooting and @@ -297,7 +311,7 @@ safely borrowed for the lifetime `LT` with mutability `MQ`". The Rust code corresponding to this predicate is the module `middle::borrowck::gather_loans::lifetime`. -## The Scope function +### The Scope function Several of the rules refer to a helper function `SCOPE(LV)=LT`. The `SCOPE(LV)` yields the lifetime `LT` for which the lvalue `LV` is @@ -329,7 +343,7 @@ the pointer points at may actually live longer: SCOPE(*LV) = LT if LV has type &'LT T or &'LT mut T -## Checking lifetime of variables +### Checking lifetime of variables The rule for variables states that a variable can only be borrowed a lifetime `LT` that is a subregion of the variable's scope: @@ -337,7 +351,7 @@ lifetime `LT` that is a subregion of the variable's scope: LIFETIME(X, LT, MQ) // L-Local LT <= SCOPE(X) -## Checking lifetime for owned content +### Checking lifetime for owned content The lifetime of a field or owned pointer is the same as the lifetime of its owner: @@ -349,7 +363,7 @@ of its owner: TYPE(LV) = ~Ty LIFETIME(LV, LT, MQ) -## Checking lifetime for derefs of borrowed pointers +### Checking lifetime for derefs of borrowed pointers Borrowed pointers have a lifetime `LT'` associated with them. The data they point at has been guaranteed to be valid for at least this @@ -361,7 +375,7 @@ itself: TYPE(LV) = <' Ty OR <' mut Ty LT <= LT' -## Checking lifetime for derefs of managed, immutable pointers +### Checking lifetime for derefs of managed, immutable pointers Managed pointers are valid so long as the data within them is *rooted*. There are two ways that this can be achieved. The first is @@ -410,7 +424,7 @@ makes a note in a side-table that the box `LV` must be rooted into the stack when `*LV` is evaluated, and that this root can be released when the scope `LT` exits. -## Checking lifetime for derefs of managed, mutable pointers +### Checking lifetime for derefs of managed, mutable pointers Loans of the contents of mutable managed pointers are simpler in some ways that loans of immutable managed pointers, because we can never @@ -433,7 +447,7 @@ borrowed and either the old loan or the new loan is a mutable loan (multiple immutable loans are okay). The lock is released as we exit the scope `LT`. -# Computing the restrictions +## Computing the restrictions The final rules govern the computation of *restrictions*, meaning that we compute the set of actions that will be illegal for the life of the @@ -456,7 +470,7 @@ permits other immutable borows but forbids writes and mutable borows. Finally, a const borrow just wants to be sure that the value is not moved out from under it, so no actions are forbidden. -## Restrictions for loans of a local variable +### Restrictions for loans of a local variable The simplest case is a borrow of a local variable `X`: @@ -464,7 +478,7 @@ The simplest case is a borrow of a local variable `X`: In such cases we just record the actions that are not permitted. -## Restrictions for loans of fields +### Restrictions for loans of fields Restricting a field is the same as restricting the owner of that field: @@ -479,7 +493,7 @@ frozen or aliased, we cannot allow the owner to be frozen or aliased, since doing so indirectly freezes/aliases the field. This is the origin of inherited mutability. -## Restrictions for loans of owned pointees +### Restrictions for loans of owned pointees Because the mutability of owned pointees is inherited, restricting an owned pointee is similar to restricting a field, in that it implies @@ -494,7 +508,7 @@ on `LV`: TYPE(LV) = ~Ty RESTRICTIONS(LV, ACTIONS|MUTATE|CLAIM) = RS -## Restrictions for loans of immutable managed/borrowed pointees +### Restrictions for loans of immutable managed/borrowed pointees Immutable managed/borrowed pointees are freely aliasable, meaning that the compiler does not prevent you from copying the pointer. This @@ -523,7 +537,7 @@ report an error because we cannot enforce a lack of aliases on a `&Ty` or `@Ty` type. That case is described in more detail in the section on mutable borrowed pointers. -## Restrictions for loans of const aliasable pointees +### Restrictions for loans of const aliasable pointees Const pointers are read-only. There may be `&mut` or `&` aliases, and we can not prevent *anything* but moves in that case. So the @@ -535,7 +549,7 @@ result. RESTRICTIONS(*LV, []) = [] // R-Deref-Const-Borrowed TYPE(LV) = &const Ty or @const Ty -## Restrictions for loans of mutable borrowed pointees +### Restrictions for loans of mutable borrowed pointees Borrowing mutable borrowed pointees is a bit subtle because we permit users to freeze or claim `&mut` pointees. To see what I mean, consider this @@ -645,7 +659,7 @@ any restrictions (beyond the default of "no move"): Moving from an `&mut` pointee is never legal, so no special restrictions are needed. -## Restrictions for loans of mutable managed pointees +### Restrictions for loans of mutable managed pointees With `@mut` pointees, we don't make any static guarantees. But as a convenience, we still register a restriction against `*LV`, because @@ -654,7 +668,159 @@ that way if we *can* find a simple static error, we will: RESTRICTIONS(*LV, ACTIONS) = [*LV, ACTIONS] // R-Deref-Managed-Borrowed TYPE(LV) = @mut Ty -# Some notes for future work +# Moves and initialization + +The borrow checker is also in charge of ensuring that: + +- all memory which is accessed is initialized +- immutable local variables are assigned at most once. + +These are two separate dataflow analyses built on the same +framework. Let's look at checking that memory is initialized first; +the checking of immutable local variabe assignments works in a very +similar way. + +To track the initialization of memory, we actually track all the +points in the program that *create uninitialized memory*, meaning +moves and the declaration of uninitialized variables. For each of +these points, we create a bit in the dataflow set. Assignments to a +variable `x` or path `a.b.c` kill the move/uninitialization bits for +those paths and any subpaths (e.g., `x`, `x.y`, `a.b.c`, `*a.b.c`). +The bits are also killed when the root variables (`x`, `a`) go out of +scope. Bits are unioned when two control-flow paths join. Thus, the +presence of a bit indicates that the move may have occurred without an +intervening assignment to the same memory. At each use of a variable, +we examine the bits in scope, and check that none of them are +moves/uninitializations of the variable that is being used. + +Let's look at a simple example: + + fn foo(a: ~int) { + let b: ~int; // Gen bit 0. + + if cond { // Bits: 0 + use(&*a); + b = a; // Gen bit 1, kill bit 0. + use(&*b); + } else { + // Bits: 0 + } + // Bits: 0,1 + use(&*a); // Error. + use(&*b); // Error. + } + + fn use(a: &int) { } + +In this example, the variable `b` is created uninitialized. In one +branch of an `if`, we then move the variable `a` into `b`. Once we +exit the `if`, therefore, it is an error to use `a` or `b` since both +are only conditionally initialized. I have annotated the dataflow +state using comments. There are two dataflow bits, with bit 0 +corresponding to the creation of `b` without an initializer, and bit 1 +corresponding to the move of `a`. The assignment `b = a` both +generates bit 1, because it is a move of `a`, and kills bit 0, because +`b` is now initialized. On the else branch, though, `b` is never +initialized, and so bit 0 remains untouched. When the two flows of +control join, we union the bits from both sides, resulting in both +bits 0 and 1 being set. Thus any attempt to use `a` uncovers the bit 1 +from the "then" branch, showing that `a` may be moved, and any attempt +to use `b` uncovers bit 0, from the "else" branch, showing that `b` +may not be initialized. + +## Initialization of immutable variables + +Initialization of immutable variables works in a very similar way, +except that: + +1. we generate bits for each assignment to a variable; +2. the bits are never killed except when the variable goes out of scope. + +Thus the presence of an assignment bit indicates that the assignment +may have occurred. Note that assignments are only killed when the +variable goes out of scope, as it is not relevant whether or not there +has been a move in the meantime. Using these bits, we can declare that +an assignment to an immutable variable is legal iff there is no other +assignment bit to that same variable in scope. + +## Why is the design made this way? + +It may seem surprising that we assign dataflow bits to *each move* +rather than *each path being moved*. This is somewhat less efficient, +since on each use, we must iterate through all moves and check whether +any of them correspond to the path in question. Similar concerns apply +to the analysis for double assignments to immutable variables. The +main reason to do it this way is that it allows us to print better +error messages, because when a use occurs, we can print out the +precise move that may be in scope, rather than simply having to say +"the variable may not be initialized". + +## Data structures used in the move analysis + +The move analysis maintains several data structures that enable it to +cross-reference moves and assignments to determine when they may be +moving/assigning the same memory. These are all collected into the +`MoveData` and `FlowedMoveData` structs. The former represents the set +of move paths, moves, and assignments, and the latter adds in the +results of a dataflow computation. + +### Move paths + +The `MovePath` tree tracks every path that is moved or assigned to. +These paths have the same form as the `LoanPath` data structure, which +in turn is the "real world version of the lvalues `LV` that we +introduced earlier. The difference between a `MovePath` and a `LoanPath` +is that move paths are: + +1. Canonicalized, so that we have exactly one copy of each, and + we can refer to move paths by index; +2. Cross-referenced with other paths into a tree, so that given a move + path we can efficiently find all parent move paths and all + extensions (e.g., given the `a.b` move path, we can easily find the + move path `a` and also the move paths `a.b.c`) +3. Cross-referenced with moves and assignments, so that we can + easily find all moves and assignments to a given path. + +The mechanism that we use is to create a `MovePath` record for each +move path. These are arranged in an array and are referenced using +`MovePathIndex` values, which are newtype'd indices. The `MovePath` +structs are arranged into a tree, representing using the standard +Knuth representation where each node has a child 'pointer' and a "next +sibling" 'pointer'. In addition, each `MovePath` has a parent +'pointer'. In this case, the 'pointers' are just `MovePathIndex` +values. + +In this way, if we want to find all base paths of a given move path, +we can just iterate up the parent pointers (see `each_base_path()` in +the `move_data` module). If we want to find all extensions, we can +iterate through the subtree (see `each_extending_path()`). + +### Moves and assignments + +There are structs to represent moves (`Move`) and assignments +(`Assignment`), and these are also placed into arrays and referenced +by index. All moves of a particular path are arranged into a linked +lists, beginning with `MovePath.first_move` and continuing through +`Move.next_move`. + +We distinguish between "var" assignments, which are assignments to a +variable like `x = foo`, and "path" assignments (`x.f = foo`). This +is because we need to assign dataflows to the former, but not the +latter, so as to check for double initialization of immutable +variables. + +### Gathering and checking moves + +Like loans, we distinguish two phases. The first, gathering, is where +we uncover all the moves and assignments. As with loans, we do some +basic sanity checking in this phase, so we'll report errors if you +attempt to move out of a borrowed pointer etc. Then we do the dataflow +(see `FlowedMoveData::new`). Finally, in the `check_loans.rs` code, we +walk back over, identify all uses, assignments, and captures, and +check that they are legal given the set of dataflow bits we have +computed for that program point. + +# Future work While writing up these docs, I encountered some rules I believe to be stricter than necessary: @@ -671,5 +837,26 @@ stricter than necessary: extra error message in some cases, though. - I have not described how closures interact. Current code is unsound. I am working on describing and implementing the fix. +- If we wish, we can easily extend the move checking to allow finer-grained + tracking of what is initialized and what is not, enabling code like + this: + + a = x.f.g; // x.f.g is now uninitialized + // here, x and x.f are not usable, but x.f.h *is* + x.f.g = b; // x.f.g is not initialized + // now x, x.f, x.f.g, x.f.h are all usable + + What needs to change here, most likely, is that the `moves` module + should record not only what paths are moved, but what expressions + are actual *uses*. For example, the reference to `x` in `x.f.g = b` + is not a true *use* in the sense that it requires `x` to be fully + initialized. This is in fact why the above code produces an error + today: the reference to `x` in `x.f.g = b` is considered illegal + because `x` is not fully initialized. + +There are also some possible refactorings: + +- It might be nice to replace all loan paths with the MovePath mechanism, + since they allow lightweight comparison using an integer. */ diff --git a/src/librustc/middle/borrowck/move_data.rs b/src/librustc/middle/borrowck/move_data.rs index 84bd7ecb1a167..91962f17d596f 100644 --- a/src/librustc/middle/borrowck/move_data.rs +++ b/src/librustc/middle/borrowck/move_data.rs @@ -10,7 +10,8 @@ /*! -Data structures used for tracking moves. +Data structures used for tracking moves. Please see the extensive +comments in the section "Moves and initialization" and in `doc.rs`. */ @@ -29,11 +30,24 @@ use syntax::opt_vec; use util::ppaux::Repr; pub struct MoveData { + /// Move paths. See section "Move paths" in `doc.rs`. paths: ~[MovePath], + + /// Cache of loan path to move path index, for easy lookup. path_map: HashMap<@LoanPath, MovePathIndex>, + + /// Each move or uninitialized variable gets an entry here. moves: ~[Move], - path_assignments: ~[Assignment], + + /// Assignments to a variable, like `x = foo`. These are assigned + /// bits for dataflow, since we must track them to ensure that + /// immutable variables are assigned at most once along each path. var_assignments: ~[Assignment], + + /// Assignments to a path, like `x.f = foo`. These are not + /// assigned dataflow bits, but we track them because they still + /// kill move bits. + path_assignments: ~[Assignment], assignee_ids: HashSet, } @@ -43,34 +57,45 @@ pub struct FlowedMoveData { // It makes me sad to use @mut here, except that due to // the visitor design, this is what gather_loans // must produce. + dfcx_moves: MoveDataFlow, + + // We could (and maybe should, for efficiency) combine both move + // and assign data flow into one, but this way it's easier to + // distinguish the bits that correspond to moves and assignments. dfcx_assign: AssignDataFlow } +/// Index into `MoveData.paths`, used like a pointer #[deriving(Eq)] pub struct MovePathIndex(uint); static InvalidMovePathIndex: MovePathIndex = MovePathIndex(uint::max_value); +/// Index into `MoveData.moves`, used like a pointer #[deriving(Eq)] pub struct MoveIndex(uint); static InvalidMoveIndex: MoveIndex = MoveIndex(uint::max_value); -#[deriving(Eq)] -pub struct VarAssignmentIndex(uint); - -static InvalidVarAssignmentIndex: VarAssignmentIndex = - VarAssignmentIndex(uint::max_value); - pub struct MovePath { - index: MovePathIndex, + /// Loan path corresponding to this move path loan_path: @LoanPath, + + /// Parent pointer, `InvalidMovePathIndex` if root parent: MovePathIndex, + + /// Head of linked list of moves to this path, + /// `InvalidMoveIndex` if not moved first_move: MoveIndex, + + /// First node in linked list of children, `InvalidMovePathIndex` if leaf first_child: MovePathIndex, + + /// Next node in linked list of parent's children (siblings), + /// `InvalidMovePathIndex` if none. next_sibling: MovePathIndex, } @@ -82,15 +107,27 @@ pub enum MoveKind { } pub struct Move { + /// Path being moved. path: MovePathIndex, + + /// id of node that is doing the move. id: ast::node_id, + + /// Kind of move, for error messages. kind: MoveKind, + + /// Next node in linked list of moves from `path`, or `InvalidMoveIndex` next_move: MoveIndex, } pub struct Assignment { + /// Path being assigned. path: MovePathIndex, + + /// id where assignment occurs id: ast::node_id, + + /// span of node where assignment occurs span: span, } @@ -153,7 +190,6 @@ impl MoveData { let index = MovePathIndex(self.paths.len()); self.paths.push(MovePath { - index: index, loan_path: lp, parent: InvalidMovePathIndex, first_move: InvalidMoveIndex, @@ -172,7 +208,6 @@ impl MoveData { self.mut_path(parent_index).first_child = index; self.paths.push(MovePath { - index: index, loan_path: lp, parent: parent_index, first_move: InvalidMoveIndex, diff --git a/src/librustc/middle/moves.rs b/src/librustc/middle/moves.rs index 159f7707dd383..abec56d32d79e 100644 --- a/src/librustc/middle/moves.rs +++ b/src/librustc/middle/moves.rs @@ -112,84 +112,8 @@ variables are captured and how (by ref, by copy, by move). ## Enforcement of Moves -FIXME out of date - -The enforcement of moves is somewhat complicated because it is divided -amongst the liveness and borrowck modules. In general, the borrow -checker is responsible for guaranteeing that *only owned data is -moved*. The liveness checker, in contrast, is responsible for -checking that *no variable is used after it is moved*. - -To see the difference, let's look at a few examples. Here is a -program fragment where the error would be caught by liveness: - - struct Foo { a: int, b: ~int } - let x: Foo = ...; - let y = x.b; // (1) - let z = x; // (2) //~ ERROR use of moved value `x` - -Here the liveness checker will see the assignment to `y` moves -invalidates the variable `x` because it moves the expression `x.b`. -An error is resported because `x` is not dead at the point where it is -invalidated. - -In more concrete terms, the `moves_map` generated from this example -would contain both the expression `x.b` (1) and the expression `x` -(2). Note that it would not contain `x` (1), because `moves_map` only -contains the outermost expressions that are moved. However, the id of -`x` would be present in the `moved_variables_set`. - -Now let's look at another illegal example, but one where liveness would -not catch the error: - - struct Foo { a: int, b: ~int } - let x: @Foo = ...; - let y = x.b; //~ ERROR move from managed (@) box - -This is an interesting example because the only change I've made is -to make `x` have type `@Foo` and not `Foo`. Thanks to auto-deref, -the expression `x.b` still works, but now it is short for `{x).b`, -and hence the move is actually moving out of the contents of a -managed box, which is illegal. However, liveness knows nothing of -this. It only tracks what variables are used where. The moves -pass (that is, this pass) is also ignorant of such details. From -the perspective of the moves pass, the `let y = x.b` line above -will be categorized as follows: - - let y = {(x{Move}) {Move}).b; {Move} - -Therefore, the reference to `x` will be present in -`variable_moves_map`, but liveness will not report an error because -there is no subsequent use. - -This is where the borrow checker comes in. When the borrow checker -runs, it will see that `x.b` is present in the `moves_map`. It will -use the `mem_categorization` module to determine where the result of -this expression resides in memory and see that it is owned by managed -data, and report an error. - -In principle, liveness could use the `mem_categorization` module -itself and check that moves always originate from owned data -(historically, of course, this was not the case; `mem_categorization` -used to be private to the borrow checker). However, there is another -kind of error which liveness could not possibly detect. Sometimes a -move is an error due to an outstanding loan, and it is borrow -checker's job to compute those loans. That is, consider *this* -example: - - struct Foo { a: int, b: ~int } - let x: Foo = ...; - let y = &x.b; //~ NOTE loan issued here - let z = x.b; //~ ERROR move with outstanding loan - -In this case, `y` is a pointer into `x`, so when `z` tries to move out -of `x`, we get an error. There is no way that liveness could compute -this information without redoing the efforts of the borrow checker. - -### Closures - -Liveness is somewhat complicated by having to deal with stack -closures. More information to come! +The enforcement of moves is done by the borrow checker. Please see +the section "Moves and initialization" in `middle/borrowck/doc.rs`. ## Distributive property