[PropertyInfo] Fix phpstan extractor issues #44578
Conversation
49e66ab to
76b7c45
Compare
|
Looking at the phpstan helper makes me think it also misses
|
|
I guess we could define the scope of this PR better. My goal wasn't to fix all PhpstanExtractor issues in existence here, but the ones that affected my closed-source project. Rest of the issues could be fixed in separate PR. Or, I could also fix them here, but I want to avoid someone coming here finding even more issues and hence extending the scope of this PR again and again. |
|
There is #44451 to introduce support for simple pseudo-types - |
I was not aware that either syntax was actively supported in 5.3. What types did the extractor detect for those cases and how does the "crash" look like? Sorry for being overly carefule here. I'd like to keep the changes as minimal as posssible and only restore the 5.3 behavior for your cases. This PR has the potential of sneaking in new functionality which is something we should not do in a bugfix. |
|
In Symfony 5.3, because PhpStanExtractor doesn't exist, it falls back to ReflectionExtractor which will extract info correctly throught reflection thanks to specified type declarations. |
|
And can we make this fallback work again? |
|
It does work, but only if PhpStanExtractor fails to detect the type completely. That's what I did in this PR for consttypes: Make consttypes return empty array, so that it falls back to another extractor. |
src/Symfony/Bridge/Doctrine/Tests/PropertyInfo/Fixtures/Foo.php
Outdated
Show resolved
Hide resolved
| /** @var self::*|null */ | ||
| public $foo; | ||
| }))->foo); | ||
| } |
There was a problem hiding this comment.
this was passing before the change on PropertyInfo, isn't it? (I'm saying that because deps=low tests still pass)
did you add the test just to increase the coveage? (which is a good thing :) )
There was a problem hiding this comment.
Yes I wanted to have some "integration" test with serializer to demonstrate the problem, so to increase coverage yes. We didn't have any tests in serializer that do use PhpStanExtractor. This test doesn't pass without my changes in propertyinfo
76b7c45 to
24eecfa
Compare
| @@ -161,6 +166,8 @@ private function extractTypes(TypeNode $node, NameScope $nameScope): array | ||
| return [new Type(Type::BUILTIN_TYPE_INT)]; | ||
| case 'list': | ||
| return [new Type(Type::BUILTIN_TYPE_ARRAY, false, null, true, new Type(Type::BUILTIN_TYPE_INT))]; | ||
| case 'non-empty-array': |
There was a problem hiding this comment.
I'm just wondering: why do we want to add this to 5.4 while the other pseudo-types are added on 6.1 (see #44451)?
There was a problem hiding this comment.
I think root of the problem is fallback case here on L182. If PhpStanTypeHelper doesn't recognize the type, it returns object. This is what causes issues during 5.3 -> 5.4 upgrade for my second use case. Potentially we could change that to return empty array instead, but that's very aggressive change compared to just adding support for non-empty-array (it might break various cases of @return Foo, @return Foo<bar> and so on. Alternatively, in my opinion backporting changes from #44451 to 5.4 would be fine too. All of those cases that were added there were most likely working fine in 5.3, but got broken in 5.4. non-empty-array just happens to be more often used than the rest of them and that's what blowed up in my projects.
There was a problem hiding this comment.
Thanks, that's exactly why I posted #44451 (review)
@derrabus WDYT?
There was a problem hiding this comment.
I wouldn't add support for that type here. If we get a proper fallback if the PHPStan extractor reported null here, that's the way to go imho.
There was a problem hiding this comment.
Well then any idea how to properly make a fallback here? We need a mechanism how to detect object, instead of falling back to object on everything that's not recognized.
There was a problem hiding this comment.
Sorry, your two answers were not displayed when I posted mine which obviously does not make sense in that context. 🙈
I agree that in that particular case, active support for non-empty-array (and possibly non-empty-list as well) would be the least invasive fix.
There was a problem hiding this comment.
I've added non-empty-list :)
24eecfa to
9468852
Compare
9468852 to
0302128
Compare
|
Thank you @ostrolucky. |
Since this crashes denormalization for codebase that was upgraded from 5.3 to 5.4, I am classifying this as a bugfix.
This PR fixes 2 issues. One is with docblocks like
/** @var Foo::*|null */, where Extractor returnsnullonly and afterwards Serializer fails to accept anything else. Second is/** @var non-empty-array<...> */where extractor doesn't recognize thatnon-empty-arrayis still an array and afterwards Serializer fails complaining thatarrayis not annon-empty-array