Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PropertyInfo] Fix phpstan extractor issues #44578

Merged
merged 1 commit into from Dec 25, 2021

Conversation

@ostrolucky
Copy link
Contributor

@ostrolucky ostrolucky commented Dec 12, 2021

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #44491
License MIT
Doc PR

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 returns null only and afterwards Serializer fails to accept anything else. Second is /** @var non-empty-array<...> */ where extractor doesn't recognize that non-empty-array is still an array and afterwards Serializer fails complaining that array is not an non-empty-array

@ostrolucky ostrolucky requested a review from dunglas as a code owner Dec 12, 2021
@carsonbot carsonbot added this to the 5.4 milestone Dec 12, 2021
@ostrolucky ostrolucky force-pushed the fix-phpstan-extractor-issues branch from 49e66ab to 76b7c45 Dec 12, 2021
@staabm
Copy link
Contributor

@staabm staabm commented Dec 12, 2021

Looking at the phpstan helper makes me think it also misses

  • negative-int
  • positive-int
  • int<1, -0> like IntegerRanges

@ostrolucky
Copy link
Contributor Author

@ostrolucky ostrolucky commented Dec 13, 2021

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.

@EmilMassey
Copy link
Contributor

@EmilMassey EmilMassey commented Dec 14, 2021

There is #44451 to introduce support for simple pseudo-types - non-empty-array, positive-int, negative-int etc.

@derrabus
Copy link
Member

@derrabus derrabus commented Dec 14, 2021

This PR fixes 2 issues.

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.

@ostrolucky
Copy link
Contributor Author

@ostrolucky ostrolucky commented Dec 14, 2021

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.

@derrabus
Copy link
Member

@derrabus derrabus commented Dec 14, 2021

And can we make this fallback work again?

@ostrolucky
Copy link
Contributor Author

@ostrolucky ostrolucky commented Dec 15, 2021

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.

@carsonbot carsonbot changed the title Fix phpstan extractor issues [PropertyInfo] Fix phpstan extractor issues Dec 16, 2021
/** @var self::*|null */
public $foo;
}))->foo);
}
Copy link
Member

@nicolas-grekas nicolas-grekas Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :) )

Copy link
Contributor Author

@ostrolucky ostrolucky Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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':
Copy link
Member

@nicolas-grekas nicolas-grekas Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@ostrolucky ostrolucky Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nicolas-grekas nicolas-grekas Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's exactly why I posted #44451 (review)

@derrabus WDYT?

Copy link
Member

@derrabus derrabus Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ostrolucky ostrolucky Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@derrabus derrabus Dec 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ostrolucky ostrolucky Dec 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added non-empty-list :)

@nicolas-grekas nicolas-grekas force-pushed the fix-phpstan-extractor-issues branch from 9468852 to 0302128 Dec 25, 2021
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 25, 2021

Thank you @ostrolucky.

@nicolas-grekas nicolas-grekas merged commit d6a29ae into symfony:5.4 Dec 25, 2021
10 of 12 checks passed
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants