Skip to content

Commit 78fecaa

Browse files
RalfJungRalfJung
authored
Rollup merge of #133655 - dtolnay:maybeparen, r=lcnr
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 dtolnay/syn#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``
2 parents 2aee158 + 9453803 commit 78fecaa

File tree

2 files changed

+104
-52
lines changed
  • compiler
    • rustc_ast_pretty/src/pprust/state
    • rustc_hir_pretty/src

2 files changed

+104
-52
lines changed

‎compiler/rustc_ast_pretty/src/pprust/state/expr.rs

+87-31
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,6 @@ impl<'a> State<'a> {
5858
self.pclose()
5959
}
6060

61-
fn print_expr_maybe_paren(&mut self, expr: &ast::Expr, prec: i8, fixup: FixupContext) {
62-
self.print_expr_cond_paren(expr, expr.precedence() < prec, fixup);
63-
}
64-
6561
/// Prints an expr using syntax that's acceptable in a condition position, such as the `cond` in
6662
/// `if cond { ... }`.
6763
fn print_expr_as_cond(&mut self, expr: &ast::Expr) {
@@ -237,7 +233,7 @@ impl<'a> State<'a> {
237233
// because the latter is valid syntax but with the incorrect meaning.
238234
// It's a match-expression followed by tuple-expression, not a function
239235
// call.
240-
self.print_expr_maybe_paren(func, prec, fixup.leftmost_subexpression());
236+
self.print_expr_cond_paren(func, func.precedence() < prec, fixup.leftmost_subexpression());
241237

242238
self.print_call_post(args)
243239
}
@@ -258,7 +254,11 @@ impl<'a> State<'a> {
258254
// boundaries, `$receiver.method()` can be parsed back as a statement
259255
// containing an expression if and only if `$receiver` can be parsed as
260256
// a statement containing an expression.
261-
self.print_expr_maybe_paren(receiver, parser::PREC_UNAMBIGUOUS, fixup);
257+
self.print_expr_cond_paren(
258+
receiver,
259+
receiver.precedence() < parser::PREC_UNAMBIGUOUS,
260+
fixup,
261+
);
262262

263263
self.word(".");
264264
self.print_ident(segment.ident);
@@ -306,17 +306,29 @@ impl<'a> State<'a> {
306306
_ => left_prec,
307307
};
308308

309-
self.print_expr_maybe_paren(lhs, left_prec, fixup.leftmost_subexpression());
309+
self.print_expr_cond_paren(
310+
lhs,
311+
lhs.precedence() < left_prec,
312+
fixup.leftmost_subexpression(),
313+
);
310314

311315
self.space();
312316
self.word_space(op.node.as_str());
313317

314-
self.print_expr_maybe_paren(rhs, right_prec, fixup.subsequent_subexpression());
318+
self.print_expr_cond_paren(
319+
rhs,
320+
rhs.precedence() < right_prec,
321+
fixup.subsequent_subexpression(),
322+
);
315323
}
316324

317325
fn print_expr_unary(&mut self, op: ast::UnOp, expr: &ast::Expr, fixup: FixupContext) {
318326
self.word(op.as_str());
319-
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX, fixup.subsequent_subexpression());
327+
self.print_expr_cond_paren(
328+
expr,
329+
expr.precedence() < parser::PREC_PREFIX,
330+
fixup.subsequent_subexpression(),
331+
);
320332
}
321333

322334
fn print_expr_addr_of(
@@ -334,7 +346,11 @@ impl<'a> State<'a> {
334346
self.print_mutability(mutability, true);
335347
}
336348
}
337-
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX, fixup.subsequent_subexpression());
349+
self.print_expr_cond_paren(
350+
expr,
351+
expr.precedence() < parser::PREC_PREFIX,
352+
fixup.subsequent_subexpression(),
353+
);
338354
}
339355

340356
pub(super) fn print_expr(&mut self, expr: &ast::Expr, fixup: FixupContext) {
@@ -417,7 +433,11 @@ impl<'a> State<'a> {
417433
}
418434
ast::ExprKind::Cast(expr, ty) => {
419435
let prec = AssocOp::As.precedence() as i8;
420-
self.print_expr_maybe_paren(expr, prec, fixup.leftmost_subexpression());
436+
self.print_expr_cond_paren(
437+
expr,
438+
expr.precedence() < prec,
439+
fixup.leftmost_subexpression(),
440+
);
421441
self.space();
422442
self.word_space("as");
423443
self.print_type(ty);
@@ -490,7 +510,11 @@ impl<'a> State<'a> {
490510
self.space();
491511
}
492512
MatchKind::Postfix => {
493-
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS, fixup);
513+
self.print_expr_cond_paren(
514+
expr,
515+
expr.precedence() < parser::PREC_UNAMBIGUOUS,
516+
fixup,
517+
);
494518
self.word_nbsp(".match");
495519
}
496520
}
@@ -550,33 +574,57 @@ impl<'a> State<'a> {
550574
self.print_block_with_attrs(blk, attrs);
551575
}
552576
ast::ExprKind::Await(expr, _) => {
553-
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS, fixup);
577+
self.print_expr_cond_paren(
578+
expr,
579+
expr.precedence() < parser::PREC_UNAMBIGUOUS,
580+
fixup,
581+
);
554582
self.word(".await");
555583
}
556584
ast::ExprKind::Assign(lhs, rhs, _) => {
557585
let prec = AssocOp::Assign.precedence() as i8;
558-
self.print_expr_maybe_paren(lhs, prec + 1, fixup.leftmost_subexpression());
586+
self.print_expr_cond_paren(
587+
lhs,
588+
lhs.precedence() <= prec,
589+
fixup.leftmost_subexpression(),
590+
);
559591
self.space();
560592
self.word_space("=");
561-
self.print_expr_maybe_paren(rhs, prec, fixup.subsequent_subexpression());
593+
self.print_expr_cond_paren(
594+
rhs,
595+
rhs.precedence() < prec,
596+
fixup.subsequent_subexpression(),
597+
);
562598
}
563599
ast::ExprKind::AssignOp(op, lhs, rhs) => {
564600
let prec = AssocOp::Assign.precedence() as i8;
565-
self.print_expr_maybe_paren(lhs, prec + 1, fixup.leftmost_subexpression());
601+
self.print_expr_cond_paren(
602+
lhs,
603+
lhs.precedence() <= prec,
604+
fixup.leftmost_subexpression(),
605+
);
566606
self.space();
567607
self.word(op.node.as_str());
568608
self.word_space("=");
569-
self.print_expr_maybe_paren(rhs, prec, fixup.subsequent_subexpression());
609+
self.print_expr_cond_paren(
610+
rhs,
611+
rhs.precedence() < prec,
612+
fixup.subsequent_subexpression(),
613+
);
570614
}
571615
ast::ExprKind::Field(expr, ident) => {
572-
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS, fixup);
616+
self.print_expr_cond_paren(
617+
expr,
618+
expr.precedence() < parser::PREC_UNAMBIGUOUS,
619+
fixup,
620+
);
573621
self.word(".");
574622
self.print_ident(*ident);
575623
}
576624
ast::ExprKind::Index(expr, index, _) => {
577-
self.print_expr_maybe_paren(
625+
self.print_expr_cond_paren(
578626
expr,
579-
parser::PREC_UNAMBIGUOUS,
627+
expr.precedence() < parser::PREC_UNAMBIGUOUS,
580628
fixup.leftmost_subexpression(),
581629
);
582630
self.word("[");
@@ -590,14 +638,22 @@ impl<'a> State<'a> {
590638
// a "normal" binop gets parenthesized. (`LOr` is the lowest-precedence binop.)
591639
let fake_prec = AssocOp::LOr.precedence() as i8;
592640
if let Some(e) = start {
593-
self.print_expr_maybe_paren(e, fake_prec, fixup.leftmost_subexpression());
641+
self.print_expr_cond_paren(
642+
e,
643+
e.precedence() < fake_prec,
644+
fixup.leftmost_subexpression(),
645+
);
594646
}
595647
match limits {
596648
ast::RangeLimits::HalfOpen => self.word(".."),
597649
ast::RangeLimits::Closed => self.word("..="),
598650
}
599651
if let Some(e) = end {
600-
self.print_expr_maybe_paren(e, fake_prec, fixup.subsequent_subexpression());
652+
self.print_expr_cond_paren(
653+
e,
654+
e.precedence() < fake_prec,
655+
fixup.subsequent_subexpression(),
656+
);
601657
}
602658
}
603659
ast::ExprKind::Underscore => self.word("_"),
@@ -632,9 +688,9 @@ impl<'a> State<'a> {
632688
self.word("return");
633689
if let Some(expr) = result {
634690
self.word(" ");
635-
self.print_expr_maybe_paren(
691+
self.print_expr_cond_paren(
636692
expr,
637-
parser::PREC_JUMP,
693+
expr.precedence() < parser::PREC_JUMP,
638694
fixup.subsequent_subexpression(),
639695
);
640696
}
@@ -645,19 +701,19 @@ impl<'a> State<'a> {
645701
self.word("yeet");
646702
if let Some(expr) = result {
647703
self.word(" ");
648-
self.print_expr_maybe_paren(
704+
self.print_expr_cond_paren(
649705
expr,
650-
parser::PREC_JUMP,
706+
expr.precedence() < parser::PREC_JUMP,
651707
fixup.subsequent_subexpression(),
652708
);
653709
}
654710
}
655711
ast::ExprKind::Become(result) => {
656712
self.word("become");
657713
self.word(" ");
658-
self.print_expr_maybe_paren(
714+
self.print_expr_cond_paren(
659715
result,
660-
parser::PREC_JUMP,
716+
result.precedence() < parser::PREC_JUMP,
661717
fixup.subsequent_subexpression(),
662718
);
663719
}
@@ -709,15 +765,15 @@ impl<'a> State<'a> {
709765

710766
if let Some(expr) = e {
711767
self.space();
712-
self.print_expr_maybe_paren(
768+
self.print_expr_cond_paren(
713769
expr,
714-
parser::PREC_JUMP,
770+
expr.precedence() < parser::PREC_JUMP,
715771
fixup.subsequent_subexpression(),
716772
);
717773
}
718774
}
719775
ast::ExprKind::Try(e) => {
720-
self.print_expr_maybe_paren(e, parser::PREC_UNAMBIGUOUS, fixup);
776+
self.print_expr_cond_paren(e, e.precedence() < parser::PREC_UNAMBIGUOUS, fixup);
721777
self.word("?")
722778
}
723779
ast::ExprKind::TryBlock(blk) => {

‎compiler/rustc_hir_pretty/src/lib.rs

+17-21
Original file line numberDiff line numberDiff line change
@@ -1010,10 +1010,6 @@ impl<'a> State<'a> {
10101010
self.pclose()
10111011
}
10121012

1013-
fn print_expr_maybe_paren(&mut self, expr: &hir::Expr<'_>, prec: i8) {
1014-
self.print_expr_cond_paren(expr, expr.precedence() < prec)
1015-
}
1016-
10171013
/// Prints an expr using syntax that's acceptable in a condition position, such as the `cond` in
10181014
/// `if cond { ... }`.
10191015
fn print_expr_as_cond(&mut self, expr: &hir::Expr<'_>) {
@@ -1141,7 +1137,7 @@ impl<'a> State<'a> {
11411137
_ => parser::PREC_UNAMBIGUOUS,
11421138
};
11431139

1144-
self.print_expr_maybe_paren(func, prec);
1140+
self.print_expr_cond_paren(func, func.precedence() < prec);
11451141
self.print_call_post(args)
11461142
}
11471143

@@ -1152,7 +1148,7 @@ impl<'a> State<'a> {
11521148
args: &[hir::Expr<'_>],
11531149
) {
11541150
let base_args = args;
1155-
self.print_expr_maybe_paren(receiver, parser::PREC_UNAMBIGUOUS);
1151+
self.print_expr_cond_paren(receiver, receiver.precedence() < parser::PREC_UNAMBIGUOUS);
11561152
self.word(".");
11571153
self.print_ident(segment.ident);
11581154

@@ -1188,15 +1184,15 @@ impl<'a> State<'a> {
11881184
_ => left_prec,
11891185
};
11901186

1191-
self.print_expr_maybe_paren(lhs, left_prec);
1187+
self.print_expr_cond_paren(lhs, lhs.precedence() < left_prec);
11921188
self.space();
11931189
self.word_space(op.node.as_str());
1194-
self.print_expr_maybe_paren(rhs, right_prec)
1190+
self.print_expr_cond_paren(rhs, rhs.precedence() < right_prec)
11951191
}
11961192

11971193
fn print_expr_unary(&mut self, op: hir::UnOp, expr: &hir::Expr<'_>) {
11981194
self.word(op.as_str());
1199-
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)
1195+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_PREFIX)
12001196
}
12011197

12021198
fn print_expr_addr_of(
@@ -1213,7 +1209,7 @@ impl<'a> State<'a> {
12131209
self.print_mutability(mutability, true);
12141210
}
12151211
}
1216-
self.print_expr_maybe_paren(expr, parser::PREC_PREFIX)
1212+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_PREFIX)
12171213
}
12181214

12191215
fn print_literal(&mut self, lit: &hir::Lit) {
@@ -1352,7 +1348,7 @@ impl<'a> State<'a> {
13521348
}
13531349
hir::ExprKind::Cast(expr, ty) => {
13541350
let prec = AssocOp::As.precedence() as i8;
1355-
self.print_expr_maybe_paren(expr, prec);
1351+
self.print_expr_cond_paren(expr, expr.precedence() < prec);
13561352
self.space();
13571353
self.word_space("as");
13581354
self.print_type(ty);
@@ -1454,26 +1450,26 @@ impl<'a> State<'a> {
14541450
}
14551451
hir::ExprKind::Assign(lhs, rhs, _) => {
14561452
let prec = AssocOp::Assign.precedence() as i8;
1457-
self.print_expr_maybe_paren(lhs, prec + 1);
1453+
self.print_expr_cond_paren(lhs, lhs.precedence() <= prec);
14581454
self.space();
14591455
self.word_space("=");
1460-
self.print_expr_maybe_paren(rhs, prec);
1456+
self.print_expr_cond_paren(rhs, rhs.precedence() < prec);
14611457
}
14621458
hir::ExprKind::AssignOp(op, lhs, rhs) => {
14631459
let prec = AssocOp::Assign.precedence() as i8;
1464-
self.print_expr_maybe_paren(lhs, prec + 1);
1460+
self.print_expr_cond_paren(lhs, lhs.precedence() <= prec);
14651461
self.space();
14661462
self.word(op.node.as_str());
14671463
self.word_space("=");
1468-
self.print_expr_maybe_paren(rhs, prec);
1464+
self.print_expr_cond_paren(rhs, rhs.precedence() < prec);
14691465
}
14701466
hir::ExprKind::Field(expr, ident) => {
1471-
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS);
1467+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_UNAMBIGUOUS);
14721468
self.word(".");
14731469
self.print_ident(ident);
14741470
}
14751471
hir::ExprKind::Index(expr, index, _) => {
1476-
self.print_expr_maybe_paren(expr, parser::PREC_UNAMBIGUOUS);
1472+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_UNAMBIGUOUS);
14771473
self.word("[");
14781474
self.print_expr(index);
14791475
self.word("]");
@@ -1487,7 +1483,7 @@ impl<'a> State<'a> {
14871483
}
14881484
if let Some(expr) = opt_expr {
14891485
self.space();
1490-
self.print_expr_maybe_paren(expr, parser::PREC_JUMP);
1486+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_JUMP);
14911487
}
14921488
}
14931489
hir::ExprKind::Continue(destination) => {
@@ -1501,13 +1497,13 @@ impl<'a> State<'a> {
15011497
self.word("return");
15021498
if let Some(expr) = result {
15031499
self.word(" ");
1504-
self.print_expr_maybe_paren(expr, parser::PREC_JUMP);
1500+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_JUMP);
15051501
}
15061502
}
15071503
hir::ExprKind::Become(result) => {
15081504
self.word("become");
15091505
self.word(" ");
1510-
self.print_expr_maybe_paren(result, parser::PREC_JUMP);
1506+
self.print_expr_cond_paren(result, result.precedence() < parser::PREC_JUMP);
15111507
}
15121508
hir::ExprKind::InlineAsm(asm) => {
15131509
self.word("asm!");
@@ -1532,7 +1528,7 @@ impl<'a> State<'a> {
15321528
}
15331529
hir::ExprKind::Yield(expr, _) => {
15341530
self.word_space("yield");
1535-
self.print_expr_maybe_paren(expr, parser::PREC_JUMP);
1531+
self.print_expr_cond_paren(expr, expr.precedence() < parser::PREC_JUMP);
15361532
}
15371533
hir::ExprKind::Err(_) => {
15381534
self.popen();

0 commit comments

Comments
 (0)