syntax/rustc: Improve error message for misuse of for loop

Print out a clearer error message when a `for` gets
used with the wrong type of iterator. Also fix spans on `for` loop
bodies, and suppress some more derived errors.

r=brson

Closes #3651

Author: catamorphism

src/librustc/middle/typeck/check/mod.rs

@@ -1497,7 +1497,7 @@ fn check_expr_with_unifier(fcx: @fn_ctxt,
           match ty::get(lhs_resolved_t).sty {
             ty::ty_fn(_) => {
               tcx.sess.span_note(
-                  ex.span, ~"did you forget the 'do' keyword for the call?");
+                  ex.span, ~"did you forget the `do` keyword for the call?");
             }
             _ => ()
           }
@@ -2207,23 +2207,33 @@ fn check_expr_with_unifier(fcx: @fn_ctxt,
           Some(ty::ty_fn(ref fty)) => {
             match fcx.mk_subty(false, expr.span,
                                (*fty).sig.output, ty::mk_bool(tcx)) {
-              result::Ok(_) => (),
+              result::Ok(_) =>
+                  ty::mk_fn(tcx, FnTyBase {
+                      meta: (*fty).meta,
+                      sig: FnSig {output: ty::mk_nil(tcx),
+                                  ../*bad*/copy (*fty).sig}
+                  }),
               result::Err(_) => {
                    fcx.type_error_message(expr.span,
                       |actual| {
-                          fmt!("a `loop` function's last argument \
-                                should return `bool`, not `%s`", actual)
+                          fmt!("A `for` loop iterator should expect a \
+                                closure that returns `bool`. This iterator \
+                                expects a closure that returns `%s`. %s",
+                               actual, if ty::type_is_nil((*fty).sig.output) {
+                                   "\nDid you mean to use `do` instead of \
+                                        `for`?" } else { "" } )
                       },
                       (*fty).sig.output, None);
                 err_happened = true;
+                // Kind of a hack: create a function type with the result
+                // replaced with ty_err, to suppress derived errors.
+                let t = ty::replace_fn_return_type(tcx, ty::mk_fn(tcx,
+                                                                  copy *fty),
+                                                   ty::mk_err(tcx));
                 fcx.write_ty(id, ty::mk_err(tcx));
+                t
               }
             }
-            ty::mk_fn(tcx, FnTyBase {
-                meta: (*fty).meta,
-                sig: FnSig {output: ty::mk_nil(tcx),
-                            ../*bad*/copy (*fty).sig}
-            })
           }
           _ =>
               match expected {
@@ -2245,14 +2255,22 @@ fn check_expr_with_unifier(fcx: @fn_ctxt,
               }
         };
         match b.node {
-          ast::expr_fn_block(ref decl, ref body, cap_clause) => {
-            check_expr_fn(fcx, b, None,
-                          decl, *body, ForLoop, Some(inner_ty));
-            demand::suptype(fcx, b.span, inner_ty, fcx.expr_ty(b));
-            capture::check_capture_clause(tcx, b.id, cap_clause);
-          }
-          // argh
-          _ => fail ~"expr_fn_block"
+                ast::expr_fn_block(ref decl, ref body, cap_clause) => {
+                    // If an error occurred, we pretend this isn't a for
+                    // loop, so as to assign types to all nodes while also
+                    // propagating ty_err throughout so as to suppress
+                    // derived errors. If we passed in ForLoop in the
+                    // error case, we'd potentially emit a spurious error
+                    // message because of the indirect_ret_ty.
+                    let fn_kind = if err_happened { Vanilla }
+                                  else { ForLoop };
+                    check_expr_fn(fcx, b, None,
+                                  decl, *body, fn_kind, Some(inner_ty));
+                    demand::suptype(fcx, b.span, inner_ty, fcx.expr_ty(b));
+                    capture::check_capture_clause(tcx, b.id, cap_clause);
+                }
+                // argh
+                _ => fail ~"expr_fn_block"
         }
         let block_ty = structurally_resolved_type(
             fcx, expr.span, fcx.node_ty(b.id));

src/libsyntax/parse/parser.rs

@@ -1660,43 +1660,45 @@ impl Parser {
         // them as the lambda arguments
         let e = self.parse_expr_res(RESTRICT_NO_BAR_OR_DOUBLEBAR_OP);
         match e.node {
-          expr_call(f, args, false) => {
-            let block = self.parse_lambda_block_expr();
-            let last_arg = self.mk_expr(block.span.lo, block.span.hi,
-                                    ctor(block));
-            let args = vec::append(args, ~[last_arg]);
-            @expr {node: expr_call(f, args, true), .. *e}
-          }
-          expr_method_call(f, i, tps, args, false) => {
-            let block = self.parse_lambda_block_expr();
-            let last_arg = self.mk_expr(block.span.lo, block.span.hi,
-                                    ctor(block));
-            let args = vec::append(args, ~[last_arg]);
-            @expr {node: expr_method_call(f, i, tps, args, true), .. *e}
-          }
-          expr_field(f, i, tps) => {
-            let block = self.parse_lambda_block_expr();
-            let last_arg = self.mk_expr(block.span.lo, block.span.hi,
-                                    ctor(block));
-            @expr {node: expr_method_call(f, i, tps, ~[last_arg], true),
-                   .. *e}
-          }
-          expr_path(*) | expr_call(*) | expr_method_call(*) |
-          expr_paren(*) => {
-            let block = self.parse_lambda_block_expr();
-            let last_arg = self.mk_expr(block.span.lo, block.span.hi,
-                                    ctor(block));
-            self.mk_expr(lo.lo, last_arg.span.hi,
-                         expr_call(e, ~[last_arg], true))
-          }
-          _ => {
-            // There may be other types of expressions that can
-            // represent the callee in `for` and `do` expressions
-            // but they aren't represented by tests
-            debug!("sugary call on %?", e.node);
-            self.span_fatal(
-                lo, fmt!("`%s` must be followed by a block call", keyword));
-          }
+            expr_call(f, args, false) => {
+                let block = self.parse_lambda_block_expr();
+                let last_arg = self.mk_expr(block.span.lo, block.span.hi,
+                                            ctor(block));
+                let args = vec::append(args, ~[last_arg]);
+                self.mk_expr(lo.lo, block.span.hi, expr_call(f, args, true))
+            }
+            expr_method_call(f, i, tps, args, false) => {
+                let block = self.parse_lambda_block_expr();
+                let last_arg = self.mk_expr(block.span.lo, block.span.hi,
+                                            ctor(block));
+                let args = vec::append(args, ~[last_arg]);
+                self.mk_expr(lo.lo, block.span.hi,
+                             expr_method_call(f, i, tps, args, true))
+            }
+            expr_field(f, i, tps) => {
+                let block = self.parse_lambda_block_expr();
+                let last_arg = self.mk_expr(block.span.lo, block.span.hi,
+                                            ctor(block));
+                self.mk_expr(lo.lo, block.span.hi,
+                             expr_method_call(f, i, tps, ~[last_arg], true))
+            }
+            expr_path(*) | expr_call(*) | expr_method_call(*) |
+                expr_paren(*) => {
+                let block = self.parse_lambda_block_expr();
+                let last_arg = self.mk_expr(block.span.lo, block.span.hi,
+                                            ctor(block));
+                self.mk_expr(lo.lo, last_arg.span.hi,
+                             expr_call(e, ~[last_arg], true))
+            }
+            _ => {
+                // There may be other types of expressions that can
+                // represent the callee in `for` and `do` expressions
+                // but they aren't represented by tests
+                debug!("sugary call on %?", e.node);
+                self.span_fatal(
+                    lo, fmt!("`%s` must be followed by a block call",
+                             keyword));
+            }
         }
     }
 

src/test/compile-fail/bad-for-loop.rs

@@ -10,5 +10,5 @@
 
 fn main() {
     fn baz(_x: fn(y: int) -> int) {}
-    for baz |_e| { } //~ ERROR should return `bool`
+    for baz |_e| { } //~ ERROR A `for` loop iterator should expect a closure that returns `bool`
 }

src/test/compile-fail/issue-2817-2.rs

@@ -14,13 +14,12 @@ fn main() {
     for uint::range(0, 100000) |_i| { //~ ERROR A for-loop body must return (), but
         false
     };
-    for not_bool |_i| { //~ ERROR a `loop` function's last argument should return `bool`
-        //~^ ERROR A for-loop body must return (), but
+    for not_bool |_i| { //~ ERROR a `for` loop iterator should expect a closure that returns `bool`
         ~"hi"
     };
     for uint::range(0, 100000) |_i| { //~ ERROR A for-loop body must return (), but
         ~"hi"
     };
-    for not_bool() |_i| { //~ ERROR a `loop` function's last argument
+    for not_bool() |_i| { //~ ERROR a `for` loop iterator should expect a closure that returns `bool`
     };
 }

src/test/compile-fail/issue-3651-2.rs

@@ -0,0 +1,13 @@
+// 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 <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() {
+    do 5.times {} //~ ERROR Do-block body must return bool, but returns () here. Perhaps
+}

src/test/compile-fail/issue-3651.rs

@@ -0,0 +1,13 @@
+// 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 <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 task::spawn { return true; } //~ ERROR A `for` loop iterator should expect a closure that
+}

src/test/compile-fail/missing-do.rs

@@ -15,5 +15,5 @@ fn foo(f: fn()) { f() }
 fn main() {
     ~"" || 42; //~ ERROR binary operation || cannot be applied to type `~str`
     foo || {}; //~ ERROR binary operation || cannot be applied to type `extern fn(&fn())`
-    //~^ NOTE did you forget the 'do' keyword for the call?
+    //~^ NOTE did you forget the `do` keyword for the call?
 }