ConstantArrayTypeBuilder: Raise ARRAY_COUNT_LIMIT to 512 #4856
Conversation
|
//cc @gnutix please try the .phar file from the build artifacts of this PR and check it on your problematic codebase from phpstan/phpstan#8636 |
|
This pull request has been marked as ready for review. |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
I feel like phpstan/phpstan#8636 (comment) would be a great way to solve such issue rather than updating the limit for every projects
| ]; | ||
| } | ||
|
|
||
| assertType('int<0, max>', count($alerts)); |
There was a problem hiding this comment.
Most of those tests were about oversizedArray.
Like this one phpstan/phpstan#11703 reporting an oversized array was considered as always non-empty.
With this "test update", the regression is not covered anymore since we are not testing oversizedArray anymore but just big constantArray.
If you change ARRAY_COUNT_LIMIT, IMHO all the oversized array in the tests need to be updated to stay oversized array.
Which is some works...
There was a problem hiding this comment.
I guess I should adjust that some tests still use the 256 limit
There was a problem hiding this comment.
I guess I should adjust that some tests still use the 256 limit
That could be a great way indeed ;
And if the limit is configurable for tests (only cf #4856 (comment) indeed) it could give us easier test writing of oversize array by lowering the limit when needed (?)
see #4850 (comment) |
| $container->getService('typeSpecifier'); | ||
|
|
||
| BleedingEdgeToggle::setBleedingEdge($container->getParameter('featureToggles')['bleedingEdge']); | ||
| ConstantArrayTypeBuilder::setArrayCountLimit(256); |
There was a problem hiding this comment.
Should it be the opposite ?
Letting 512 as default, and manually calling setArrayCountLimit(256) for test which need it ?
Maybe it's because I'm not familiar with this code but setting 256 in postInitializeContainer won't also set it for user of phpstan (and not only for tests) ?
|
tried a few different things but was not yet successful to make the setting have a different value for certain test cases, as I don't want to introduce a new container parameter (which would allow people to define the setting at app level). |
|
I ran benchmarks on our codebase (~4min total PHPStan runtime) by patching the ARRAY_COUNT_LIMIT constant directly in the 2.1.38 PHAR. Cold cache for each run.
Single run each (no iterations), so variance across rows is just noise. The totals are essentially identical — no measurable performance impact from doubling or even quadrupling the limit on this codebase. Given that there's no performance regression (or at least measurable on this codebase, I know), would it make sense to remove the limit entirely? or make it a configurable parameter? I can't say I understand the strategy of "just doubling it", it feels arbitrary if we can't measure differences in benchmarks. |
4 participants
closes phpstan/phpstan#8636
running
make testsbefore and after this PR results in ~30 seconds runtime on my machine, so it seems we are fast enough to allow bigger arraysI changed the
tests/PHPStan/Analyser/data/bug-5081.phptest to accomodate 1 element more as we allow.all other tests have just been adjusted to reflect the new reality.
tests/PHPStan/Rules/Methods/data/bug-8636.phpreportedMethod Config::huge() should return string but returns array<int, string>|string.before this PR and does not longer after this PR.