From 851066f57e19b645e2dd33392ee7f822cbb2e374 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 18 Jun 2019 01:28:20 +0200 Subject: [PATCH] let_chains: Fix bugs in pretty printing. --- src/libsyntax/parse/parser.rs | 4 ++-- src/libsyntax/print/pprust.rs | 40 ++++++++++++++++++++++++----------- src/libsyntax/util/parser.rs | 16 ++++++++++++++ 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 65936345fe1..b2003e2d6bd 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -41,7 +41,7 @@ use crate::parse::lexer::comments::{doc_comment_style, strip_doc_comment_decoration}; use crate::parse::token::{Token, TokenKind, DelimToken}; use crate::parse::{new_sub_parser_from_file, ParseSess, Directory, DirectoryOwnership}; -use crate::util::parser::{AssocOp, Fixity}; +use crate::util::parser::{AssocOp, Fixity, prec_let_scrutinee_needs_par}; use crate::print::pprust; use crate::ptr::P; use crate::parse::PResult; @@ -3208,7 +3208,7 @@ fn parse_let_expr(&mut self, attrs: ThinVec) -> PResult<'a, P> self.expect(&token::Eq)?; let expr = self.with_res( Restrictions::NO_STRUCT_LITERAL, - |this| this.parse_assoc_expr_with(1 + AssocOp::LAnd.precedence(), None.into()) + |this| this.parse_assoc_expr_with(1 + prec_let_scrutinee_needs_par(), None.into()) )?; let span = lo.to(expr.span); self.sess.let_chains_spans.borrow_mut().push(span); diff --git a/src/libsyntax/print/pprust.rs b/src/libsyntax/print/pprust.rs index c525f2efb49..164fe2f36e1 100644 --- a/src/libsyntax/print/pprust.rs +++ b/src/libsyntax/print/pprust.rs @@ -1715,6 +1715,7 @@ pub fn print_block_maybe_unclosed(&mut self, self.ann.post(self, AnnNode::Block(blk)) } + /// Print a `let pats = scrutinee` expression. pub fn print_let(&mut self, pats: &[P], scrutinee: &ast::Expr) -> io::Result<()> { self.s.word("let ")?; @@ -1722,7 +1723,11 @@ pub fn print_let(&mut self, pats: &[P], scrutinee: &ast::Expr) -> io:: self.s.space()?; self.word_space("=")?; - self.print_expr_as_cond(scrutinee) + self.print_expr_cond_paren( + scrutinee, + Self::cond_needs_par(scrutinee) + || parser::needs_par_as_let_scrutinee(scrutinee.precedence().order()) + ) } fn print_else(&mut self, els: Option<&ast::Expr>) -> io::Result<()> { @@ -1794,21 +1799,18 @@ fn print_call_post(&mut self, args: &[P]) -> io::Result<()> { } pub fn print_expr_maybe_paren(&mut self, expr: &ast::Expr, prec: i8) -> io::Result<()> { - let needs_par = expr.precedence().order() < prec; - if needs_par { - self.popen()?; - } - self.print_expr(expr)?; - if needs_par { - self.pclose()?; - } - Ok(()) + self.print_expr_cond_paren(expr, expr.precedence().order() < prec) } /// Print an expr using syntax that's acceptable in a condition position, such as the `cond` in /// `if cond { ... }`. pub fn print_expr_as_cond(&mut self, expr: &ast::Expr) -> io::Result<()> { - let needs_par = match expr.node { + self.print_expr_cond_paren(expr, Self::cond_needs_par(expr)) + } + + /// Does `expr` need parenthesis when printed in a condition position? + fn cond_needs_par(expr: &ast::Expr) -> bool { + match expr.node { // These cases need parens due to the parse error observed in #26461: `if return {}` // parses as the erroneous construct `if (return {})`, not `if (return) {}`. ast::ExprKind::Closure(..) | @@ -1816,8 +1818,11 @@ pub fn print_expr_as_cond(&mut self, expr: &ast::Expr) -> io::Result<()> { ast::ExprKind::Break(..) => true, _ => parser::contains_exterior_struct_lit(expr), - }; + } + } + /// Print `expr` or `(expr)` when `needs_par` holds. + fn print_expr_cond_paren(&mut self, expr: &ast::Expr, needs_par: bool) -> io::Result<()> { if needs_par { self.popen()?; } @@ -1949,6 +1954,17 @@ fn print_expr_binary(&mut self, // of `(x as i32) < ...`. We need to convince it _not_ to do that. (&ast::ExprKind::Cast { .. }, ast::BinOpKind::Lt) | (&ast::ExprKind::Cast { .. }, ast::BinOpKind::Shl) => parser::PREC_FORCE_PAREN, + // We are given `(let _ = a) OP b`. + // + // - When `OP <= LAnd` we should print `let _ = a OP b` to avoid redundant parens + // as the parser will interpret this as `(let _ = a) OP b`. + // + // - Otherwise, e.g. when we have `(let a = b) < c` in AST, + // parens are required since the parser would interpret `let a = b < c` as + // `let a = (b < c)`. To achieve this, we force parens. + (&ast::ExprKind::Let { .. }, _) if !parser::needs_par_as_let_scrutinee(prec) => { + parser::PREC_FORCE_PAREN + } _ => left_prec, }; diff --git a/src/libsyntax/util/parser.rs b/src/libsyntax/util/parser.rs index 81b5b085937..1e52186a106 100644 --- a/src/libsyntax/util/parser.rs +++ b/src/libsyntax/util/parser.rs @@ -318,6 +318,9 @@ pub fn order(self) -> i8 { ExprPrecedence::Box | ExprPrecedence::AddrOf | // Here `let pats = expr` has `let pats =` as a "unary" prefix of `expr`. + // However, this is not exactly right. When `let _ = a` is the LHS of a binop we + // need parens sometimes. E.g. we can print `(let _ = a) && b` as `let _ = a && b` + // but we need to print `(let _ = a) < b` as-is with parens. ExprPrecedence::Let | ExprPrecedence::Unary => PREC_PREFIX, @@ -352,6 +355,19 @@ pub fn order(self) -> i8 { } } +/// In `let p = e`, operators with precedence `<=` this one requires parenthesis in `e`. +crate fn prec_let_scrutinee_needs_par() -> usize { + AssocOp::LAnd.precedence() +} + +/// Suppose we have `let _ = e` and the `order` of `e`. +/// Is the `order` such that `e` in `let _ = e` needs parenthesis when it is on the RHS? +/// +/// Conversely, suppose that we have `(let _ = a) OP b` and `order` is that of `OP`. +/// Can we print this as `let _ = a OP b`? +crate fn needs_par_as_let_scrutinee(order: i8) -> bool { + order <= prec_let_scrutinee_needs_par() as i8 +} /// Expressions that syntactically contain an "exterior" struct literal i.e., not surrounded by any /// parens or other delimiters, e.g., `X { y: 1 }`, `X { y: 1 }.method()`, `foo == X { y: 1 }` and -- GitLab