Skip to content

Conversation

gnutix
Copy link
Contributor

@gnutix gnutix commented Oct 1, 2025

Claude Code (AI) : explanation of the two changes

  1. TokensNewliner.php (2 lines moved)

What it does: When iterating backward through constructor parameters, the code needs to check each token.

The bug: The token was captured in a variable BEFORE skipping over nested blocks, but checked AFTER skipping. So it was checking the wrong token.

The fix: Simply moved 2 lines - now the token variable is assigned AFTER skipping, so it always examines the correct token.

  // Before (buggy):
  $currentToken = $tokens[$i];        // ← captured too early
  $i = $this->tokenSkipper->skipBlocksReversed($tokens, $i);  // ← $i changes
  if ($currentToken->getContent() === ',') {  // ← checking old token!

  // After (fixed):
  $i = $this->tokenSkipper->skipBlocksReversed($tokens, $i);  // ← $i changes first
  $currentToken = $tokens[$i];        // ← now captures correct token
  if ($currentToken->getContent() === ',') {  // ← checking correct token
  1. TokenSkipper.php (added attribute support)

What it does: When going backward through tokens, it skips over "blocks" (parentheses, arrays, etc.) so commas inside those blocks aren't treated as parameter separators.

The bug: It wasn't skipping attribute blocks like #[JMS\Expose, JMS\Type('integer')], so the comma inside was incorrectly treated as a parameter separator.

The fix:

  • Added CT::T_ATTRIBUTE_CLOSE (the ] in #[...]) to the list of tokens to check
  • Added a simple findAttributeStart() method that searches backward for the matching #[ token

The findAttributeStart() is just a 10-line loop that searches backward until it finds T_ATTRIBUTE (the #[ token). I could have used more complex existing infrastructure, but a simple backward search is the most straightforward solution.

What I did (simple approach):

  private function findAttributeStart(Tokens $tokens, int $closingBracketPosition): ?int
  {
      // Simple backward loop
      for ($i = $closingBracketPosition - 1; $i >= 0; --$i) {
          if ($tokens[$i]->isGivenKind(T_ATTRIBUTE)) {
              return $i;
          }
          if ($tokens[$i]->equals(']') || $tokens[$i]->equals(';') /* ... */) {
              break;
          }
      }
      return null;
  }

Alternative using existing infrastructure:

The codebase already has BlockFinder (injected in the constructor) that knows how to find attribute blocks:

  private function findAttributeStart(Tokens $tokens, int $closingBracketPosition): ?int
  {
      $blockInfo = $this->blockFinder->findInTokensByEdge($tokens, $closingBracketPosition);
      if ($blockInfo !== null) {
          // Need to check if it's actually an attribute block
          $prevPosition = $tokens->getPrevMeaningfulToken($blockInfo->getStart());
          if ($prevPosition !== null && $tokens[$prevPosition]->isGivenKind(T_ATTRIBUTE)) {
              return $prevPosition;
          }
      }
      return null;
  }

Why I didn't use it:

Looking at BlockFinder::findInTokensByEdge() (lines 40-90 in BlockFinder.php), it's designed to be called on opening tokens like (, [, {, or specific kinds like T_ATTRIBUTE, T_FUNCTION, etc. When
called on a closing ], it would treat it as an array bracket, not an attribute bracket.

So I'd need to add special logic or modify BlockFinder, which would be more invasive. The simple backward loop is more minimalist and self-contained.

@TomasVotruba
Copy link
Member

Tldr; Can you fix failing CI?

@gnutix
Copy link
Contributor Author

gnutix commented Oct 1, 2025

Failing CI seems completely unrelated to my changes. It doesn't concern the same files at all. 🤔

@TomasVotruba
Copy link
Member

Could be the case, but we use boy scout rule here :) failing CI is a blocker.
If you can fix it, we'll get to merge faster. If not, might take weeks before someone will pick this up.

gnutix and others added 2 commits October 2, 2025 08:15
- Sort named parameters in rector.php
- Update PHPStan ignored errors to match new PHP version range
- Add new PHPStan ignores for offset access and array filter issues
- Remove obsolete PHPStan ignored error patterns

Co-Authored-By: Claude Code 🤖 <[email protected]>
@gnutix
Copy link
Contributor Author

gnutix commented Oct 2, 2025

Fixed. I added PHPStan inspections to the baseline, given there were already quite a few in there. I suppose they were caused by the absence of a composer.lock, therefore PHPStan gets upgraded to the latest version automatically, and reports new inspections from time to time.

@TomasVotruba
Copy link
Member

Looks good 👌 there are always issue with that token iterator.
Thanks

@TomasVotruba TomasVotruba merged commit 7e789fa into symplify:main Oct 2, 2025
6 checks passed
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.

2 participants