auto merge of #14444 : huonw/rust/nice-for-errors, r=alexcrichton

Change `for` desugaring & make refutable pattern errors more precise

This changes for to desugar to the `let`-based pattern match as described in #14390, and adjusts the compiler to use this information for error messages that even mention that it's in a `for` loop.

Also, it makes the compiler record the exact positions of refutable parts of a pattern, to point to exactly them in error messages.

8 files changed, 140 insertions, 28 deletions

diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs
index fb797a02795..9b751447504 100644
--- a/src/librustc/middle/check_match.rs
+++ b/src/librustc/middle/check_match.rs
@@ -863,9 +863,18 @@ fn default(cx: &MatchCheckCtxt, r: &[@Pat]) -> Option<Vec<@Pat> > {
 
 fn check_local(cx: &mut MatchCheckCtxt, loc: &Local) {
     visit::walk_local(cx, loc, ());
-    if is_refutable(cx, loc.pat) {
-        cx.tcx.sess.span_err(loc.pat.span,
-                             "refutable pattern in local binding");
+
+    let name = match loc.source {
+        LocalLet => "local",
+        LocalFor => "`for` loop"
+    };
+
+    let mut spans = vec![];
+    find_refutable(cx, loc.pat, &mut spans);
+
+    for span in spans.iter() {
+        cx.tcx.sess.span_err(*span,
+                             format!("refutable pattern in {} binding", name).as_slice());
     }
 
     // Check legality of move bindings.
@@ -879,53 +888,65 @@ fn check_fn(cx: &mut MatchCheckCtxt,
             sp: Span) {
     visit::walk_fn(cx, kind, decl, body, sp, ());
     for input in decl.inputs.iter() {
-        if is_refutable(cx, input.pat) {
-            cx.tcx.sess.span_err(input.pat.span,
+        let mut spans = vec![];
+        find_refutable(cx, input.pat, &mut spans);
+
+        for span in spans.iter() {
+            cx.tcx.sess.span_err(*span,
                                  "refutable pattern in function argument");
         }
     }
 }
 
-fn is_refutable(cx: &MatchCheckCtxt, pat: &Pat) -> bool {
+fn find_refutable(cx: &MatchCheckCtxt, pat: &Pat, spans: &mut Vec<Span>) {
+    macro_rules! this_pattern {
+        () => {
+            {
+                spans.push(pat.span);
+                return
+            }
+        }
+    }
     let opt_def = cx.tcx.def_map.borrow().find_copy(&pat.id);
     match opt_def {
       Some(DefVariant(enum_id, _, _)) => {
         if ty::enum_variants(cx.tcx, enum_id).len() != 1u {
-            return true;
+            this_pattern!()
         }
       }
-      Some(DefStatic(..)) => return true,
+      Some(DefStatic(..)) => this_pattern!(),
       _ => ()
     }
 
     match pat.node {
       PatUniq(sub) | PatRegion(sub) | PatIdent(_, _, Some(sub)) => {
-        is_refutable(cx, sub)
+        find_refutable(cx, sub, spans)
       }
-      PatWild | PatWildMulti | PatIdent(_, _, None) => { false }
+      PatWild | PatWildMulti | PatIdent(_, _, None) => {}
       PatLit(lit) => {
           match lit.node {
             ExprLit(lit) => {
                 match lit.node {
-                    LitNil => false,    // `()`
-                    _ => true,
+                    LitNil => {}    // `()`
+                    _ => this_pattern!(),
                 }
             }
-            _ => true,
+            _ => this_pattern!(),
           }
       }
-      PatRange(_, _) => { true }
+      PatRange(_, _) => { this_pattern!() }
       PatStruct(_, ref fields, _) => {
-        fields.iter().any(|f| is_refutable(cx, f.pat))
-      }
-      PatTup(ref elts) => {
-        elts.iter().any(|elt| is_refutable(cx, *elt))
+          for f in fields.iter() {
+              find_refutable(cx, f.pat, spans);
+          }
       }
-      PatEnum(_, Some(ref args)) => {
-        args.iter().any(|a| is_refutable(cx, *a))
+      PatTup(ref elts) | PatEnum(_, Some(ref elts))=> {
+          for elt in elts.iter() {
+              find_refutable(cx, *elt, spans)
+          }
       }
-      PatEnum(_,_) => { false }
-      PatVec(..) => { true }
+      PatEnum(_,_) => {}
+      PatVec(..) => { this_pattern!() }
     }
 }
 
diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
index e77d1faf05d..69a92a87185 100644
--- a/src/libsyntax/ast.rs
+++ b/src/libsyntax/ast.rs
@@ -417,6 +417,14 @@ pub enum Stmt_ {
     StmtMac(Mac, bool),
 }
 
+/// Where a local declaration came from: either a true `let ... =
+/// ...;`, or one desugared from the pattern of a for loop.
+#[deriving(Clone, Eq, TotalEq, Encodable, Decodable, Hash)]
+pub enum LocalSource {
+    LocalLet,
+    LocalFor,
+}
+
 // FIXME (pending discussion of #1697, #2178...): local should really be
 // a refinement on pat.
 /// Local represents a `let` statement, e.g., `let <pat>:<ty> = <expr>;`
@@ -427,6 +435,7 @@ pub struct Local {
     pub init: Option<@Expr>,
     pub id: NodeId,
     pub span: Span,
+    pub source: LocalSource,
 }
 
 pub type Decl = Spanned<Decl_>;
diff --git a/src/libsyntax/ext/build.rs b/src/libsyntax/ext/build.rs
index 449feb3afbf..bb7b73c5f81 100644
--- a/src/libsyntax/ext/build.rs
+++ b/src/libsyntax/ext/build.rs
@@ -439,6 +439,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
             init: Some(ex),
             id: ast::DUMMY_NODE_ID,
             span: sp,
+            source: ast::LocalLet,
         };
         let decl = respan(sp, ast::DeclLocal(local));
         @respan(sp, ast::StmtDecl(@decl, ast::DUMMY_NODE_ID))
@@ -462,6 +463,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
             init: Some(ex),
             id: ast::DUMMY_NODE_ID,
             span: sp,
+            source: ast::LocalLet,
         };
         let decl = respan(sp, ast::DeclLocal(local));
         @respan(sp, ast::StmtDecl(@decl, ast::DUMMY_NODE_ID))
diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs
index 6c6bf520104..b8766200ccd 100644
--- a/src/libsyntax/ext/expand.rs
+++ b/src/libsyntax/ext/expand.rs
@@ -147,11 +147,17 @@ pub fn expand_expr(e: @ast::Expr, fld: &mut MacroExpander) -> @ast::Expr {
             //       ['<ident>:] loop {
             //         match i.next() {
             //           None => break,
-            //           Some(<src_pat>) => <src_loop_block>
+            //           Some(mut value) => {
+            //             let <src_pat> = value;
+            //             <src_loop_block>
+            //           }
             //         }
             //       }
             //     }
             //   }
+            //
+            // (The use of the `let` is to give better error messages
+            // when the pattern is refutable.)
 
             let local_ident = token::gensym_ident("__i"); // FIXME #13573
             let next_ident = fld.cx.ident_of("next");
@@ -167,11 +173,33 @@ pub fn expand_expr(e: @ast::Expr, fld: &mut MacroExpander) -> @ast::Expr {
                 fld.cx.arm(span, vec!(none_pat), break_expr)
             };
 
-            // `Some(<src_pat>) => <src_loop_block>`
+            // let <src_pat> = value;
+            let value_ident = token::gensym_ident("__value");
+            // this is careful to use src_pat.span so that error
+            // messages point exact at that.
+            let local = @ast::Local {
+                ty: fld.cx.ty_infer(src_pat.span),
+                pat: src_pat,
+                init: Some(fld.cx.expr_ident(src_pat.span, value_ident)),
+                id: ast::DUMMY_NODE_ID,
+                span: src_pat.span,
+                source: ast::LocalFor
+            };
+            let local = codemap::respan(src_pat.span, ast::DeclLocal(local));
+            let local = @codemap::respan(span, ast::StmtDecl(@local, ast::DUMMY_NODE_ID));
+
+            // { let ...; <src_loop_block> }
+            let block = fld.cx.block(span, vec![local],
+                                     Some(fld.cx.expr_block(src_loop_block)));
+
+            // `Some(mut value) => { ... }`
+            // Note the _'s in the name will stop any unused mutability warnings.
+            let value_pat = fld.cx.pat_ident_binding_mode(span, value_ident,
+                                                          ast::BindByValue(ast::MutMutable));
             let some_arm =
                 fld.cx.arm(span,
-                           vec!(fld.cx.pat_enum(span, some_path, vec!(src_pat))),
-                           fld.cx.expr_block(src_loop_block));
+                           vec!(fld.cx.pat_enum(span, some_path, vec!(value_pat))),
+                           fld.cx.expr_block(block));
 
             // `match i.next() { ... }`
             let match_expr = {
@@ -669,7 +697,8 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander)
                         pat: pat,
                         init: init,
                         id: id,
-                        span: span
+                        span: span,
+                        source: source,
                     } = **local;
                     // expand the pat (it might contain exprs... #:(o)>
                     let expanded_pat = fld.fold_pat(pat);
@@ -703,6 +732,7 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander)
                             init: new_init_opt,
                             id: id,
                             span: span,
+                            source: source
                         };
                     SmallVector::one(@Spanned {
                         node: StmtDecl(@Spanned {
diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs
index ae5cf550bb9..6a9f26ae83d 100644
--- a/src/libsyntax/fold.rs
+++ b/src/libsyntax/fold.rs
@@ -288,6 +288,7 @@ pub trait Folder {
             pat: self.fold_pat(l.pat),
             init: l.init.map(|e| self.fold_expr(e)),
             span: self.new_span(l.span),
+            source: l.source,
         }
     }
 
diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs
index ae5f16c2580..65687ce4d94 100644
--- a/src/libsyntax/parse/parser.rs
+++ b/src/libsyntax/parse/parser.rs
@@ -34,7 +34,7 @@ use ast::{Ident, NormalFn, Inherited, Item, Item_, ItemStatic};
 use ast::{ItemEnum, ItemFn, ItemForeignMod, ItemImpl};
 use ast::{ItemMac, ItemMod, ItemStruct, ItemTrait, ItemTy, Lit, Lit_};
 use ast::{LitBool, LitFloat, LitFloatUnsuffixed, LitInt, LitChar};
-use ast::{LitIntUnsuffixed, LitNil, LitStr, LitUint, Local};
+use ast::{LitIntUnsuffixed, LitNil, LitStr, LitUint, Local, LocalLet};
 use ast::{MutImmutable, MutMutable, Mac_, MacInvocTT, Matcher, MatchNonterminal};
 use ast::{MatchSeq, MatchTok, Method, MutTy, BiMul, Mutability};
 use ast::{NamedField, UnNeg, NoReturn, UnNot, P, Pat, PatEnum};
@@ -3034,6 +3034,7 @@ impl<'a> Parser<'a> {
             init: init,
             id: ast::DUMMY_NODE_ID,
             span: mk_sp(lo, self.last_span.hi),
+            source: LocalLet,
         }
     }
 
diff --git a/src/test/compile-fail/for-loop-refutable-pattern-error-message.rs b/src/test/compile-fail/for-loop-refutable-pattern-error-message.rs
new file mode 100644
index 00000000000..8b00b614909
--- /dev/null
+++ b/src/test/compile-fail/for-loop-refutable-pattern-error-message.rs
@@ -0,0 +1,16 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+
+fn main() {
+    for
+        &1 //~ ERROR refutable pattern in `for` loop binding
+        in [1].iter() {}
+}
diff --git a/src/test/compile-fail/precise-refutable-pattern-errors.rs b/src/test/compile-fail/precise-refutable-pattern-errors.rs
new file mode 100644
index 00000000000..efa2dbad83f
--- /dev/null
+++ b/src/test/compile-fail/precise-refutable-pattern-errors.rs
@@ -0,0 +1,32 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+
+fn func(
+    (
+        1, //~ ERROR refutable pattern in function argument
+        (
+            Some( //~ ERROR refutable pattern in function argument
+                1), // nested, so no warning.
+            2..3 //~ ERROR refutable pattern in function argument
+            )
+        ): (int, (Option<int>, int))
+        ) {}
+
+fn main() {
+    let (
+        1, //~ ERROR refutable pattern in local binding
+        (
+            Some( //~ ERROR refutable pattern in local binding
+                1), // nested, so no warning.
+            2..3 //~ ERROR refutable pattern in local binding
+            )
+        ) = (1, (None, 2));
+}