From 86212b8b9a0cdfbb0cb7f0565e0bbe3243ab76c1 Mon Sep 17 00:00:00 2001 From: Luke Arms Date: Fri, 6 Dec 2024 23:02:11 +1100 Subject: [PATCH] Improve ternary expression formatting - In `AlignTernaryOperators`, when operators are aligned with an expression that breaks over multiple lines, align with the first token on the previous line (like in `AlignChains` and `AlignArrowFunctions`) - To better reflect operator precedence and associativity in chained ternary expressions--whether unambiguous (e.g. `1 ? 2 ? 3 : 4 : 5`) or deprecated (e.g. `1 ? 2 : 3 ? 4 : 5`)--stop iterating over previous siblings in `TokenUtil::getTernaryContext()` when a ternary expression with expr2 is found - Make equivalent changes to `TokenUtil::getTernaryEndExpression()` - Add and adopt `TokenUtil::getTernaryExpression()` - Add `TokenUtil::getPrecedenceOf()` - Add `Token::hasNewlineAfterPrevCode()` - Add `TokenCollection::isEmpty()` - Add alignment callbacks to token data and use them to re-align ternary operators after hanging indentation is collapsed - Fix issue where ternary expressions are incorrectly aligned across logical operator boundaries - Add tests --- docs/Rules.md | 4 +- src/Catalog/TokenData.php | 8 +- src/Internal/TokenCollection.php | 9 + src/Rule/AlignTernaryOperators.php | 32 ++- src/Rule/HangingIndentation.php | 99 ++++++- src/Token.php | 11 +- src/TokenIndex.php | 8 + src/TokenUtil.php | 132 +++++++-- tests/unit/Rule/AlignTernaryOperatorsTest.php | 268 +++++++++++++++++- tests/unit/Rule/HangingIndentationTest.php | 223 +++++++++++++-- 10 files changed, 739 insertions(+), 55 deletions(-) diff --git a/docs/Rules.md b/docs/Rules.md index 263ffa2f..3c8f0ddb 100644 --- a/docs/Rules.md +++ b/docs/Rules.md @@ -232,7 +232,7 @@ If an arrow function expression starts on a new line, a callback is registered t ### `AlignTernaryOperators`, if enabled (call 1: `processTokens()`) -If a ternary or null coalescing operator has a leading newline, a callback is registered to align it with its expression. +If a ternary or null coalescing operator has a leading newline, a callback is registered to align it with its expression, or with the first token on the previous line if its expression breaks over multiple lines. ### `AlignChains`, if enabled (call 2: _`callback`_) @@ -255,7 +255,7 @@ This is achieved by: ### `AlignTernaryOperators`, if enabled (call 2: _`callback`_) -Ternary and null coalescing operators with leading newlines are aligned with their expressions. +Ternary and null coalescing operators with leading newlines are aligned with their expressions, or with the first token on the previous line if their expressions break over multiple lines. This is achieved by: diff --git a/src/Catalog/TokenData.php b/src/Catalog/TokenData.php index ce5e50e7..bb1d5fa9 100644 --- a/src/Catalog/TokenData.php +++ b/src/Catalog/TokenData.php @@ -63,8 +63,14 @@ interface TokenData */ public const PROPERTY_HOOKS = 9; + /** + * A list of closures that align other tokens with the token when its output + * column changes + */ + public const ALIGNMENT_CALLBACKS = 10; + /** * The type applied to an open bracket by the HangingIndentation rule */ - public const HANGING_INDENT_PARENT_TYPE = 10; + public const HANGING_INDENT_PARENT_TYPE = 11; } diff --git a/src/Internal/TokenCollection.php b/src/Internal/TokenCollection.php index 67bf8126..88d0639a 100644 --- a/src/Internal/TokenCollection.php +++ b/src/Internal/TokenCollection.php @@ -35,6 +35,15 @@ public static function collect(Token $from, Token $to): self return $instance; } + /** + * @phpstan-assert-if-false !null $this->first() + * @phpstan-assert-if-false !null $this->last() + */ + public function isEmpty(): bool + { + return !$this->Items; + } + /** * @phpstan-assert-if-true !null $this->first() * @phpstan-assert-if-true !null $this->last() diff --git a/src/Rule/AlignTernaryOperators.php b/src/Rule/AlignTernaryOperators.php index 33404452..741c87c0 100644 --- a/src/Rule/AlignTernaryOperators.php +++ b/src/Rule/AlignTernaryOperators.php @@ -53,10 +53,12 @@ public static function needsSortedTokens(): bool * Apply the rule to the given tokens * * If a ternary or null coalescing operator has a leading newline, a - * callback is registered to align it with its expression. + * callback is registered to align it with its expression, or with the first + * token on the previous line if its expression breaks over multiple lines. * * @prettyphp-callback Ternary and null coalescing operators with leading - * newlines are aligned with their expressions. + * newlines are aligned with their expressions, or with the first token on + * the previous line if their expressions break over multiple lines. * * This is achieved by: * @@ -85,7 +87,17 @@ public function processTokens(array $tokens): void continue; } - $alignWith = TokenUtil::getOperatorExpression($prevTernary ?? $token); + $expr = TokenUtil::getTernaryExpression($prevTernary ?? $token); + /** @var Token */ + $prev = ($prevTernary ?? $token)->PrevCode; + /** @var Token */ + $alignWith = $expr->collect($prev) + ->reverse() + ->find(fn(Token $t) => + $t === $expr || ( + $t->Flags & TokenFlag::CODE + && $t->hasNewlineBefore() + )); $this->setAlignedWith($token, $alignWith); @@ -96,13 +108,19 @@ public function processTokens(array $tokens): void static::class, $token, static function () use ($token, $alignWith, $until, $tabSize) { - $delta = $token->getColumnDelta($alignWith, true) + $tabSize; while ($adjacent = $until->adjacentBeforeNewline()) { $until = TokenUtil::getOperatorEndExpression($adjacent); } - foreach ($token->collect($until) as $token) { - $token->LinePadding += $delta; - } + $tokens = $token->collect($until); + ($callback = static function () use ($token, $alignWith, $tabSize, $tokens) { + $delta = $token->getColumnDelta($alignWith, true) + $tabSize; + if ($delta) { + foreach ($tokens as $token) { + $token->LinePadding += $delta; + } + } + })(); + $alignWith->Data[TokenData::ALIGNMENT_CALLBACKS][] = $callback; } ); } diff --git a/src/Rule/HangingIndentation.php b/src/Rule/HangingIndentation.php index e64661f9..df1690b7 100644 --- a/src/Rule/HangingIndentation.php +++ b/src/Rule/HangingIndentation.php @@ -203,8 +203,11 @@ public function processTokens(array $tokens): void $context[] = TokenUtil::getTernaryContext($trigger) ?? TokenUtil::getTernary1($trigger) ?? $trigger; - $until = TokenUtil::getTernaryEndExpression($trigger); $apply = $trigger; + $until = TokenUtil::getTernaryEndExpression($trigger); + while ($adjacent = $until->adjacentBeforeNewline()) { + $until = TokenUtil::getOperatorEndExpression($adjacent); + } } elseif ($this->Idx->Chain[$token->id]) { $context[] = $token->Data[TokenData::CHAIN_OPENED_BY]; } elseif ($token->Heredoc && $token->Heredoc === $prev) { @@ -262,7 +265,7 @@ public function processTokens(array $tokens): void // $baz->qux()) || // $baz->quux()) // ``` - $until = $until ?? $apply->pragmaticEndOfExpression(true, true, true); + $until = $until ?? $this->getLastIndentable($apply); $indent = 0; $hanging = []; $parents = @@ -306,7 +309,7 @@ public function processTokens(array $tokens): void if ($adjacent = $until->adjacentBeforeNewline()) { foreach ($adjacent->collect($adjacent->endOfLine()) as $t) { if ($t->AlignedWith) { - $until = $adjacent->pragmaticEndOfExpression(); + $until = $this->getLastIndentable($adjacent); break; } } @@ -355,12 +358,93 @@ public function processTokens(array $tokens): void } } + private function getLastIndentable(Token $token): Token + { + // Check if the token is part of a declaration with a body that hasn't + // been reached yet, not including anonymous functions or classes like + // the following, which can be moved around in their entirety: + // + // ``` + // $foo = new + // #[Attribute] + // class implements + // Bar, + // Baz + // { + // // ... + // }; + // ``` + // + // But anonymous classes like this cannot: + // + // ``` + // $foo = new class implements + // Bar, + // Baz + // { + // // ... + // }; + // ``` + $parts = $token->skipToStartOfDeclaration() + ->declarationParts(); + if ( + !$parts->isEmpty() + && ( + $parts->hasOneFrom($this->Idx->DeclarationTopLevel) + || ($token->Statement && $token->Statement->isProperty()) + ) + && ($last = $parts->last())->index >= $token->index + && $last->skipPrevSiblingFrom($this->Idx->Ampersand)->id !== T_FUNCTION + && !( + ($first = $parts->first())->id === T_NEW + && $first->NextCode === $token + && $first->nextSiblingOf(T_CLASS)->hasNewlineAfterPrevCode() + ) && ($body = $last->nextSiblingOf(T_OPEN_BRACE, true))->id !== T_NULL + ) { + /** @var Token */ + return $body->PrevCode; + } + + // If the token is between `?` and `:` in a ternary expression, return + // the last token before `:` + $t = $token; + $ternaryColon = null; + while (($t = $t->prevSiblingFrom($this->Idx->OperatorTernary, true))->id !== T_NULL) { + if ($t->Flags & TokenFlag::TERNARY_OPERATOR) { + if ($t->id === T_QUESTION) { + /** @var Token */ + return $t->Data[TokenData::OTHER_TERNARY_OPERATOR]->PrevCode; + } + $ternaryColon = $t; + break; + } + } + + if ( + $ternaryColon + && TokenUtil::getOperatorEndExpression($ternaryColon)->index >= $token->index + ) { + // If the token is between `:` and `?` in chained ternary + // expressions, return the last token before `?` + $t = $token; + while (($t = $t->nextSiblingOf(T_QUESTION, true))->id !== T_NULL) { + if ($t->Flags & TokenFlag::TERNARY_OPERATOR) { + /** @var Token */ + return $t->PrevCode; + } + } + } + + return $token->EndStatement ?? $token; + } + /** * @param array $hanging */ private function maybeCollapseOverhanging(Token $token, Token $until, array $hanging): void { $tokens = $token->collect($until); + $collapsed = []; foreach ($hanging as $index => $count) { for ($i = 0; $i < $count; $i++) { // The purpose of overhanging indents is to visually separate @@ -442,10 +526,19 @@ private function maybeCollapseOverhanging(Token $token, Token $until, array $han if ($t->HangingIndentParentLevels[$index] ?? 0) { $t->HangingIndent--; $t->HangingIndentParentLevels[$index]--; + $collapsed[$t->index] = $t; } } } } + + foreach ($collapsed as $t) { + if ($callbacks = $t->Data[TokenData::ALIGNMENT_CALLBACKS] ?? null) { + foreach ($callbacks as $callback) { + $callback(); + } + } + } } private function indent(Token $token): int diff --git a/src/Token.php b/src/Token.php index 45c5962b..58ddd93c 100644 --- a/src/Token.php +++ b/src/Token.php @@ -68,7 +68,7 @@ final class Token extends GenericToken implements HasTokenNames, JsonSerializabl /** * @var array - * @phpstan-var array{string,TokenCollection,int,self,self,self,self,TokenCollection,int,TokenCollection,int} + * @phpstan-var array{string,TokenCollection,int,self,self,self,self,TokenCollection,int,TokenCollection,Closure[],int} */ public array $Data; @@ -1116,6 +1116,15 @@ public function removeWhitespace(int $whitespace): void } } + /** + * Check if, between the previous code token and the token, there's a + * newline between tokens + */ + public function hasNewlineAfterPrevCode(): bool + { + return $this->PrevCode && $this->PrevCode->hasNewlineBeforeNextCode(); + } + /** * Check if, between the token and the next code token, there's a newline * between tokens diff --git a/src/TokenIndex.php b/src/TokenIndex.php index 80165bab..6e839f6f 100644 --- a/src/TokenIndex.php +++ b/src/TokenIndex.php @@ -582,6 +582,14 @@ class TokenIndex implements HasTokenIndex, Immutable + self::OPERATOR_COMPARISON + self::TOKEN_INDEX; + /** + * Ternary operators + * + * @var array + */ + public array $OperatorTernary = self::OPERATOR_TERNARY + + self::TOKEN_INDEX; + /** * T_AT, T_DEC, T_DOLLAR, T_INC, T_LOGICAL_NOT, T_NOT * diff --git a/src/TokenUtil.php b/src/TokenUtil.php index df514ec3..da73a8ff 100644 --- a/src/TokenUtil.php +++ b/src/TokenUtil.php @@ -196,6 +196,41 @@ public static function getOperatorPrecedence( return -1; } + /** + * Get the precedence of an operator with the given token ID, or -1 if it is + * not an operator + * + * Lower numbers indicate higher precedence. + * + * @param-out bool $leftAssociative + * @param-out bool $rightAssociative + */ + public static function getPrecedenceOf( + int $id, + bool $unary = false, + bool $ternary = false, + ?bool &$leftAssociative = null, + ?bool &$rightAssociative = null + ): int { + if ($precedence = self::OPERATOR_PRECEDENCE_INDEX[$id]) { + foreach ($precedence as [$arity, $precedence, $leftAssoc, $rightAssoc]) { + if ( + $arity === 0 + || ($arity === self::UNARY && $unary) + || ($arity === self::BINARY && !$unary) + || ($arity === self::TERNARY && $ternary) + ) { + $leftAssociative = $leftAssoc; + $rightAssociative = $rightAssoc; + return $precedence; + } + } + } + $leftAssociative = false; + $rightAssociative = false; + return -1; + } + /** * Get the first ternary or null coalescing operator that is one of a given * ternary or null coalescing operator's preceding siblings in the same @@ -206,43 +241,100 @@ public static function getOperatorPrecedence( */ public static function getTernaryContext(Token $token): ?Token { - $before = self::getTernary1($token) ?? $token; + $precedence = self::getPrecedenceOf(\T_QUESTION, false, true); + if ($ternary = $token->Flags & TokenFlag::TERNARY_OPERATOR) { + /** @var Token */ + $before = self::getTernary1($token); + $short = $before->NextCode === self::getTernary2($token); + } else { + $before = $token; + $short = false; + } $t = $before; while ( - ($t = $t->PrevSibling) + (!$ternary || $short) + && ($t = $t->PrevSibling) && $t->Statement === $token->Statement ) { - if (( - $t->id === \T_COALESCE - && $t->index < $before->index - ) || ( - $t->Flags & TokenFlag::TERNARY_OPERATOR - && self::getTernary1($t) === $t - && $t->Data[TokenData::OTHER_TERNARY_OPERATOR]->index - < $before->index - )) { + if ($t->id === \T_COALESCE) { $context = $t; + } elseif ($t->Flags & TokenFlag::TERNARY_OPERATOR) { + if ( + self::getTernary1($t) === $t + && $t->Data[TokenData::OTHER_TERNARY_OPERATOR]->index + < $before->index + ) { + $context = $t; + $ternary = true; + $short = $t->NextCode === self::getTernary2($t); + } + } elseif (self::OPERATOR_PRECEDENCE_INDEX[$t->id]) { + $prevPrecedence = self::getOperatorPrecedence($t); + if ($prevPrecedence !== -1 && $prevPrecedence > $precedence) { + break; + } } } return $context ?? null; } + /** + * Get the first token in the expression to which a given ternary or null + * coalescing context applies + * + * @param Token $context A token returned by + * {@see TokenUtil::getTernaryContext()}, or the first ternary or null + * coalescing operator in a statement. + */ + public static function getTernaryExpression(Token $context): Token + { + $precedence = self::getPrecedenceOf(\T_QUESTION, false, true); + $t = $context; + while ( + ($prev = $t->PrevSibling) + && $prev->Statement === $context->Statement + ) { + if ($prev->Flags & TokenFlag::TERNARY_OPERATOR) { + return $t; + } + if (self::OPERATOR_PRECEDENCE_INDEX[$prev->id]) { + $prevPrecedence = self::getOperatorPrecedence($prev); + if ($prevPrecedence !== -1 && $prevPrecedence > $precedence) { + return $t; + } + } + $t = $prev; + } + return $t; + } + /** * Get the last token in the expression to which a given ternary or null * coalescing operator applies */ public static function getTernaryEndExpression(Token $token): Token { - $t = self::getOperatorEndExpression($token); - // If `$token` is a null coalescing operator, `$t` could have a - // subsequent ternary operator - if ( - $t->NextSibling - && $t->NextSibling->Flags & TokenFlag::TERNARY_OPERATOR + $precedence = self::getPrecedenceOf(\T_QUESTION, false, true); + $t = self::getTernary2($token) ?? $token; + while ( + ($next = $t->NextSibling) + && $next->Statement === $token->Statement + && $next !== $token->EndStatement ) { - $t = self::getOperatorEndExpression($t->NextSibling); + if ($next->Flags & TokenFlag::TERNARY_OPERATOR) { + if ($next->id === \T_COLON) { + return $t->CloseBracket ?? $t; + } + $next = $next->Data[TokenData::OTHER_TERNARY_OPERATOR]; + } elseif (self::OPERATOR_PRECEDENCE_INDEX[$next->id]) { + $nextPrecedence = self::getOperatorPrecedence($next); + if ($nextPrecedence !== -1 && $nextPrecedence > $precedence) { + return $t->CloseBracket ?? $t; + } + } + $t = $next; } - return $t; + return $t->CloseBracket ?? $t; } /** @@ -356,7 +448,7 @@ public static function serialize(Token $token): array ? self::describe($value) : ($value instanceof TokenCollection ? $value->toString(' ') - : $value); + : (is_array($value) ? count($value) : $value)); } } diff --git a/tests/unit/Rule/AlignTernaryOperatorsTest.php b/tests/unit/Rule/AlignTernaryOperatorsTest.php index e4e6b6ee..72576433 100644 --- a/tests/unit/Rule/AlignTernaryOperatorsTest.php +++ b/tests/unit/Rule/AlignTernaryOperatorsTest.php @@ -2,6 +2,7 @@ namespace Lkrms\PrettyPHP\Tests\Rule; +use Lkrms\PrettyPHP\Rule\AlignChains; use Lkrms\PrettyPHP\Rule\AlignTernaryOperators; use Lkrms\PrettyPHP\Tests\TestCase; @@ -12,7 +13,7 @@ final class AlignTernaryOperatorsTest extends TestCase */ public function testOutput(string $expected, string $code): void { - $this->assertCodeFormatIs($expected, $code, [AlignTernaryOperators::class]); + $this->assertCodeFormatIs($expected, $code, [AlignTernaryOperators::class, AlignChains::class]); } /** @@ -24,6 +25,88 @@ public static function outputProvider(): array [ <<<'PHP' ghi() + ->klm() + ?: $abc; + +PHP, + <<<'PHP' +ghi() +->klm() +?: $abc; +PHP, + ], + [ + <<<'PHP' +c(fn() => + $d && + $e, + $f && + $g) + ?: $start; + +PHP, + <<<'PHP' +c(fn() => +$d && +$e, +$f && +$g) +?: $start; +PHP, + ], + [ + <<<'PHP' +