Fix DocBlockVar fixer corrupting callable/Closure types#69
Merged
Conversation
The var-annotation fixer splits the type on its first space to separate a trailing comment, assuming types have no internal spaces. A callable/Closure signature such as `\Closure(string): string` has a space after the colon, so the split cut the type in half and the null-append produced invalid output (`\Closure(string):|null string`), which PHPStan then rejects as an unparseable doc type. Skip types containing `(` (callable/Closure signatures) the same way generics and array shapes are already skipped, since the first-space split cannot handle their internal structure. Adds a regression fixture.
There was a problem hiding this comment.
Pull request overview
Prevents DocBlockVar’s fixer logic from corrupting PHPDoc @var types for callable/\Closure signatures by skipping annotations that contain internal structure the current “split on first space” parsing can’t safely handle.
Changes:
- Add a regression fixture for a
?\Closure-typed property with@var \Closure(string): string. - Update
DocBlockVarSniff::handle()to early-return for@vartype strings containing((in addition to existing{/<skips).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/_data/DocBlockVar/before.php |
Adds a fixture case involving a \Closure(string): string doc type. |
tests/_data/DocBlockVar/after.php |
Expects the fixer to leave the new callable/\Closure doc type unchanged. |
PhpCollective/Sniffs/Commenting/DocBlockVarSniff.php |
Skips processing for doc types with ( to avoid corrupting callable/\Closure signatures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Split the doc type from its trailing description before the structural check, then test only the type token. The previous check ran on the full string, so a simple type with a parenthetical description (e.g. `@var string (deprecated)`) was wrongly skipped and never got its missing `null` appended. Closure/generic/array-shape types still skip correctly because their opening token sits before the first space. Also use strict `!== false` for the space split so an index-0 match is not treated as "no space". Add a regression fixture for the description-with-parentheses case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
DocBlockVarfixer corrupts callable/Closureproperty types when it appends amissing
null.handle()splits the doc type on its first space to peel off a trailing comment,assuming the type itself has no internal spaces. A callable/
Closuresignature has aspace after the colon, so the split cuts the type in half:
phpcbfturns that into invalid output:PHPStan then rejects it as an unparseable doc type ("Unexpected token (").
Fix
Skip types whose syntax contains internal spaces/structure that the simple first-space
split cannot handle. Generics (
<) and array shapes ({) are already skipped; this addscallable/
Closuresignatures ((). The fixer then leaves such annotations untouchedinstead of mangling them.
A regression fixture (a
?Closureproperty with a\Closure(string): stringtype) isadded to
tests/_data/DocBlockVar; the fixer must leave it unchanged.Found while it corrupted a real
?Closureproperty annotation in another project.