Skip to content

Commit

Permalink
bug #52699 [Serializer] [PropertyAccessor] Ignore non-collection inte…
Browse files Browse the repository at this point in the history
…rface generics (mtarld)

This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] [PropertyAccessor] Ignore non-collection interface generics

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #41996 #49924
| License       | MIT

`PhpDocExtractor` and `PhpStanDocExtractor` are following an old version of PSR-5 with used to define collections as the following:
```
generic = collection-type "<" [type-expression "," *SP] type-expression ">"
```
But, it does conflict with non-collection generics.

This issue will automatically be solved if the `TypeInfo` is merged in Symfony. But for older versions (<7.1 at least), it needs a fix.

~I was not able to find a proper bug fix without introducing a BC break, so this PR proposes to opt-on the "non-collection generics" parsing, such as `stcClass<int>` for example.~

~To opt-out from parsing these generics, it'll be required to set `ignore_docblock_generics` in the context. And this key/value will become obsolete as soon as the `TypeInfo` is introduced.~

~I'm not sure how to proceed though, should we only merge it in 5.4, and 6.3 supposing that the `TypeInfo` might be merged in 7.x? Should we document it only in these branches?~

EDIT: I finally ignored PHPDoc generics that aren't well known collection generic types so that the process will fall back to other resolvers (such as reflection resolver for example)

Commits
-------

e31aeebbba [PropertyAccessor] Fix unexpected collection when generics
  • Loading branch information
fabpot committed Jun 9, 2024
2 parents b999cc2 + d0bbc49 commit 2c96c24
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 6 deletions.
5 changes: 5 additions & 0 deletions Tests/Extractor/PhpDocExtractorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,11 @@ public function testUnknownPseudoType()
$this->assertEquals([new Type(Type::BUILTIN_TYPE_OBJECT, false, 'scalar')], $this->extractor->getTypes(PseudoTypeDummy::class, 'unknownPseudoType'));
}

public function testGenericInterface()
{
$this->assertNull($this->extractor->getTypes(Dummy::class, 'genericInterface'));
}

protected static function isPhpDocumentorV5()
{
if (class_exists(InvalidTag::class)) {
Expand Down
7 changes: 6 additions & 1 deletion Tests/Extractor/PhpStanExtractorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ public static function unionTypesProvider(): array
['b', [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [new Type(Type::BUILTIN_TYPE_INT)], [new Type(Type::BUILTIN_TYPE_STRING), new Type(Type::BUILTIN_TYPE_INT)])]],
['c', [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [], [new Type(Type::BUILTIN_TYPE_STRING), new Type(Type::BUILTIN_TYPE_INT)])]],
['d', [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [new Type(Type::BUILTIN_TYPE_STRING), new Type(Type::BUILTIN_TYPE_INT)], [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [], [new Type(Type::BUILTIN_TYPE_STRING)])])]],
['e', [new Type(Type::BUILTIN_TYPE_OBJECT, true, Dummy::class, true, [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [], [new Type(Type::BUILTIN_TYPE_STRING)])], [new Type(Type::BUILTIN_TYPE_INT), new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [new Type(Type::BUILTIN_TYPE_INT)], [new Type(Type::BUILTIN_TYPE_STRING, false, null, true, [], [new Type(Type::BUILTIN_TYPE_OBJECT, false, DefaultValue::class)])])]), new Type(Type::BUILTIN_TYPE_OBJECT, false, ParentDummy::class)]],
['e', [new Type(Type::BUILTIN_TYPE_OBJECT, true, Dummy::class, false, [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [], [new Type(Type::BUILTIN_TYPE_STRING)])], [new Type(Type::BUILTIN_TYPE_INT), new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [new Type(Type::BUILTIN_TYPE_INT)], [new Type(Type::BUILTIN_TYPE_OBJECT, false, \Traversable::class, true, [], [new Type(Type::BUILTIN_TYPE_OBJECT, false, DefaultValue::class)])])]), new Type(Type::BUILTIN_TYPE_OBJECT, false, ParentDummy::class)]],
['f', null],
['g', [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, [], [new Type(Type::BUILTIN_TYPE_STRING), new Type(Type::BUILTIN_TYPE_INT)])]],
];
Expand Down Expand Up @@ -429,6 +429,11 @@ public static function intRangeTypeProvider(): array
['c', [new Type(Type::BUILTIN_TYPE_INT)]],
];
}

public function testGenericInterface()
{
$this->assertNull($this->extractor->getTypes(Dummy::class, 'genericInterface'));
}
}

class PhpStanOmittedParamTagTypeDocBlock
Expand Down
3 changes: 3 additions & 0 deletions Tests/Extractor/ReflectionExtractorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public function testGetProperties()
'arrayOfMixed',
'listOfStrings',
'parentAnnotation',
'genericInterface',
'foo',
'foo2',
'foo3',
Expand Down Expand Up @@ -137,6 +138,7 @@ public function testGetPropertiesWithCustomPrefixes()
'arrayOfMixed',
'listOfStrings',
'parentAnnotation',
'genericInterface',
'foo',
'foo2',
'foo3',
Expand Down Expand Up @@ -190,6 +192,7 @@ public function testGetPropertiesWithNoPrefixes()
'arrayOfMixed',
'listOfStrings',
'parentAnnotation',
'genericInterface',
'foo',
'foo2',
'foo3',
Expand Down
5 changes: 5 additions & 0 deletions Tests/Fixtures/Dummy.php
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ class Dummy extends ParentDummy
*/
public $parentAnnotation;

/**
* @var \BackedEnum<string>
*/
public $genericInterface;

public static function getStatic()
{
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/Fixtures/DummyUnionType.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class DummyUnionType
public $d;

/**
* @var (Dummy<array<mixed, string>, (int | (string<DefaultValue>)[])> | ParentDummy | null)
* @var (Dummy<array<mixed, string>, (int | (\Traversable<DefaultValue>)[])> | ParentDummy | null)
*/
public $e;

Expand Down
13 changes: 10 additions & 3 deletions Util/PhpDocTypeHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ public function getTypes(DocType $varType): array
/**
* Creates a {@see Type} from a PHPDoc type.
*/
private function createType(DocType $type, bool $nullable, ?string $docType = null): ?Type
private function createType(DocType $type, bool $nullable): ?Type
{
$docType = $docType ?? (string) $type;
$docType = (string) $type;

if ($type instanceof Collection) {
$fqsen = $type->getFqsen();
Expand All @@ -115,10 +115,17 @@ private function createType(DocType $type, bool $nullable, ?string $docType = nu

[$phpType, $class] = $this->getPhpTypeAndClass((string) $fqsen);

$collection = \is_a($class, \Traversable::class, true) || \is_a($class, \ArrayAccess::class, true);

// it's safer to fall back to other extractors if the generic type is too abstract
if (!$collection && !class_exists($class)) {
return null;
}

$keys = $this->getTypes($type->getKeyType());
$values = $this->getTypes($type->getValueType());

return new Type($phpType, $nullable, $class, true, $keys, $values);
return new Type($phpType, $nullable, $class, $collection, $keys, $values);
}

// Cannot guess
Expand Down
9 changes: 8 additions & 1 deletion Util/PhpStanTypeHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ private function extractTypes(TypeNode $node, NameScope $nameScope): array
return [$mainType];
}

$collection = $mainType->isCollection() || \in_array($mainType->getClassName(), [\Traversable::class, \Iterator::class, \IteratorAggregate::class, \ArrayAccess::class, \Generator::class], true);

// it's safer to fall back to other extractors if the generic type is too abstract
if (!$collection && !class_exists($mainType->getClassName())) {
return [];
}

$collectionKeyTypes = $mainType->getCollectionKeyTypes();
$collectionKeyValues = [];
if (1 === \count($node->genericTypes)) {
Expand All @@ -136,7 +143,7 @@ private function extractTypes(TypeNode $node, NameScope $nameScope): array
}
}

return [new Type($mainType->getBuiltinType(), $mainType->isNullable(), $mainType->getClassName(), true, $collectionKeyTypes, $collectionKeyValues)];
return [new Type($mainType->getBuiltinType(), $mainType->isNullable(), $mainType->getClassName(), $collection, $collectionKeyTypes, $collectionKeyValues)];
}
if ($node instanceof ArrayShapeNode) {
return [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true)];
Expand Down

0 comments on commit 2c96c24

Please sign in to comment.