From 6ddd40d4368b1dbbc6bfa18d73d47beb05a55447 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Mon, 26 May 2014 22:00:08 +1000 Subject: [PATCH 1/3] syntax: Add a source field to `Local` for tracking if it comes from `let`s or `for`s. --- src/libsyntax/ast.rs | 9 +++++++++ src/libsyntax/ext/build.rs | 2 ++ src/libsyntax/ext/expand.rs | 4 +++- src/libsyntax/fold.rs | 1 + src/libsyntax/parse/parser.rs | 3 ++- 5 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index e77d1faf05d89..69a92a871855c 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 : = ;` @@ -427,6 +435,7 @@ pub struct Local { pub init: Option<@Expr>, pub id: NodeId, pub span: Span, + pub source: LocalSource, } pub type Decl = Spanned; diff --git a/src/libsyntax/ext/build.rs b/src/libsyntax/ext/build.rs index 449feb3afbf96..bb7b73c5f81f2 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 6c6bf52010406..762363f3abeaf 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -669,7 +669,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 +704,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 ae5cf550bb9bc..6a9f26ae83d68 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 ae5f16c2580e2..65687ce4d9447 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, } } From f2a137829e07505aaaa2be4ae97d6ecfb6ef6823 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Mon, 26 May 2014 22:01:09 +1000 Subject: [PATCH 2/3] syntax: desugar a `for` loop to a let binding to get better error messages when the pattern is refutable. This means the compiler points directly to the pattern and said that the problem is the pattern being refutable (rather than just saying that some value isn't covered in the `match` as it did previously). Fixes #14390. --- src/librustc/middle/check_match.rs | 7 +++- src/libsyntax/ext/expand.rs | 36 ++++++++++++++++--- ...or-loop-refutable-pattern-error-message.rs | 16 +++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 src/test/compile-fail/for-loop-refutable-pattern-error-message.rs diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index fb797a027956f..cc319b70e396f 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -864,8 +864,13 @@ fn default(cx: &MatchCheckCtxt, r: &[@Pat]) -> Option > { fn check_local(cx: &mut MatchCheckCtxt, loc: &Local) { visit::walk_local(cx, loc, ()); if is_refutable(cx, loc.pat) { + let name = match loc.source { + LocalLet => "local", + LocalFor => "`for` loop" + }; + cx.tcx.sess.span_err(loc.pat.span, - "refutable pattern in local binding"); + format!("refutable pattern in {} binding", name).as_slice()); } // Check legality of move bindings. diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 762363f3abeaf..b8766200ccd00 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 { // [':] loop { // match i.next() { // None => break, - // Some() => + // Some(mut value) => { + // let = value; + // + // } // } // } // } // } + // + // (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() => ` + // let = 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 ...; } + 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 = { 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 0000000000000..8b00b61490993 --- /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 or the MIT license +// , 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() {} +} From 0df221e993a1f7f1fc4d80c6a4fa0e43c05c30c7 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Mon, 26 May 2014 22:42:48 +1000 Subject: [PATCH 3/3] rustc: provide more precise information about refutable patterns. The compiler now points exactly which part(s) of a pattern are refutable, rather than just highlighting the whole pattern. --- src/librustc/middle/check_match.rs | 66 ++++++++++++------- .../precise-refutable-pattern-errors.rs | 32 +++++++++ 2 files changed, 73 insertions(+), 25 deletions(-) create mode 100644 src/test/compile-fail/precise-refutable-pattern-errors.rs diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index cc319b70e396f..9b751447504ef 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -863,13 +863,17 @@ fn default(cx: &MatchCheckCtxt, r: &[@Pat]) -> Option > { fn check_local(cx: &mut MatchCheckCtxt, loc: &Local) { visit::walk_local(cx, loc, ()); - if is_refutable(cx, loc.pat) { - let name = match loc.source { - LocalLet => "local", - LocalFor => "`for` loop" - }; - cx.tcx.sess.span_err(loc.pat.span, + 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()); } @@ -884,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) { + 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/test/compile-fail/precise-refutable-pattern-errors.rs b/src/test/compile-fail/precise-refutable-pattern-errors.rs new file mode 100644 index 0000000000000..efa2dbad83fda --- /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 or the MIT license +// , 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)) + ) {} + +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)); +}