From 820d2da4ae789cefd340c27ff18dbba146c54e9d Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Sun, 30 Aug 2026 16:11:56 +0200 Subject: [PATCH] Fix three ways a valid PHPDoc is read or written back wrong All three were found by walking the grammars of the PHPDoc language the other way round and asking the parser to read what came out. * A nullable type written inside another one was printed as "??Foo", which is no type at all: "?(?Foo)" is read back as the very same thing while "??Foo" is not read at all. * What a conditional type asks about lost its parentheses where it needed them: "(?Foo) is Bar ? ... : ..." was printed as "?Foo is Bar ? ... : ...", which reads as a nullable type followed by something the type says nothing about. The printer dropped them for a union and an intersection as well, so "((Foo|Bar) is Baz ? ... )" came back out as "(Foo|Bar is Baz ? ... )". * A comment written inside a generic type was left waiting for a node where no attributes are asked for, which is the default: the reading of the whole PHPDoc then gave up with "Comments should already be flushed". TypeParser::parseGeneric() only gave the comments away as part of placing the node, and it only places one where the type it is written after carries a position of its own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GavWEzia8ZVUYEe6JEde9i --- src/Ast/Type/ConditionalTypeNode.php | 7 +- src/Ast/Type/NullableTypeNode.php | 6 ++ src/Parser/TypeParser.php | 5 ++ src/Printer/Printer.php | 39 +++++++++- tests/PHPStan/Parser/PhpDocParserTest.php | 35 +++++++++ tests/PHPStan/Parser/TypeParserTest.php | 68 ++++++++++++++++ tests/PHPStan/Printer/PrinterTest.php | 94 +++++++++++++++++++++++ 7 files changed, 251 insertions(+), 3 deletions(-) diff --git a/src/Ast/Type/ConditionalTypeNode.php b/src/Ast/Type/ConditionalTypeNode.php index 7970a701..6ee3ec65 100644 --- a/src/Ast/Type/ConditionalTypeNode.php +++ b/src/Ast/Type/ConditionalTypeNode.php @@ -33,7 +33,12 @@ public function __toString(): string { return sprintf( '(%s %s %s ? %s : %s)', - $this->subjectType, + // "?Foo is Bar ? ... : ..." reads as a nullable type followed by + // something else, so the subject keeps the parentheses it was read + // with + $this->subjectType instanceof NullableTypeNode + ? '(' . $this->subjectType . ')' + : $this->subjectType, $this->negated ? 'is not' : 'is', $this->targetType, $this->if, diff --git a/src/Ast/Type/NullableTypeNode.php b/src/Ast/Type/NullableTypeNode.php index f79bad0f..7b70d608 100644 --- a/src/Ast/Type/NullableTypeNode.php +++ b/src/Ast/Type/NullableTypeNode.php @@ -18,6 +18,12 @@ public function __construct(TypeNode $type) public function __toString(): string { + if ($this->type instanceof self) { + // "??Foo" is no type at all, so the one written inside keeps the + // parentheses it was read with + return '?(' . $this->type . ')'; + } + return '?' . $this->type; } diff --git a/src/Parser/TypeParser.php b/src/Parser/TypeParser.php index 077dd6b0..a2db4fce 100644 --- a/src/Parser/TypeParser.php +++ b/src/Parser/TypeParser.php @@ -470,6 +470,11 @@ public function parseGeneric(TokenIterator $tokens, Ast\Type\IdentifierTypeNode $type = new Ast\Type\GenericTypeNode($baseType, $genericTypes, $variances); if ($startLine !== null && $startIndex !== null) { $type = $this->enrichWithAttributes($tokens, $type, $startLine, $startIndex); + } else { + // The comments read between the arguments have to be given away + // even where the node is not placed, or they would still be waiting + // for a node once the whole PHPDoc has been read + $tokens->flushComments(); } $tokens->consumeTokenType(Lexer::TOKEN_CLOSE_ANGLE_BRACKET); diff --git a/src/Printer/Printer.php b/src/Printer/Printer.php index 623cb95b..db7d3db7 100644 --- a/src/Printer/Printer.php +++ b/src/Printer/Printer.php @@ -150,6 +150,16 @@ final class Printer IntersectionTypeNode::class, NullableTypeNode::class, ], + NullableTypeNode::class . '->type' => [ + UnionTypeNode::class, + IntersectionTypeNode::class, + NullableTypeNode::class, + ], + ConditionalTypeNode::class . '->subjectType' => [ + UnionTypeNode::class, + IntersectionTypeNode::class, + NullableTypeNode::class, + ], ]; /** @var array>> */ @@ -441,7 +451,7 @@ private function printType(TypeNode $node): string if ($node instanceof ConditionalTypeNode) { return sprintf( '(%s %s %s ? %s : %s)', - $this->printType($node->subjectType), + $this->printConditionalSubjectType($node->subjectType), $node->negated ? 'is not' : 'is', $this->printType($node->targetType), $this->printType($node->if), @@ -491,7 +501,11 @@ private function printType(TypeNode $node): string return (string) $node; } if ($node instanceof NullableTypeNode) { - if ($node->type instanceof IntersectionTypeNode || $node->type instanceof UnionTypeNode) { + if ( + $node->type instanceof IntersectionTypeNode + || $node->type instanceof UnionTypeNode + || $node->type instanceof NullableTypeNode + ) { return '?(' . $this->printType($node->type) . ')'; } @@ -517,6 +531,27 @@ private function wrapInParentheses(TypeNode $node): string return '(' . $this->printType($node) . ')'; } + /** + * What a conditional type asks about, written so that it is read back as + * the very same type. + * + * "?Foo is Bar ? ... : ..." and "Foo|Bar is Baz ? ... : ..." both read as a + * type followed by something the type says nothing about, so a subject of + * either kind keeps its parentheses. + */ + private function printConditionalSubjectType(TypeNode $type): string + { + if ( + $type instanceof UnionTypeNode + || $type instanceof IntersectionTypeNode + || $type instanceof NullableTypeNode + ) { + return $this->wrapInParentheses($type); + } + + return $this->printType($type); + } + private function printOffsetAccessType(TypeNode $type): string { if ( diff --git a/tests/PHPStan/Parser/PhpDocParserTest.php b/tests/PHPStan/Parser/PhpDocParserTest.php index 3e6aa430..b118798d 100644 --- a/tests/PHPStan/Parser/PhpDocParserTest.php +++ b/tests/PHPStan/Parser/PhpDocParserTest.php @@ -8245,4 +8245,39 @@ public function testTextBetweenTagsBelongsToDescription( $this->assertSame(Lexer::TOKEN_END, $tokens->currentTokenType()); } + /** + * @return iterable + */ + public function dataCommentInsideGenericType(): iterable + { + yield ['/**' . PHP_EOL . ' * @use Foo' . PHP_EOL . ' */']; + yield ['/**' . PHP_EOL . ' * @var Foo' . PHP_EOL . ' */']; + yield ['/**' . PHP_EOL . ' * @extends Foo<*, // a comment' . PHP_EOL . ' * string>' . PHP_EOL . ' */']; + yield ['/**' . PHP_EOL . ' * @implements Foo description' . PHP_EOL . ' */']; + } + + /** + * A comment written inside a generic type is read and given to the node the + * reading reaches first after it. Where no attributes are asked for there is + * no node to give it to, and it used to be left waiting for one until the + * whole PHPDoc had been read, which is where the reading gave up. + * + * @dataProvider dataCommentInsideGenericType + */ + public function testCommentInsideGenericTypeIsAlwaysFlushed(string $input): void + { + foreach ([[], ['lines' => true, 'indexes' => true, 'comments' => true]] as $usedAttributes) { + $config = new ParserConfig($usedAttributes); + $constExprParser = new ConstExprParser($config); + $typeParser = new TypeParser($config, $constExprParser); + $phpDocParser = new PhpDocParser($config, $typeParser, $constExprParser); + + $tokens = new TokenIterator($this->lexer->tokenize($input)); + $phpDocNode = $phpDocParser->parse($tokens); + + $this->assertCount(1, $phpDocNode->children); + $this->assertSame(Lexer::TOKEN_END, $tokens->currentTokenType()); + } + } + } diff --git a/tests/PHPStan/Parser/TypeParserTest.php b/tests/PHPStan/Parser/TypeParserTest.php index b0123ef9..3a9d67e7 100644 --- a/tests/PHPStan/Parser/TypeParserTest.php +++ b/tests/PHPStan/Parser/TypeParserTest.php @@ -2380,6 +2380,74 @@ public function provideParseData(): array ), ), ], + [ + '?(?Foo)', + new NullableTypeNode( + new NullableTypeNode( + new IdentifierTypeNode('Foo'), + ), + ), + ], + [ + '?(?(Foo|Bar))', + new NullableTypeNode( + new NullableTypeNode( + new UnionTypeNode([ + new IdentifierTypeNode('Foo'), + new IdentifierTypeNode('Bar'), + ]), + ), + ), + ], + [ + 'Foo[?(?Bar)]', + new OffsetAccessTypeNode( + new IdentifierTypeNode('Foo'), + new NullableTypeNode( + new NullableTypeNode( + new IdentifierTypeNode('Bar'), + ), + ), + ), + ], + [ + '((?Foo) is Bar ? true : false)', + new ConditionalTypeNode( + new NullableTypeNode( + new IdentifierTypeNode('Foo'), + ), + new IdentifierTypeNode('Bar'), + new IdentifierTypeNode('true'), + new IdentifierTypeNode('false'), + false, + ), + ], + [ + '((Foo | Bar) is Baz ? true : false)', + new ConditionalTypeNode( + new UnionTypeNode([ + new IdentifierTypeNode('Foo'), + new IdentifierTypeNode('Bar'), + ]), + new IdentifierTypeNode('Baz'), + new IdentifierTypeNode('true'), + new IdentifierTypeNode('false'), + false, + ), + ], + [ + '((Foo & Bar) is Baz ? true : false)', + new ConditionalTypeNode( + new IntersectionTypeNode([ + new IdentifierTypeNode('Foo'), + new IdentifierTypeNode('Bar'), + ]), + new IdentifierTypeNode('Baz'), + new IdentifierTypeNode('true'), + new IdentifierTypeNode('false'), + false, + ), + ], [ '(T is Foo ? true : T is Bar ? false : null)', new ConditionalTypeNode( diff --git a/tests/PHPStan/Printer/PrinterTest.php b/tests/PHPStan/Printer/PrinterTest.php index ab24032b..3f791927 100644 --- a/tests/PHPStan/Printer/PrinterTest.php +++ b/tests/PHPStan/Printer/PrinterTest.php @@ -35,6 +35,7 @@ use PHPStan\PhpDocParser\Ast\Type\ArrayTypeNode; use PHPStan\PhpDocParser\Ast\Type\CallableTypeNode; use PHPStan\PhpDocParser\Ast\Type\CallableTypeParameterNode; +use PHPStan\PhpDocParser\Ast\Type\ConditionalTypeNode; use PHPStan\PhpDocParser\Ast\Type\ConstTypeNode; use PHPStan\PhpDocParser\Ast\Type\GenericTypeNode; use PHPStan\PhpDocParser\Ast\Type\IdentifierTypeNode; @@ -300,6 +301,52 @@ public function enterNode(Node $node) $changeReturnType, ]; + $wrapNullableTypeInAnotherOne = new class extends AbstractNodeVisitor { + + public function enterNode(Node $node) + { + if ( + $node instanceof NullableTypeNode + && $node->type instanceof IdentifierTypeNode + && $node->type->name === 'Foo' + ) { + $node->type = new NullableTypeNode(new IdentifierTypeNode('Bar')); + + return $node; + } + + return $node; + } + + }; + + yield [ + '/** @return ?Foo */', + '/** @return ?(?Bar) */', + $wrapNullableTypeInAnotherOne, + ]; + + $changeConditionalSubjectType = new class extends AbstractNodeVisitor { + + public function enterNode(Node $node) + { + if ($node instanceof ConditionalTypeNode && $node->subjectType instanceof IdentifierTypeNode) { + $node->subjectType = new NullableTypeNode(new IdentifierTypeNode('Bar')); + + return $node; + } + + return $node; + } + + }; + + yield [ + '/** @return (Foo is Bar ? true : false) */', + '/** @return ((?Bar) is Bar ? true : false) */', + $changeConditionalSubjectType, + ]; + yield [ '/** @return Foo*/', '/** @return Bar*/', @@ -2773,6 +2820,53 @@ public function dataPrintType(): iterable new ArrayTypeNode(new NullableTypeNode(new IdentifierTypeNode('Foo'))), '(?Foo)[]', ]; + yield [ + new NullableTypeNode(new NullableTypeNode(new IdentifierTypeNode('Foo'))), + '?(?Foo)', + ]; + yield [ + new OffsetAccessTypeNode( + new IdentifierTypeNode('Foo'), + new NullableTypeNode(new NullableTypeNode(new IdentifierTypeNode('Bar'))), + ), + 'Foo[?(?Bar)]', + ]; + yield [ + new ConditionalTypeNode( + new NullableTypeNode(new IdentifierTypeNode('Foo')), + new IdentifierTypeNode('Bar'), + new IdentifierTypeNode('true'), + new IdentifierTypeNode('false'), + false, + ), + '((?Foo) is Bar ? true : false)', + ]; + yield [ + new ConditionalTypeNode( + new UnionTypeNode([ + new IdentifierTypeNode('Foo'), + new IdentifierTypeNode('Bar'), + ]), + new IdentifierTypeNode('Baz'), + new IdentifierTypeNode('true'), + new IdentifierTypeNode('false'), + false, + ), + '((Foo|Bar) is Baz ? true : false)', + ]; + yield [ + new ConditionalTypeNode( + new IntersectionTypeNode([ + new IdentifierTypeNode('Foo'), + new IdentifierTypeNode('Bar'), + ]), + new IdentifierTypeNode('Baz'), + new IdentifierTypeNode('true'), + new IdentifierTypeNode('false'), + false, + ), + '((Foo&Bar) is Baz ? true : false)', + ]; yield [ new UnionTypeNode([ new IdentifierTypeNode('Foo'),