From c4672f8e879a0c31fbe26990551693da16f00a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 13 Aug 2017 14:50:27 -0700 Subject: [PATCH 1/3] Point out missing if conditional On a case where an else conditional is missing, point this out instead of the token immediately after the (incorrect) else block: ``` error: missing condition for `if` statemementt push fork -f --> $DIR/issue-13483.rs:16:5 | 13 | } else if { | ^ expected if condition here ``` instead of ``` error: expected `{`, found `else` --> ../../src/test/ui/issue-13483.rs:14:7 | 14 | } else { | ^^^^ ``` --- src/libsyntax/parse/parser.rs | 20 ++++++++++++++++---- src/test/ui/issue-13483.rs | 22 ++++++++++++++++++++++ src/test/ui/issue-13483.stderr | 14 ++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/issue-13483.rs create mode 100644 src/test/ui/issue-13483.stderr diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 15f05df58b507..93902d06e52d8 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2130,7 +2130,7 @@ impl<'a> Parser<'a> { return self.parse_lambda_expr(lo, CaptureBy::Value, attrs); } if self.eat_keyword(keywords::If) { - return self.parse_if_expr(attrs); + return self.parse_if_expr(attrs, false); } if self.eat_keyword(keywords::For) { let lo = self.prev_span; @@ -2962,13 +2962,25 @@ impl<'a> Parser<'a> { } /// Parse an 'if' or 'if let' expression ('if' token already eaten) - pub fn parse_if_expr(&mut self, attrs: ThinVec) -> PResult<'a, P> { + pub fn parse_if_expr(&mut self, attrs: ThinVec, + in_else: bool) -> PResult<'a, P> { if self.check_keyword(keywords::Let) { return self.parse_if_let_expr(attrs); } let lo = self.prev_span; let cond = self.parse_expr_res(RESTRICTION_NO_STRUCT_LITERAL, None)?; - let thn = self.parse_block()?; + let thn = self.parse_block().map_err(|mut err| { + if in_else { + err.cancel(); + let sp = lo.next_point(); + let mut err = self.diagnostic() + .struct_span_err(sp, "missing condition for `if` statemement"); + err.span_label(sp, "expected if condition here"); + err + } else { + err + } + })?; let mut els: Option> = None; let mut hi = thn.span; if self.eat_keyword(keywords::Else) { @@ -3025,7 +3037,7 @@ impl<'a> Parser<'a> { // `else` token already eaten pub fn parse_else_expr(&mut self) -> PResult<'a, P> { if self.eat_keyword(keywords::If) { - return self.parse_if_expr(ThinVec::new()); + return self.parse_if_expr(ThinVec::new(), true); } else { let blk = self.parse_block()?; return Ok(self.mk_expr(blk.span, ExprKind::Block(blk), ThinVec::new())); diff --git a/src/test/ui/issue-13483.rs b/src/test/ui/issue-13483.rs new file mode 100644 index 0000000000000..31beab55699a6 --- /dev/null +++ b/src/test/ui/issue-13483.rs @@ -0,0 +1,22 @@ +// Copyright 2017 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() { + if true { + } else if { //ERROR: MISSING CONDITIONAL + } else { + }; +} + +fn foo() { + if true { + } else if { //ERROR: MISSING CONDITIONAL + }; +} diff --git a/src/test/ui/issue-13483.stderr b/src/test/ui/issue-13483.stderr new file mode 100644 index 0000000000000..be0b53d3d9832 --- /dev/null +++ b/src/test/ui/issue-13483.stderr @@ -0,0 +1,14 @@ +error: missing condition for `if` statemement + --> $DIR/issue-13483.rs:13:14 + | +13 | } else if { //ERROR: MISSING CONDITIONAL + | ^ expected if condition here + +error: missing conditional + --> $DIR/issue-13483.rs:20:14 + | +20 | } else if { //ERROR: MISSING CONDITIONAL + | ^ expected if condition here + +error: aborting due to 2 previous errors + From 20a2716206ca879ad9848cbb4ac9fcc739dd3f77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 17 Aug 2017 12:14:35 -0700 Subject: [PATCH 2/3] Check for `else` keyword on missing `if` condition --- src/libsyntax/parse/parser.rs | 27 +++++++++++---------------- src/test/ui/issue-13483.rs | 6 ------ src/test/ui/issue-13483.stderr | 8 +------- 3 files changed, 12 insertions(+), 29 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 93902d06e52d8..22999afb48ea2 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2130,7 +2130,7 @@ impl<'a> Parser<'a> { return self.parse_lambda_expr(lo, CaptureBy::Value, attrs); } if self.eat_keyword(keywords::If) { - return self.parse_if_expr(attrs, false); + return self.parse_if_expr(attrs); } if self.eat_keyword(keywords::For) { let lo = self.prev_span; @@ -2962,25 +2962,20 @@ impl<'a> Parser<'a> { } /// Parse an 'if' or 'if let' expression ('if' token already eaten) - pub fn parse_if_expr(&mut self, attrs: ThinVec, - in_else: bool) -> PResult<'a, P> { + pub fn parse_if_expr(&mut self, attrs: ThinVec) -> PResult<'a, P> { if self.check_keyword(keywords::Let) { return self.parse_if_let_expr(attrs); } let lo = self.prev_span; let cond = self.parse_expr_res(RESTRICTION_NO_STRUCT_LITERAL, None)?; - let thn = self.parse_block().map_err(|mut err| { - if in_else { - err.cancel(); - let sp = lo.next_point(); - let mut err = self.diagnostic() - .struct_span_err(sp, "missing condition for `if` statemement"); - err.span_label(sp, "expected if condition here"); - err - } else { - err - } - })?; + if self.eat_keyword(keywords::Else) { + let sp = lo.next_point(); + let mut err = self.diagnostic() + .struct_span_err(sp, "missing condition for `if` statemement"); + err.span_label(sp, "expected if condition here"); + return Err(err) + } + let thn = self.parse_block()?; let mut els: Option> = None; let mut hi = thn.span; if self.eat_keyword(keywords::Else) { @@ -3037,7 +3032,7 @@ impl<'a> Parser<'a> { // `else` token already eaten pub fn parse_else_expr(&mut self) -> PResult<'a, P> { if self.eat_keyword(keywords::If) { - return self.parse_if_expr(ThinVec::new(), true); + return self.parse_if_expr(ThinVec::new()); } else { let blk = self.parse_block()?; return Ok(self.mk_expr(blk.span, ExprKind::Block(blk), ThinVec::new())); diff --git a/src/test/ui/issue-13483.rs b/src/test/ui/issue-13483.rs index 31beab55699a6..e94dc38c5e924 100644 --- a/src/test/ui/issue-13483.rs +++ b/src/test/ui/issue-13483.rs @@ -14,9 +14,3 @@ fn main() { } else { }; } - -fn foo() { - if true { - } else if { //ERROR: MISSING CONDITIONAL - }; -} diff --git a/src/test/ui/issue-13483.stderr b/src/test/ui/issue-13483.stderr index be0b53d3d9832..e49fdcf6d20a3 100644 --- a/src/test/ui/issue-13483.stderr +++ b/src/test/ui/issue-13483.stderr @@ -4,11 +4,5 @@ error: missing condition for `if` statemement 13 | } else if { //ERROR: MISSING CONDITIONAL | ^ expected if condition here -error: missing conditional - --> $DIR/issue-13483.rs:20:14 - | -20 | } else if { //ERROR: MISSING CONDITIONAL - | ^ expected if condition here - -error: aborting due to 2 previous errors +error: aborting due to previous error From f06323337dd0cf748e8a0ca49d91dac2eae6b4a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 17 Aug 2017 16:51:52 -0700 Subject: [PATCH 3/3] Verify that an `if` condition block returns a value --- src/libsyntax/ast.rs | 26 ++++++++++++++++++++++++++ src/libsyntax/parse/parser.rs | 7 ++++++- src/test/ui/issue-13483.rs | 13 +++++++++++-- src/test/ui/issue-13483.stderr | 10 ++++++++-- 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index 38ef79822c7b5..747d1a44923f1 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -844,6 +844,32 @@ pub struct Expr { pub attrs: ThinVec } +impl Expr { + /// Wether this expression would be valid somewhere that expects a value, for example, an `if` + /// condition. + pub fn returns(&self) -> bool { + if let ExprKind::Block(ref block) = self.node { + match block.stmts.last().map(|last_stmt| &last_stmt.node) { + // implicit return + Some(&StmtKind::Expr(_)) => true, + Some(&StmtKind::Semi(ref expr)) => { + if let ExprKind::Ret(_) = expr.node { + // last statement is explicit return + true + } else { + false + } + } + // This is a block that doesn't end in either an implicit or explicit return + _ => false, + } + } else { + // This is not a block, it is a value + true + } + } +} + impl fmt::Debug for Expr { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "expr({}: {})", self.id, pprust::expr_to_string(self)) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 22999afb48ea2..85d2a485bfa75 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2968,7 +2968,12 @@ impl<'a> Parser<'a> { } let lo = self.prev_span; let cond = self.parse_expr_res(RESTRICTION_NO_STRUCT_LITERAL, None)?; - if self.eat_keyword(keywords::Else) { + + // Verify that the parsed `if` condition makes sense as a condition. If it is a block, then + // verify that the last statement is either an implicit return (no `;`) or an explicit + // return. This won't catch blocks with an explicit `return`, but that would be caught by + // the dead code lint. + if self.eat_keyword(keywords::Else) || !cond.returns() { let sp = lo.next_point(); let mut err = self.diagnostic() .struct_span_err(sp, "missing condition for `if` statemement"); diff --git a/src/test/ui/issue-13483.rs b/src/test/ui/issue-13483.rs index e94dc38c5e924..8637804391276 100644 --- a/src/test/ui/issue-13483.rs +++ b/src/test/ui/issue-13483.rs @@ -10,7 +10,16 @@ fn main() { if true { - } else if { //ERROR: MISSING CONDITIONAL + } else if { } else { - }; + } } + +fn foo() { + if true { + } else if { + } + bar(); +} + +fn bar() {} diff --git a/src/test/ui/issue-13483.stderr b/src/test/ui/issue-13483.stderr index e49fdcf6d20a3..3446969dfd213 100644 --- a/src/test/ui/issue-13483.stderr +++ b/src/test/ui/issue-13483.stderr @@ -1,8 +1,14 @@ error: missing condition for `if` statemement --> $DIR/issue-13483.rs:13:14 | -13 | } else if { //ERROR: MISSING CONDITIONAL +13 | } else if { | ^ expected if condition here -error: aborting due to previous error +error: missing condition for `if` statemement + --> $DIR/issue-13483.rs:20:14 + | +20 | } else if { + | ^ expected if condition here + +error: aborting due to 2 previous errors