From b79dada4530378fa4cb744b9da922389131840e7 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Thu, 20 Jul 2017 16:53:56 -0400 Subject: [PATCH 1/4] pprust: fix parenthesization of exprs --- src/librustc_lint/unused.rs | 38 +-- src/libsyntax/print/pprust.rs | 147 +++++++----- src/libsyntax/util/parser.rs | 106 +++++++- .../pprust-expr-roundtrip.rs | 227 ++++++++++++++++++ 4 files changed, 417 insertions(+), 101 deletions(-) create mode 100644 src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 15efc14b061f0..91646ce9f8b96 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -22,6 +22,7 @@ use syntax::attr; use syntax::feature_gate::{BUILTIN_ATTRIBUTES, AttributeType}; use syntax::symbol::keywords; use syntax::ptr::P; +use syntax::util::parser; use syntax_pos::Span; use rustc_back::slice; @@ -313,47 +314,14 @@ impl UnusedParens { msg: &str, struct_lit_needs_parens: bool) { if let ast::ExprKind::Paren(ref inner) = value.node { - let necessary = struct_lit_needs_parens && contains_exterior_struct_lit(&inner); + let necessary = struct_lit_needs_parens && + parser::contains_exterior_struct_lit(&inner); if !necessary { cx.span_lint(UNUSED_PARENS, value.span, &format!("unnecessary parentheses around {}", msg)) } } - - /// 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: &ast::Expr) -> bool { - match value.node { - ast::ExprKind::Struct(..) => true, - - ast::ExprKind::Assign(ref lhs, ref rhs) | - ast::ExprKind::AssignOp(_, ref lhs, ref rhs) | - ast::ExprKind::Binary(_, ref lhs, ref rhs) => { - // X { y: 1 } + X { y: 2 } - contains_exterior_struct_lit(&lhs) || contains_exterior_struct_lit(&rhs) - } - ast::ExprKind::Unary(_, ref x) | - ast::ExprKind::Cast(ref x, _) | - ast::ExprKind::Type(ref x, _) | - ast::ExprKind::Field(ref x, _) | - ast::ExprKind::TupField(ref x, _) | - ast::ExprKind::Index(ref x, _) => { - // &X { y: 1 }, X { y: 1 }.y - contains_exterior_struct_lit(&x) - } - - ast::ExprKind::MethodCall(.., ref exprs) => { - // X { y: 1 }.bar(...) - contains_exterior_struct_lit(&exprs[0]) - } - - _ => false, - } - } } } diff --git a/src/libsyntax/print/pprust.rs b/src/libsyntax/print/pprust.rs index 3b5ec1caf0de9..9903dc50f36fb 100644 --- a/src/libsyntax/print/pprust.rs +++ b/src/libsyntax/print/pprust.rs @@ -14,7 +14,7 @@ use abi::{self, Abi}; use ast::{self, BlockCheckMode, PatKind, RangeEnd}; use ast::{SelfKind, RegionTyParamBound, TraitTyParamBound, TraitBoundModifier}; use ast::Attribute; -use util::parser::AssocOp; +use util::parser::{self, AssocOp, Fixity}; use attr; use codemap::{self, CodeMap}; use syntax_pos::{self, BytePos}; @@ -421,16 +421,6 @@ pub fn visibility_qualified(vis: &ast::Visibility, s: &str) -> String { format!("{}{}", to_string(|s| s.print_visibility(vis)), s) } -fn needs_parentheses(expr: &ast::Expr) -> bool { - match expr.node { - ast::ExprKind::Assign(..) | ast::ExprKind::Binary(..) | - ast::ExprKind::Closure(..) | - ast::ExprKind::AssignOp(..) | ast::ExprKind::Cast(..) | - ast::ExprKind::InPlace(..) | ast::ExprKind::Type(..) => true, - _ => false, - } -} - pub trait PrintState<'a> { fn writer(&mut self) -> &mut pp::Printer<'a>; fn boxes(&mut self) -> &mut Vec; @@ -1736,7 +1726,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_block(then)?; self.print_else(e.as_ref().map(|e| &**e)) @@ -1749,7 +1739,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(then)?; self.print_else(e.as_ref().map(|e| &**e)) @@ -1774,7 +1764,7 @@ impl<'a> State<'a> { pub fn print_if(&mut self, test: &ast::Expr, blk: &ast::Block, elseopt: Option<&ast::Expr>) -> io::Result<()> { self.head("if")?; - self.print_expr(test)?; + self.print_expr_as_cond(test)?; self.s.space()?; self.print_block(blk)?; self.print_else(elseopt) @@ -1786,7 +1776,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) @@ -1821,19 +1811,31 @@ impl<'a> State<'a> { self.pclose() } - pub fn check_expr_bin_needs_paren(&mut self, sub_expr: &ast::Expr, - binop: ast::BinOp) -> bool { - match sub_expr.node { - ast::ExprKind::Binary(ref sub_op, _, _) => { - AssocOp::from_ast_binop(sub_op.node).precedence() < - AssocOp::from_ast_binop(binop.node).precedence() - } - _ => true + pub fn print_expr_maybe_paren(&mut self, expr: &ast::Expr, prec: i8) -> io::Result<()> { + let needs_par = parser::expr_precedence(expr) < prec; + if needs_par { + self.popen()?; } + self.print_expr(expr)?; + if needs_par { + self.pclose()?; + } + Ok(()) } - pub fn print_expr_maybe_paren(&mut self, expr: &ast::Expr) -> io::Result<()> { - let needs_par = needs_parentheses(expr); + /// 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 { + // 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(..) | + ast::ExprKind::Ret(..) | + ast::ExprKind::Break(..) => true, + + _ => parser::contains_exterior_struct_lit(expr), + }; + if needs_par { self.popen()?; } @@ -1847,10 +1849,11 @@ impl<'a> State<'a> { fn print_expr_in_place(&mut self, place: &ast::Expr, expr: &ast::Expr) -> io::Result<()> { - self.print_expr_maybe_paren(place)?; + let prec = AssocOp::Inplace.precedence() as i8; + self.print_expr_maybe_paren(place, prec + 1)?; self.s.space()?; self.word_space("<-")?; - self.print_expr_maybe_paren(expr) + self.print_expr_maybe_paren(expr, prec) } fn print_expr_vec(&mut self, exprs: &[P], @@ -1931,7 +1934,14 @@ impl<'a> State<'a> { fn print_expr_call(&mut self, func: &ast::Expr, args: &[P]) -> io::Result<()> { - self.print_expr_maybe_paren(func)?; + let prec = + match func.node { + ast::ExprKind::Field(..) | + ast::ExprKind::TupField(..) => parser::PREC_FORCE_PAREN, + _ => parser::PREC_POSTFIX, + }; + + self.print_expr_maybe_paren(func, prec)?; self.print_call_post(args) } @@ -1939,7 +1949,7 @@ impl<'a> State<'a> { segment: &ast::PathSegment, args: &[P]) -> 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_ident(segment.identifier)?; if let Some(ref parameters) = segment.parameters { @@ -1952,25 +1962,27 @@ impl<'a> State<'a> { op: ast::BinOp, lhs: &ast::Expr, rhs: &ast::Expr) -> io::Result<()> { - if self.check_expr_bin_needs_paren(lhs, op) { - self.print_expr_maybe_paren(lhs)?; - } else { - self.print_expr(lhs)?; - } + let assoc_op = AssocOp::from_ast_binop(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.to_string())?; - if self.check_expr_bin_needs_paren(rhs, op) { - self.print_expr_maybe_paren(rhs) - } else { - self.print_expr(rhs) - } + self.print_expr_maybe_paren(rhs, right_prec) } fn print_expr_unary(&mut self, op: ast::UnOp, expr: &ast::Expr) -> io::Result<()> { self.s.word(ast::UnOp::to_string(op))?; - self.print_expr_maybe_paren(expr) + self.print_expr_maybe_paren(expr, parser::PREC_PREFIX) } fn print_expr_addr_of(&mut self, @@ -1978,7 +1990,7 @@ impl<'a> State<'a> { expr: &ast::Expr) -> 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: &ast::Expr) -> io::Result<()> { @@ -2002,7 +2014,7 @@ impl<'a> State<'a> { match expr.node { ast::ExprKind::Box(ref expr) => { self.word_space("box")?; - self.print_expr(expr)?; + self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)?; } ast::ExprKind::InPlace(ref place, ref expr) => { self.print_expr_in_place(place, expr)?; @@ -2038,17 +2050,15 @@ impl<'a> State<'a> { self.print_literal(lit)?; } ast::ExprKind::Cast(ref expr, ref ty) => { - if let ast::ExprKind::Cast(..) = expr.node { - self.print_expr(expr)?; - } else { - self.print_expr_maybe_paren(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)?; } ast::ExprKind::Type(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)?; } @@ -2064,7 +2074,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_with_attrs(blk, attrs)?; } @@ -2077,7 +2087,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_with_attrs(blk, attrs)?; } @@ -2090,7 +2100,7 @@ impl<'a> State<'a> { self.print_pat(pat)?; self.s.space()?; self.word_space("in")?; - self.print_expr(iter)?; + self.print_expr_as_cond(iter)?; self.s.space()?; self.print_block_with_attrs(blk, attrs)?; } @@ -2107,7 +2117,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()?; self.print_inner_attributes_no_trailing_hardbreak(attrs)?; @@ -2137,37 +2147,44 @@ impl<'a> State<'a> { self.print_block_with_attrs(blk, attrs)?; } ast::ExprKind::Assign(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)?; } ast::ExprKind::AssignOp(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.to_string())?; self.word_space("=")?; - self.print_expr(rhs)?; + self.print_expr_maybe_paren(rhs, prec)?; } ast::ExprKind::Field(ref expr, id) => { - self.print_expr(expr)?; + self.print_expr_maybe_paren(expr, parser::PREC_POSTFIX)?; self.s.word(".")?; self.print_ident(id.node)?; } ast::ExprKind::TupField(ref expr, id) => { - self.print_expr(expr)?; + self.print_expr_maybe_paren(expr, parser::PREC_POSTFIX)?; self.s.word(".")?; self.print_usize(id.node)?; } ast::ExprKind::Index(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("]")?; } ast::ExprKind::Range(ref start, ref end, limits) => { + // Special case for `Range`. `AssocOp` claims that `Range` has higher precedence + // than `Assign`, but `x .. x = x` gives a parse error instead of `x .. (x = x)`. + // Here we use a fake precedence value so that any child with lower precedence than + // a "normal" binop gets parenthesized. (`LOr` is the lowest-precedence binop.) + let fake_prec = AssocOp::LOr.precedence() as i8; if let Some(ref e) = *start { - self.print_expr(e)?; + self.print_expr_maybe_paren(e, fake_prec)?; } if limits == ast::RangeLimits::HalfOpen { self.s.word("..")?; @@ -2175,7 +2192,7 @@ impl<'a> State<'a> { self.s.word("...")?; } if let Some(ref e) = *end { - self.print_expr(e)?; + self.print_expr_maybe_paren(e, fake_prec)?; } } ast::ExprKind::Path(None, ref path) => { @@ -2192,7 +2209,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()?; } } @@ -2208,7 +2225,7 @@ impl<'a> State<'a> { self.s.word("return")?; if let Some(ref expr) = *result { self.s.word(" ")?; - self.print_expr(expr)?; + self.print_expr_maybe_paren(expr, parser::PREC_JUMP)?; } } ast::ExprKind::InlineAsm(ref a) => { @@ -2286,13 +2303,13 @@ impl<'a> State<'a> { match *e { Some(ref expr) => { self.s.space()?; - self.print_expr(&expr)?; + self.print_expr_maybe_paren(expr, parser::PREC_JUMP)?; } _ => () } } ast::ExprKind::Try(ref e) => { - self.print_expr(e)?; + self.print_expr_maybe_paren(e, parser::PREC_POSTFIX)?; self.s.word("?")? } ast::ExprKind::Catch(ref blk) => { diff --git a/src/libsyntax/util/parser.rs b/src/libsyntax/util/parser.rs index ce24fe1eb61e2..af42b4be6941a 100644 --- a/src/libsyntax/util/parser.rs +++ b/src/libsyntax/util/parser.rs @@ -9,7 +9,7 @@ // except according to those terms. use parse::token::{Token, BinOpToken}; use symbol::keywords; -use ast::BinOpKind; +use ast::{self, BinOpKind, ExprKind}; /// Associative operator with precedence. /// @@ -215,3 +215,107 @@ impl AssocOp { } } } + +pub const PREC_RESET: i8 = -100; +pub const PREC_CLOSURE: i8 = -40; +pub const PREC_JUMP: i8 = -30; +pub const PREC_BLOCK: i8 = -20; +pub const PREC_RANGE: i8 = -10; +// The range 2 ... 14 is reserved for AssocOp binary operator precedences. +pub const PREC_PREFIX: i8 = 50; +pub const PREC_POSTFIX: i8 = 60; +pub const PREC_PAREN: i8 = 99; +pub const PREC_FORCE_PAREN: i8 = 100; + +pub fn expr_precedence(expr: &ast::Expr) -> i8 { + match expr.node { + ExprKind::Closure(..) => PREC_CLOSURE, + + ExprKind::Break(..) | + ExprKind::Continue(..) | + ExprKind::Ret(..) | + ExprKind::Yield(..) => PREC_JUMP, + + ExprKind::If(..) | + ExprKind::IfLet(..) | + ExprKind::While(..) | + ExprKind::WhileLet(..) | + ExprKind::ForLoop(..) | + ExprKind::Loop(..) | + ExprKind::Match(..) | + ExprKind::Block(..) | + ExprKind::Catch(..) => PREC_BLOCK, + + // `Range` claims to have higher precedence than `Assign`, but `x .. x = x` fails to parse, + // instead of parsing as `(x .. x) = x`. Giving `Range` a lower precedence ensures that + // `pprust` will add parentheses in the right places to get the desired parse. + ExprKind::Range(..) => PREC_RANGE, + + // Binop-like expr kinds, handled by `AssocOp`. + ExprKind::Binary(op, _, _) => + AssocOp::from_ast_binop(op.node).precedence() as i8, + + ExprKind::InPlace(..) => AssocOp::Inplace.precedence() as i8, + ExprKind::Cast(..) => AssocOp::As.precedence() as i8, + ExprKind::Type(..) => AssocOp::Colon.precedence() as i8, + + ExprKind::Assign(..) | + ExprKind::AssignOp(..) => AssocOp::Assign.precedence() as i8, + + // Unary, prefix + ExprKind::Box(..) | + ExprKind::AddrOf(..) | + ExprKind::Unary(..) => PREC_PREFIX, + + // Unary, postfix + ExprKind::Call(..) | + ExprKind::MethodCall(..) | + ExprKind::Field(..) | + ExprKind::TupField(..) | + ExprKind::Index(..) | + ExprKind::Try(..) | + ExprKind::InlineAsm(..) | + ExprKind::Mac(..) => PREC_POSTFIX, + + // Never need parens + ExprKind::Array(..) | + ExprKind::Repeat(..) | + ExprKind::Tup(..) | + ExprKind::Lit(..) | + ExprKind::Path(..) | + ExprKind::Paren(..) | + ExprKind::Struct(..) => PREC_PAREN, + } +} + +/// 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. +pub fn contains_exterior_struct_lit(value: &ast::Expr) -> bool { + match value.node { + ast::ExprKind::Struct(..) => true, + + ast::ExprKind::Assign(ref lhs, ref rhs) | + ast::ExprKind::AssignOp(_, ref lhs, ref rhs) | + ast::ExprKind::Binary(_, ref lhs, ref rhs) => { + // X { y: 1 } + X { y: 2 } + contains_exterior_struct_lit(&lhs) || contains_exterior_struct_lit(&rhs) + } + ast::ExprKind::Unary(_, ref x) | + ast::ExprKind::Cast(ref x, _) | + ast::ExprKind::Type(ref x, _) | + ast::ExprKind::Field(ref x, _) | + ast::ExprKind::TupField(ref x, _) | + ast::ExprKind::Index(ref x, _) => { + // &X { y: 1 }, X { y: 1 }.y + contains_exterior_struct_lit(&x) + } + + ast::ExprKind::MethodCall(.., ref exprs) => { + // X { y: 1 }.bar(...) + contains_exterior_struct_lit(&exprs[0]) + } + + _ => false, + } +} diff --git a/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs b/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs new file mode 100644 index 0000000000000..d614f452b0d59 --- /dev/null +++ b/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs @@ -0,0 +1,227 @@ +// 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. + +// ignore-cross-compile + +#![feature(rustc_private)] + +extern crate syntax; + +use syntax::ast::*; +use syntax::codemap::{Spanned, DUMMY_SP}; +use syntax::codemap::FilePathMapping; +use syntax::fold::{self, Folder}; +use syntax::parse::{self, ParseSess}; +use syntax::print::pprust; +use syntax::ptr::P; +use syntax::util::ThinVec; + + +fn parse_expr(ps: &ParseSess, src: &str) -> P { + let mut p = parse::new_parser_from_source_str(ps, + "".to_owned(), + src.to_owned()); + p.parse_expr().unwrap() +} + + +// Helper functions for building exprs +fn expr(kind: ExprKind) -> P { + P(Expr { + id: DUMMY_NODE_ID, + node: kind, + span: DUMMY_SP, + attrs: ThinVec::new(), + }) +} + +fn make_x() -> P { + let seg = PathSegment { + identifier: Ident::from_str("x"), + span: DUMMY_SP, + parameters: None, + }; + let path = Path { + span: DUMMY_SP, + segments: vec![seg], + }; + expr(ExprKind::Path(None, path)) +} + +/// Iterate over exprs of depth up to `depth`. The goal is to explore all "interesting" +/// combinations of expression nesting. For example, we explore combinations using `if`, but not +/// `while` or `match`, since those should print and parse in much the same way as `if`. +fn iter_exprs(depth: usize, f: &mut FnMut(P)) { + if depth == 0 { + f(make_x()); + return; + } + + let mut g = |e| f(expr(e)); + + for kind in 0 .. 17 { + match kind { + 0 => iter_exprs(depth - 1, &mut |e| g(ExprKind::Box(e))), + 1 => { + // Note that for binary expressions, we explore each side separately. The + // parenthesization decisions for the LHS and RHS should be independent, and this + // way produces `O(n)` results instead of `O(n^2)`. + iter_exprs(depth - 1, &mut |e| g(ExprKind::InPlace(e, make_x()))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::InPlace(make_x(), e))); + }, + 2 => iter_exprs(depth - 1, &mut |e| g(ExprKind::Call(e, vec![]))), + 3 => { + let seg = PathSegment { + identifier: Ident::from_str("x"), + span: DUMMY_SP, + parameters: None, + }; + + iter_exprs(depth - 1, &mut |e| g(ExprKind::MethodCall( + seg.clone(), vec![e, make_x()]))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::MethodCall( + seg.clone(), vec![make_x(), e]))); + }, + 4 => { + let op = Spanned { span: DUMMY_SP, node: BinOpKind::Add }; + iter_exprs(depth - 1, &mut |e| g(ExprKind::Binary(op, e, make_x()))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::Binary(op, make_x(), e))); + }, + 5 => { + let op = Spanned { span: DUMMY_SP, node: BinOpKind::Mul }; + iter_exprs(depth - 1, &mut |e| g(ExprKind::Binary(op, e, make_x()))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::Binary(op, make_x(), e))); + }, + 6 => { + let op = Spanned { span: DUMMY_SP, node: BinOpKind::Shl }; + iter_exprs(depth - 1, &mut |e| g(ExprKind::Binary(op, e, make_x()))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::Binary(op, make_x(), e))); + }, + 7 => { + iter_exprs(depth - 1, &mut |e| g(ExprKind::Unary(UnOp::Deref, e))); + }, + 8 => { + let block = P(Block { + stmts: Vec::new(), + id: DUMMY_NODE_ID, + rules: BlockCheckMode::Default, + span: DUMMY_SP, + }); + iter_exprs(depth - 1, &mut |e| g(ExprKind::If(e, block.clone(), None))); + }, + 9 => { + let decl = P(FnDecl { + inputs: vec![], + output: FunctionRetTy::Default(DUMMY_SP), + variadic: false, + }); + iter_exprs(depth - 1, &mut |e| g( + ExprKind::Closure(CaptureBy::Value, decl.clone(), e, DUMMY_SP))); + }, + 10 => { + iter_exprs(depth - 1, &mut |e| g(ExprKind::Assign(e, make_x()))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::Assign(make_x(), e))); + }, + 11 => { + let ident = Spanned { span: DUMMY_SP, node: Ident::from_str("f") }; + iter_exprs(depth - 1, &mut |e| g(ExprKind::Field(e, ident))); + }, + 12 => { + iter_exprs(depth - 1, &mut |e| g(ExprKind::Range( + Some(e), Some(make_x()), RangeLimits::HalfOpen))); + iter_exprs(depth - 1, &mut |e| g(ExprKind::Range( + Some(make_x()), Some(e), RangeLimits::HalfOpen))); + }, + 13 => { + iter_exprs(depth - 1, &mut |e| g(ExprKind::AddrOf(Mutability::Immutable, e))); + }, + 14 => { + g(ExprKind::Ret(None)); + iter_exprs(depth - 1, &mut |e| g(ExprKind::Ret(Some(e)))); + }, + 15 => { + let seg = PathSegment { + identifier: Ident::from_str("S"), + span: DUMMY_SP, + parameters: None, + }; + let path = Path { + span: DUMMY_SP, + segments: vec![seg], + }; + g(ExprKind::Struct(path, vec![], Some(make_x()))); + }, + 16 => { + iter_exprs(depth - 1, &mut |e| g(ExprKind::Try(e))); + }, + _ => panic!("bad counter value in iter_exprs"), + } + } +} + + +// Folders for manipulating the placement of `Paren` nodes. See below for why this is needed. + +/// Folder that removes all `ExprKind::Paren` nodes. +struct RemoveParens; + +impl Folder for RemoveParens { + fn fold_expr(&mut self, e: P) -> P { + let e = match e.node { + ExprKind::Paren(ref inner) => inner.clone(), + _ => e.clone(), + }; + e.map(|e| fold::noop_fold_expr(e, self)) + } +} + + +/// Folder that inserts `ExprKind::Paren` nodes around every `Expr`. +struct AddParens; + +impl Folder for AddParens { + fn fold_expr(&mut self, e: P) -> P { + let e = e.map(|e| fold::noop_fold_expr(e, self)); + P(Expr { + id: DUMMY_NODE_ID, + node: ExprKind::Paren(e), + span: DUMMY_SP, + attrs: ThinVec::new(), + }) + } +} + + +fn main() { + let ps = ParseSess::new(FilePathMapping::empty()); + + iter_exprs(2, &mut |e| { + // If the pretty printer is correct, then `parse(print(e))` should be identical to `e`, + // modulo placement of `Paren` nodes. + let printed = pprust::expr_to_string(&e); + println!("printed: {}", printed); + + let parsed = parse_expr(&ps, &printed); + + // We want to know if `parsed` is structurally identical to `e`, ignoring trivial + // differences like placement of `Paren`s or the exact ranges of node spans. + // Unfortunately, there is no easy way to make this comparison. Instead, we add `Paren`s + // everywhere we can, then pretty-print. This should give an unambiguous representation of + // each `Expr`, and it bypasses nearly all of the parenthesization logic, so we aren't + // relying on the correctness of the very thing we're testing. + let e1 = AddParens.fold_expr(RemoveParens.fold_expr(e)); + let text1 = pprust::expr_to_string(&e1); + let e2 = AddParens.fold_expr(RemoveParens.fold_expr(parsed)); + let text2 = pprust::expr_to_string(&e2); + assert!(text1 == text2, + "exprs are not equal:\n e = {:?}\n parsed = {:?}", + text1, text2); + }); +} From 5b2151ea21c2b1fa517bda90ca881e3f27e9188a Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Mon, 14 Aug 2017 18:26:55 -0400 Subject: [PATCH 2/4] better explanatory comment for the pprust-expr-roundtrip test --- .../run-pass-fulldeps/pprust-expr-roundtrip.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs b/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs index d614f452b0d59..456088b2c5285 100644 --- a/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs +++ b/src/test/run-pass-fulldeps/pprust-expr-roundtrip.rs @@ -10,6 +10,24 @@ // ignore-cross-compile + +// The general idea of this test is to enumerate all "interesting" expressions and check that +// `parse(print(e)) == e` for all `e`. Here's what's interesting, for the purposes of this test: +// +// 1. The test focuses on expression nesting, because interactions between different expression +// types are harder to test manually than single expression types in isolation. +// +// 2. The test only considers expressions of at most two nontrivial nodes. So it will check `x + +// x` and `x + (x - x)` but not `(x * x) + (x - x)`. The assumption here is that the correct +// handling of an expression might depend on the expression's parent, but doesn't depend on its +// siblings or any more distant ancestors. +// +// 3. The test only checks certain expression kinds. The assumption is that similar expression +// types, such as `if` and `while` or `+` and `-`, will be handled identically in the printer +// and parser. So if all combinations of exprs involving `if` work correctly, then combinations +// using `while`, `if let`, and so on will likely work as well. + + #![feature(rustc_private)] extern crate syntax; From 347db068a5786d3cac6e2204d7e14b635ece558d Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Mon, 14 Aug 2017 18:27:20 -0400 Subject: [PATCH 3/4] hir::print: fix parenthesization of exprs --- src/librustc/hir/print.rs | 218 +++++++++++++++++++++++++++++++------- 1 file changed, 180 insertions(+), 38 deletions(-) diff --git a/src/librustc/hir/print.rs b/src/librustc/hir/print.rs index dce824bd513a7..dce7dd33db423 100644 --- a/src/librustc/hir/print.rs +++ b/src/librustc/hir/print.rs @@ -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; @@ -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); @@ -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)) @@ -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) @@ -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) @@ -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()?; } @@ -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) } @@ -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() || @@ -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, @@ -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<()> { @@ -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)?; @@ -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)?; } @@ -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)?; } @@ -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 { @@ -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("]")?; @@ -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()?; } } @@ -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)?; } _ => (), } @@ -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))?; @@ -2246,3 +2279,112 @@ 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, + + hir::ExprIf(..) | + hir::ExprWhile(..) | + hir::ExprLoop(..) | + hir::ExprMatch(..) | + hir::ExprBlock(..) => PREC_BLOCK, + + // 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::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, + } +} From 3454d99cb6355ab60c95e72d96e04eb9c69a6138 Mon Sep 17 00:00:00 2001 From: Stuart Pernsteiner Date: Thu, 7 Sep 2017 10:28:31 -0400 Subject: [PATCH 4/4] pprust: increase precedence of block-like exprs --- src/librustc/hir/print.rs | 11 +++++------ src/libsyntax/util/parser.rs | 20 +++++++++----------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/librustc/hir/print.rs b/src/librustc/hir/print.rs index dce7dd33db423..a06ea0af2a9e6 100644 --- a/src/librustc/hir/print.rs +++ b/src/librustc/hir/print.rs @@ -2292,12 +2292,6 @@ fn expr_precedence(expr: &hir::Expr) -> i8 { hir::ExprRet(..) | hir::ExprYield(..) => PREC_JUMP, - hir::ExprIf(..) | - hir::ExprWhile(..) | - hir::ExprLoop(..) | - hir::ExprMatch(..) | - hir::ExprBlock(..) => PREC_BLOCK, - // Binop-like expr kinds, handled by `AssocOp`. hir::ExprBinary(op, _, _) => bin_op_to_assoc_op(op.node).precedence() as i8, @@ -2326,6 +2320,11 @@ fn expr_precedence(expr: &hir::Expr) -> i8 { hir::ExprTup(..) | hir::ExprLit(..) | hir::ExprPath(..) | + hir::ExprIf(..) | + hir::ExprWhile(..) | + hir::ExprLoop(..) | + hir::ExprMatch(..) | + hir::ExprBlock(..) | hir::ExprStruct(..) => PREC_PAREN, } } diff --git a/src/libsyntax/util/parser.rs b/src/libsyntax/util/parser.rs index af42b4be6941a..a4f06cb1b45da 100644 --- a/src/libsyntax/util/parser.rs +++ b/src/libsyntax/util/parser.rs @@ -219,7 +219,6 @@ impl AssocOp { pub const PREC_RESET: i8 = -100; pub const PREC_CLOSURE: i8 = -40; pub const PREC_JUMP: i8 = -30; -pub const PREC_BLOCK: i8 = -20; pub const PREC_RANGE: i8 = -10; // The range 2 ... 14 is reserved for AssocOp binary operator precedences. pub const PREC_PREFIX: i8 = 50; @@ -236,16 +235,6 @@ pub fn expr_precedence(expr: &ast::Expr) -> i8 { ExprKind::Ret(..) | ExprKind::Yield(..) => PREC_JUMP, - ExprKind::If(..) | - ExprKind::IfLet(..) | - ExprKind::While(..) | - ExprKind::WhileLet(..) | - ExprKind::ForLoop(..) | - ExprKind::Loop(..) | - ExprKind::Match(..) | - ExprKind::Block(..) | - ExprKind::Catch(..) => PREC_BLOCK, - // `Range` claims to have higher precedence than `Assign`, but `x .. x = x` fails to parse, // instead of parsing as `(x .. x) = x`. Giving `Range` a lower precedence ensures that // `pprust` will add parentheses in the right places to get the desired parse. @@ -284,6 +273,15 @@ pub fn expr_precedence(expr: &ast::Expr) -> i8 { ExprKind::Lit(..) | ExprKind::Path(..) | ExprKind::Paren(..) | + ExprKind::If(..) | + ExprKind::IfLet(..) | + ExprKind::While(..) | + ExprKind::WhileLet(..) | + ExprKind::ForLoop(..) | + ExprKind::Loop(..) | + ExprKind::Match(..) | + ExprKind::Block(..) | + ExprKind::Catch(..) | ExprKind::Struct(..) => PREC_PAREN, } }