From 976438f78fdce8092430f4c81ca272293c48f1a0 Mon Sep 17 00:00:00 2001 From: Kevin Ballard Date: Mon, 25 Aug 2014 14:55:00 -0700 Subject: [PATCH] Produce a better error for irrefutable `if let` patterns Modify ast::ExprMatch to include a new value of type ast::MatchSource, making it easy to tell whether the match was written literally or produced via desugaring. This allows us to customize error messages appropriately. --- src/librustc/diagnostics.rs | 5 ++ src/librustc/lint/builtin.rs | 7 ++- src/librustc/middle/cfg/construct.rs | 2 +- src/librustc/middle/check_match.rs | 26 +++++++-- src/librustc/middle/expr_use_visitor.rs | 2 +- src/librustc/middle/liveness.rs | 2 +- src/librustc/middle/trans/debuginfo.rs | 2 +- src/librustc/middle/trans/expr.rs | 2 +- src/librustc/middle/typeck/check/mod.rs | 2 +- src/librustc/middle/typeck/check/regionck.rs | 2 +- src/librustc/util/ppaux.rs | 1 + src/libsyntax/ast.rs | 8 ++- src/libsyntax/config.rs | 4 +- src/libsyntax/ext/build.rs | 2 +- src/libsyntax/ext/expand.rs | 2 +- src/libsyntax/fold.rs | 5 +- src/libsyntax/parse/parser.rs | 4 +- src/libsyntax/print/pprust.rs | 2 +- src/test/compile-fail/if-let.rs | 57 +++++++++++++++++++ .../compile-fail/lint-unnecessary-parens.rs | 1 + 20 files changed, 115 insertions(+), 23 deletions(-) create mode 100644 src/test/compile-fail/if-let.rs diff --git a/src/librustc/diagnostics.rs b/src/librustc/diagnostics.rs index 36ba28dfc2a..10fff4db8d6 100644 --- a/src/librustc/diagnostics.rs +++ b/src/librustc/diagnostics.rs @@ -19,6 +19,11 @@ register_diagnostic!(E0001, r##" one is too specific or the ordering is incorrect. "##) +register_diagnostic!(E0162, r##" + This error is produced by an `if let` expression where the pattern is irrefutable. + An `if let` that can never fail is considered an error. +"##) + register_diagnostics!( E0002, E0003, diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index b812073c3a8..b1cd431818c 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -1091,7 +1091,10 @@ impl LintPass for UnnecessaryParens { let (value, msg, struct_lit_needs_parens) = match e.node { ast::ExprIf(ref cond, _, _) => (cond, "`if` condition", true), ast::ExprWhile(ref cond, _, _) => (cond, "`while` condition", true), - ast::ExprMatch(ref head, _) => (head, "`match` head expression", true), + ast::ExprMatch(ref head, _, source) => match source { + ast::MatchNormal => (head, "`match` head expression", true), + ast::MatchIfLetDesugar => (head, "`if let` head expression", true) + }, ast::ExprRet(Some(ref value)) => (value, "`return` value", false), ast::ExprAssign(_, ref value) => (value, "assigned value", false), ast::ExprAssignOp(_, _, ref value) => (value, "assigned value", false), @@ -1241,7 +1244,7 @@ impl LintPass for UnusedMut { fn check_expr(&mut self, cx: &Context, e: &ast::Expr) { match e.node { - ast::ExprMatch(_, ref arms) => { + ast::ExprMatch(_, ref arms, _) => { for a in arms.iter() { self.check_unused_mut_pat(cx, a.pats.as_slice()) } diff --git a/src/librustc/middle/cfg/construct.rs b/src/librustc/middle/cfg/construct.rs index 1f0171de351..f370de31fdc 100644 --- a/src/librustc/middle/cfg/construct.rs +++ b/src/librustc/middle/cfg/construct.rs @@ -324,7 +324,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> { expr_exit } - ast::ExprMatch(ref discr, ref arms) => { + ast::ExprMatch(ref discr, ref arms, _) => { // // [pred] // | diff --git a/src/librustc/middle/check_match.rs b/src/librustc/middle/check_match.rs index d3321e555a4..600d449ffe5 100644 --- a/src/librustc/middle/check_match.rs +++ b/src/librustc/middle/check_match.rs @@ -147,7 +147,7 @@ pub fn check_crate(tcx: &ty::ctxt) { fn check_expr(cx: &mut MatchCheckCtxt, ex: &Expr) { visit::walk_expr(cx, ex); match ex.node { - ExprMatch(ref scrut, ref arms) => { + ExprMatch(ref scrut, ref arms, source) => { // First, check legality of move bindings. for arm in arms.iter() { check_legality_of_move_bindings(cx, @@ -184,7 +184,7 @@ fn check_expr(cx: &mut MatchCheckCtxt, ex: &Expr) { } // Fourth, check for unreachable arms. - check_arms(cx, inlined_arms.as_slice()); + check_arms(cx, inlined_arms.as_slice(), source); // Finally, check if the whole match expression is exhaustive. // Check for empty enum, because is_useful only works on inhabited types. @@ -252,13 +252,31 @@ fn check_for_static_nan(cx: &MatchCheckCtxt, pats: &[P]) { } // Check for unreachable patterns -fn check_arms(cx: &MatchCheckCtxt, arms: &[(Vec>, Option<&Expr>)]) { +fn check_arms(cx: &MatchCheckCtxt, arms: &[(Vec>, Option<&Expr>)], source: MatchSource) { let mut seen = Matrix(vec![]); + let mut printed_if_let_err = false; for &(ref pats, guard) in arms.iter() { for pat in pats.iter() { let v = vec![&**pat]; + match is_useful(cx, &seen, v.as_slice(), LeaveOutWitness) { - NotUseful => span_err!(cx.tcx.sess, pat.span, E0001, "unreachable pattern"), + NotUseful => { + if source == MatchIfLetDesugar { + if printed_if_let_err { + // we already printed an irrefutable if-let pattern error. + // We don't want two, that's just confusing. + } else { + // find the first arm pattern so we can use its span + let &(ref first_arm_pats, _) = &arms[0]; // we know there's at least 1 arm + let first_pat = first_arm_pats.get(0); // and it's safe to assume 1 pat + let span = first_pat.span; + span_err!(cx.tcx.sess, span, E0162, "irrefutable if-let pattern"); + printed_if_let_err = true; + } + } else { + span_err!(cx.tcx.sess, pat.span, E0001, "unreachable pattern"); + } + } Useful => (), UsefulWithWitness(_) => unreachable!() } diff --git a/src/librustc/middle/expr_use_visitor.rs b/src/librustc/middle/expr_use_visitor.rs index 875416feb2e..4423b83c745 100644 --- a/src/librustc/middle/expr_use_visitor.rs +++ b/src/librustc/middle/expr_use_visitor.rs @@ -376,7 +376,7 @@ impl<'d,'t,'tcx,TYPER:mc::Typer<'tcx>> ExprUseVisitor<'d,'t,TYPER> { ast::ExprIfLet(..) => fail!("non-desugared ExprIfLet"), - ast::ExprMatch(ref discr, ref arms) => { + ast::ExprMatch(ref discr, ref arms, _) => { let discr_cmt = return_if_err!(self.mc.cat_expr(&**discr)); self.borrow_expr(&**discr, ty::ReEmpty, ty::ImmBorrow, MatchDiscriminant); diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs index 061f110afa9..00e65f34cf3 100644 --- a/src/librustc/middle/liveness.rs +++ b/src/librustc/middle/liveness.rs @@ -1029,7 +1029,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { self.propagate_through_loop(expr, LoopLoop, &**blk, succ) } - ExprMatch(ref e, ref arms) => { + ExprMatch(ref e, ref arms, _) => { // // (e) // | diff --git a/src/librustc/middle/trans/debuginfo.rs b/src/librustc/middle/trans/debuginfo.rs index 5221faa598c..aada90d260e 100644 --- a/src/librustc/middle/trans/debuginfo.rs +++ b/src/librustc/middle/trans/debuginfo.rs @@ -3659,7 +3659,7 @@ fn populate_scope_map(cx: &CrateContext, } } - ast::ExprMatch(ref discriminant_exp, ref arms) => { + ast::ExprMatch(ref discriminant_exp, ref arms, _) => { walk_expr(cx, &**discriminant_exp, scope_stack, scope_map); // For each arm we have to first walk the pattern as these might diff --git a/src/librustc/middle/trans/expr.rs b/src/librustc/middle/trans/expr.rs index 120e8404f2c..eeea10e8eb0 100644 --- a/src/librustc/middle/trans/expr.rs +++ b/src/librustc/middle/trans/expr.rs @@ -1013,7 +1013,7 @@ fn trans_rvalue_dps_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, ast::ExprIf(ref cond, ref thn, ref els) => { controlflow::trans_if(bcx, expr.id, &**cond, &**thn, els.as_ref().map(|e| &**e), dest) } - ast::ExprMatch(ref discr, ref arms) => { + ast::ExprMatch(ref discr, ref arms, _) => { _match::trans_match(bcx, expr, &**discr, arms.as_slice(), dest) } ast::ExprBlock(ref blk) => { diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs index 56620383408..2c866b9ee38 100644 --- a/src/librustc/middle/typeck/check/mod.rs +++ b/src/librustc/middle/typeck/check/mod.rs @@ -4144,7 +4144,7 @@ fn check_expr_with_unifier(fcx: &FnCtxt, fcx.write_nil(id); } } - ast::ExprMatch(ref discrim, ref arms) => { + ast::ExprMatch(ref discrim, ref arms, _) => { _match::check_match(fcx, expr, &**discrim, arms.as_slice()); } ast::ExprFnBlock(_, ref decl, ref body) => { diff --git a/src/librustc/middle/typeck/check/regionck.rs b/src/librustc/middle/typeck/check/regionck.rs index b853616991f..9e20028569b 100644 --- a/src/librustc/middle/typeck/check/regionck.rs +++ b/src/librustc/middle/typeck/check/regionck.rs @@ -725,7 +725,7 @@ fn visit_expr(rcx: &mut Rcx, expr: &ast::Expr) { visit::walk_expr(rcx, expr); } - ast::ExprMatch(ref discr, ref arms) => { + ast::ExprMatch(ref discr, ref arms, _) => { link_match(rcx, &**discr, arms.as_slice()); visit::walk_expr(rcx, expr); diff --git a/src/librustc/util/ppaux.rs b/src/librustc/util/ppaux.rs index fcb613426fc..638aea10e37 100644 --- a/src/librustc/util/ppaux.rs +++ b/src/librustc/util/ppaux.rs @@ -92,6 +92,7 @@ pub fn explain_region_and_span(cx: &ctxt, region: ty::Region) ast::ExprMethodCall(..) => { explain_span(cx, "method call", expr.span) }, + ast::ExprMatch(_, _, ast::MatchIfLetDesugar) => explain_span(cx, "if let", expr.span), ast::ExprMatch(..) => explain_span(cx, "match", expr.span), _ => explain_span(cx, "expression", expr.span) } diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index 02922fe3655..59c824d0eae 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -529,7 +529,7 @@ pub enum Expr_ { // Conditionless loop (can be exited with break, cont, or ret) // FIXME #6993: change to Option ... or not, if these are hygienic. ExprLoop(P, Option), - ExprMatch(P, Vec), + ExprMatch(P, Vec, MatchSource), ExprFnBlock(CaptureClause, P, P), ExprProc(P, P), ExprUnboxedFn(CaptureClause, UnboxedClosureKind, P, P), @@ -577,6 +577,12 @@ pub struct QPath { pub item_name: Ident, } +#[deriving(Clone, PartialEq, Eq, Encodable, Decodable, Hash, Show)] +pub enum MatchSource { + MatchNormal, + MatchIfLetDesugar +} + #[deriving(Clone, PartialEq, Eq, Encodable, Decodable, Hash, Show)] pub enum CaptureClause { CaptureByValue, diff --git a/src/libsyntax/config.rs b/src/libsyntax/config.rs index 5b17f6f004a..41b7218da0c 100644 --- a/src/libsyntax/config.rs +++ b/src/libsyntax/config.rs @@ -210,10 +210,10 @@ fn fold_expr(cx: &mut Context, expr: P) -> P { fold::noop_fold_expr(ast::Expr { id: id, node: match node { - ast::ExprMatch(m, arms) => { + ast::ExprMatch(m, arms, source) => { ast::ExprMatch(m, arms.into_iter() .filter(|a| (cx.in_cfg)(a.attrs.as_slice())) - .collect()) + .collect(), source) } _ => node }, diff --git a/src/libsyntax/ext/build.rs b/src/libsyntax/ext/build.rs index 0586868eb45..f2b806f43cc 100644 --- a/src/libsyntax/ext/build.rs +++ b/src/libsyntax/ext/build.rs @@ -845,7 +845,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> { } fn expr_match(&self, span: Span, arg: P, arms: Vec) -> P { - self.expr(span, ast::ExprMatch(arg, arms)) + self.expr(span, ast::ExprMatch(arg, arms, ast::MatchNormal)) } fn expr_if(&self, span: Span, cond: P, diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 341be15b739..de6a8675e13 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -127,7 +127,7 @@ pub fn expand_expr(e: P, fld: &mut MacroExpander) -> P { arms.push_all_move(else_if_arms); arms.push(else_arm); - let match_expr = fld.cx.expr_match(span, expr, arms); + let match_expr = fld.cx.expr(span, ast::ExprMatch(expr, arms, ast::MatchIfLetDesugar)); fld.fold_expr(match_expr) } diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs index 8bdc64a926d..31bec58a4da 100644 --- a/src/libsyntax/fold.rs +++ b/src/libsyntax/fold.rs @@ -1226,9 +1226,10 @@ pub fn noop_fold_expr(Expr {id, node, span}: Expr, folder: &mut T) -> ExprLoop(folder.fold_block(body), opt_ident.map(|i| folder.fold_ident(i))) } - ExprMatch(expr, arms) => { + ExprMatch(expr, arms, source) => { ExprMatch(folder.fold_expr(expr), - arms.move_map(|x| folder.fold_arm(x))) + arms.move_map(|x| folder.fold_arm(x)), + source) } ExprFnBlock(capture_clause, decl, body) => { ExprFnBlock(capture_clause, diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index d4ba81c737b..ccb398bf2fb 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -38,7 +38,7 @@ use ast::{ItemMac, ItemMod, ItemStruct, ItemTrait, ItemTy}; use ast::{LifetimeDef, Lit, Lit_}; use ast::{LitBool, LitChar, LitByte, LitBinary}; use ast::{LitNil, LitStr, LitInt, Local, LocalLet}; -use ast::{MutImmutable, MutMutable, Mac_, MacInvocTT, Matcher, MatchNonterminal}; +use ast::{MutImmutable, MutMutable, Mac_, MacInvocTT, Matcher, MatchNonterminal, MatchNormal}; use ast::{MatchSeq, MatchTok, Method, MutTy, BiMul, Mutability}; use ast::{MethodImplItem, NamedField, UnNeg, NoReturn, UnNot}; use ast::{Pat, PatEnum, PatIdent, PatLit, PatRange, PatRegion, PatStruct}; @@ -2973,7 +2973,7 @@ impl<'a> Parser<'a> { } let hi = self.span.hi; self.bump(); - return self.mk_expr(lo, hi, ExprMatch(discriminant, arms)); + return self.mk_expr(lo, hi, ExprMatch(discriminant, arms, MatchNormal)); } pub fn parse_arm(&mut self) -> Arm { diff --git a/src/libsyntax/print/pprust.rs b/src/libsyntax/print/pprust.rs index 1e5810fb311..a8e99b4e85f 100644 --- a/src/libsyntax/print/pprust.rs +++ b/src/libsyntax/print/pprust.rs @@ -1535,7 +1535,7 @@ impl<'a> State<'a> { try!(space(&mut self.s)); try!(self.print_block(&**blk)); } - ast::ExprMatch(ref expr, ref arms) => { + ast::ExprMatch(ref expr, ref arms, _) => { try!(self.cbox(indent_unit)); try!(self.ibox(4)); try!(self.word_nbsp("match")); diff --git a/src/test/compile-fail/if-let.rs b/src/test/compile-fail/if-let.rs new file mode 100644 index 00000000000..88b6854bb1d --- /dev/null +++ b/src/test/compile-fail/if-let.rs @@ -0,0 +1,57 @@ +// 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. + +#![feature(macro_rules)] + +fn macros() { + macro_rules! foo{ + ($p:pat, $e:expr, $b:block) => {{ + if let $p = $e $b + }} + } + macro_rules! bar{ + ($p:pat, $e:expr, $b:block) => {{ + foo!($p, $e, $b) + }} + } + + foo!(a, 1i, { //~ ERROR irrefutable if-let + println!("irrefutable pattern"); + }); + bar!(a, 1i, { //~ ERROR irrefutable if-let + println!("irrefutable pattern"); + }); +} + +pub fn main() { + if let a = 1i { //~ ERROR irrefutable if-let + println!("irrefutable pattern"); + } + + if let a = 1i { //~ ERROR irrefutable if-let + println!("irrefutable pattern"); + } else if true { + println!("else-if in irrefutable if-let"); + } else { + println!("else in irrefutable if-let"); + } + + if let 1i = 2i { + println!("refutable pattern"); + } else if let a = 1i { //~ ERROR irrefutable if-let + println!("irrefutable pattern"); + } + + if true { + println!("if"); + } else if let a = 1i { //~ ERROR irrefutable if-let + println!("irrefutable pattern"); + } +} diff --git a/src/test/compile-fail/lint-unnecessary-parens.rs b/src/test/compile-fail/lint-unnecessary-parens.rs index d51d5b4af87..82fb42571af 100644 --- a/src/test/compile-fail/lint-unnecessary-parens.rs +++ b/src/test/compile-fail/lint-unnecessary-parens.rs @@ -32,6 +32,7 @@ fn main() { match (true) { //~ ERROR unnecessary parentheses around `match` head expression _ => {} } + if let 1i = (1i) {} //~ ERROR unnecessary parentheses around `if let` head expression let v = X { y: false }; // struct lits needs parens, so these shouldn't warn. if (v == X { y: true }) {}