[PropertyInfo] Incorrect handling of pseudo-types #44455

EmilMassey · 2021-12-03T19:11:08Z

Symfony version(s) affected

4.x, 5.x, 6.0

Description

Since static-analysis tools like PHPStan or Psalm became more popular, it is not uncommon to see usage of pseudo-type like:

/** @var non-empty-string */
public $foo;

Some pseudo-types are currently supported (see #43916 introducing list type) but others are not. We can add support for popular custom types used by PHPStan or phpDocumentor (see #44451) but there will always be less common pseudo-types which we are not aware of.

Handling of these cases is inconsistent between extractors and PhpDocExtractor has a bug where the first character of the pseudo-type is trimmed. That one bug is quite easy to fix, but I believe handling of custom, unknown types is overall broken and we should decide how we want to handle them.

How to reproduce

Tested on v6.0.0 tag:

class TestClass {
    /** @var non-empty-string */
    public $foo;

    /** @var some-other-custom-type */
    public $bar;
}

$extractor = new \Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor();
$fooType = $extractor->getTypes(TestClass::class, 'foo'); // $builtinType: object, $class: on-empty-string (first character trimmed)
$barType = $extractor->getTypes(TestClass::class, 'bar'); // NULL

$extractor = new \Symfony\Component\PropertyInfo\Extractor\PhpStanExtractor();
$fooType = $extractor->getTypes(TestClass::class, 'foo'); // $builtinType: object, $class: \non-empty-string (backslash prepended)
$barType = $extractor->getTypes(TestClass::class, 'bar'); // $builtinType: object, $class: \some-other-custom-type

Possible Solution

Expected result

pseudo-types shouldn't be automatically treated as objects
if the type cannot be correctly mapped to built-in type, getType() should return null or throw an exception
or the developer should be informed it is a pseudo-type and decide what to do with doctype
the doctype shouldn't be modified and returned as-is (i.e. some-other-custom-type and not \some-other-custom-type)
it would be perfect if the results were consistent between both extractors

Additional Context

The first character is trimmed due to bug in \Symfony\Component\PropertyInfo\Util\PhpDocTypeHelper. I believe there's assumption that the $docType is FQCN with backslash prefix but in case of /** @var non-empty-string */ it is passed as-is to the function.

// src/Symfony/Component/PropertyInfo/Util/PhpDocTypeHelper.php

  // $docType: non-empty-string

    private function getPhpTypeAndClass(string $docType): array
    {
        // ...

        return ['object', substr($docType, 1)]; // substr to strip the namespace's `\`-prefix
    }

The text was updated successfully, but these errors were encountered:

DannyMeyer · 2022-03-10T21:25:38Z

Any Update on this issue? It's very annoying.

DannyMeyer · 2022-03-10T22:35:37Z

As an workaround you can inject the pseudo types into \Symfony\Component\PropertyInfo\Type::$builtinTypes
I'll do this dynamically in my config provider, to have a fresh list when phpDocumentor updated their list.

use phpDocumentor\Reflection\PseudoType;
use phpDocumentor\Reflection\TypeResolver;
use ReflectionClass;
use Symfony\Component\PropertyInfo\Type;

...

protected function injectPseudoTypes(): void
{
    $classReflection = new ReflectionClass(TypeResolver::class);

    if ($classReflection->hasProperty('keywords') === false) {
        return;
    }

    $propertyReflection = $classReflection->getProperty('keywords');
    $propertyReflection->setAccessible(true);
    $typeList = $propertyReflection->getValue(new TypeResolver());

    foreach ($typeList as $type => $class) {
        if (is_subclass_of($class, PseudoType::class)) {
            Type::$builtinTypes[] = $type;
        }
    }
}

derrabus · 2022-03-10T23:17:55Z

pseudo-types shouldn't be automatically treated as objects

How do I know that what I'm looking at is a pseudo-type that we're just not aware of (yet) and not a reference to a class?

Any Update on this issue? It's very annoying.

Do you want to work on a PR?

EmilMassey · 2022-03-11T14:50:36Z

How do I know that what I'm looking at is a pseudo-type that we're just not aware of (yet) and not a reference to a class?

That's why I didn't provide PR for the issue - I am not sure what is the way to detect it (if any?). I was thinking about class_exists - e.g. class_exists('non-empty-string') is false, so let's assume it is a pseudo-type. But it might as well be just a mistake - dev provided class that does not exist (or the class was deleted, but the doc still references it), etc.

derrabus · 2022-03-11T15:03:09Z

But it might as well be just a mistake - dev provided class that does not exist (or the class was deleted, but the doc still references it), etc.

Exactly. So your proposal wouldn't work. Assuming that an unknown type is a class reference is the sanest thing to do, unfortunately.

nicolas-grekas · 2022-03-11T15:07:31Z

When there is a - it could be a good heuristic?

DannyMeyer · 2022-03-11T16:16:47Z

You can check if DocType (\phpDocumentor\Reflection\Type) is an instance of \phpDocumentor\Reflection\PseudoType

EmilMassey · 2022-03-11T16:27:58Z

When there is a - it could be a good heuristic?

I'm afraid there are too many cases when pseudo-type does not contain -. PHPStan recognizes: scalar, double, number, numeric, true, etc. Unfortunately I don't know any other such tools besides PHPStan, PHPDocumentor (which we already cover - #44451) and Psalm, so I can't say if this heuristic is accurate enough.

You can check if DocType (\phpDocumentor\Reflection\Type) is an instance of \phpDocumentor\Reflection\PseudoType

Yes but it only covers phpDocumentor types and we need an universal approach.

DannyMeyer · 2022-03-11T16:31:37Z

My patch proposal:

Index: Type.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/Type.php b/Type.php
--- a/Type.php	(revision bcc2b6904cbcf16b2e5d618da16117cd8e132f9a)
+++ b/Type.php	(date 1647016126777)
@@ -69,6 +69,7 @@
     private $collection;
     private $collectionKeyType;
     private $collectionValueType;
+    private $pseudoType;
 
     /**
      * @param Type[]|Type|null $collectionKeyType
@@ -76,10 +77,10 @@
      *
      * @throws \InvalidArgumentException
      */
-    public function __construct(string $builtinType, bool $nullable = false, string $class = null, bool $collection = false, $collectionKeyType = null, $collectionValueType = null)
+    public function __construct(string $builtinType, bool $nullable = false, string $class = null, bool $collection = false, $collectionKeyType = null, $collectionValueType = null, bool $pseudoType = false)
     {
-        if (!\in_array($builtinType, self::$builtinTypes)) {
-            throw new \InvalidArgumentException(sprintf('"%s" is not a valid PHP type.', $builtinType));
+        if ($pseudoType === false && !\in_array($builtinType, self::$builtinTypes)) {
+            throw new \InvalidArgumentException(\sprintf('"%s" is not a valid PHP type.', $builtinType));
         }
 
         $this->builtinType = $builtinType;
@@ -88,6 +89,7 @@
         $this->collection = $collection;
         $this->collectionKeyType = $this->validateCollectionArgument($collectionKeyType, 5, '$collectionKeyType') ?? [];
         $this->collectionValueType = $this->validateCollectionArgument($collectionValueType, 6, '$collectionValueType') ?? [];
+        $this->pseudoType = $pseudoType;
     }
 
     private function validateCollectionArgument($collectionArgument, int $argumentIndex, string $argumentName): ?array
@@ -97,13 +99,13 @@
         }
 
         if (!\is_array($collectionArgument) && !$collectionArgument instanceof self) {
-            throw new \TypeError(sprintf('"%s()": Argument #%d (%s) must be of type "%s[]", "%s" or "null", "%s" given.', __METHOD__, $argumentIndex, $argumentName, self::class, self::class, get_debug_type($collectionArgument)));
+            throw new \TypeError(\sprintf('"%s()": Argument #%d (%s) must be of type "%s[]", "%s" or "null", "%s" given.', __METHOD__, $argumentIndex, $argumentName, self::class, self::class, get_debug_type($collectionArgument)));
         }
 
         if (\is_array($collectionArgument)) {
             foreach ($collectionArgument as $type) {
                 if (!$type instanceof self) {
-                    throw new \TypeError(sprintf('"%s()": Argument #%d (%s) must be of type "%s[]", "%s" or "null", array value "%s" given.', __METHOD__, $argumentIndex, $argumentName, self::class, self::class, get_debug_type($collectionArgument)));
+                    throw new \TypeError(\sprintf('"%s()": Argument #%d (%s) must be of type "%s[]", "%s" or "null", array value "%s" given.', __METHOD__, $argumentIndex, $argumentName, self::class, self::class, get_debug_type($collectionArgument)));
                 }
             }
 
@@ -152,7 +154,7 @@
      */
     public function getCollectionKeyType(): ?self
     {
-        trigger_deprecation('symfony/property-info', '5.3', 'The "%s()" method is deprecated, use "getCollectionKeyTypes()" instead.', __METHOD__);
+        \trigger_deprecation('symfony/property-info', '5.3', 'The "%s()" method is deprecated, use "getCollectionKeyTypes()" instead.', __METHOD__);
 
         $type = $this->getCollectionKeyTypes();
         if (0 === \count($type)) {
@@ -187,7 +189,7 @@
      */
     public function getCollectionValueType(): ?self
     {
-        trigger_deprecation('symfony/property-info', '5.3', 'The "%s()" method is deprecated, use "getCollectionValueTypes()" instead.', __METHOD__);
+        \trigger_deprecation('symfony/property-info', '5.3', 'The "%s()" method is deprecated, use "getCollectionValueTypes()" instead.', __METHOD__);
 
         return $this->getCollectionValueTypes()[0] ?? null;
     }
@@ -203,4 +205,9 @@
     {
         return $this->collectionValueType;
     }
+
+    public function isPseudoType(): bool
+    {
+        return $this->pseudoType;
+    }
 }
Index: Util/PhpDocTypeHelper.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/Util/PhpDocTypeHelper.php b/Util/PhpDocTypeHelper.php
--- a/Util/PhpDocTypeHelper.php	(revision bcc2b6904cbcf16b2e5d618da16117cd8e132f9a)
+++ b/Util/PhpDocTypeHelper.php	(date 1647016126772)
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\PropertyInfo\Util;
 
+use phpDocumentor\Reflection\PseudoType;
 use phpDocumentor\Reflection\PseudoTypes\List_;
 use phpDocumentor\Reflection\Type as DocType;
 use phpDocumentor\Reflection\Types\Array_;
@@ -22,7 +23,7 @@
 
 // Workaround for phpdocumentor/type-resolver < 1.6
 // We trigger the autoloader here, so we don't need to trigger it inside the loop later.
-class_exists(List_::class);
+\class_exists(List_::class);
 
 /**
  * Transforms a php doc type to a {@link Type} instance.
@@ -97,7 +98,7 @@
 
         if ($type instanceof Collection) {
             $fqsen = $type->getFqsen();
-            if ($fqsen && 'list' === $fqsen->getName() && !class_exists(List_::class, false) && !class_exists((string) $fqsen)) {
+            if ($fqsen && 'list' === $fqsen->getName() && !\class_exists(List_::class, false) && !\class_exists((string) $fqsen)) {
                 // Workaround for phpdocumentor/type-resolver < 1.6
                 return new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, new Type(Type::BUILTIN_TYPE_INT), $this->getTypes($type->getValueType()));
             }
@@ -122,18 +123,18 @@
 
         if (str_ends_with($docType, '[]')) {
             $collectionKeyType = new Type(Type::BUILTIN_TYPE_INT);
-            $collectionValueType = $this->createType($type, false, substr($docType, 0, -2));
+            $collectionValueType = $this->createType($type, false, \substr($docType, 0, -2));
 
             return new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, $collectionKeyType, $collectionValueType);
         }
 
-        if ((str_starts_with($docType, 'list<') || str_starts_with($docType, 'array<')) && $type instanceof Array_) {
+        if ($type instanceof Array_ && (\str_starts_with($docType, 'list<') || \str_starts_with($docType, 'array<'))) {
             // array<value> is converted to x[] which is handled above
             // so it's only necessary to handle array<key, value> here
             $collectionKeyType = $this->getTypes($type->getKeyType())[0];
 
             $collectionValueTypes = $this->getTypes($type->getValueType());
-            if (1 != \count($collectionValueTypes)) {
+            if (1 !== \count($collectionValueTypes)) {
                 // the Type class does not support union types yet, so assume that no type was defined
                 $collectionValueType = null;
             } else {
@@ -143,6 +144,10 @@
             return new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, $collectionKeyType, $collectionValueType);
         }
 
+        if ($type instanceof PseudoType) {
+            return new Type((string) $type, $nullable, null, false, null, null, true);
+        }
+
         $docType = $this->normalizeType($docType);
         [$phpType, $class] = $this->getPhpTypeAndClass($docType);
 
@@ -187,6 +192,10 @@
             return ['object', $docType];
         }
 
-        return ['object', substr($docType, 1)]; // substr to strip the namespace's `\`-prefix
+        if (\str_starts_with('\\', $docType)) {
+            $docType = \substr($docType, 1); // substr to strip the namespace's `\`-prefix
+        }
+
+        return ['object', $docType];
     }
 }

DannyMeyer · 2022-03-11T16:34:44Z

Yes but it only covers phpDocumentor types and we need an universal approach.

In this case you may be limited to use a list like used in Type::$builtinTypes 😕

@var

… (EmilMassey) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [PropertyInfo] strip only leading `\` when unknown docType | Q | A | ------------- | --- | Branch? | 4.4, 5.4, 6.0 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #44455 (partially) | License | MIT | Doc PR | - Fixes issue in `PhpDocExtractor` when dealing with some pseudo-types (`non-empty-string`, `positive-int`, etc.): ```php class TestClass { /** @var non-empty-string */ public $foo; /** @var positive-int */ public $bar; } $extractor = new \Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor(); $fooType = $extractor->getTypes(TestClass::class, 'foo'); // $builtinType: object, $class: on-empty-string (first character trimmed) $barType = $extractor->getTypes(TestClass::class, 'bar'); // $builtinType: object, $class: ositive-int (first character trimmed) ``` The bug exists in 4.4, 5.4 and 6.0. It may exist in 6.1 as the invalid line is still there, but due to changes made in #44451, above snippet will no longer produce the bug. I was not able to reproduce the error in 6.1, but I suspect there may exist some pseudo-type that is still affected by the bug. I'm not sure if the test I wrote is OK, but it is the only way I could think of to validate my fix. In my opinion (see #44455), unknown pseudo-type should not be mapped to an object, so after the fix handling of pseudo-types is still broken but at least the results are consistent between `PhpStanExtractor` and `PhpDocExtractor`. But I agree with @derrabus (symfony/symfony#44455 (comment)) that until there is no way to detect if we are dealing with a pseudo-type, assuming it is a reference to an object is the sanest thing to do. Commits ------- 723cd0919d [PropertyInfo] strip only leading `\` when unknown docType

EmilMassey added the Bug label Dec 3, 2021

derrabus added the PropertyInfo label Mar 10, 2022

EmilMassey mentioned this issue Mar 11, 2022

[PropertyInfo] strip only leading \ when unknown docType #45720

Merged

nicolas-grekas closed this as completed in a2cfd7e Mar 18, 2022

EmilMassey mentioned this issue Dec 28, 2023

[TypeInfo] Introduce component #52510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[PropertyInfo] Incorrect handling of pseudo-types #44455

[PropertyInfo] Incorrect handling of pseudo-types #44455

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[PropertyInfo] Incorrect handling of pseudo-types #44455

[PropertyInfo] Incorrect handling of pseudo-types #44455

Comments

Uh oh!

Symfony version(s) affected

Description

How to reproduce

Possible Solution

Expected result

Additional Context

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!