From b431eec6f28d64cd3852584f9a59736c6c09ee68 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 27 Dec 2023 18:45:58 -0800 Subject: [PATCH 01/17] Expand on expr_requires_semi_to_be_stmt documentation --- compiler/rustc_ast/src/util/classify.rs | 51 ++++++++++++++++++++----- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_ast/src/util/classify.rs b/compiler/rustc_ast/src/util/classify.rs index f21a9cabb81be..7f9775a8e0cb7 100644 --- a/compiler/rustc_ast/src/util/classify.rs +++ b/compiler/rustc_ast/src/util/classify.rs @@ -1,16 +1,47 @@ -//! Routines the parser uses to classify AST nodes - -// Predicates on exprs and stmts that the pretty-printer and parser use +//! Routines the parser and pretty-printer use to classify AST nodes. use crate::{ast, token::Delimiter}; -/// Does this expression require a semicolon to be treated -/// as a statement? The negation of this: 'can this expression -/// be used as a statement without a semicolon' -- is used -/// as an early-bail-out in the parser so that, for instance, -/// if true {...} else {...} -/// |x| 5 -/// isn't parsed as (if true {...} else {...} | x) | 5 +/// Does this expression require a semicolon to be treated as a statement? +/// +/// The negation of this: "can this expression be used as a statement without a +/// semicolon" -- is used as an early bail-out in the parser so that, for +/// instance, +/// +/// ```ignore (illustrative) +/// if true {...} else {...} +/// |x| 5 +/// ``` +/// +/// isn't parsed as `(if true {...} else {...} | x) | 5`. +/// +/// Nearly the same early bail-out also occurs in the right-hand side of match +/// arms: +/// +/// ```ignore (illustrative) +/// match i { +/// 0 => if true {...} else {...} +/// | x => {} +/// } +/// ``` +/// +/// Here the `|` is a leading vert in a second match arm. It is not a binary +/// operator with the If as its left operand. If the first arm were some other +/// expression for which `expr_requires_semi_to_be_stmt` returns true, then the +/// `|` on the next line would be a binary operator (leading to a parse error). +/// +/// The statement case and the match-arm case are "nearly" the same early +/// bail-out because of 1 edge case. Macro calls with brace delimiter terminate +/// a statement without a semicolon, but do not terminate a match-arm without +/// comma. +/// +/// ```ignore (illustrative) +/// m! {} - 1; // two statements: a macro call followed by -1 literal +/// +/// match () { +/// _ => m! {} - 1, // binary subtraction operator +/// } +/// ``` pub fn expr_requires_semi_to_be_stmt(e: &ast::Expr) -> bool { !matches!( e.kind, From cbb8714a3f8a04cce698719df338fb095c40f479 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 16:28:47 -0800 Subject: [PATCH 02/17] Mark expr_requires_semi_to_be_stmt call sites For each of these, we need to decide whether they need to be using `expr_requires_semi_to_be_stmt`, or `expr_requires_comma_to_be_match_arm`, which are supposed to be 2 different behaviors. Previously they were conflated into one, causing either too much or too little parenthesization. --- compiler/rustc_ast/src/util/classify.rs | 3 ++- compiler/rustc_ast_pretty/src/pprust/state.rs | 2 +- compiler/rustc_ast_pretty/src/pprust/state/fixup.rs | 2 +- compiler/rustc_lint/src/unused.rs | 2 +- compiler/rustc_parse/src/parser/expr.rs | 6 +++--- compiler/rustc_parse/src/parser/stmt.rs | 5 +++-- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_ast/src/util/classify.rs b/compiler/rustc_ast/src/util/classify.rs index 7f9775a8e0cb7..5ed8d95b12d3a 100644 --- a/compiler/rustc_ast/src/util/classify.rs +++ b/compiler/rustc_ast/src/util/classify.rs @@ -42,7 +42,8 @@ use crate::{ast, token::Delimiter}; /// _ => m! {} - 1, // binary subtraction operator /// } /// ``` -pub fn expr_requires_semi_to_be_stmt(e: &ast::Expr) -> bool { +#[allow(non_snake_case)] +pub fn expr_requires_semi_to_be_stmt_FIXME(e: &ast::Expr) -> bool { !matches!( e.kind, ast::ExprKind::If(..) diff --git a/compiler/rustc_ast_pretty/src/pprust/state.rs b/compiler/rustc_ast_pretty/src/pprust/state.rs index 2c176828c841f..be98b7d37d4aa 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state.rs @@ -1253,7 +1253,7 @@ impl<'a> State<'a> { ast::StmtKind::Expr(expr) => { self.space_if_not_bol(); self.print_expr_outer_attr_style(expr, false, FixupContext::new_stmt()); - if classify::expr_requires_semi_to_be_stmt(expr) { + if classify::expr_requires_semi_to_be_stmt_FIXME(expr) { self.word(";"); } } diff --git a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs index d21cb82f83b28..363243215a370 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs @@ -128,7 +128,7 @@ impl FixupContext { /// The documentation on `FixupContext::leftmost_subexpression_in_stmt` has /// examples. pub fn would_cause_statement_boundary(self, expr: &Expr) -> bool { - self.leftmost_subexpression_in_stmt && !classify::expr_requires_semi_to_be_stmt(expr) + self.leftmost_subexpression_in_stmt && !classify::expr_requires_semi_to_be_stmt_FIXME(expr) } /// Determine whether parentheses are needed around the given `let` diff --git a/compiler/rustc_lint/src/unused.rs b/compiler/rustc_lint/src/unused.rs index 2b147e052ae0f..7dca701795038 100644 --- a/compiler/rustc_lint/src/unused.rs +++ b/compiler/rustc_lint/src/unused.rs @@ -688,7 +688,7 @@ trait UnusedDelimLint { ExprKind::Index(base, _subscript, _) => base, _ => break, }; - if !classify::expr_requires_semi_to_be_stmt(innermost) { + if !classify::expr_requires_semi_to_be_stmt_FIXME(innermost) { return true; } } diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 577003e94fb28..951c3495995c2 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -498,7 +498,7 @@ impl<'a> Parser<'a> { /// Checks if this expression is a successfully parsed statement. fn expr_is_complete(&self, e: &Expr) -> bool { self.restrictions.contains(Restrictions::STMT_EXPR) - && !classify::expr_requires_semi_to_be_stmt(e) + && !classify::expr_requires_semi_to_be_stmt_FIXME(e) } /// Parses `x..y`, `x..=y`, and `x..`/`x..=`. @@ -2694,7 +2694,7 @@ impl<'a> Parser<'a> { // If it's not a free-standing expression, and is followed by a block, // then it's very likely the condition to an `else if`. if self.check(&TokenKind::OpenDelim(Delimiter::Brace)) - && classify::expr_requires_semi_to_be_stmt(&cond) => + && classify::expr_requires_semi_to_be_stmt_FIXME(&cond) => { self.dcx().emit_err(errors::ExpectedElseBlock { first_tok_span, @@ -3136,7 +3136,7 @@ impl<'a> Parser<'a> { err })?; - let require_comma = classify::expr_requires_semi_to_be_stmt(&expr) + let require_comma = classify::expr_requires_semi_to_be_stmt_FIXME(&expr) && this.token != token::CloseDelim(Delimiter::Brace); if !require_comma { diff --git a/compiler/rustc_parse/src/parser/stmt.rs b/compiler/rustc_parse/src/parser/stmt.rs index d70afebf1b2da..f64a480a18c32 100644 --- a/compiler/rustc_parse/src/parser/stmt.rs +++ b/compiler/rustc_parse/src/parser/stmt.rs @@ -648,7 +648,7 @@ impl<'a> Parser<'a> { match &mut stmt.kind { // Expression without semicolon. StmtKind::Expr(expr) - if classify::expr_requires_semi_to_be_stmt(expr) + if classify::expr_requires_semi_to_be_stmt_FIXME(expr) && !expr.attrs.is_empty() && ![token::Eof, token::Semi, token::CloseDelim(Delimiter::Brace)] .contains(&self.token.kind) => @@ -662,7 +662,8 @@ impl<'a> Parser<'a> { // Expression without semicolon. StmtKind::Expr(expr) - if self.token != token::Eof && classify::expr_requires_semi_to_be_stmt(expr) => + if self.token != token::Eof + && classify::expr_requires_semi_to_be_stmt_FIXME(expr) => { // Just check for errors and recover; do not eat semicolon yet. From 9e1cf2098d68356bccf7112bbff1d9b565e80a02 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 16:30:34 -0800 Subject: [PATCH 03/17] Macro call with braces does not require semicolon to be statement This commit by itself is supposed to have no effect on behavior. All of the call sites are updated to preserve their previous behavior. The behavior changes are in the commits that follow. --- compiler/rustc_ast/src/util/classify.rs | 30 +++++++++++-------- compiler/rustc_ast_pretty/src/pprust/state.rs | 5 +++- .../src/pprust/state/fixup.rs | 6 +++- compiler/rustc_lint/src/unused.rs | 5 +++- compiler/rustc_parse/src/parser/expr.rs | 16 +++++++--- compiler/rustc_parse/src/parser/stmt.rs | 11 +++++-- 6 files changed, 50 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_ast/src/util/classify.rs b/compiler/rustc_ast/src/util/classify.rs index 5ed8d95b12d3a..86383af1f7c83 100644 --- a/compiler/rustc_ast/src/util/classify.rs +++ b/compiler/rustc_ast/src/util/classify.rs @@ -42,19 +42,23 @@ use crate::{ast, token::Delimiter}; /// _ => m! {} - 1, // binary subtraction operator /// } /// ``` -#[allow(non_snake_case)] -pub fn expr_requires_semi_to_be_stmt_FIXME(e: &ast::Expr) -> bool { - !matches!( - e.kind, - ast::ExprKind::If(..) - | ast::ExprKind::Match(..) - | ast::ExprKind::Block(..) - | ast::ExprKind::While(..) - | ast::ExprKind::Loop(..) - | ast::ExprKind::ForLoop { .. } - | ast::ExprKind::TryBlock(..) - | ast::ExprKind::ConstBlock(..) - ) +pub fn expr_requires_semi_to_be_stmt(e: &ast::Expr) -> bool { + use ast::ExprKind::*; + + match &e.kind { + If(..) + | Match(..) + | Block(..) + | While(..) + | Loop(..) + | ForLoop { .. } + | TryBlock(..) + | ConstBlock(..) => false, + + MacCall(mac_call) => mac_call.args.delim != Delimiter::Brace, + + _ => true, + } } /// If an expression ends with `}`, returns the innermost expression ending in the `}` diff --git a/compiler/rustc_ast_pretty/src/pprust/state.rs b/compiler/rustc_ast_pretty/src/pprust/state.rs index be98b7d37d4aa..fe17ff41bc34d 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state.rs @@ -1253,7 +1253,10 @@ impl<'a> State<'a> { ast::StmtKind::Expr(expr) => { self.space_if_not_bol(); self.print_expr_outer_attr_style(expr, false, FixupContext::new_stmt()); - if classify::expr_requires_semi_to_be_stmt_FIXME(expr) { + if match expr.kind { + ast::ExprKind::MacCall(_) => true, + _ => classify::expr_requires_semi_to_be_stmt(expr), + } { self.word(";"); } } diff --git a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs index 363243215a370..9934f972f9bfd 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs @@ -128,7 +128,11 @@ impl FixupContext { /// The documentation on `FixupContext::leftmost_subexpression_in_stmt` has /// examples. pub fn would_cause_statement_boundary(self, expr: &Expr) -> bool { - self.leftmost_subexpression_in_stmt && !classify::expr_requires_semi_to_be_stmt_FIXME(expr) + self.leftmost_subexpression_in_stmt + && match expr.kind { + ExprKind::MacCall(_) => false, + _ => !classify::expr_requires_semi_to_be_stmt(expr), + } } /// Determine whether parentheses are needed around the given `let` diff --git a/compiler/rustc_lint/src/unused.rs b/compiler/rustc_lint/src/unused.rs index 7dca701795038..19b71564e2ebb 100644 --- a/compiler/rustc_lint/src/unused.rs +++ b/compiler/rustc_lint/src/unused.rs @@ -688,7 +688,10 @@ trait UnusedDelimLint { ExprKind::Index(base, _subscript, _) => base, _ => break, }; - if !classify::expr_requires_semi_to_be_stmt_FIXME(innermost) { + if match innermost.kind { + ExprKind::MacCall(_) => false, + _ => !classify::expr_requires_semi_to_be_stmt(innermost), + } { return true; } } diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 951c3495995c2..5f7bd0835d3d6 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -498,7 +498,10 @@ impl<'a> Parser<'a> { /// Checks if this expression is a successfully parsed statement. fn expr_is_complete(&self, e: &Expr) -> bool { self.restrictions.contains(Restrictions::STMT_EXPR) - && !classify::expr_requires_semi_to_be_stmt_FIXME(e) + && match e.kind { + ExprKind::MacCall(_) => false, + _ => !classify::expr_requires_semi_to_be_stmt(e), + } } /// Parses `x..y`, `x..=y`, and `x..`/`x..=`. @@ -2694,7 +2697,10 @@ impl<'a> Parser<'a> { // If it's not a free-standing expression, and is followed by a block, // then it's very likely the condition to an `else if`. if self.check(&TokenKind::OpenDelim(Delimiter::Brace)) - && classify::expr_requires_semi_to_be_stmt_FIXME(&cond) => + && match cond.kind { + ExprKind::MacCall(_) => true, + _ => classify::expr_requires_semi_to_be_stmt(&cond), + } => { self.dcx().emit_err(errors::ExpectedElseBlock { first_tok_span, @@ -3136,8 +3142,10 @@ impl<'a> Parser<'a> { err })?; - let require_comma = classify::expr_requires_semi_to_be_stmt_FIXME(&expr) - && this.token != token::CloseDelim(Delimiter::Brace); + let require_comma = match expr.kind { + ExprKind::MacCall(_) => true, + _ => classify::expr_requires_semi_to_be_stmt(&expr), + } && this.token != token::CloseDelim(Delimiter::Brace); if !require_comma { arm_body = Some(expr); diff --git a/compiler/rustc_parse/src/parser/stmt.rs b/compiler/rustc_parse/src/parser/stmt.rs index f64a480a18c32..684799eb6a78d 100644 --- a/compiler/rustc_parse/src/parser/stmt.rs +++ b/compiler/rustc_parse/src/parser/stmt.rs @@ -648,8 +648,10 @@ impl<'a> Parser<'a> { match &mut stmt.kind { // Expression without semicolon. StmtKind::Expr(expr) - if classify::expr_requires_semi_to_be_stmt_FIXME(expr) - && !expr.attrs.is_empty() + if match expr.kind { + ExprKind::MacCall(_) => true, + _ => classify::expr_requires_semi_to_be_stmt(expr), + } && !expr.attrs.is_empty() && ![token::Eof, token::Semi, token::CloseDelim(Delimiter::Brace)] .contains(&self.token.kind) => { @@ -663,7 +665,10 @@ impl<'a> Parser<'a> { // Expression without semicolon. StmtKind::Expr(expr) if self.token != token::Eof - && classify::expr_requires_semi_to_be_stmt_FIXME(expr) => + && match expr.kind { + ExprKind::MacCall(_) => true, + _ => classify::expr_requires_semi_to_be_stmt(expr), + } => { // Just check for errors and recover; do not eat semicolon yet. From c5a0eb12466b6591bc3a3d373c51fabd19735533 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 27 Dec 2023 17:29:52 -0800 Subject: [PATCH 04/17] Add ExprKind::MacCall statement boundary tests --- tests/ui/macros/stringify.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/ui/macros/stringify.rs b/tests/ui/macros/stringify.rs index 492bd2450b1bf..df1cf5d8a9102 100644 --- a/tests/ui/macros/stringify.rs +++ b/tests/ui/macros/stringify.rs @@ -213,6 +213,21 @@ fn test_expr() { "match () { _ => ({ 1 }) - 1, }", "match () { _ => { 1 } - 1 }", ); + c2_match_arm!( + [ m!() - 1 ], + "match () { _ => m!() - 1, }", + "match () { _ => m!() - 1 }", + ); + c2_match_arm!( + [ m![] - 1 ], + "match () { _ => m![] - 1, }", + "match () { _ => m![] - 1 }", + ); + c2_match_arm!( + [ m! {} - 1 ], + "match () { _ => m! {} - 1, }", + "match () { _ => m! {} - 1 }", + ); // ExprKind::Closure c1!(expr, [ || {} ], "|| {}"); @@ -720,6 +735,21 @@ fn test_stmt() { "(loop { break 1; }) - 1;", "loop { break 1; } - 1", ); + c2_minus_one!( + [ m!() ], + "m!() - 1;", + "m!() - 1" + ); + c2_minus_one!( + [ m![] ], + "m![] - 1;", + "m![] - 1" + ); + c2_minus_one!( + [ m! {} ], + "m! {} - 1;", // FIXME(dtolnay): needs parens, otherwise this is 2 separate statements + "m! {} - 1" + ); // StmtKind::Empty c1!(stmt, [ ; ], ";"); From 7f2ffbdbc6c0d5e1c7fabe0bef56ec49d10bfeab Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 27 Dec 2023 17:30:54 -0800 Subject: [PATCH 05/17] Fix pretty printer statement boundaries after braced macro call --- compiler/rustc_ast_pretty/src/pprust/state/fixup.rs | 6 +----- tests/ui/macros/stringify.rs | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs index 9934f972f9bfd..d21cb82f83b28 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs @@ -128,11 +128,7 @@ impl FixupContext { /// The documentation on `FixupContext::leftmost_subexpression_in_stmt` has /// examples. pub fn would_cause_statement_boundary(self, expr: &Expr) -> bool { - self.leftmost_subexpression_in_stmt - && match expr.kind { - ExprKind::MacCall(_) => false, - _ => !classify::expr_requires_semi_to_be_stmt(expr), - } + self.leftmost_subexpression_in_stmt && !classify::expr_requires_semi_to_be_stmt(expr) } /// Determine whether parentheses are needed around the given `let` diff --git a/tests/ui/macros/stringify.rs b/tests/ui/macros/stringify.rs index df1cf5d8a9102..a66a5513ffae3 100644 --- a/tests/ui/macros/stringify.rs +++ b/tests/ui/macros/stringify.rs @@ -225,7 +225,7 @@ fn test_expr() { ); c2_match_arm!( [ m! {} - 1 ], - "match () { _ => m! {} - 1, }", + "match () { _ => (m! {}) - 1, }", // parenthesis is redundant "match () { _ => m! {} - 1 }", ); @@ -747,7 +747,7 @@ fn test_stmt() { ); c2_minus_one!( [ m! {} ], - "m! {} - 1;", // FIXME(dtolnay): needs parens, otherwise this is 2 separate statements + "(m! {}) - 1;", "m! {} - 1" ); From d9bb73331eff4bbcfb5610b96d2411ef751db20d Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 27 Dec 2023 19:32:49 -0800 Subject: [PATCH 06/17] Delete MacCall case from pretty-printing semicolon after StmtKind::Expr I didn't figure out how to reach this condition with `expr` containing `ExprKind::MacCall`. All the approaches I tried ended up with the macro call ending up in the `StmtKind::MacCall` case below instead. In any case, from visual inspection this is a bugfix. If we do end up with a `StmtKind::Expr` containing `ExprKind::MacCall` with brace delimiter, it would not need ";" printed after it. --- compiler/rustc_ast_pretty/src/pprust/state.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/rustc_ast_pretty/src/pprust/state.rs b/compiler/rustc_ast_pretty/src/pprust/state.rs index fe17ff41bc34d..2c176828c841f 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state.rs @@ -1253,10 +1253,7 @@ impl<'a> State<'a> { ast::StmtKind::Expr(expr) => { self.space_if_not_bol(); self.print_expr_outer_attr_style(expr, false, FixupContext::new_stmt()); - if match expr.kind { - ast::ExprKind::MacCall(_) => true, - _ => classify::expr_requires_semi_to_be_stmt(expr), - } { + if classify::expr_requires_semi_to_be_stmt(expr) { self.word(";"); } } From 0ca322c774144ea023561bc0ab556a73c7fc7337 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 27 Dec 2023 19:49:31 -0800 Subject: [PATCH 07/17] Add test of unused_parens lint involving macro calls --- tests/ui/lint/lint-unnecessary-parens.fixed | 31 ++++++++ tests/ui/lint/lint-unnecessary-parens.rs | 31 ++++++++ tests/ui/lint/lint-unnecessary-parens.stderr | 74 +++++++++++++++----- 3 files changed, 117 insertions(+), 19 deletions(-) diff --git a/tests/ui/lint/lint-unnecessary-parens.fixed b/tests/ui/lint/lint-unnecessary-parens.fixed index 973bbd70f257f..760897c5143f1 100644 --- a/tests/ui/lint/lint-unnecessary-parens.fixed +++ b/tests/ui/lint/lint-unnecessary-parens.fixed @@ -46,6 +46,28 @@ pub fn parens_with_keyword(e: &[()]) -> i32 { macro_rules! baz { ($($foo:expr),+) => { ($($foo),*) + }; +} + +macro_rules! unit { + () => { + () + }; +} + +struct One; + +impl std::ops::Sub for () { + type Output = i32; + fn sub(self, _: One) -> Self::Output { + -1 + } +} + +impl std::ops::Neg for One { + type Output = i32; + fn neg(self) -> Self::Output { + -1 } } @@ -94,4 +116,13 @@ fn main() { let _a = baz!(3, 4); let _b = baz!(3); + + let _ = { + unit!() - One //~ ERROR unnecessary parentheses around block return value + } + { + unit![] - One //~ ERROR unnecessary parentheses around block return value + } + { + // FIXME: false positive. This parenthesis is required. + unit! {} - One //~ ERROR unnecessary parentheses around block return value + }; } diff --git a/tests/ui/lint/lint-unnecessary-parens.rs b/tests/ui/lint/lint-unnecessary-parens.rs index 40cd61fcc2c0e..7cbaac8ae5403 100644 --- a/tests/ui/lint/lint-unnecessary-parens.rs +++ b/tests/ui/lint/lint-unnecessary-parens.rs @@ -46,6 +46,28 @@ pub fn parens_with_keyword(e: &[()]) -> i32 { macro_rules! baz { ($($foo:expr),+) => { ($($foo),*) + }; +} + +macro_rules! unit { + () => { + () + }; +} + +struct One; + +impl std::ops::Sub for () { + type Output = i32; + fn sub(self, _: One) -> Self::Output { + -1 + } +} + +impl std::ops::Neg for One { + type Output = i32; + fn neg(self) -> Self::Output { + -1 } } @@ -94,4 +116,13 @@ fn main() { let _a = baz!(3, 4); let _b = baz!(3); + + let _ = { + (unit!() - One) //~ ERROR unnecessary parentheses around block return value + } + { + (unit![] - One) //~ ERROR unnecessary parentheses around block return value + } + { + // FIXME: false positive. This parenthesis is required. + (unit! {} - One) //~ ERROR unnecessary parentheses around block return value + }; } diff --git a/tests/ui/lint/lint-unnecessary-parens.stderr b/tests/ui/lint/lint-unnecessary-parens.stderr index ba7a78b8da1a9..755dd5fc3094b 100644 --- a/tests/ui/lint/lint-unnecessary-parens.stderr +++ b/tests/ui/lint/lint-unnecessary-parens.stderr @@ -124,7 +124,7 @@ LL + return 1; | error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:52:31 + --> $DIR/lint-unnecessary-parens.rs:74:31 | LL | pub const CONST_ITEM: usize = (10); | ^ ^ @@ -136,7 +136,7 @@ LL + pub const CONST_ITEM: usize = 10; | error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:53:33 + --> $DIR/lint-unnecessary-parens.rs:75:33 | LL | pub static STATIC_ITEM: usize = (10); | ^ ^ @@ -148,7 +148,7 @@ LL + pub static STATIC_ITEM: usize = 10; | error: unnecessary parentheses around function argument - --> $DIR/lint-unnecessary-parens.rs:57:9 + --> $DIR/lint-unnecessary-parens.rs:79:9 | LL | bar((true)); | ^ ^ @@ -160,7 +160,7 @@ LL + bar(true); | error: unnecessary parentheses around `if` condition - --> $DIR/lint-unnecessary-parens.rs:59:8 + --> $DIR/lint-unnecessary-parens.rs:81:8 | LL | if (true) {} | ^ ^ @@ -172,7 +172,7 @@ LL + if true {} | error: unnecessary parentheses around `while` condition - --> $DIR/lint-unnecessary-parens.rs:60:11 + --> $DIR/lint-unnecessary-parens.rs:82:11 | LL | while (true) {} | ^ ^ @@ -184,7 +184,7 @@ LL + while true {} | error: unnecessary parentheses around `match` scrutinee expression - --> $DIR/lint-unnecessary-parens.rs:61:11 + --> $DIR/lint-unnecessary-parens.rs:83:11 | LL | match (true) { | ^ ^ @@ -196,7 +196,7 @@ LL + match true { | error: unnecessary parentheses around `let` scrutinee expression - --> $DIR/lint-unnecessary-parens.rs:64:16 + --> $DIR/lint-unnecessary-parens.rs:86:16 | LL | if let 1 = (1) {} | ^ ^ @@ -208,7 +208,7 @@ LL + if let 1 = 1 {} | error: unnecessary parentheses around `let` scrutinee expression - --> $DIR/lint-unnecessary-parens.rs:65:19 + --> $DIR/lint-unnecessary-parens.rs:87:19 | LL | while let 1 = (2) {} | ^ ^ @@ -220,7 +220,7 @@ LL + while let 1 = 2 {} | error: unnecessary parentheses around method argument - --> $DIR/lint-unnecessary-parens.rs:81:24 + --> $DIR/lint-unnecessary-parens.rs:103:24 | LL | X { y: false }.foo((true)); | ^ ^ @@ -232,7 +232,7 @@ LL + X { y: false }.foo(true); | error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:83:18 + --> $DIR/lint-unnecessary-parens.rs:105:18 | LL | let mut _a = (0); | ^ ^ @@ -244,7 +244,7 @@ LL + let mut _a = 0; | error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:84:10 + --> $DIR/lint-unnecessary-parens.rs:106:10 | LL | _a = (0); | ^ ^ @@ -256,7 +256,7 @@ LL + _a = 0; | error: unnecessary parentheses around assigned value - --> $DIR/lint-unnecessary-parens.rs:85:11 + --> $DIR/lint-unnecessary-parens.rs:107:11 | LL | _a += (1); | ^ ^ @@ -268,7 +268,7 @@ LL + _a += 1; | error: unnecessary parentheses around pattern - --> $DIR/lint-unnecessary-parens.rs:87:8 + --> $DIR/lint-unnecessary-parens.rs:109:8 | LL | let(mut _a) = 3; | ^ ^ @@ -280,7 +280,7 @@ LL + let mut _a = 3; | error: unnecessary parentheses around pattern - --> $DIR/lint-unnecessary-parens.rs:88:9 + --> $DIR/lint-unnecessary-parens.rs:110:9 | LL | let (mut _a) = 3; | ^ ^ @@ -292,7 +292,7 @@ LL + let mut _a = 3; | error: unnecessary parentheses around pattern - --> $DIR/lint-unnecessary-parens.rs:89:8 + --> $DIR/lint-unnecessary-parens.rs:111:8 | LL | let( mut _a) = 3; | ^^ ^ @@ -304,7 +304,7 @@ LL + let mut _a = 3; | error: unnecessary parentheses around pattern - --> $DIR/lint-unnecessary-parens.rs:91:8 + --> $DIR/lint-unnecessary-parens.rs:113:8 | LL | let(_a) = 3; | ^ ^ @@ -316,7 +316,7 @@ LL + let _a = 3; | error: unnecessary parentheses around pattern - --> $DIR/lint-unnecessary-parens.rs:92:9 + --> $DIR/lint-unnecessary-parens.rs:114:9 | LL | let (_a) = 3; | ^ ^ @@ -328,7 +328,7 @@ LL + let _a = 3; | error: unnecessary parentheses around pattern - --> $DIR/lint-unnecessary-parens.rs:93:8 + --> $DIR/lint-unnecessary-parens.rs:115:8 | LL | let( _a) = 3; | ^^ ^ @@ -339,5 +339,41 @@ LL - let( _a) = 3; LL + let _a = 3; | -error: aborting due to 28 previous errors +error: unnecessary parentheses around block return value + --> $DIR/lint-unnecessary-parens.rs:121:9 + | +LL | (unit!() - One) + | ^ ^ + | +help: remove these parentheses + | +LL - (unit!() - One) +LL + unit!() - One + | + +error: unnecessary parentheses around block return value + --> $DIR/lint-unnecessary-parens.rs:123:9 + | +LL | (unit![] - One) + | ^ ^ + | +help: remove these parentheses + | +LL - (unit![] - One) +LL + unit![] - One + | + +error: unnecessary parentheses around block return value + --> $DIR/lint-unnecessary-parens.rs:126:9 + | +LL | (unit! {} - One) + | ^ ^ + | +help: remove these parentheses + | +LL - (unit! {} - One) +LL + unit! {} - One + | + +error: aborting due to 31 previous errors From c6c18a0151a374e9376952aa7ef62110b14055d5 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Wed, 27 Dec 2023 20:11:00 -0800 Subject: [PATCH 08/17] Document the situation with unused_parens lint and braced macro calls --- compiler/rustc_lint/src/unused.rs | 32 +++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/unused.rs b/compiler/rustc_lint/src/unused.rs index 19b71564e2ebb..8866b2be07847 100644 --- a/compiler/rustc_lint/src/unused.rs +++ b/compiler/rustc_lint/src/unused.rs @@ -677,6 +677,33 @@ trait UnusedDelimLint { } // Check if LHS needs parens to prevent false-positives in cases like `fn x() -> u8 { ({ 0 } + 1) }`. + // + // FIXME: https://github.com/rust-lang/rust/issues/119426 + // The syntax tree in this code is from after macro expansion, so the + // current implementation has both false negatives and false positives + // related to expressions containing macros. + // + // macro_rules! m1 { + // () => { + // 1 + // }; + // } + // + // fn f1() -> u8 { + // // Lint says parens are not needed, but they are. + // (m1! {} + 1) + // } + // + // macro_rules! m2 { + // () => { + // loop { break 1; } + // }; + // } + // + // fn f2() -> u8 { + // // Lint says parens are needed, but they are not. + // (m2!() + 1) + // } { let mut innermost = inner; loop { @@ -688,10 +715,7 @@ trait UnusedDelimLint { ExprKind::Index(base, _subscript, _) => base, _ => break, }; - if match innermost.kind { - ExprKind::MacCall(_) => false, - _ => !classify::expr_requires_semi_to_be_stmt(innermost), - } { + if !classify::expr_requires_semi_to_be_stmt(innermost) { return true; } } From 4a80865437f03e918225ed13ce692122e32c8045 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 15:25:34 -0800 Subject: [PATCH 09/17] Add parser tests for statement boundary insertion --- tests/ui/parser/macro/statement-boundaries.rs | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 tests/ui/parser/macro/statement-boundaries.rs diff --git a/tests/ui/parser/macro/statement-boundaries.rs b/tests/ui/parser/macro/statement-boundaries.rs new file mode 100644 index 0000000000000..67a6aa30f0c5e --- /dev/null +++ b/tests/ui/parser/macro/statement-boundaries.rs @@ -0,0 +1,104 @@ +//@ run-pass +//@ edition:2021 + +// This is a test of several uses of rustc_ast::util::classify::expr_requires_semi_to_be_stmt +// by the Rust parser, which relates to the insertion of statement boundaries +// after certain kinds of expressions if they appear at the head of a statement. + +#![allow(unused_braces, unused_unsafe)] + +macro_rules! unit { + () => { + { () } + }; +} + +#[derive(Copy, Clone)] +struct X; + +fn main() { + let x = X; + + // There is a statement boundary before `|x| x`, so it's a closure. + let _: fn(X) -> X = { if true {} |x| x }; + let _: fn(X) -> X = { if true {} else {} |x| x }; + let _: fn(X) -> X = { match () { () => {} } |x| x }; + let _: fn(X) -> X = { { () } |x| x }; + let _: fn(X) -> X = { unsafe {} |x| x }; + let _: fn(X) -> X = { while false {} |x| x }; + let _: fn(X) -> X = { loop { break; } |x| x }; + let _: fn(X) -> X = { for _ in 0..0 {} |x| x }; + let _: fn(X) -> X = { const {} |x| x }; + let _: fn(X) -> X = { unit! {} |x| x }; + + // No statement boundary, so `|x| x` is 2× BitOr operation. + () = { "" |x| x }; + () = { ("") |x| x }; + () = { [""] |x| x }; + () = { unit!() |x| x }; + () = { unit![] |x| x }; + + // All the same cases, but as a match arm. + () = match x { + // Statement boundary before `| X`, which becomes a new arm with leading vert. + X if false => if true {} | X if false => {} + X if false => if true {} else {} | X if false => {} + X if false => match () { () => {} } | X if false => {} + X if false => { () } | X if false => {} + X if false => unsafe {} | X if false => {} + X if false => while false {} | X if false => {} + X if false => loop { break; } | X if false => {} + X if false => for _ in 0..0 {} | X if false => {} + X if false => const {} | X if false => {} + + // No statement boundary, so `| X` is BitOr. + X if false => "" | X, + X if false => ("") | X, + X if false => [""] | X, + X if false => unit! {} | X, // !! inconsistent with braced mac call in statement position + X if false => unit!() | X, + X if false => unit![] | X, + + X => {} + }; + + // Test how the statement boundary logic interacts with macro metavariables / + // "invisible delimiters". + macro_rules! assert_statement_boundary { + ($expr:expr) => { + let _: fn(X) -> X = { $expr |x| x }; + + () = match X { + X if false => $expr | X if false => {} + X => {} + }; + }; + } + macro_rules! assert_no_statement_boundary { + ($expr:expr) => { + () = { $expr |x| x }; + + () = match x { + X if false => $expr | X, + X => {} + }; + }; + } + assert_statement_boundary!(if true {}); + assert_no_statement_boundary!(""); +} + +impl std::ops::BitOr for () { + type Output = (); + fn bitor(self, _: X) {} +} + +impl std::ops::BitOr for &str { + type Output = (); + fn bitor(self, _: X) {} +} + +impl std::ops::BitOr for [T; N] { + type Output = (); + fn bitor(self, _: X) {} +} From 8adcaf5df27ac63b5e83612bd08cae12d3d1725e Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 16:13:55 -0800 Subject: [PATCH 10/17] Mark Parser::expr_is_complete call sites --- compiler/rustc_parse/src/parser/expr.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 5f7bd0835d3d6..602dbbb572502 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -190,7 +190,7 @@ impl<'a> Parser<'a> { } }; - if !self.should_continue_as_assoc_expr(&lhs) { + if !self.should_continue_as_assoc_expr_FIXME(&lhs) { return Ok(lhs); } @@ -383,8 +383,9 @@ impl<'a> Parser<'a> { Ok(lhs) } - fn should_continue_as_assoc_expr(&mut self, lhs: &Expr) -> bool { - match (self.expr_is_complete(lhs), AssocOp::from_token(&self.token)) { + #[allow(non_snake_case)] + fn should_continue_as_assoc_expr_FIXME(&mut self, lhs: &Expr) -> bool { + match (self.expr_is_complete_FIXME(lhs), AssocOp::from_token(&self.token)) { // Semi-statement forms are odd: // See https://github.com/rust-lang/rust/issues/29071 (true, None) => false, @@ -496,7 +497,8 @@ impl<'a> Parser<'a> { } /// Checks if this expression is a successfully parsed statement. - fn expr_is_complete(&self, e: &Expr) -> bool { + #[allow(non_snake_case)] + fn expr_is_complete_FIXME(&self, e: &Expr) -> bool { self.restrictions.contains(Restrictions::STMT_EXPR) && match e.kind { ExprKind::MacCall(_) => false, @@ -1012,7 +1014,7 @@ impl<'a> Parser<'a> { e = self.parse_dot_suffix_expr(lo, e)?; continue; } - if self.expr_is_complete(&e) { + if self.expr_is_complete_FIXME(&e) { return Ok(e); } e = match self.token.kind { From 9dbe33d256e6b3919d2fc0a6561b78d2ed3c628c Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 16:41:25 -0800 Subject: [PATCH 11/17] Document MacCall special case in Parser::expr_is_complete --- compiler/rustc_parse/src/parser/expr.rs | 51 +++++++++++++++++++++---- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 602dbbb572502..98b41013a2c65 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -190,7 +190,7 @@ impl<'a> Parser<'a> { } }; - if !self.should_continue_as_assoc_expr_FIXME(&lhs) { + if !self.should_continue_as_assoc_expr(&lhs) { return Ok(lhs); } @@ -383,9 +383,8 @@ impl<'a> Parser<'a> { Ok(lhs) } - #[allow(non_snake_case)] - fn should_continue_as_assoc_expr_FIXME(&mut self, lhs: &Expr) -> bool { - match (self.expr_is_complete_FIXME(lhs), AssocOp::from_token(&self.token)) { + fn should_continue_as_assoc_expr(&mut self, lhs: &Expr) -> bool { + match (self.expr_is_complete(lhs), AssocOp::from_token(&self.token)) { // Semi-statement forms are odd: // See https://github.com/rust-lang/rust/issues/29071 (true, None) => false, @@ -497,10 +496,48 @@ impl<'a> Parser<'a> { } /// Checks if this expression is a successfully parsed statement. - #[allow(non_snake_case)] - fn expr_is_complete_FIXME(&self, e: &Expr) -> bool { + /// + /// This determines whether to continue parsing more of an expression in a + /// match arm (false) vs continue to the next arm (true). + /// + /// ```ignore (illustrative) + /// match ... { + /// // Is this calling $e as a function, or is it the start of a new arm + /// // with a tuple pattern? + /// _ => $e ( + /// ^ ) + /// + /// // Is this an Index operation, or new arm with a slice pattern? + /// _ => $e [ + /// ^ ] + /// + /// // Is this a binary operator, or leading vert in a new arm? Same for + /// // other punctuation which can either be a binary operator in + /// // expression or unary operator in pattern, such as `&` and `-`. + /// _ => $e | + /// ^ + /// } + /// ``` + /// + /// If $e is something like `path::to` or `(…)`, continue parsing the same + /// arm. + /// + /// If $e is something like `{}` or `if … {}`, then terminate the current + /// arm and parse a new arm. + fn expr_is_complete(&self, e: &Expr) -> bool { self.restrictions.contains(Restrictions::STMT_EXPR) && match e.kind { + // Surprising special case: even though braced macro calls like + // `m! {}` normally introduce a statement boundary when found at + // the head of a statement, in match arms they do not terminate + // the arm. + // + // let _ = { m! {} () }; // macro call followed by unit + // + // match ... { + // _ => m! {} (), // macro that expands to a function, which is then called + // } + // ExprKind::MacCall(_) => false, _ => !classify::expr_requires_semi_to_be_stmt(e), } @@ -1014,7 +1051,7 @@ impl<'a> Parser<'a> { e = self.parse_dot_suffix_expr(lo, e)?; continue; } - if self.expr_is_complete_FIXME(&e) { + if self.expr_is_complete(&e) { return Ok(e); } e = match self.token.kind { From 728e117166e1dab6f8333d61e1315172c558fce5 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 17:15:34 -0800 Subject: [PATCH 12/17] Document MacCall special case in Parser::parse_arm --- compiler/rustc_parse/src/parser/expr.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 98b41013a2c65..bb0873a814dfa 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -3182,6 +3182,17 @@ impl<'a> Parser<'a> { })?; let require_comma = match expr.kind { + // Special case: braced macro calls require comma in a match + // arm, even though they do not require semicolon in a + // statement. + // + // m! {} // okay without semicolon + // + // match ... { + // _ => m! {}, // requires comma + // _ => ... + // } + // ExprKind::MacCall(_) => true, _ => classify::expr_requires_semi_to_be_stmt(&expr), } && this.token != token::CloseDelim(Delimiter::Brace); From 0f6a51d4958dff5a29c121801bcdd619d71db541 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 17:34:50 -0800 Subject: [PATCH 13/17] Add macro calls to else-no-if parser test --- tests/ui/parser/else-no-if.rs | 30 ++++++++++++++++ tests/ui/parser/else-no-if.stderr | 58 ++++++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/tests/ui/parser/else-no-if.rs b/tests/ui/parser/else-no-if.rs index f0b40ecde6660..ad5262cd2cc93 100644 --- a/tests/ui/parser/else-no-if.rs +++ b/tests/ui/parser/else-no-if.rs @@ -1,3 +1,7 @@ +macro_rules! falsy { + () => { false }; +} + fn foo() { if true { } else false { @@ -25,6 +29,32 @@ fn foo4() { {} } +fn foo5() { + if true { + } else falsy!() { + //~^ ERROR expected `{`, found `falsy` + } +} + +fn foo6() { + if true { + } else falsy!(); + //~^ ERROR expected `{`, found `falsy` +} + +fn foo7() { + if true { + } else falsy! {} { + //~^ ERROR expected `{`, found `falsy` + } +} + +fn foo8() { + if true { + } else falsy! {}; + //~^ ERROR expected `{`, found `falsy` +} + fn falsy() -> bool { false } diff --git a/tests/ui/parser/else-no-if.stderr b/tests/ui/parser/else-no-if.stderr index b9c1a75276c1f..9954505e7c8d4 100644 --- a/tests/ui/parser/else-no-if.stderr +++ b/tests/ui/parser/else-no-if.stderr @@ -1,5 +1,5 @@ error: expected `{`, found keyword `false` - --> $DIR/else-no-if.rs:3:12 + --> $DIR/else-no-if.rs:7:12 | LL | } else false { | ---- ^^^^^ @@ -12,7 +12,7 @@ LL | } else if false { | ++ error: expected `{`, found `falsy` - --> $DIR/else-no-if.rs:10:12 + --> $DIR/else-no-if.rs:14:12 | LL | } else falsy() { | ---- ^^^^^ @@ -25,7 +25,7 @@ LL | } else if falsy() { | ++ error: expected `{`, found `falsy` - --> $DIR/else-no-if.rs:17:12 + --> $DIR/else-no-if.rs:21:12 | LL | } else falsy(); | ^^^^^ expected `{` @@ -36,7 +36,7 @@ LL | } else { falsy() }; | + + error: expected `{`, found keyword `loop` - --> $DIR/else-no-if.rs:23:12 + --> $DIR/else-no-if.rs:27:12 | LL | } else loop{} | ^^^^ expected `{` @@ -46,5 +46,53 @@ help: try placing this code inside a block LL | } else { loop{} } | + + -error: aborting due to 4 previous errors +error: expected `{`, found `falsy` + --> $DIR/else-no-if.rs:34:12 + | +LL | } else falsy!() { + | ---- ^^^^^ + | | + | expected an `if` or a block after this `else` + | +help: add an `if` if this is the condition of a chained `else if` statement + | +LL | } else if falsy!() { + | ++ + +error: expected `{`, found `falsy` + --> $DIR/else-no-if.rs:41:12 + | +LL | } else falsy!(); + | ^^^^^ expected `{` + | +help: try placing this code inside a block + | +LL | } else { falsy!() }; + | + + + +error: expected `{`, found `falsy` + --> $DIR/else-no-if.rs:47:12 + | +LL | } else falsy! {} { + | ---- ^^^^^ + | | + | expected an `if` or a block after this `else` + | +help: add an `if` if this is the condition of a chained `else if` statement + | +LL | } else if falsy! {} { + | ++ + +error: expected `{`, found `falsy` + --> $DIR/else-no-if.rs:54:12 + | +LL | } else falsy! {}; + | ^^^^^ expected `{` + | +help: try placing this code inside a block + | +LL | } else { falsy! {} }; + | + + + +error: aborting due to 8 previous errors From aedc1b6ad4845242d06a3f7cfb9b57db227b3511 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 17:37:32 -0800 Subject: [PATCH 14/17] Remove MacCall special case from recovery after missing 'if' after 'else' The change to the test is a little goofy because the compiler was guessing "correctly" before that `falsy! {}` is the condition as opposed to the else body. But I believe this change is fundamentally correct. Braced macro invocations in statement position are most often item-like (`thread_local! {...}`) as opposed to parenthesized macro invocations which are condition-like (`cfg!(...)`). --- compiler/rustc_parse/src/parser/expr.rs | 34 ++++++++++++++++++++----- tests/ui/parser/else-no-if.stderr | 10 +++----- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index bb0873a814dfa..52e3e33691a13 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -2733,13 +2733,35 @@ impl<'a> Parser<'a> { let first_tok_span = self.token.span; match self.parse_expr() { Ok(cond) - // If it's not a free-standing expression, and is followed by a block, - // then it's very likely the condition to an `else if`. + // Try to guess the difference between a "condition-like" vs + // "statement-like" expression. + // + // We are seeing the following code, in which $cond is neither + // ExprKind::Block nor ExprKind::If (the 2 cases wherein this + // would be valid syntax). + // + // if ... { + // } else $cond + // + // If $cond is "condition-like" such as ExprKind::Binary, we + // want to suggest inserting `if`. + // + // if ... { + // } else if a == b { + // ^^ + // } + // + // If $cond is "statement-like" such as ExprKind::While then we + // want to suggest wrapping in braces. + // + // if ... { + // } else { + // ^ + // while true {} + // } + // ^ if self.check(&TokenKind::OpenDelim(Delimiter::Brace)) - && match cond.kind { - ExprKind::MacCall(_) => true, - _ => classify::expr_requires_semi_to_be_stmt(&cond), - } => + && classify::expr_requires_semi_to_be_stmt(&cond) => { self.dcx().emit_err(errors::ExpectedElseBlock { first_tok_span, diff --git a/tests/ui/parser/else-no-if.stderr b/tests/ui/parser/else-no-if.stderr index 9954505e7c8d4..2e3e8f6b50e95 100644 --- a/tests/ui/parser/else-no-if.stderr +++ b/tests/ui/parser/else-no-if.stderr @@ -74,14 +74,12 @@ error: expected `{`, found `falsy` --> $DIR/else-no-if.rs:47:12 | LL | } else falsy! {} { - | ---- ^^^^^ - | | - | expected an `if` or a block after this `else` + | ^^^^^ expected `{` | -help: add an `if` if this is the condition of a chained `else if` statement +help: try placing this code inside a block | -LL | } else if falsy! {} { - | ++ +LL | } else { falsy! {} } { + | + + error: expected `{`, found `falsy` --> $DIR/else-no-if.rs:54:12 From 53521faf06cb5ada4df7984fcd53b602b4ee2dd0 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 29 Dec 2023 18:04:04 -0800 Subject: [PATCH 15/17] Remove MacCall special cases from Parser::parse_full_stmt It is impossible for expr here to be a braced macro call. Expr comes from `parse_stmt_without_recovery`, in which macro calls are parsed by `parse_stmt_mac`. See this part: let kind = if (style == MacStmtStyle::Braces && self.token != token::Dot && self.token != token::Question) || self.token == token::Semi || self.token == token::Eof { StmtKind::MacCall(P(MacCallStmt { mac, style, attrs, tokens: None })) } else { // Since none of the above applied, this is an expression statement macro. let e = self.mk_expr(lo.to(hi), ExprKind::MacCall(mac)); let e = self.maybe_recover_from_bad_qpath(e)?; let e = self.parse_expr_dot_or_call_with(e, lo, attrs)?; let e = self.parse_expr_assoc_with( 0, LhsExpr::AlreadyParsed { expr: e, starts_statement: false }, )?; StmtKind::Expr(e) }; A braced macro call at the head of a statement is always either extended into ExprKind::Field / MethodCall / Await / Try / Binary, or else returned as StmtKind::MacCall. We can never get a StmtKind::Expr containing ExprKind::MacCall containing brace delimiter. --- compiler/rustc_parse/src/parser/stmt.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_parse/src/parser/stmt.rs b/compiler/rustc_parse/src/parser/stmt.rs index 684799eb6a78d..d70afebf1b2da 100644 --- a/compiler/rustc_parse/src/parser/stmt.rs +++ b/compiler/rustc_parse/src/parser/stmt.rs @@ -648,10 +648,8 @@ impl<'a> Parser<'a> { match &mut stmt.kind { // Expression without semicolon. StmtKind::Expr(expr) - if match expr.kind { - ExprKind::MacCall(_) => true, - _ => classify::expr_requires_semi_to_be_stmt(expr), - } && !expr.attrs.is_empty() + if classify::expr_requires_semi_to_be_stmt(expr) + && !expr.attrs.is_empty() && ![token::Eof, token::Semi, token::CloseDelim(Delimiter::Brace)] .contains(&self.token.kind) => { @@ -664,11 +662,7 @@ impl<'a> Parser<'a> { // Expression without semicolon. StmtKind::Expr(expr) - if self.token != token::Eof - && match expr.kind { - ExprKind::MacCall(_) => true, - _ => classify::expr_requires_semi_to_be_stmt(expr), - } => + if self.token != token::Eof && classify::expr_requires_semi_to_be_stmt(expr) => { // Just check for errors and recover; do not eat semicolon yet. From 10227eaee70fb9e042bb0b317ad562677c81661d Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 19 Apr 2024 18:46:16 -0700 Subject: [PATCH 16/17] Add classify::expr_is_complete --- compiler/rustc_ast/src/util/classify.rs | 96 +++++++++++++++---------- compiler/rustc_parse/src/parser/expr.rs | 62 +--------------- 2 files changed, 60 insertions(+), 98 deletions(-) diff --git a/compiler/rustc_ast/src/util/classify.rs b/compiler/rustc_ast/src/util/classify.rs index 86383af1f7c83..f6e9e1a87c4bb 100644 --- a/compiler/rustc_ast/src/util/classify.rs +++ b/compiler/rustc_ast/src/util/classify.rs @@ -1,12 +1,60 @@ //! Routines the parser and pretty-printer use to classify AST nodes. +use crate::ast::ExprKind::*; use crate::{ast, token::Delimiter}; +/// This classification determines whether various syntactic positions break out +/// of parsing the current expression (true) or continue parsing more of the +/// same expression (false). +/// +/// For example, it's relevant in the parsing of match arms: +/// +/// ```ignore (illustrative) +/// match ... { +/// // Is this calling $e as a function, or is it the start of a new arm +/// // with a tuple pattern? +/// _ => $e ( +/// ^ ) +/// +/// // Is this an Index operation, or new arm with a slice pattern? +/// _ => $e [ +/// ^ ] +/// +/// // Is this a binary operator, or leading vert in a new arm? Same for +/// // other punctuation which can either be a binary operator in +/// // expression or unary operator in pattern, such as `&` and `-`. +/// _ => $e | +/// ^ +/// } +/// ``` +/// +/// If $e is something like `{}` or `if … {}`, then terminate the current +/// arm and parse a new arm. +/// +/// If $e is something like `path::to` or `(…)`, continue parsing the same +/// arm. +/// +/// *Almost* the same classification is used as an early bail-out for parsing +/// statements. See `expr_requires_semi_to_be_stmt`. +pub fn expr_is_complete(e: &ast::Expr) -> bool { + matches!( + e.kind, + If(..) + | Match(..) + | Block(..) + | While(..) + | Loop(..) + | ForLoop { .. } + | TryBlock(..) + | ConstBlock(..) + ) +} + /// Does this expression require a semicolon to be treated as a statement? /// /// The negation of this: "can this expression be used as a statement without a -/// semicolon" -- is used as an early bail-out in the parser so that, for -/// instance, +/// semicolon" -- is used as an early bail-out when parsing statements so that, +/// for instance, /// /// ```ignore (illustrative) /// if true {...} else {...} @@ -15,56 +63,26 @@ use crate::{ast, token::Delimiter}; /// /// isn't parsed as `(if true {...} else {...} | x) | 5`. /// -/// Nearly the same early bail-out also occurs in the right-hand side of match -/// arms: +/// Surprising special case: even though braced macro calls like `m! {}` +/// normally do not introduce a boundary when found at the head of a match arm, +/// they do terminate the parsing of a statement. /// /// ```ignore (illustrative) -/// match i { -/// 0 => if true {...} else {...} -/// | x => {} +/// match ... { +/// _ => m! {} (), // macro that expands to a function, which is then called /// } -/// ``` -/// -/// Here the `|` is a leading vert in a second match arm. It is not a binary -/// operator with the If as its left operand. If the first arm were some other -/// expression for which `expr_requires_semi_to_be_stmt` returns true, then the -/// `|` on the next line would be a binary operator (leading to a parse error). /// -/// The statement case and the match-arm case are "nearly" the same early -/// bail-out because of 1 edge case. Macro calls with brace delimiter terminate -/// a statement without a semicolon, but do not terminate a match-arm without -/// comma. -/// -/// ```ignore (illustrative) -/// m! {} - 1; // two statements: a macro call followed by -1 literal -/// -/// match () { -/// _ => m! {} - 1, // binary subtraction operator -/// } +/// let _ = { m! {} () }; // macro call followed by unit /// ``` pub fn expr_requires_semi_to_be_stmt(e: &ast::Expr) -> bool { - use ast::ExprKind::*; - match &e.kind { - If(..) - | Match(..) - | Block(..) - | While(..) - | Loop(..) - | ForLoop { .. } - | TryBlock(..) - | ConstBlock(..) => false, - MacCall(mac_call) => mac_call.args.delim != Delimiter::Brace, - - _ => true, + _ => !expr_is_complete(e), } } /// If an expression ends with `}`, returns the innermost expression ending in the `}` pub fn expr_trailing_brace(mut expr: &ast::Expr) -> Option<&ast::Expr> { - use ast::ExprKind::*; - loop { match &expr.kind { AddrOf(_, _, e) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 52e3e33691a13..441aa5b0806da 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -496,51 +496,8 @@ impl<'a> Parser<'a> { } /// Checks if this expression is a successfully parsed statement. - /// - /// This determines whether to continue parsing more of an expression in a - /// match arm (false) vs continue to the next arm (true). - /// - /// ```ignore (illustrative) - /// match ... { - /// // Is this calling $e as a function, or is it the start of a new arm - /// // with a tuple pattern? - /// _ => $e ( - /// ^ ) - /// - /// // Is this an Index operation, or new arm with a slice pattern? - /// _ => $e [ - /// ^ ] - /// - /// // Is this a binary operator, or leading vert in a new arm? Same for - /// // other punctuation which can either be a binary operator in - /// // expression or unary operator in pattern, such as `&` and `-`. - /// _ => $e | - /// ^ - /// } - /// ``` - /// - /// If $e is something like `path::to` or `(…)`, continue parsing the same - /// arm. - /// - /// If $e is something like `{}` or `if … {}`, then terminate the current - /// arm and parse a new arm. fn expr_is_complete(&self, e: &Expr) -> bool { - self.restrictions.contains(Restrictions::STMT_EXPR) - && match e.kind { - // Surprising special case: even though braced macro calls like - // `m! {}` normally introduce a statement boundary when found at - // the head of a statement, in match arms they do not terminate - // the arm. - // - // let _ = { m! {} () }; // macro call followed by unit - // - // match ... { - // _ => m! {} (), // macro that expands to a function, which is then called - // } - // - ExprKind::MacCall(_) => false, - _ => !classify::expr_requires_semi_to_be_stmt(e), - } + self.restrictions.contains(Restrictions::STMT_EXPR) && classify::expr_is_complete(e) } /// Parses `x..y`, `x..=y`, and `x..`/`x..=`. @@ -3203,21 +3160,8 @@ impl<'a> Parser<'a> { err })?; - let require_comma = match expr.kind { - // Special case: braced macro calls require comma in a match - // arm, even though they do not require semicolon in a - // statement. - // - // m! {} // okay without semicolon - // - // match ... { - // _ => m! {}, // requires comma - // _ => ... - // } - // - ExprKind::MacCall(_) => true, - _ => classify::expr_requires_semi_to_be_stmt(&expr), - } && this.token != token::CloseDelim(Delimiter::Brace); + let require_comma = !classify::expr_is_complete(&expr) + && this.token != token::CloseDelim(Delimiter::Brace); if !require_comma { arm_body = Some(expr); From 78c8dc123495f19b2da8e655cd5b95fff161c8c1 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sat, 20 Apr 2024 11:23:01 -0700 Subject: [PATCH 17/17] Fix redundant parens around braced macro call in match arms --- .../rustc_ast_pretty/src/pprust/state/expr.rs | 2 +- .../src/pprust/state/fixup.rs | 57 +++++++++++++++++-- tests/ui/macros/stringify.rs | 2 +- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_ast_pretty/src/pprust/state/expr.rs b/compiler/rustc_ast_pretty/src/pprust/state/expr.rs index 93400c67949d9..1e117c46b6e29 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/expr.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/expr.rs @@ -780,7 +780,7 @@ impl<'a> State<'a> { } _ => { self.end(); // Close the ibox for the pattern. - self.print_expr(body, FixupContext::new_stmt()); + self.print_expr(body, FixupContext::new_match_arm()); self.word(","); } } diff --git a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs index d21cb82f83b28..86d4796e9ce3b 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state/fixup.rs @@ -49,6 +49,38 @@ pub(crate) struct FixupContext { /// No parentheses required. leftmost_subexpression_in_stmt: bool, + /// Print expression such that it can be parsed as a match arm. + /// + /// This is almost equivalent to `stmt`, but the grammar diverges a tiny bit + /// between statements and match arms when it comes to braced macro calls. + /// Macro calls with brace delimiter terminate a statement without a + /// semicolon, but do not terminate a match-arm without comma. + /// + /// ```ignore (illustrative) + /// m! {} - 1; // two statements: a macro call followed by -1 literal + /// + /// match () { + /// _ => m! {} - 1, // binary subtraction operator + /// } + /// ``` + match_arm: bool, + + /// This is almost equivalent to `leftmost_subexpression_in_stmt`, other + /// than for braced macro calls. + /// + /// If we have `m! {} - 1` as an expression, the leftmost subexpression + /// `m! {}` will need to be parenthesized in the statement case but not the + /// match-arm case. + /// + /// ```ignore (illustrative) + /// (m! {}) - 1; // subexpression needs parens + /// + /// match () { + /// _ => m! {} - 1, // no parens + /// } + /// ``` + leftmost_subexpression_in_match_arm: bool, + /// This is the difference between: /// /// ```ignore (illustrative) @@ -68,6 +100,8 @@ impl Default for FixupContext { FixupContext { stmt: false, leftmost_subexpression_in_stmt: false, + match_arm: false, + leftmost_subexpression_in_match_arm: false, parenthesize_exterior_struct_lit: false, } } @@ -76,13 +110,16 @@ impl Default for FixupContext { impl FixupContext { /// Create the initial fixup for printing an expression in statement /// position. - /// - /// This is currently also used for printing an expression as a match-arm, - /// but this is incorrect and leads to over-parenthesizing. pub fn new_stmt() -> Self { FixupContext { stmt: true, ..FixupContext::default() } } + /// Create the initial fixup for printing an expression as the right-hand + /// side of a match arm. + pub fn new_match_arm() -> Self { + FixupContext { match_arm: true, ..FixupContext::default() } + } + /// Create the initial fixup for printing an expression as the "condition" /// of an `if` or `while`. There are a few other positions which are /// grammatically equivalent and also use this, such as the iterator @@ -106,6 +143,9 @@ impl FixupContext { FixupContext { stmt: false, leftmost_subexpression_in_stmt: self.stmt || self.leftmost_subexpression_in_stmt, + match_arm: false, + leftmost_subexpression_in_match_arm: self.match_arm + || self.leftmost_subexpression_in_match_arm, ..self } } @@ -119,7 +159,13 @@ impl FixupContext { /// example the `$b` in `$a + $b` and `-$b`, but not the one in `[$b]` or /// `$a.f($b)`. pub fn subsequent_subexpression(self) -> Self { - FixupContext { stmt: false, leftmost_subexpression_in_stmt: false, ..self } + FixupContext { + stmt: false, + leftmost_subexpression_in_stmt: false, + match_arm: false, + leftmost_subexpression_in_match_arm: false, + ..self + } } /// Determine whether parentheses are needed around the given expression to @@ -128,7 +174,8 @@ impl FixupContext { /// The documentation on `FixupContext::leftmost_subexpression_in_stmt` has /// examples. pub fn would_cause_statement_boundary(self, expr: &Expr) -> bool { - self.leftmost_subexpression_in_stmt && !classify::expr_requires_semi_to_be_stmt(expr) + (self.leftmost_subexpression_in_stmt && !classify::expr_requires_semi_to_be_stmt(expr)) + || (self.leftmost_subexpression_in_match_arm && classify::expr_is_complete(expr)) } /// Determine whether parentheses are needed around the given `let` diff --git a/tests/ui/macros/stringify.rs b/tests/ui/macros/stringify.rs index a66a5513ffae3..472cb4d417be8 100644 --- a/tests/ui/macros/stringify.rs +++ b/tests/ui/macros/stringify.rs @@ -225,7 +225,7 @@ fn test_expr() { ); c2_match_arm!( [ m! {} - 1 ], - "match () { _ => (m! {}) - 1, }", // parenthesis is redundant + "match () { _ => m! {} - 1, }", "match () { _ => m! {} - 1 }", );