diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index 7a81568117313..95c354b6800e7 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -803,7 +803,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) => { diff --git a/src/librustc/middle/dataflow.rs b/src/librustc/middle/dataflow.rs index 9c9f72adc4df2..f3439262b110a 100644 --- a/src/librustc/middle/dataflow.rs +++ b/src/librustc/middle/dataflow.rs @@ -824,7 +824,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 4608f773a1c26..fbad8b83a2d4f 100644 --- a/src/librustc/middle/liveness.rs +++ b/src/librustc/middle/liveness.rs @@ -1443,10 +1443,10 @@ fn check_expr(expr: @expr, this: @Liveness, vt: vt<@Liveness>) { match this.ir.variable_moves_map.find(&expr.id) { None => {} - Some(&entire_expr) => { + Some(&reason) => { debug!("(checking expr) is a move: `%s`", expr_to_str(expr, this.tcx.sess.intr())); - this.check_move_from_var(ln, *var, entire_expr); + this.check_move_from_var(ln, *var, reason); } } } @@ -1459,7 +1459,7 @@ fn check_expr(expr: @expr, this: @Liveness, vt: vt<@Liveness>) { 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); + this.check_move_from_var(cap.ln, var, moves::MovedByExpr(expr)); } } @@ -1549,16 +1549,12 @@ pub impl Liveness { fn check_move_from_var(&self, ln: LiveNode, var: Variable, - move_expr: @expr) { + move_reason: moves::Reason) { /*! * 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 + * `move_reason` is the reason 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)", @@ -1566,7 +1562,7 @@ pub impl Liveness { match self.live_on_exit(ln, var) { None => {} - Some(lnk) => self.report_illegal_move(lnk, var, move_expr) + Some(lnk) => self.report_illegal_move(lnk, var, move_reason) } } @@ -1640,82 +1636,79 @@ pub impl Liveness { 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)); - } + move_reason: moves::Reason) { + match move_reason { + moves::MovedByExpr(move_expr) => { + 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.id, move_expr.span, var, + "", "it", "copy"); + } + expr_field(*) => { + self.report_illegal_read( + move_expr.span, lnk, var, PartiallyMovedValue); + self.report_move_location( + move_expr.id, move_expr.span, var, + "field of ", "the field", "copy"); + } + expr_index(*) => { + self.report_illegal_read( + move_expr.span, lnk, var, PartiallyMovedValue); + self.report_move_location( + move_expr.id, move_expr.span, var, + "element of ", "the element", "copy"); + } + _ => { + self.report_illegal_read( + move_expr.span, lnk, var, PartiallyMovedValue); + self.report_move_location( + move_expr.id, move_expr.span, var, + "subcomponent of ", "the subcomponent", "copy"); + } + } } - } - 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"); - } - _ => { + moves::MovedByPat(move_binding) => { self.report_illegal_read( - move_expr.span, lnk, var, PartiallyMovedValue); + move_binding.span, lnk, var, PartiallyMovedValue); self.report_move_location( - move_expr, var, "subcomponent of ", "the subcomponent"); + move_binding.id, move_binding.span, var, + "", "the binding", "ref"); } }; } fn report_move_location(&self, - move_expr: @expr, + move_loc_id: node_id, + move_loc_span: span, var: Variable, expr_descr: &str, - pronoun: &str) { - let move_expr_ty = ty::expr_ty(self.tcx, move_expr); + pronoun: &str, + override: &str) { + let move_expr_ty = ty::node_id_to_type(self.tcx, move_loc_id); let name = self.ir.variable_name(var); self.tcx.sess.span_note( - move_expr.span, + move_loc_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))); + which is moved by default (use `%s` to override)", + expr_descr, + *name, + pronoun, + ty_to_str(self.tcx, move_expr_ty), + override)); } fn report_illegal_read(&self, diff --git a/src/librustc/middle/moves.rs b/src/librustc/middle/moves.rs index e81a9d6b78fcd..ef6401e61ab9f 100644 --- a/src/librustc/middle/moves.rs +++ b/src/librustc/middle/moves.rs @@ -211,6 +211,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}; @@ -239,10 +240,15 @@ pub type CaptureMap = @mut HashMap; pub type MovesMap = @mut HashSet; +pub enum Reason { + MovedByExpr(@expr), + MovedByPat(@pat), +} + /** * For each variable which will be moved, links to the * expression */ -pub type VariableMovesMap = @mut HashMap; +pub type VariableMovesMap = @mut HashMap; /** * Set of variable node-ids that are moved. @@ -267,8 +273,8 @@ struct VisitContext { } enum UseMode { - MoveInWhole, // Move the entire value. - MoveInPart(@expr), // Some subcomponent will be moved + MoveInWhole(Reason), // Move the entire value. + MoveInPart(Reason), // Some subcomponent will be moved Read // Read no matter what the type. } @@ -316,16 +322,15 @@ fn compute_modes_for_expr(expr: @expr, } pub impl UseMode { - fn component_mode(&self, expr: @expr) -> UseMode { + fn component_mode(&self) -> 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) + MoveInWhole(r) => MoveInPart(r) } } } @@ -343,19 +348,30 @@ pub impl VisitContext { fn consume_expr(&self, expr: @expr, visitor: vt) + { + self.consume_expr_with_reason(expr, MovedByExpr(expr), visitor); + } + + fn consume_expr_with_reason(&self, + expr: @expr, + reason: Reason, + visitor: vt) { /*! - * * Indicates that the value of `expr` will be consumed, * meaning either copied or moved depending on its type. + * `reason` indicates why the value should be moved. */ - debug!("consume_expr(expr=%?/%s)", - expr.id, - pprust::expr_to_str(expr, self.tcx.sess.intr())); + debug!("consume_expr_with_reason(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); + let mode = if ty::type_moves_by_default(self.tcx, expr_ty) { + MoveInWhole(reason) + } else { + Read + }; self.use_expr(expr, mode, visitor); } @@ -364,7 +380,6 @@ pub impl VisitContext { visitor: vt) { /*! - * * Indicates that the value of `blk` will be consumed, * meaning either copied or moved depending on its type. */ @@ -380,43 +395,22 @@ 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); } + MoveInWhole(_) => { self.move_maps.moves_map.insert(expr.id); } MoveInPart(_) | Read => {} } @@ -427,7 +421,7 @@ pub impl VisitContext { Some(&@ty::AutoDerefRef( ty::AutoDerefRef { autoref: Some(_), _})) => Read, - _ => expr_mode.component_mode(expr) + _ => expr_mode.component_mode() }; debug!("comp_mode = %?", comp_mode); @@ -435,17 +429,16 @@ pub impl VisitContext { match expr.node { expr_path(*) | expr_self => { match comp_mode { - MoveInPart(entire_expr) => { + MoveInPart(reason) => { self.move_maps.variable_moves_map.insert( - expr.id, entire_expr); - + expr.id, reason); 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 => { + MoveInWhole(_) => { self.tcx.sess.span_bug( expr.span, "Component mode can never be MoveInWhole"); @@ -544,18 +537,24 @@ pub impl VisitContext { self.consume_arm(arm, visitor); } - let by_move_bindings_present = + let opt_by_move_binding = 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); + match opt_by_move_binding { + Some(p) => { + // If one of the arms moves a value out of the + // discriminant, then the discriminant itself is + // moved. + self.consume_expr_with_reason( + discr, + MovedByPat(p), + visitor); + } + None => { + // Otherwise, the discriminant is merely read. + self.use_expr(discr, Read, visitor); + } } } @@ -717,18 +716,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); } } } @@ -757,20 +755,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] { @@ -804,3 +800,12 @@ pub impl VisitContext { } } } + +impl Reason { + pub fn span(&self) -> span { + match *self { + MovedByPat(p) => p.span, + MovedByExpr(e) => e.span, + } + } +} \ No newline at end of file diff --git a/src/librustc/middle/pat_util.rs b/src/librustc/middle/pat_util.rs index b87adb75bc37a..d80b0b5dcb66b 100644 --- a/src/librustc/middle/pat_util.rs +++ b/src/librustc/middle/pat_util.rs @@ -70,8 +70,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 057b85730a0a6..bc32b06dd5455 100644 --- a/src/librustc/middle/resolve.rs +++ b/src/librustc/middle/resolve.rs @@ -4063,7 +4063,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/libsyntax/ast_util.rs b/src/libsyntax/ast_util.rs index e209fd14b5e02..39440445e74c2 100644 --- a/src/libsyntax/ast_util.rs +++ b/src/libsyntax/ast_util.rs @@ -502,36 +502,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/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() {}