Add config to check BenevolentUnion #1930
Conversation
VincentLanglet
force-pushed
the
benevolentUnion
branch
2 times, most recently
from
5290873 to
706c831
Compare
|
This pull request has been marked as ready for review. |
| @@ -76,6 +76,7 @@ parameters: | |||
| checkNullables: false | |||
| checkThisOnly: true | |||
| checkUnionTypes: false | |||
| checkBenevolentUnionTypes: false | |||
There was a problem hiding this comment.
should it be true on bleedingEdge?
There was a problem hiding this comment.
CheckUnionTypes is not true on bleedingEdge, so checkBenevolentUnionTypes which is stricter shouldn't be true too.
There was a problem hiding this comment.
checkBenevolentUnionTypes will never be true by default, that's why it's "benevolent".
There was a problem hiding this comment.
But I'm kinda worried about someone using the config
checkUnionTypes: false
checkBenevolentUnionTypes: true
what should be the behavior ?
- checking both ?
- checking none ?
- only checking the benevolent ?
- throwing an error ?
src/Rules/RuleLevelHelper.php
Outdated
|
|
||
| if ( | ||
| $this->checkBenevolentUnionTypes | ||
| && $acceptedType instanceof BenevolentUnionType |
There was a problem hiding this comment.
This is too naive - will not see nested BenevolentUnionType. Needs similar logic as checkExplicitMixed and use TypeTraverser.
There was a problem hiding this comment.
I have a test for (\DateTimeImmutable|false) and for array<(\DateTimeImmutable|false)>.
What do you have in mind with "nested BenevolentUnionType", I'll add new tests.
There was a problem hiding this comment.
I guess things like (float|int)|string
VincentLanglet
force-pushed
the
benevolentUnion
branch
from
706c831 to
f92afc7
Compare
|
I think it's fine as it is. Please also send a documentation update to https://phpstan.org/config-reference (https://github.com/phpstan/phpstan/edit/1.9.x/website/src/config-reference.md). Thank you. |
Sure, I started phpstan/phpstan#8234 |
Closes phpstan/phpstan#8223