Skip to content

Commit

Permalink
Fix bracket placement idempotence issues
Browse files Browse the repository at this point in the history
- Move bracket mirroring from `StandardIndentation` to standalone rule
  `PlaceBrackets` and prioritise it to run earlier
- Refactor and/or prioritise other rules so changes to vertical
  whitespace near brackets are applied before `PlaceBrackets`, incl.:
  - Move suppression of whitespace in empty `for` expressions from
    `VerticalSpacing` to `StandardSpacing`
  - Place `for` expressions on their own lines in `StrictExpressions`
    and `SemiStrictExpressions`
  - Run `StrictLists` after `*StrictExpressions` to ensure `for`
    expressions that break over multiple lines are not collapsed
- Add `FOR_PARTS` to token data and parse `for` expressions in `Parser`
  instead of duplicating code in `VerticalSpacing`
- Add `TokenCollection::setWhitespace()` and adopt in scenarios where
  suppression of whitespace should not be removed
- Similarly, replace `TokenCollection::applyInnerWhitespace()` with
  `setInnerWhitespace()`
  • Loading branch information
lkrms committed Jan 17, 2025
1 parent cfd7e36 commit 938f8a8
Show file tree
Hide file tree
Showing 18 changed files with 330 additions and 202 deletions.
203 changes: 107 additions & 96 deletions docs/Rules.md

Large diffs are not rendered by default.

24 changes: 15 additions & 9 deletions src/Catalog/TokenData.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,50 +53,56 @@ interface TokenData
*/
public const PROPERTY_HOOKS = 7;

/**
* Collections of tokens associated with a T_FOR loop: expr1, expr2, expr3,
* semicolon delimiters, comma delimiters
*/
public const FOR_PARTS = 8;

/**
* The last token of the string opened by the token
*/
public const END_STRING = 8;
public const END_STRING = 9;

/**
* The first object operator in a chain of method calls
*/
public const CHAIN = 9;
public const CHAIN = 10;

/**
* The other T_QUESTION or T_COLON associated with a TERNARY
*/
public const OTHER_TERNARY = 10;
public const OTHER_TERNARY = 11;

/**
* The token ID of the delimiter associated with a LIST_PARENT token
*/
public const LIST_DELIMITER = 11;
public const LIST_DELIMITER = 12;

/**
* A collection of items associated with a LIST_PARENT token
*/
public const LIST_ITEMS = 12;
public const LIST_ITEMS = 13;

/**
* The number of items associated with a LIST_PARENT token
*/
public const LIST_ITEM_COUNT = 13;
public const LIST_ITEM_COUNT = 14;

/**
* The LIST_PARENT of the first token in a LIST_ITEM
*/
public const LIST_PARENT = 14;
public const LIST_PARENT = 15;

/**
* The content of a normalised DocBlock token (T_DOC_COMMENT or T_COMMENT)
* after delimiters and trailing whitespace are removed
*/
public const COMMENT_CONTENT = 15;
public const COMMENT_CONTENT = 16;

/**
* An array of closures that align other tokens with the token when its
* output column changes
*/
public const ALIGNMENT_CALLBACKS = 16;
public const ALIGNMENT_CALLBACKS = 17;
}
2 changes: 1 addition & 1 deletion src/Concern/RuleTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private function preserveOneLine(
}

$start->collect($end)
->applyInnerWhitespace(Space::CRITICAL_NO_BLANK | Space::CRITICAL_NO_LINE);
->setInnerWhitespace(Space::CRITICAL_NO_BLANK | Space::CRITICAL_NO_LINE);

return true;
}
Expand Down
2 changes: 2 additions & 0 deletions src/Formatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
use Lkrms\PrettyPHP\Rule\NormaliseStrings;
use Lkrms\PrettyPHP\Rule\OperatorSpacing;
use Lkrms\PrettyPHP\Rule\PlaceBraces;
use Lkrms\PrettyPHP\Rule\PlaceBrackets;
use Lkrms\PrettyPHP\Rule\PlaceComments;
use Lkrms\PrettyPHP\Rule\PreserveNewlines;
use Lkrms\PrettyPHP\Rule\PreserveOneLineStatements;
Expand Down Expand Up @@ -151,6 +152,7 @@ final class Formatter implements Buildable, Immutable
PlaceBraces::class,
PlaceComments::class,
PreserveNewlines::class,
PlaceBrackets::class,
VerticalSpacing::class,
DeclarationSpacing::class,
StandardIndentation::class,
Expand Down
19 changes: 13 additions & 6 deletions src/Internal/TokenCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,18 @@ public function toString(string $delimiter = ''): string
/**
* @return $this
*/
public function applyWhitespace(int $whitespace)
public function setWhitespace(int $whitespace): self
{
foreach ($this->Items as $token) {
$token->Whitespace |= $whitespace;
}
return $this;
}

/**
* @return $this
*/
public function applyWhitespace(int $whitespace): self
{
// Shift *_BEFORE and *_AFTER to their NO_* counterparts, then clear
// other bits
Expand All @@ -342,7 +353,7 @@ public function applyWhitespace(int $whitespace)
/**
* @return $this
*/
public function applyInnerWhitespace(int $whitespace)
public function setInnerWhitespace(int $whitespace): self
{
$this->assertCollected();

Expand All @@ -354,7 +365,6 @@ public function applyInnerWhitespace(int $whitespace)
if ($this->count() < 2) {
return $this;
}
$remove = $whitespace << 6 & 0b111111000000;
$ignore = true;
foreach ($this->Items as $token) {
if ($ignore) {
Expand All @@ -364,9 +374,6 @@ public function applyInnerWhitespace(int $whitespace)
|| $token->Data[Data::BOUND_TO]->index > $token->index
) {
$token->Whitespace |= $whitespace;
if ($remove) {
$token->doRemoveWhitespace($remove);
}
}
}
return $this;
Expand Down
38 changes: 36 additions & 2 deletions src/Parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -719,13 +719,14 @@ private function parseStatements(array $scopes, ?array &$statements): void
}

/**
* Pass 4: identify declarations and parse (some) expressions
* Pass 4: parse expressions
*
* Token properties set:
*
* - `Data[Data::DECLARATION_PARTS]`
* - `Data[Data::DECLARATION_TYPE]`
* - `Data[Data::PROPERTY_HOOKS]`
* - `Data[Data::FOR_PARTS]`
* - `Data[Data::OTHER_TERNARY]`
* - `Data[Data::CHAIN]`
*
Expand All @@ -750,7 +751,7 @@ private function parseExpressions(
$end = $statement->EndStatement;
$end = $end->OpenBracket ?? $end;

// Detect non-anonymous declarations
// Parse non-anonymous declarations and `for` expressions
if (
$idx->AttributeOrDeclaration[$statement->id]
&& ($first = $this->skipNextSiblingsFrom($statement, $idx->Attribute))
Expand Down Expand Up @@ -835,6 +836,39 @@ private function parseExpressions(
}
}
}
} elseif ($statement->id === \T_FOR) {
/** @var Token */
$open = $statement->NextCode;
/** @var Token */
$close = $open->CloseBracket;
/** @var Token */
$first = $open->Next;
/** @var Token */
$last = $close->Prev;

$children = $open->children();
$semicolons = $children->getAnyOf(\T_SEMICOLON);
$commas = $children->getAnyOf(\T_COMMA);
/** @var Token */
$semi1 = $semicolons->first();
/** @var Token */
$second = $semi1->Next;
/** @var Token */
$semi2 = $semicolons->last();
/** @var Token */
$third = $semi2->Next;

$expr1 = $first->collect($semi1);
$expr2 = $second->collect($semi2);
$expr3 = $third->collect($last);

$statement->Data[Data::FOR_PARTS] = [
$expr1,
$expr2,
$expr3,
$semicolons,
$commas,
];
}

$token = $statement;
Expand Down
2 changes: 1 addition & 1 deletion src/Rule/BlankBeforeReturn.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ final class BlankBeforeReturn implements TokenRule
public static function getPriority(string $method): ?int
{
return [
self::PROCESS_TOKENS => 220,
self::PROCESS_TOKENS => 204,
][$method] ?? null;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Rule/DeclarationSpacing.php
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public function processDeclarations(array $declarations): void
&& !$prev->isMultiLine()
&& !$decl->isMultiLine()
)) {
$prev->End->collect($token)->applyInnerWhitespace(Space::NO_BLANK);
$prev->End->collect($token)->setInnerWhitespace(Space::NO_BLANK);
$noBlankApplied = true;
} elseif (!$decl->isCollapsible()) {
// Apply "loose" spacing to multi-line declarations
Expand Down
2 changes: 1 addition & 1 deletion src/Rule/OperatorSpacing.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public function processTokens(array $tokens): void
$token->Whitespace |= Space::NONE_BEFORE | Space::NONE_AFTER;
if (!$inTypeContext) {
/** @var Token $parent */
$parent->outer()->applyInnerWhitespace(Space::NONE);
$parent->outer()->setInnerWhitespace(Space::NONE);
if (
$parent->PrevCode
&& $parent->PrevCode->id !== \T_OR
Expand Down
76 changes: 76 additions & 0 deletions src/Rule/PlaceBrackets.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php declare(strict_types=1);

namespace Lkrms\PrettyPHP\Rule;

use Lkrms\PrettyPHP\Catalog\TokenFlag as Flag;
use Lkrms\PrettyPHP\Catalog\WhitespaceFlag as Space;
use Lkrms\PrettyPHP\Concern\TokenRuleTrait;
use Lkrms\PrettyPHP\Contract\TokenRule;
use Lkrms\PrettyPHP\AbstractTokenIndex;
use Lkrms\PrettyPHP\Token;

/**
* Make brackets symmetrical
*
* @api
*/
final class PlaceBrackets implements TokenRule
{
use TokenRuleTrait;

/**
* @inheritDoc
*/
public static function getPriority(string $method): ?int
{
return [
self::PROCESS_TOKENS => 240,
][$method] ?? null;
}

/**
* @inheritDoc
*/
public static function getTokens(AbstractTokenIndex $idx): array
{
return $idx->OpenBracket;
}

/**
* @inheritDoc
*/
public static function needsSortedTokens(): bool
{
return false;
}

/**
* Apply the rule to the given tokens
*
* Inner whitespace is copied from open brackets to close brackets.
*
* Structural and `match` expression braces are ignored.
*/
public function processTokens(array $tokens): void
{
foreach ($tokens as $token) {
if (
$token->Flags & Flag::STRUCTURAL_BRACE
|| ($token->id === \T_OPEN_BRACE && $token->isMatchOpenBrace())
) {
continue;
}

/** @var Token */
$close = $token->CloseBracket;
if (!$token->hasNewlineBeforeNextCode()) {
$close->Whitespace |= Space::NO_BLANK_BEFORE | Space::NO_LINE_BEFORE;
} else {
$close->Whitespace |= Space::LINE_BEFORE;
if (!$close->hasNewlineBefore()) {
$close->removeWhitespace(Space::NO_LINE_BEFORE);

Check warning on line 71 in src/Rule/PlaceBrackets.php

View check run for this annotation

Codecov / codecov/patch

src/Rule/PlaceBrackets.php#L71

Added line #L71 was not covered by tests
}
}
}
}
}
2 changes: 1 addition & 1 deletion src/Rule/Preset/Symfony.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,6 @@ public function processList(Token $parent, TokenCollection $items, Token $lastCh
}
}

$parent->outer()->applyInnerWhitespace(Space::NO_BLANK | Space::NO_LINE);
$parent->outer()->setInnerWhitespace(Space::NO_BLANK | Space::NO_LINE);
}
}
24 changes: 16 additions & 8 deletions src/Rule/SemiStrictExpressions.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@

namespace Lkrms\PrettyPHP\Rule;

use Lkrms\PrettyPHP\Catalog\TokenData as Data;
use Lkrms\PrettyPHP\Catalog\WhitespaceFlag as Space;
use Lkrms\PrettyPHP\Concern\TokenRuleTrait;
use Lkrms\PrettyPHP\Contract\TokenRule;
use Lkrms\PrettyPHP\Internal\TokenCollection;
use Lkrms\PrettyPHP\AbstractTokenIndex;
use Lkrms\PrettyPHP\Token;

Expand All @@ -24,7 +26,7 @@ final class SemiStrictExpressions implements TokenRule
public static function getPriority(string $method): ?int
{
return [
self::PROCESS_TOKENS => 246,
self::PROCESS_TOKENS => 224,
][$method] ?? null;
}

Expand All @@ -48,7 +50,8 @@ public static function needsSortedTokens(): bool
* Apply the rule to the given tokens
*
* Newlines are added before and after control structure expressions with
* newlines between siblings.
* newlines between siblings. In `for` expressions that break over multiple
* lines, newlines are also added after semicolons between expressions.
*
* > Unlike `StrictExpressions`, this rule does not apply leading and
* > trailing newlines to expressions that would not break over multiple
Expand All @@ -58,15 +61,20 @@ public function processTokens(array $tokens): void
{
foreach ($tokens as $token) {
/** @var Token */
$first = $token->NextCode;
if ($first->hasNewlineAfter()) {
$open = $token->NextCode;
if ($open->hasNewlineAfter()) {
continue;
}
if ($first->children()->pop()->tokenHasNewlineAfter(true)) {
if ($open->children()->pop()->tokenHasNewlineAfter(true)) {
/** @var Token */
$last = $first->CloseBracket;
$first->applyWhitespace(Space::LINE_AFTER);
$last->applyWhitespace(Space::LINE_BEFORE);
$close = $open->CloseBracket;
$open->applyWhitespace(Space::LINE_AFTER);
$close->applyWhitespace(Space::LINE_BEFORE);
if ($token->id === \T_FOR) {
/** @var TokenCollection */
$semicolons = $token->Data[Data::FOR_PARTS][3];
$semicolons->setWhitespace(Space::LINE_AFTER);

Check warning on line 76 in src/Rule/SemiStrictExpressions.php

View check run for this annotation

Codecov / codecov/patch

src/Rule/SemiStrictExpressions.php#L75-L76

Added lines #L75 - L76 were not covered by tests
}
}
}
}
Expand Down
Loading

0 comments on commit 938f8a8

Please sign in to comment.