Skip to content

Fix parentheses for moved nodes in format-preserving printer - #1181

Open
Enfiladeguide wants to merge 1 commit into
nikic:masterfrom
Enfiladeguide:fix-preserved-parens
Open

Enfiladeguide wants to merge 1 commit into
nikic:masterfrom
Enfiladeguide:fix-preserved-parens

Conversation

@Enfiladeguide

Copy link
Copy Markdown

Fixes #1119 (reproducer in #1120).

Since PHP-Parser 5, an operator decides about its own parentheses from the precedence its parent passes down (pInfixOp(), pPrefixOp(), pPostfixOp()). A node with preserved formatting reuses its original code, so that decision never runs, and a node moved into a context that binds more tightly loses parentheses it needs:

// $x = a() && b();
$stmts[0]->expr->expr = new Expr\BooleanNot($stmts[0]->expr->expr);
// printFormatPreserving(): $x = !a() && b();   expected: $x = !(a() && b());

This PR performs the same check in p() for nodes with preserved formatting:

  • preservedOpRequiresParens() is the check of pInfixOp() / pPostfixOp() (against $precedence) and of pPrefixOp() (against $lhsPrecedence);
  • a prefix operator at the end of the reused code ($c === $d = $e, $f >> print $g) extends as far to the right as possible, so the trailing operand (getTrailingOperandName()) is printed with the $lhsPrecedence of the new context, as pInfixOp() / pPrefixOp() do, unless the original code already has parentheses around it.

As a result, a moved node gets the same parentheses as a newly created node in the same place, including the existing conservative approximation in pFixup().

New fixture test/code/formatPreservation/fixupMovedNodes.test: the #1119 case, $a ?? ($b ? 'yes' : 'no'), $a - ($b - $c), a move into a preserved parent, trailing prefix operators, and cases that must stay without parentheses (left-associative minus, assignment on the RHS, a slot that already has parentheses). It fails on master and passes here. The full test suite, PHPStan and php-cs-fixer pass (PHP 8.5.4).

To check the change beyond hand-picked cases, we ran a differential fuzz: random expressions are pretty-printed and parsed, one node is wrapped in a new operator, moved to another position, or replaces its parent, and the output of printFormatPreserving() is parsed again and compared with the modified AST. Over 60,000 rewrites (3 seeds):

wrong meaning parse error
master 6,235 126
this PR 0 0

832 outputs that were already correct on master change. In 829 of them the output is identical to printing a freshly constructed copy of the moved node in the same slot (the added parentheses come from the lhsPrecedence == precedence approximation in pFixup()); in the other 3 the moved node keeps its own original inner parentheses. Concat (see the FIXME on its precedence) and Pipe were left out of the fuzz. Happy to contribute the fuzz script if it is useful.

We ran into this through Rector's TernaryToNullCoalescingRector, which turned $a !== null ? $a : ($b ? 'yes' : 'no') into $a ?? $b ? 'yes' : 'no' (rectorphp/rector#9912, worked around in the rule in rectorphp/rector-src#8674).

Disclosure: the investigation and this change were prepared with automated tooling; the reproduction, tests and proposed change were checked by a person before posting.

Since PHP-Parser 5, an operator prints its own parentheses, based on the
precedence passed down by its parent. A node with preserved formatting
reuses its original code instead, so this check never runs, and a node
moved into a context that binds more tightly loses parentheses it needs:
wrapping `a() && b()` in a new BooleanNot printed `!a() && b()`.

Perform the same check for nodes with preserved formatting. A prefix
operator at the end of the reused code (`$a === $b = $c`) extends as far
to the right as possible, so pass the lhsPrecedence of the new context
on to the trailing operand, unless it already has parentheses.

Fixes nikic#1119.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Format-preserving printer loses parentheses around BooleanNot expressions

1 participant