Fix "Offset X might not exist on..." in UnionTypeTest/IntersectionTypeTest #4502
Conversation
tests/PHPStan/Type/UnionTypeTest.php
Outdated
| ]); | ||
| if ( | ||
| !isset($unionTypeA->getTypes()[0]) | ||
| || !isset($unionTypeA->getTypes()[1]) |
There was a problem hiding this comment.
This doesn't get reported with reportPossiblyNonexistentConstantArrayOffset: true, right? I'm not sure why we'd need these conditions :)
There was a problem hiding this comment.
see
------ -------------------------------------------------------
Line UnionTypeTest.php
------ -------------------------------------------------------
186 Offset 0 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:186
192 Offset 1 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:192
272 Offset 0 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:272
278 Offset 1 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:278
501 Offset 0 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:501
507 Offset 1 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:507
594 Offset 0 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:594
600 Offset 1 might not exist on array<PHPStan\Type\Type>.
🪪 offsetAccess.notFound
at tests/PHPStan/Type/UnionTypeTest.php:600
------ -------------------------------------------------------
There was a problem hiding this comment.
When do you see this? What config change causes it?
There was a problem hiding this comment.
reportPossiblyNonexistentGeneralArrayOffset: true
There was a problem hiding this comment.
Oh, that's not surprising. But I don't know if I ever want to enable that, because of needing to write code like that. It's not a very big difference whether we get "internal error" or "access on an undefined offset" notice when someone runs that code path. It's just that we need more code for the former.
How many errors are currently remaining when you enable that option?
There was a problem hiding this comment.
For similar reasons I don't plan to increase to level 9/10. Not enough benefits...
There was a problem hiding this comment.
I am currently looking into how easily we can get the count down.
I started with ~1500 and after open PRs are merged we are at ~1000
We might e.g. ignore this error in tests/ but not in src/ or similar
There was a problem hiding this comment.
The improvements are useful if they don't make the code worse but better.
Also some situations might inspire us to improve the type inference.
But to add a bunch of ShouldNotHappen throws just so we can enable the option isn't great.
Also in tests it's often easier to fix by adding an assert which is okay in tests ("we assume this") but not in src/ where the assumption might actually be wrong.
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
I'd much rather refactor this by saving IntegerType into a variable and then reference it in both yields. Instead of these issets and throws.
|
done |
|
Thank you! |
2 participants
fixes 10 errors