user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Rollup merge of #133655 - dtolnay:maybeparen, r=lcnr

Browse source 
Eliminate print_expr_maybe_paren function from pretty printers

This PR is part of backporting Syn's expression precedence design into rustc. (See #133603 for other work on this.)

In Syn, our version of `print_expr_cond_paren` is called `print_subexpression` and it is called from 19 places. Of those calls, 12 of them need a "custom" behavior for the `needs_paren` argument, whereas only 7 use a "standard" behavior resembling `print_subexpression($e, $e.precedence() < Precedence::$Variant, ...)`. In other words the behavior that rustc_ast_pretty's `print_expr_maybe_paren` implements is actually not what you want most of the time. The current usage you see in rustc is overuse.

<details>
<summary>Aside: am I confident about the correctness of Syn's parenthesization? Yes. Click for details.</summary>

---

The behavior is constrained by the following pair of tests which both run over every Rust source file of rustc and the standard library and tools and test suites:

- To rule out **false positives**: for every expression in every source file, print the expression, parse it back, and verify that not a single new parenthesis got added. Since these are expressions parsed from source code, not macro-generated syntax trees, we know they must never need automatic parenthesis insertion. Rustc's pretty printer does not pass this.

    Pseudocode: `assert(expr == parse(print(expr)))`

- To rule out **false negatives**: for every expression in every source file, replace every Expr::Paren node in the syntax tree with just its contents, i.e. stripping the parentheses but otherwise preserving the syntax tree structure. Then print the stripped expression performing parenthesis insertion wherever needed, and reparse it. Verify that the reparsed expression has identical structure to the original, despite there being no parentheses in the original prior to printing, i.e. all the right parentheses got re-inserted by the printer to preserve the expression's structure. Rustc's pretty printer does not pass this. See https://github.com/dtolnay/syn/pull/1788 which reveals multiple rustc_ast_pretty bugs.

    Pseudocode: `assert(unparenthesize(expr) == unparenthesize(parse(print(unparenthesize(expr)))))`

---
</details>

If `print_expr_maybe_paren` is usually not correct, is there harm in keeping it for the minority of cases where it is correct? I think the answer is yes and Syn doesn't use any equivalent of this helper function. The problems with it are:

- Having both `print_expr_maybe_paren` and `print_expr_cond_paren` applies counterproductive inertia against moving from the first to the second. When looking at a call site like `print_expr_maybe_paren(e, Precedence::$Variant, ...)` with parentheses not being inserted where they should be, anyone's first inclination would be to solve the bug by tweaking $Variant because that is the only knob that visibly appears in the function call. For example to pass "prec + 1", like tweaking the code to conditionally pass `Precedence::Prefix` instead of `Precedence::Cast`.

    Experience in Syn shows this is (almost?) never what you want the person to do. In a call `print_expr_cond_paren(e, e.precedence() < ExprPrecedence::$Variant, ...)` almost always the best fix involves one of:

    - Changing `e.precedence()`, e.g. to `fixup.leading_precedence(e)` and `fixup.trailing_precedence(e)` in cases of asymmetrical precedence (`(return 1) + 1` vs `1 + return 1`).

    - Changing `<` to `<=`, to handle associativity and other grammar restrictions like chained comparisons (which rustc gets wrong today).

    - Adding `||` and/or `&&` clauses to the condition.

    By using these 3 better knobs instead of $Variant, it upholds the property that any time we talk about precedence, it is always the precedence of some actual expression that our code is actively manipulating, instead of a value standing in for some imaginary precedence level that would exist between two consecutive [real levels](https://doc.rust-lang.org/1.83.0/reference/expressions.html#expression-precedence). For example consider that "`Cast` + 1" might be `Prefix` today, but only until some new Rust syntax ends up adding a level between those.

- The `print_expr_maybe_paren` call sites look shorter, but they are not clearer. For myself, a function argument that says "does this subexpression need parenthesization" is a concrete thing that is easy to think about, while a function argument that is "what is the effective precedence level associated with this subexpression's placement inside its parent expression" is abstract and tricky to even state a precise meaning for. I expect that for someone less familiar with the pretty printer working on adding a new expression kind (like postfix match, recently), having every subexpression consistently printed using `print_expr_cond_paren` will be more beneficial, for the same reason, than having `print_expr_maybe_paren` available.

r? ``@lcnr``
This commit is contained in:
Ralf Jung 2024-11-30 19:24:42 +01:00 • committed by GitHub
commit 78fecaaec8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 104 additions and 52 deletions
Download patch file Download diff file Expand all files Collapse all files

118
compiler/rustc_ast_pretty/src/pprust/state/expr.rs
View file

@ -58,10 +58,6 @@ impl<'a> State<'a> {
self.pclose()
}
fn print_expr_maybe_paren(&mut self, expr: &ast::Expr, prec: i8, fixup: FixupContext) {
self.print_expr_cond_paren(expr, expr.precedence() < prec, fixup);
}
/// Prints an expr using syntax that's acceptable in a condition position, such as the `cond` in
/// `if cond { ... }`.
fn print_expr_as_cond(&mut self, expr: &ast::Expr) {
@ -237,7 +233,7 @@ impl<'a> State<'a> {
// because the latter is valid syntax but with the incorrect meaning.
// It's a match-expression followed by tuple-expression, not a function
// call.
self.print_expr_maybe_paren(func, prec, fixup.leftmost_subexpression());
self.print_expr_cond_paren(func, func.precedence() < prec, fixup.leftmost_subexpression());
self.print_call_post(args)
}
@ -258,7 +254,11 @@ impl<'a> State<'a> {
// boundaries, `$receiver.method()` can be parsed back as a statement
// containing an expression if and only if `$receiver` can be parsed as
// a statement containing an expression.
self.print_expr_maybe_paren(receiver, parser::PREC_UNAMBIGUOUS, fixup);
self.print_expr_cond_paren(
receiver,
receiver.precedence() < parser::PREC_UNAMBIGUOUS,
fixup,
);
self.word(".");
self.print_ident(segment.ident);
@ -306,17 +306,29 @@ impl<'a> State<'a> {
_ => left_prec,
};
self.print_expr_maybe_paren(lhs, left_prec, fixup.leftmost_subexpression());
self.print_expr_cond_paren(
lhs,
lhs.precedence() < left_prec,
fixup.leftmost_subexpression(),
);
self.space();
self.word_space(op.node.as_str());
self.print_expr_maybe_paren(rhs, right_prec, fixup.subsequent_subexpression());
self.print_expr_cond_paren(
rhs,
rhs.precedence() < right_prec,
fixup.subsequent_subexpression(),
);
}
fn print_expr_unary(&mut self, op: ast::UnOp, expr: &ast::Expr, fixup: FixupContext) {
self.word(op.as_str());
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX, fixup.subsequent_subexpression());
self.print_expr_cond_paren(
expr,
expr.precedence() < parser::PREC_PREFIX,
fixup.subsequent_subexpression(),
);
}
fn print_expr_addr_of(
@ -334,7 +346,11 @@ impl<'a> State<'a> {
self.print_mutability(mutability, true);
}
}
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX, fixup.subsequent_subexpression());
self.print_expr_cond_paren(
expr,
expr.precedence() < parser::PREC_PREFIX,
fixup.subsequent_subexpression(),
);
}
pub(super) fn print_expr(&mut self, expr: &ast::Expr, fixup: FixupContext) {
@ -417,7 +433,11 @@ impl<'a> State<'a> {
}
ast::ExprKind::Cast(expr, ty) => {
let prec = AssocOp::As.precedence() as i8;
self.print_expr_maybe_paren(expr, prec, fixup.leftmost_subexpression());
self.print_expr_cond_paren(
expr,
expr.precedence() < prec,
fixup.leftmost_subexpression(),
);
self.space();
self.word_space("as");
self.print_type(ty);
@ -490,7 +510,11 @@ impl<'a> State<'a> {
self.space();
}
MatchKind::Postfix => {
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS, fixup);
self.print_expr_cond_paren(
expr,
expr.precedence() < parser::PREC_UNAMBIGUOUS,
fixup,
);
self.word_nbsp(".match");
}
}
@ -550,33 +574,57 @@ impl<'a> State<'a> {
self.print_block_with_attrs(blk, attrs);
}
ast::ExprKind::Await(expr, _) => {
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS, fixup);
self.print_expr_cond_paren(
expr,
expr.precedence() < parser::PREC_UNAMBIGUOUS,
fixup,
);
self.word(".await");
}
ast::ExprKind::Assign(lhs, rhs, _) => {
let prec = AssocOp::Assign.precedence() as i8;
self.print_expr_maybe_paren(lhs, prec + 1, fixup.leftmost_subexpression());
self.print_expr_cond_paren(
lhs,
lhs.precedence() <= prec,
fixup.leftmost_subexpression(),
);
self.space();
self.word_space("=");
self.print_expr_maybe_paren(rhs, prec, fixup.subsequent_subexpression());
self.print_expr_cond_paren(
rhs,
rhs.precedence() < prec,
fixup.subsequent_subexpression(),
);
}
ast::ExprKind::AssignOp(op, lhs, rhs) => {
let prec = AssocOp::Assign.precedence() as i8;
self.print_expr_maybe_paren(lhs, prec + 1, fixup.leftmost_subexpression());
self.print_expr_cond_paren(
lhs,
lhs.precedence() <= prec,
fixup.leftmost_subexpression(),
);
self.space();
self.word(op.node.as_str());
self.word_space("=");
self.print_expr_maybe_paren(rhs, prec, fixup.subsequent_subexpression());
self.print_expr_cond_paren(
rhs,
rhs.precedence() < prec,
fixup.subsequent_subexpression(),
);
}
ast::ExprKind::Field(expr, ident) => {
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS, fixup);
self.print_expr_cond_paren(
expr,
expr.precedence() < parser::PREC_UNAMBIGUOUS,
fixup,
);
self.word(".");
self.print_ident(*ident);
}
ast::ExprKind::Index(expr, index, _) => {
self.print_expr_maybe_paren(
self.print_expr_cond_paren(
expr,
parser::PREC_UNAMBIGUOUS,
expr.precedence() < parser::PREC_UNAMBIGUOUS,
fixup.leftmost_subexpression(),
);
self.word("[");
@ -590,14 +638,22 @@ impl<'a> State<'a> {
// a "normal" binop gets parenthesized. (`LOr` is the lowest-precedence binop.)
let fake_prec = AssocOp::LOr.precedence() as i8;
if let Some(e) = start {
self.print_expr_maybe_paren(e, fake_prec, fixup.leftmost_subexpression());
self.print_expr_cond_paren(
e,
e.precedence() < fake_prec,
fixup.leftmost_subexpression(),
);
}
match limits {
ast::RangeLimits::HalfOpen => self.word(".."),
ast::RangeLimits::Closed => self.word("..="),
}
if let Some(e) = end {
self.print_expr_maybe_paren(e, fake_prec, fixup.subsequent_subexpression());
self.print_expr_cond_paren(
e,
e.precedence() < fake_prec,
fixup.subsequent_subexpression(),
);
}
}
ast::ExprKind::Underscore => self.word("_"),
@ -632,9 +688,9 @@ impl<'a> State<'a> {
self.word("return");
if let Some(expr) = result {
self.word(" ");
self.print_expr_maybe_paren(
self.print_expr_cond_paren(
expr,
parser::PREC_JUMP,
expr.precedence() < parser::PREC_JUMP,
fixup.subsequent_subexpression(),
);
}
@ -645,9 +701,9 @@ impl<'a> State<'a> {
self.word("yeet");
if let Some(expr) = result {
self.word(" ");
self.print_expr_maybe_paren(
self.print_expr_cond_paren(
expr,
parser::PREC_JUMP,
expr.precedence() < parser::PREC_JUMP,
fixup.subsequent_subexpression(),
);
}
@ -655,9 +711,9 @@ impl<'a> State<'a> {
ast::ExprKind::Become(result) => {
self.word("become");
self.word(" ");
self.print_expr_maybe_paren(
self.print_expr_cond_paren(
result,
parser::PREC_JUMP,
result.precedence() < parser::PREC_JUMP,
fixup.subsequent_subexpression(),
);
}
@ -709,15 +765,15 @@ impl<'a> State<'a> {
if let Some(expr) = e {
self.space();
self.print_expr_maybe_paren(
self.print_expr_cond_paren(
expr,
parser::PREC_JUMP,
expr.precedence() < parser::PREC_JUMP,
fixup.subsequent_subexpression(),
);
}
}
ast::ExprKind::Try(e) => {
self.print_expr_maybe_paren(e, parser::PREC_UNAMBIGUOUS, fixup);
self.print_expr_cond_paren(e, e.precedence() < parser::PREC_UNAMBIGUOUS, fixup);
self.word("?")
}
ast::ExprKind::TryBlock(blk) => {

38
compiler/rustc_hir_pretty/src/lib.rs
View file

@ -1010,10 +1010,6 @@ impl<'a> State<'a> {
self.pclose()
}
fn print_expr_maybe_paren(&mut self, expr: &hir::Expr<'_>, prec: i8) {
self.print_expr_cond_paren(expr, expr.precedence() < prec)
}
/// Prints an expr using syntax that's acceptable in a condition position, such as the `cond` in
/// `if cond { ... }`.
fn print_expr_as_cond(&mut self, expr: &hir::Expr<'_>) {
@ -1141,7 +1137,7 @@ impl<'a> State<'a> {
_ => parser::PREC_UNAMBIGUOUS,
};
self.print_expr_maybe_paren(func, prec);
self.print_expr_cond_paren(func, func.precedence() < prec);
self.print_call_post(args)
}
@ -1152,7 +1148,7 @@ impl<'a> State<'a> {
args: &[hir::Expr<'_>],
) {
let base_args = args;
self.print_expr_maybe_paren(receiver, parser::PREC_UNAMBIGUOUS);
self.print_expr_cond_paren(receiver, receiver.precedence() < parser::PREC_UNAMBIGUOUS);
self.word(".");
self.print_ident(segment.ident);
@ -1188,15 +1184,15 @@ impl<'a> State<'a> {
_ => left_prec,
};
self.print_expr_maybe_paren(lhs, left_prec);
self.print_expr_cond_paren(lhs, lhs.precedence() < left_prec);
self.space();
self.word_space(op.node.as_str());
self.print_expr_maybe_paren(rhs, right_prec)
self.print_expr_cond_paren(rhs, rhs.precedence() < right_prec)
}
fn print_expr_unary(&mut self, op: hir::UnOp, expr: &hir::Expr<'_>) {
self.word(op.as_str());
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_PREFIX)
}
fn print_expr_addr_of(
@ -1213,7 +1209,7 @@ impl<'a> State<'a> {
self.print_mutability(mutability, true);
}
}
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_PREFIX)
}
fn print_literal(&mut self, lit: &hir::Lit) {
@ -1352,7 +1348,7 @@ impl<'a> State<'a> {
}
hir::ExprKind::Cast(expr, ty) => {
let prec = AssocOp::As.precedence() as i8;
self.print_expr_maybe_paren(expr, prec);
self.print_expr_cond_paren(expr, expr.precedence() < prec);
self.space();
self.word_space("as");
self.print_type(ty);
@ -1454,26 +1450,26 @@ impl<'a> State<'a> {
}
hir::ExprKind::Assign(lhs, rhs, _) => {
let prec = AssocOp::Assign.precedence() as i8;
self.print_expr_maybe_paren(lhs, prec + 1);
self.print_expr_cond_paren(lhs, lhs.precedence() <= prec);
self.space();
self.word_space("=");
self.print_expr_maybe_paren(rhs, prec);
self.print_expr_cond_paren(rhs, rhs.precedence() < prec);
}
hir::ExprKind::AssignOp(op, lhs, rhs) => {
let prec = AssocOp::Assign.precedence() as i8;
self.print_expr_maybe_paren(lhs, prec + 1);
self.print_expr_cond_paren(lhs, lhs.precedence() <= prec);
self.space();
self.word(op.node.as_str());
self.word_space("=");
self.print_expr_maybe_paren(rhs, prec);
self.print_expr_cond_paren(rhs, rhs.precedence() < prec);
}
hir::ExprKind::Field(expr, ident) => {
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS);
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_UNAMBIGUOUS);
self.word(".");
self.print_ident(ident);
}
hir::ExprKind::Index(expr, index, _) => {
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS);
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_UNAMBIGUOUS);
self.word("[");
self.print_expr(index);
self.word("]");
@ -1487,7 +1483,7 @@ impl<'a> State<'a> {
}
if let Some(expr) = opt_expr {
self.space();
self.print_expr_maybe_paren(expr, parser::PREC_JUMP);
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_JUMP);
}
}
hir::ExprKind::Continue(destination) => {
@ -1501,13 +1497,13 @@ impl<'a> State<'a> {
self.word("return");
if let Some(expr) = result {
self.word(" ");
self.print_expr_maybe_paren(expr, parser::PREC_JUMP);
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_JUMP);
}
}
hir::ExprKind::Become(result) => {
self.word("become");
self.word(" ");
self.print_expr_maybe_paren(result, parser::PREC_JUMP);
self.print_expr_cond_paren(result, result.precedence() < parser::PREC_JUMP);
}
hir::ExprKind::InlineAsm(asm) => {
self.word("asm!");
@ -1532,7 +1528,7 @@ impl<'a> State<'a> {
}
hir::ExprKind::Yield(expr, _) => {
self.word_space("yield");
self.print_expr_maybe_paren(expr, parser::PREC_JUMP);
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_JUMP);
}
hir::ExprKind::Err(_) => {
self.popen();