From 5436f859e46de9057260269049865b6cdb1d9ddd Mon Sep 17 00:00:00 2001 From: gaurikholkar Date: Fri, 14 Apr 2017 21:04:37 -0700 Subject: [PATCH 1/2] Disable ref hint for pattern in let and adding ui-tests. --- src/librustc/hir/map/mod.rs | 10 +++- .../borrowck/gather_loans/gather_moves.rs | 53 +++++++++++++++++-- .../borrowck/gather_loans/move_error.rs | 34 ++++++++---- .../borrowck/borrowck-vec-pattern-nesting.rs | 3 -- .../ui/issue-40402-ref-hints/issue-40402-1.rs | 19 +++++++ .../issue-40402-1.stderr | 8 +++ .../ui/issue-40402-ref-hints/issue-40402-2.rs | 14 +++++ .../issue-40402-2.stderr | 11 ++++ 8 files changed, 132 insertions(+), 20 deletions(-) create mode 100644 src/test/ui/issue-40402-ref-hints/issue-40402-1.rs create mode 100644 src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr create mode 100644 src/test/ui/issue-40402-ref-hints/issue-40402-2.rs create mode 100644 src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr diff --git a/src/librustc/hir/map/mod.rs b/src/librustc/hir/map/mod.rs index 48b8a819fff03..1948dd909f470 100644 --- a/src/librustc/hir/map/mod.rs +++ b/src/librustc/hir/map/mod.rs @@ -95,6 +95,14 @@ enum MapEntry<'hir> { RootCrate, } +/// Represents the kind of pattern +#[derive(Debug, Clone, Copy)] +pub enum PatternSource<'hir> { + MatchExpr(&'hir Expr), + LetDecl(&'hir Local), + Other, +} + impl<'hir> Clone for MapEntry<'hir> { fn clone(&self) -> MapEntry<'hir> { *self @@ -637,7 +645,7 @@ impl<'hir> Map<'hir> { Err(id) => id, } } - + /// Returns the nearest enclosing scope. A scope is an item or block. /// FIXME it is not clear to me that all items qualify as scopes - statics /// and associated types probably shouldn't, for example. Behaviour in this diff --git a/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs b/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs index 0577ba7f45a93..18c9858e8d6b5 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs @@ -23,13 +23,47 @@ use rustc::ty::{self, Ty}; use std::rc::Rc; use syntax::ast; use syntax_pos::Span; -use rustc::hir::{self, PatKind}; +use rustc::hir::*; +use rustc::hir::map::Node::*; +use rustc::hir::map::{PatternSource}; struct GatherMoveInfo<'tcx> { id: ast::NodeId, kind: MoveKind, cmt: mc::cmt<'tcx>, - span_path_opt: Option + span_path_opt: Option> +} + +/// Returns the kind of the Pattern +fn get_pattern_source<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, pat: &Pat) -> PatternSource<'tcx> { + + let parent = tcx.hir.get_parent_node(pat.id); + + match tcx.hir.get(parent) { + NodeExpr(ref e) => { + // the enclosing expression must be a `match` or something else + assert!(match e.node { + ExprMatch(..) => true, + _ => return PatternSource::Other, + }); + PatternSource::MatchExpr(e) + } + NodeStmt(ref s) => { + // the enclosing statement must be a `let` or something else + match s.node { + StmtDecl(ref decl, _) => { + match decl.node { + DeclLocal(ref local) => PatternSource::LetDecl(local), + _ => return PatternSource::Other, + } + } + _ => return PatternSource::Other, + } + } + + _ => return PatternSource::Other, + + } } pub fn gather_decl<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, @@ -95,11 +129,15 @@ pub fn gather_move_from_pat<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, move_error_collector: &mut MoveErrorCollector<'tcx>, move_pat: &hir::Pat, cmt: mc::cmt<'tcx>) { + let source = get_pattern_source(bccx.tcx,move_pat); let pat_span_path_opt = match move_pat.node { PatKind::Binding(_, _, ref path1, _) => { - Some(MoveSpanAndPath{span: move_pat.span, - name: path1.node}) - }, + Some(MoveSpanAndPath { + span: move_pat.span, + name: path1.node, + pat_source: source, + }) + } _ => None, }; let move_info = GatherMoveInfo { @@ -108,6 +146,11 @@ pub fn gather_move_from_pat<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, cmt: cmt, span_path_opt: pat_span_path_opt, }; + + debug!("gather_move_from_pat: move_pat={:?} source={:?}", + move_pat, + source); + gather_move(bccx, move_data, move_error_collector, move_info); } diff --git a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs index ebe2de5840954..f047374cab256 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs @@ -17,6 +17,7 @@ use rustc::ty; use syntax::ast; use syntax_pos; use errors::DiagnosticBuilder; +use rustc::hir::map::PatternSource; pub struct MoveErrorCollector<'tcx> { errors: Vec> @@ -40,12 +41,12 @@ impl<'tcx> MoveErrorCollector<'tcx> { pub struct MoveError<'tcx> { move_from: mc::cmt<'tcx>, - move_to: Option + move_to: Option> } impl<'tcx> MoveError<'tcx> { pub fn with_move_info(move_from: mc::cmt<'tcx>, - move_to: Option) + move_to: Option>) -> MoveError<'tcx> { MoveError { move_from: move_from, @@ -55,32 +56,43 @@ impl<'tcx> MoveError<'tcx> { } #[derive(Clone)] -pub struct MoveSpanAndPath { +pub struct MoveSpanAndPath<'tcx> { pub span: syntax_pos::Span, pub name: ast::Name, + pub pat_source: PatternSource<'tcx>, } pub struct GroupedMoveErrors<'tcx> { move_from: mc::cmt<'tcx>, - move_to_places: Vec + move_to_places: Vec> } -fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, - errors: &Vec>) { +fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, errors: &Vec>) { let grouped_errors = group_errors_with_same_origin(errors); for error in &grouped_errors { let mut err = report_cannot_move_out_of(bccx, error.move_from.clone()); let mut is_first_note = true; - for move_to in &error.move_to_places { - err = note_move_destination(err, move_to.span, move_to.name, is_first_note); - is_first_note = false; + + if let Some(pattern_source) = error.move_to_places.get(0){ + + match pattern_source.pat_source { + PatternSource::LetDecl(_) => {} + _ => { + for move_to in &error.move_to_places { + + err = note_move_destination(err, move_to.span, move_to.name, is_first_note); + is_first_note = false; + } + } } + } if let NoteClosureEnv(upvar_id) = error.move_from.note { - err.span_label(bccx.tcx.hir.span(upvar_id.var_id), &"captured outer variable"); + err.span_label(bccx.tcx.hir.span(upvar_id.var_id), &"captured outer variable"); } err.emit(); + + } } -} fn group_errors_with_same_origin<'tcx>(errors: &Vec>) -> Vec> { diff --git a/src/test/compile-fail/borrowck/borrowck-vec-pattern-nesting.rs b/src/test/compile-fail/borrowck/borrowck-vec-pattern-nesting.rs index d26364efdbc5d..98bb6b14b945c 100644 --- a/src/test/compile-fail/borrowck/borrowck-vec-pattern-nesting.rs +++ b/src/test/compile-fail/borrowck/borrowck-vec-pattern-nesting.rs @@ -54,7 +54,6 @@ fn c() { _ => {} } let a = vec[0]; //~ ERROR cannot move out - //~^ NOTE to prevent move //~| cannot move out of here } @@ -68,7 +67,6 @@ fn d() { _ => {} } let a = vec[0]; //~ ERROR cannot move out - //~^ NOTE to prevent move //~| cannot move out of here } @@ -84,7 +82,6 @@ fn e() { _ => {} } let a = vec[0]; //~ ERROR cannot move out - //~^ NOTE to prevent move //~| cannot move out of here } diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs b/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs new file mode 100644 index 0000000000000..fd42abc752e49 --- /dev/null +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs @@ -0,0 +1,19 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +struct Foo { + pub v: Vec, +} + +fn main() { + let mut f = Foo { v: Vec::new() }; + f.v.push("hello".to_string()); + let e = f.v[0]; +} diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr b/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr new file mode 100644 index 0000000000000..03b5beee2a417 --- /dev/null +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr @@ -0,0 +1,8 @@ +error[E0507]: cannot move out of indexed content + --> $DIR/issue-40402-1.rs:18:13 + | +18 | let e = f.v[0]; + | ^^^^^^ cannot move out of indexed content + +error: aborting due to previous error + diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs b/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs new file mode 100644 index 0000000000000..0879614243fb2 --- /dev/null +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs @@ -0,0 +1,14 @@ +// Copyright 2016 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. + +fn main() { + let x = vec![(String::new(), String::new())]; + let (a, b) = x[0]; +} diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr b/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr new file mode 100644 index 0000000000000..ab72590001519 --- /dev/null +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr @@ -0,0 +1,11 @@ +error[E0507]: cannot move out of indexed content + --> $DIR/issue-40402-2.rs:13:18 + | +13 | let (a, b) = x[0]; + | - - ^^^^ cannot move out of indexed content + | | | + | | ...and here (use `ref b` or `ref mut b`) + | hint: to prevent move, use `ref a` or `ref mut a` + +error: aborting due to previous error + From ab5d16a6b21d1360bb4fed20698aca2ead5f9c85 Mon Sep 17 00:00:00 2001 From: gaurikholkar Date: Thu, 27 Apr 2017 13:26:12 -0700 Subject: [PATCH 2/2] Adding documentation, indentation fixes --- src/librustc/hir/map/mod.rs | 10 +------ .../borrowck/gather_loans/gather_moves.rs | 30 +++++++++++++++---- .../borrowck/gather_loans/move_error.rs | 28 ++++++++--------- .../ui/issue-40402-ref-hints/issue-40402-1.rs | 1 + .../issue-40402-1.stderr | 4 +-- .../ui/issue-40402-ref-hints/issue-40402-2.rs | 2 ++ .../issue-40402-2.stderr | 4 +-- 7 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/librustc/hir/map/mod.rs b/src/librustc/hir/map/mod.rs index 1948dd909f470..48b8a819fff03 100644 --- a/src/librustc/hir/map/mod.rs +++ b/src/librustc/hir/map/mod.rs @@ -95,14 +95,6 @@ enum MapEntry<'hir> { RootCrate, } -/// Represents the kind of pattern -#[derive(Debug, Clone, Copy)] -pub enum PatternSource<'hir> { - MatchExpr(&'hir Expr), - LetDecl(&'hir Local), - Other, -} - impl<'hir> Clone for MapEntry<'hir> { fn clone(&self) -> MapEntry<'hir> { *self @@ -645,7 +637,7 @@ impl<'hir> Map<'hir> { Err(id) => id, } } - + /// Returns the nearest enclosing scope. A scope is an item or block. /// FIXME it is not clear to me that all items qualify as scopes - statics /// and associated types probably shouldn't, for example. Behaviour in this diff --git a/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs b/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs index 18c9858e8d6b5..f193588dd7d6c 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs @@ -11,7 +11,7 @@ //! Computes moves. use borrowck::*; -use borrowck::gather_loans::move_error::MoveSpanAndPath; +use borrowck::gather_loans::move_error::MovePlace; use borrowck::gather_loans::move_error::{MoveError, MoveErrorCollector}; use borrowck::move_data::*; use rustc::middle::expr_use_visitor as euv; @@ -25,16 +25,36 @@ use syntax::ast; use syntax_pos::Span; use rustc::hir::*; use rustc::hir::map::Node::*; -use rustc::hir::map::{PatternSource}; struct GatherMoveInfo<'tcx> { id: ast::NodeId, kind: MoveKind, cmt: mc::cmt<'tcx>, - span_path_opt: Option> + span_path_opt: Option> } -/// Returns the kind of the Pattern +/// Represents the kind of pattern +#[derive(Debug, Clone, Copy)] +pub enum PatternSource<'tcx> { + MatchExpr(&'tcx Expr), + LetDecl(&'tcx Local), + Other, +} + +/// Analyzes the context where the pattern appears to determine the +/// kind of hint we want to give. In particular, if the pattern is in a `match` +/// or nested within other patterns, we want to suggest a `ref` binding: +/// +/// let (a, b) = v[0]; // like the `a` and `b` patterns here +/// match v[0] { a => ... } // or the `a` pattern here +/// +/// But if the pattern is the outermost pattern in a `let`, we would rather +/// suggest that the author add a `&` to the initializer: +/// +/// let x = v[0]; // suggest `&v[0]` here +/// +/// In this latter case, this function will return `PatternSource::LetDecl` +/// with a reference to the let fn get_pattern_source<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, pat: &Pat) -> PatternSource<'tcx> { let parent = tcx.hir.get_parent_node(pat.id); @@ -132,7 +152,7 @@ pub fn gather_move_from_pat<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, let source = get_pattern_source(bccx.tcx,move_pat); let pat_span_path_opt = match move_pat.node { PatKind::Binding(_, _, ref path1, _) => { - Some(MoveSpanAndPath { + Some(MovePlace { span: move_pat.span, name: path1.node, pat_source: source, diff --git a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs index f047374cab256..9a72f3866a0e1 100644 --- a/src/librustc_borrowck/borrowck/gather_loans/move_error.rs +++ b/src/librustc_borrowck/borrowck/gather_loans/move_error.rs @@ -17,7 +17,7 @@ use rustc::ty; use syntax::ast; use syntax_pos; use errors::DiagnosticBuilder; -use rustc::hir::map::PatternSource; +use borrowck::gather_loans::gather_moves::PatternSource; pub struct MoveErrorCollector<'tcx> { errors: Vec> @@ -41,12 +41,12 @@ impl<'tcx> MoveErrorCollector<'tcx> { pub struct MoveError<'tcx> { move_from: mc::cmt<'tcx>, - move_to: Option> + move_to: Option> } impl<'tcx> MoveError<'tcx> { pub fn with_move_info(move_from: mc::cmt<'tcx>, - move_to: Option>) + move_to: Option>) -> MoveError<'tcx> { MoveError { move_from: move_from, @@ -56,7 +56,7 @@ impl<'tcx> MoveError<'tcx> { } #[derive(Clone)] -pub struct MoveSpanAndPath<'tcx> { +pub struct MovePlace<'tcx> { pub span: syntax_pos::Span, pub name: ast::Name, pub pat_source: PatternSource<'tcx>, @@ -64,7 +64,7 @@ pub struct MoveSpanAndPath<'tcx> { pub struct GroupedMoveErrors<'tcx> { move_from: mc::cmt<'tcx>, - move_to_places: Vec> + move_to_places: Vec> } fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, errors: &Vec>) { @@ -72,11 +72,11 @@ fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, errors: &Vec {} + match error.move_to_places.get(0) { + Some(&MovePlace { pat_source: PatternSource::LetDecl(_), .. }) => { + // ignore patterns that are found at the top-level of a `let`; + // see `get_pattern_source()` for details + } _ => { for move_to in &error.move_to_places { @@ -85,14 +85,14 @@ fn report_move_errors<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, errors: &Vec(errors: &Vec>) -> Vec> { diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs b/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs index fd42abc752e49..7efa3bd9d5b31 100644 --- a/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-1.rs @@ -8,6 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Check that we do not suggest `ref f` here in the `main()` function. struct Foo { pub v: Vec, } diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr b/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr index 03b5beee2a417..5e743b6bd3fe7 100644 --- a/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-1.stderr @@ -1,7 +1,7 @@ error[E0507]: cannot move out of indexed content - --> $DIR/issue-40402-1.rs:18:13 + --> $DIR/issue-40402-1.rs:19:13 | -18 | let e = f.v[0]; +19 | let e = f.v[0]; | ^^^^^^ cannot move out of indexed content error: aborting due to previous error diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs b/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs index 0879614243fb2..76e038b696e8f 100644 --- a/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-2.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Check that we do suggest `(ref a, ref b)` here, since `a` and `b` +// are nested within a pattern fn main() { let x = vec![(String::new(), String::new())]; let (a, b) = x[0]; diff --git a/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr b/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr index ab72590001519..0060b683bba43 100644 --- a/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr +++ b/src/test/ui/issue-40402-ref-hints/issue-40402-2.stderr @@ -1,7 +1,7 @@ error[E0507]: cannot move out of indexed content - --> $DIR/issue-40402-2.rs:13:18 + --> $DIR/issue-40402-2.rs:15:18 | -13 | let (a, b) = x[0]; +15 | let (a, b) = x[0]; | - - ^^^^ cannot move out of indexed content | | | | | ...and here (use `ref b` or `ref mut b`)