[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types #44451

EmilMassey · 2021-12-03T16:05:11Z

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

More and more apps are using pseudo-types like non-empty-string, positive-int which are understood by static analysis tools but are not a part of the language. This PR adds support for these types to PhpDocExtractor and PhpStanExtractor. Pseudo-type is mapped to built-in type(s) (e.g. non-empty-string => string, positive-int => int, number => int|float).

This PR adds support for all pseudo-types defined by the phpDocumentor (some of them like list or true, false are already supported) and PHPStan's basic types.

carsonbot · 2021-12-03T16:05:15Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

stof · 2021-12-03T16:16:08Z

The PhpstanExtractor should support the phpstan pseudo-types

EmilMassey · 2021-12-03T16:50:47Z

The PhpstanExtractor should support the phpstan pseudo-types

I agree and it would be great if PropertyInfo supported all of PHPStan's types, but it is a very complex system with integer ranges, generics, complex array shapes, literals and constants etc. so it will need a lot of work to support them all. We can work on it in future PRs but I think it is too much for just one PR.

All pseudo-types that are currently supported by phpDocumentor are also valid PHPStan pseudo-types so it didn't make sense to me not to add them to PhpstanExtractor. It can already recognize true, false or list pseudo-types and my PR adds just some more to the list. I know it's incomplete but we can gradually add support for more types. At the moment the problem is that there's no such list with all pseudo-types recognized by the PHPStan and I'm sure there will be more and more types as the tool evolves dynamically.

stof · 2021-12-03T17:07:43Z

@EmilMassey supporting the full type system might be too complex indeed. But you could at least support the full list of pseudo-types (i.e. also negative-int).
the phpdoc types are documented at https://phpstan.org/writing-php-code/phpdoc-types. And you can look at the source code in case the doc is incomplete: https://github.com/phpstan/phpstan-src/blob/c2ff0bd31327e861f651e55db364823cba0f0262/src/PhpDoc/TypeNodeResolver.php#L130

dunglas

In a future iteration, it could be useful to give access to the pseudo type to the end use.

For instance, this could help generating a more precise JSON Schema in API Platform.

src/Symfony/Component/PropertyInfo/Tests/Fixtures/PseudoTypesDummy.php

derrabus

The low-deps test failure looks related. Can you have a look?

EmilMassey · 2021-12-13T19:38:36Z

The low-deps test failure looks related. Can you have a look?

Fixed, thanks

src/Symfony/Component/PropertyInfo/composer.json

Korbeil

Thanks for adding this feature to PhpStanExtractor !

nicolas-grekas

Not sure: do we need to merge this as a bugfix on 5.4 since we changed the default extractor, and not supporting theses types might break existing apps?

In a future iteration, it could be useful to give access to the pseudo type to the end use.

true, PR welcome or issue at least :)

src/Symfony/Component/PropertyInfo/CHANGELOG.md

src/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php

src/Symfony/Component/PropertyInfo/Tests/Extractor/PhpStanExtractorTest.php

src/Symfony/Component/PropertyInfo/Tests/Fixtures/PhpStanPseudoTypesDummy.php

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

derrabus · 2021-12-16T22:50:49Z

Not sure: do we need to merge this as a bugfix on 5.4

The change is too heavy for a bugfix, imho.

since we changed the default extractor, and not supporting theses types might break existing apps?

The PHPDoc extractor on 5.3 did not understand most pseudo-types either. See #44578.

nicolas-grekas · 2021-12-17T10:00:48Z

Thank you @EmilMassey.

@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 (#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 ------- 723cd09 [PropertyInfo] strip only leading `\` when unknown docType

astronom · 2022-09-30T09:04:37Z

@nicolas-grekas if it possible to backport this changes to 5.4 branch?

nicolas-grekas · 2022-09-30T09:07:16Z

That's a new feature. 5.4 only accepts bug fixes now.

EmilMassey · 2022-09-30T13:28:40Z

@astronom If you need to support extracting these pseudotypes in v5.4, you can implement your own extractor. Note that this implementation requires phpstan/phpdoc-parser and I tested it in v5.3, so with v5.4 it may require some tweaks.

EmilMassey requested a review from dunglas as a code owner December 3, 2021 16:05

carsonbot added Status: Needs Review Feature PropertyInfo labels Dec 3, 2021

carsonbot added this to the 6.1 milestone Dec 3, 2021

EmilMassey changed the title ~~[PropertyInfo] Add support for phpDocumentor pseudo-types~~ [PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types Dec 3, 2021

EmilMassey mentioned this pull request Dec 3, 2021

[PropertyInfo] Incorrect handling of pseudo-types #44455

Closed

dunglas approved these changes Dec 4, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 4, 2021

derrabus reviewed Dec 13, 2021

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Fixtures/PseudoTypesDummy.php Show resolved Hide resolved

derrabus approved these changes Dec 13, 2021

View reviewed changes

derrabus requested changes Dec 13, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Dec 13, 2021

EmilMassey force-pushed the propertyinfo-pseudotypes branch 2 times, most recently from 106e654 to 68cb7e6 Compare December 13, 2021 19:30

EmilMassey requested a review from derrabus December 13, 2021 19:39

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 13, 2021

derrabus reviewed Dec 13, 2021

View reviewed changes

src/Symfony/Component/PropertyInfo/composer.json Outdated Show resolved Hide resolved

Korbeil approved these changes Dec 13, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 13, 2021

EmilMassey mentioned this pull request Dec 14, 2021

[PropertyInfo] Fix phpstan extractor issues #44578

Merged

EmilMassey force-pushed the propertyinfo-pseudotypes branch from 70c0459 to 2bb7b46 Compare December 15, 2021 09:00

nicolas-grekas reviewed Dec 16, 2021

View reviewed changes

derrabus approved these changes Dec 16, 2021

View reviewed changes

[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types

3e49b0f

nicolas-grekas force-pushed the propertyinfo-pseudotypes branch from b9c8585 to 3e49b0f Compare December 17, 2021 10:00

nicolas-grekas merged commit 53e49a8 into symfony:6.1 Dec 17, 2021

nicolas-grekas mentioned this pull request Dec 17, 2021

[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types symfony/symfony-docs#16297

Closed

EmilMassey deleted the propertyinfo-pseudotypes branch December 17, 2021 12:12

EmilMassey mentioned this pull request Mar 11, 2022

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

Merged

fabpot mentioned this pull request Apr 15, 2022

Release v6.1.0-BETA1 #46049

Merged

xabbuh mentioned this pull request Sep 30, 2022

Serializer thrown exception when property have "class-string<>" annotation #47736

Closed

EmilMassey mentioned this pull request Dec 28, 2023

Serializer psalm/phpstan types support #51673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types #44451

[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types #44451

EmilMassey commented Dec 3, 2021 •

edited

carsonbot commented Dec 3, 2021

stof commented Dec 3, 2021

EmilMassey commented Dec 3, 2021

stof commented Dec 3, 2021

dunglas left a comment

derrabus left a comment

EmilMassey commented Dec 13, 2021

Korbeil left a comment

nicolas-grekas left a comment

derrabus commented Dec 16, 2021

nicolas-grekas commented Dec 17, 2021

astronom commented Sep 30, 2022

nicolas-grekas commented Sep 30, 2022

EmilMassey commented Sep 30, 2022 •

edited

[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types #44451

[PropertyInfo] Add support for phpDocumentor and PHPStan pseudo-types #44451

Conversation

EmilMassey commented Dec 3, 2021 • edited

carsonbot commented Dec 3, 2021

stof commented Dec 3, 2021

EmilMassey commented Dec 3, 2021

stof commented Dec 3, 2021

dunglas left a comment

Choose a reason for hiding this comment

derrabus left a comment

Choose a reason for hiding this comment

EmilMassey commented Dec 13, 2021

Korbeil left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

derrabus commented Dec 16, 2021

nicolas-grekas commented Dec 17, 2021

astronom commented Sep 30, 2022

nicolas-grekas commented Sep 30, 2022

EmilMassey commented Sep 30, 2022 • edited

EmilMassey commented Dec 3, 2021 •

edited

EmilMassey commented Sep 30, 2022 •

edited