Eliminate print_expr_maybe_paren function from pretty printers

[dtolnay]

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.

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

---

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 Test rustc's automatic parenthesis insertion dtolnay/syn#1788 which reveals multiple rustc_ast_pretty bugs.

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

---

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. 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

[lcnr]

makes sense to me, thanks for the explanation

@bors r+ rollup

[bors]

📌 Commit 9453803 has been approved by lcnr

It is now in the queue for this repository.