Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pprust: fix parenthesization of exprs #43742

Merged
merged 4 commits into from
Sep 8, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
217 changes: 179 additions & 38 deletions src/librustc/hir/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use syntax::print::pp::Breaks::{Consistent, Inconsistent};
use syntax::print::pprust::PrintState;
use syntax::ptr::P;
use syntax::symbol::keywords;
use syntax::util::parser::{self, AssocOp, Fixity};
use syntax_pos::{self, BytePos};

use hir;
Expand Down Expand Up @@ -210,18 +211,6 @@ pub fn visibility_qualified(vis: &hir::Visibility, w: &str) -> String {
})
}

fn needs_parentheses(expr: &hir::Expr) -> bool {
match expr.node {
hir::ExprAssign(..) |
hir::ExprBinary(..) |
hir::ExprClosure(..) |
hir::ExprAssignOp(..) |
hir::ExprCast(..) |
hir::ExprType(..) => true,
_ => false,
}
}

impl<'a> State<'a> {
pub fn cbox(&mut self, u: usize) -> io::Result<()> {
self.boxes.push(pp::Breaks::Consistent);
Expand Down Expand Up @@ -1047,7 +1036,7 @@ impl<'a> State<'a> {
self.cbox(indent_unit - 1)?;
self.ibox(0)?;
self.s.word(" else if ")?;
self.print_expr(&i)?;
self.print_expr_as_cond(&i)?;
self.s.space()?;
self.print_expr(&then)?;
self.print_else(e.as_ref().map(|e| &**e))
Expand Down Expand Up @@ -1075,7 +1064,7 @@ impl<'a> State<'a> {
elseopt: Option<&hir::Expr>)
-> io::Result<()> {
self.head("if")?;
self.print_expr(test)?;
self.print_expr_as_cond(test)?;
self.s.space()?;
self.print_expr(blk)?;
self.print_else(elseopt)
Expand All @@ -1091,7 +1080,7 @@ impl<'a> State<'a> {
self.print_pat(pat)?;
self.s.space()?;
self.word_space("=")?;
self.print_expr(expr)?;
self.print_expr_as_cond(expr)?;
self.s.space()?;
self.print_block(blk)?;
self.print_else(elseopt)
Expand All @@ -1104,8 +1093,31 @@ impl<'a> State<'a> {
self.pclose()
}

pub fn print_expr_maybe_paren(&mut self, expr: &hir::Expr) -> io::Result<()> {
let needs_par = needs_parentheses(expr);
pub fn print_expr_maybe_paren(&mut self, expr: &hir::Expr, prec: i8) -> io::Result<()> {
let needs_par = expr_precedence(expr) < prec;
if needs_par {
self.popen()?;
}
self.print_expr(expr)?;
if needs_par {
self.pclose()?;
}
Ok(())
}

/// 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: &hir::Expr) -> io::Result<()> {
let needs_par = 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) {}`.
hir::ExprClosure(..) |
hir::ExprRet(..) |
hir::ExprBreak(..) => true,

_ => contains_exterior_struct_lit(expr),
};

if needs_par {
self.popen()?;
}
Expand Down Expand Up @@ -1182,7 +1194,14 @@ impl<'a> State<'a> {
}

fn print_expr_call(&mut self, func: &hir::Expr, args: &[hir::Expr]) -> io::Result<()> {
self.print_expr_maybe_paren(func)?;
let prec =
match func.node {
hir::ExprField(..) |
hir::ExprTupField(..) => parser::PREC_FORCE_PAREN,
_ => parser::PREC_POSTFIX,
};

self.print_expr_maybe_paren(func, prec)?;
self.print_call_post(args)
}

Expand All @@ -1191,7 +1210,7 @@ impl<'a> State<'a> {
args: &[hir::Expr])
-> io::Result<()> {
let base_args = &args[1..];
self.print_expr(&args[0])?;
self.print_expr_maybe_paren(&args[0], parser::PREC_POSTFIX)?;
self.s.word(".")?;
self.print_name(segment.name)?;
if !segment.parameters.lifetimes.is_empty() ||
Expand All @@ -1207,15 +1226,25 @@ impl<'a> State<'a> {
lhs: &hir::Expr,
rhs: &hir::Expr)
-> io::Result<()> {
self.print_expr(lhs)?;
let assoc_op = bin_op_to_assoc_op(op.node);
let prec = assoc_op.precedence() as i8;
let fixity = assoc_op.fixity();

let (left_prec, right_prec) = match fixity {
Fixity::Left => (prec, prec + 1),
Fixity::Right => (prec + 1, prec),
Fixity::None => (prec + 1, prec + 1),
};

self.print_expr_maybe_paren(lhs, left_prec)?;
self.s.space()?;
self.word_space(op.node.as_str())?;
self.print_expr(rhs)
self.print_expr_maybe_paren(rhs, right_prec)
}

fn print_expr_unary(&mut self, op: hir::UnOp, expr: &hir::Expr) -> io::Result<()> {
self.s.word(op.as_str())?;
self.print_expr_maybe_paren(expr)
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)
}

fn print_expr_addr_of(&mut self,
Expand All @@ -1224,7 +1253,7 @@ impl<'a> State<'a> {
-> io::Result<()> {
self.s.word("&")?;
self.print_mutability(mutability)?;
self.print_expr_maybe_paren(expr)
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)
}

pub fn print_expr(&mut self, expr: &hir::Expr) -> io::Result<()> {
Expand All @@ -1235,7 +1264,7 @@ impl<'a> State<'a> {
match expr.node {
hir::ExprBox(ref expr) => {
self.word_space("box")?;
self.print_expr(expr)?;
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)?;
}
hir::ExprArray(ref exprs) => {
self.print_expr_vec(exprs)?;
Expand Down Expand Up @@ -1268,13 +1297,15 @@ impl<'a> State<'a> {
self.print_literal(&lit)?;
}
hir::ExprCast(ref expr, ref ty) => {
self.print_expr(&expr)?;
let prec = AssocOp::As.precedence() as i8;
self.print_expr_maybe_paren(&expr, prec)?;
self.s.space()?;
self.word_space("as")?;
self.print_type(&ty)?;
}
hir::ExprType(ref expr, ref ty) => {
self.print_expr(&expr)?;
let prec = AssocOp::Colon.precedence() as i8;
self.print_expr_maybe_paren(&expr, prec)?;
self.word_space(":")?;
self.print_type(&ty)?;
}
Expand All @@ -1287,7 +1318,7 @@ impl<'a> State<'a> {
self.word_space(":")?;
}
self.head("while")?;
self.print_expr(&test)?;
self.print_expr_as_cond(&test)?;
self.s.space()?;
self.print_block(&blk)?;
}
Expand All @@ -1304,7 +1335,7 @@ impl<'a> State<'a> {
self.cbox(indent_unit)?;
self.ibox(4)?;
self.word_nbsp("match")?;
self.print_expr(&expr)?;
self.print_expr_as_cond(&expr)?;
self.s.space()?;
self.bopen()?;
for arm in arms {
Expand Down Expand Up @@ -1335,30 +1366,32 @@ impl<'a> State<'a> {
self.print_block(&blk)?;
}
hir::ExprAssign(ref lhs, ref rhs) => {
self.print_expr(&lhs)?;
let prec = AssocOp::Assign.precedence() as i8;
self.print_expr_maybe_paren(&lhs, prec + 1)?;
self.s.space()?;
self.word_space("=")?;
self.print_expr(&rhs)?;
self.print_expr_maybe_paren(&rhs, prec)?;
}
hir::ExprAssignOp(op, ref lhs, ref rhs) => {
self.print_expr(&lhs)?;
let prec = AssocOp::Assign.precedence() as i8;
self.print_expr_maybe_paren(&lhs, prec + 1)?;
self.s.space()?;
self.s.word(op.node.as_str())?;
self.word_space("=")?;
self.print_expr(&rhs)?;
self.print_expr_maybe_paren(&rhs, prec)?;
}
hir::ExprField(ref expr, name) => {
self.print_expr(&expr)?;
self.print_expr_maybe_paren(expr, parser::PREC_POSTFIX)?;
self.s.word(".")?;
self.print_name(name.node)?;
}
hir::ExprTupField(ref expr, id) => {
self.print_expr(&expr)?;
self.print_expr_maybe_paren(&expr, parser::PREC_POSTFIX)?;
self.s.word(".")?;
self.print_usize(id.node)?;
}
hir::ExprIndex(ref expr, ref index) => {
self.print_expr(&expr)?;
self.print_expr_maybe_paren(&expr, parser::PREC_POSTFIX)?;
self.s.word("[")?;
self.print_expr(&index)?;
self.s.word("]")?;
Expand All @@ -1374,7 +1407,7 @@ impl<'a> State<'a> {
self.s.space()?;
}
if let Some(ref expr) = *opt_expr {
self.print_expr(expr)?;
self.print_expr_maybe_paren(expr, parser::PREC_JUMP)?;
self.s.space()?;
}
}
Expand All @@ -1391,7 +1424,7 @@ impl<'a> State<'a> {
match *result {
Some(ref expr) => {
self.s.word(" ")?;
self.print_expr(&expr)?;
self.print_expr_maybe_paren(&expr, parser::PREC_JUMP)?;
}
_ => (),
}
Expand Down Expand Up @@ -1463,7 +1496,7 @@ impl<'a> State<'a> {
}
hir::ExprYield(ref expr) => {
self.s.word("yield")?;
self.print_expr(&expr)?;
self.print_expr_maybe_paren(&expr, parser::PREC_JUMP)?;
}
}
self.ann.post(self, NodeExpr(expr))?;
Expand Down Expand Up @@ -2246,3 +2279,111 @@ fn stmt_ends_with_semi(stmt: &hir::Stmt_) -> bool {
}
}
}


fn expr_precedence(expr: &hir::Expr) -> i8 {
use syntax::util::parser::*;

match expr.node {
hir::ExprClosure(..) => PREC_CLOSURE,

hir::ExprBreak(..) |
hir::ExprAgain(..) |
hir::ExprRet(..) |
hir::ExprYield(..) => PREC_JUMP,

// Binop-like expr kinds, handled by `AssocOp`.
hir::ExprBinary(op, _, _) => bin_op_to_assoc_op(op.node).precedence() as i8,

hir::ExprCast(..) => AssocOp::As.precedence() as i8,
hir::ExprType(..) => AssocOp::Colon.precedence() as i8,

hir::ExprAssign(..) |
hir::ExprAssignOp(..) => AssocOp::Assign.precedence() as i8,

// Unary, prefix
hir::ExprBox(..) |
hir::ExprAddrOf(..) |
hir::ExprUnary(..) => PREC_PREFIX,

// Unary, postfix
hir::ExprCall(..) |
hir::ExprMethodCall(..) |
hir::ExprField(..) |
hir::ExprTupField(..) |
hir::ExprIndex(..) |
hir::ExprInlineAsm(..) => PREC_POSTFIX,

// Never need parens
hir::ExprArray(..) |
hir::ExprRepeat(..) |
hir::ExprTup(..) |
hir::ExprLit(..) |
hir::ExprPath(..) |
hir::ExprIf(..) |
hir::ExprWhile(..) |
hir::ExprLoop(..) |
hir::ExprMatch(..) |
hir::ExprBlock(..) |
hir::ExprStruct(..) => PREC_PAREN,
}
}

fn bin_op_to_assoc_op(op: hir::BinOp_) -> AssocOp {
use hir::BinOp_::*;
match op {
BiAdd => AssocOp::Add,
BiSub => AssocOp::Subtract,
BiMul => AssocOp::Multiply,
BiDiv => AssocOp::Divide,
BiRem => AssocOp::Modulus,

BiAnd => AssocOp::LAnd,
BiOr => AssocOp::LOr,

BiBitXor => AssocOp::BitXor,
BiBitAnd => AssocOp::BitAnd,
BiBitOr => AssocOp::BitOr,
BiShl => AssocOp::ShiftLeft,
BiShr => AssocOp::ShiftRight,

BiEq => AssocOp::Equal,
BiLt => AssocOp::Less,
BiLe => AssocOp::LessEqual,
BiNe => AssocOp::NotEqual,
BiGe => AssocOp::GreaterEqual,
BiGt => AssocOp::Greater,
}
}

/// 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
/// `X { y: 1 } == foo` all do, but `(X { y: 1 }) == foo` does not.
fn contains_exterior_struct_lit(value: &hir::Expr) -> bool {
match value.node {
hir::ExprStruct(..) => true,

hir::ExprAssign(ref lhs, ref rhs) |
hir::ExprAssignOp(_, ref lhs, ref rhs) |
hir::ExprBinary(_, ref lhs, ref rhs) => {
// X { y: 1 } + X { y: 2 }
contains_exterior_struct_lit(&lhs) || contains_exterior_struct_lit(&rhs)
}
hir::ExprUnary(_, ref x) |
hir::ExprCast(ref x, _) |
hir::ExprType(ref x, _) |
hir::ExprField(ref x, _) |
hir::ExprTupField(ref x, _) |
hir::ExprIndex(ref x, _) => {
// &X { y: 1 }, X { y: 1 }.y
contains_exterior_struct_lit(&x)
}

hir::ExprMethodCall(.., ref exprs) => {
// X { y: 1 }.bar(...)
contains_exterior_struct_lit(&exprs[0])
}

_ => false,
}
}
Loading