CONSTANT_SCALAR_UNION_THRESHOLD makes phpDoc enums unreliable

[vojtech-dobes]

Bug report

Specifying type to be from specific enumeration based on class constants is extremely useful feature. But current limitation to 8 items only can also lead to false negative rendering the feature unreliable and rather dangerous.

Code snippet that reproduces the problem

https://phpstan.org/r/d08f913e-0ad8-4fd6-9b26-fba163dddd6a

Expected output

Error detected:

Parameter #1 $arg of function run expects 1|2|3|4|5|6|7|8|9, 100 given.

Solution

Do you have vision on how this should be further improved? I can imagine:

• making the limit configurable/optional
• making the limit configurable in phpDoc directly
• not allowing such phpDoc if the limit gets hit

I think each of these solutions would allow user to depend on this feature to work reliably.

[ondrejmirtes]

PHPStan is about 33% slower if we remove this limit. We could do some performance improvements and benchmarking so that CONSTANT_SCALAR_UNION_THRESHOLD isn't needed at all.

[ondrejmirtes]

BTW this scenario could be solved by changing the union 1|2|3|4|5|6|7|8|9 to an IntegerRangeType(1, 9) :)

[Firehed]

I'm sure this is no surprise, but the same thing occurs when using strings rather than integers: https://phpstan.org/r/14792d1b-caba-4453-bb2f-5dfdd305cc40. That particular example is quite contrived; in my actual codebase I've got a list of 40-odd strings that correspond to edge labels in a graph database, so something along the lines of IntegerRangeType isn't really possible. The whole thing should be an enum, but in the best case that might become available in PHP 8.1.

Unfortunately I didn't discover this limitation until the very end of a gigantic refactor (replacing all hardcoded strings with constants in the codebase), which negated the majority of the change :( Oh well, these things happen.

I'd be happy to investigate some alternative implementations to address the performance concerns, though I'll admit I'm not entirely sure where to begin (UnionType::accepts looks relevant?). If someone can point me in the right direction I can dig around. Alternatively, emitting warnings when types get erased/generalized would at least give a heads-up before getting too far along in a refactor. This general pattern is used fairly heavily in Typescript with discriminated unions, so there should be a reasonable path to making a performant implementation.

[ondrejmirtes]

The place in question is TypeCombinator::union() where the constant is referenced.

Another issue related to this is #3530.

[danielmarschall]

If performance is the only reason why CONSTANT_SCALAR_UNION_THRESHOLD exists, why don't you do the normal check (with the threshold) and if phpstan detects that the limit is reached (and therefore the verification will fail), re-do the check of that input-file without limit? This way only a few input files would be slower in processing while all other checks are fast.

[ondrejmirtes]

I managed to solve this.

* Removed CONSTANT_SCALAR_UNION_THRESHOLD: phpstan/phpstan-src@c56d866

• Optimization of TypeCombinator::union() when a lot of scalars are involved: phpstan/phpstan-src@da9e061

Look forward to the next release (0.12.95) :)

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.