Skip to content

[BetterPhpDocParser] Don't wrap first-position @method param union in extra parens#8003

Merged
TomasVotruba merged 1 commit into
rectorphp:mainfrom
kyle-bisnow:fix-union-bracket-detection-on-method-params
May 22, 2026
Merged

[BetterPhpDocParser] Don't wrap first-position @method param union in extra parens#8003
TomasVotruba merged 1 commit into
rectorphp:mainfrom
kyle-bisnow:fix-union-bracket-detection-on-method-params

Conversation

@kyle-bisnow
Copy link
Copy Markdown
Contributor

@kyle-bisnow kyle-bisnow commented May 22, 2026

Ran into this using rector's withImportNames on a project that generates @method docblocks for Laravel facades via laravel/facade-documenter.

Setup

facade-documenter writes @method lines with fully-qualified class names. A facade that imports its proxy ends up like:

use App\Services\ConfigConventionRegistry;

/**
 * @method static \App\Services\ConfigConventionRegistry disable(string|array ...$classes)
 */
class ConfigConvention extends Facade { /* ... */ }

Then vendor/bin/rector process runs. With withImportNames enabled, rector shortens the FQCN to its imported alias, which forces it to reprint the entire @method line.

What it produces

/**
 * @method static ConfigConventionRegistry disable((string|array) ...$classes)
 */

Extra parens injected around the first parameter's union.

What it should produce

/**
 * @method static ConfigConventionRegistry disable(string|array ...$classes)
 */

FQCN shortened, union left alone.

Cause

UnionTypeNodePhpDocNodeVisitor::isWrappedInCurlyBrackets() returns true if there's either a ( immediately before the union or a ) immediately after. For a union that's the first parameter of an @method, the ( belongs to the method's parameter list, not a type wrap. But the check fires anyway, marks isWrappedInBrackets = true, and the reprinter wraps the union.

The close-paren half of that check was also broken. It looked at getEnd(), which is the position of the union's last token (e.g. array), not one past it. The comment // there is no + 1, as end is right at the next token assumed getEnd() returns one past the union, but endIndexOfLastRelevantToken() returns the index of the last token. So the close check never actually matched anything, and the OR was effectively just running the open-paren check.

Fix

Require both flanking parens, and correct the close offset to getEnd() + 1:

$previousPosition = $startAndEnd->getStart() - 1;
$nextPosition = $startAndEnd->getEnd() + 1;

return $betterTokenProvider->isTokenTypeOnPosition(Lexer::TOKEN_OPEN_PARENTHESES, $previousPosition)
    && $betterTokenProvider->isTokenTypeOnPosition(Lexer::TOKEN_CLOSE_PARENTHESES, $nextPosition);

A union is wrapped only when both parens really do flank it.

Test

Added a fixture under rules-tests/CodingStyle/Rector/Namespace_/ImportFullyQualifiedNamesRector/Fixture/ reproducing the case via name-importing on a class with an @method whose first parameter is a union. Fails on main, passes here. Full --testsuite=main (5201 tests) stays green.

@kyle-bisnow kyle-bisnow changed the title [BetterPhpDocParser] Don't wrap first-position @method param union in extra parens [BetterPhpDocParser] Don't wrap first-position @method param union in extra parens May 22, 2026
Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you!

@TomasVotruba TomasVotruba merged commit be04892 into rectorphp:main May 22, 2026
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants