Call to static method Webmozart\Assert\Assert::allCount() with array and 2 will always evaluate to true.

[VincentLanglet]

I bumped from 1.0.9 to 1.1.2 ;
I assume it's related to 56d7b27 @herndlm

class Foo
{
    /** @var array<int, array{id: string, num_order: string}> */
    protected array $orders = [];

    public function setOrders(array $orders): self
    {
        Assert::allCount($orders, 2);
        Assert::allKeyExists($orders, 'id');
        Assert::allKeyExists($orders, 'num_order');

        $this->orders = $orders;

        return $this;
    }
}

[herndlm]

I'm on it, git bisect is telling me that

• it was not erroring until a00dad3, which makes sense because count was just not supported until then
• then it started erroring with Call to static method Webmozart\Assert\Assert::allCount() with array and 2 will always evaluate to false. which also makes sense I guess, because phpstorm was not correctly figuring out types for the count part
• but interestingly the behaviour did not change with the count fix that was done in 586d049. most likely because the way the all* assertions work is a bit weird
• and then, as you mentioned, it started behaving differently since 56d7b27 and erroring with Call to static method Webmozart\Assert\Assert::allCount() with array and 2 will always evaluate to true.

To me it looks like it never worked correctly, at least since count was supported, am I right? With 1.0.9 it errors with false instead of true, which is also wrong, right?

I'll continue debugging the resulting types in the meantime, I assume there is something lost on the way..

[herndlm]

OK, I know already what the problem is. The expression is good and PHPStan can figure out types for it

But via arrayOrIterable we basically overwrite the SpecifiedTypes and only use what we got for the arg, $orders in this case. So the additional array_filter type is lost. And that, of course, leads to the error reported here in ImpossibleCheckTypeStaticMethodCallRule.

I can think of 2 adaptions right now:

1. try to preserve the additional types somehow via arrayOrIterable
2. enable overwrite via SpecifiedTypes in arrayOrIterable

I don't really like both solutions, the second is simple, but feels wrong and leads to other things not being reported I guess. The first one feels more correct but might be complicated, I have to check :)
What do you think, based on these findings, @ondrejmirtes?
I'm going to look into 2. in the meantime, maybe it's easier than I think..

[VincentLanglet]

To me it looks like it never worked correctly, at least since count was supported, am I right? With 1.0.9 it errors with false instead of true, which is also wrong, right?

I'm using phpstan-strict-rules, so always-false and always-true conditions are reported ; and I had no errors before (with 1.0.9). Weirdly.

OK, I know already what the problem is.

👍

[herndlm]

Oh boy, this is complicated..
I tried to preserve the other types via unionizing SpecifiedTypes but this lead to 2 problems

• my array_filter expression thingy creates an array type for args that are mixed which is more specific than the iterable we create in arrayOrIterable, see https://github.com/phpstan/phpstan-webmozart-assert/blob/1.1.2/tests/Type/WebMozartAssert/data/collection.php#L17 this would turn into array then which is not correct
• the additional array_filter type information makes https://github.com/phpstan/phpstan-webmozart-assert/blob/1.1.2/tests/Type/WebMozartAssert/data/impossible-check.php#L26 and therefore presumable all all* assertions not error anymore 🤦‍♂️

maybe I should wait for the ImpossibleCheckTypeHelper improvements Ondrej had in mind that should solve related non-empty-string issues. Do you have a rough plan for that Ondrej, is it something worth I could look into too?

[ondrejmirtes]

My idea for ImpossibleCheckTypeHelper is the following:

1. Some method type-specifying extensions simply delegate to TypeSpecifier by constructing an expression and passing it to specifyTypesInCondition.
2. What we could do is somehow preserve this "root expression" in SpecifiedTypes and pass it along.
3. When the root expression is present in SpecifiedTypes, ImpossibleCheckTypeHelper should simply evaluate $scope->getType($rootExpr) and return true/false if the type is ConstantBooleanType.
4. For SpecifiedTypes without root expression preserved, e.g. cases where TypeSpecifier::create() is called directly, the original logic in ImpossibleCheckTypeHelper would be called.

With this logic implemented, some issues should go away. What needs to be solved additionally to have more bugs fixed, is to realize what's happening with current false positives related to ImpossibleCheckTypeHelper.

For example - this PR phpstan/phpstan-src#1068 or PRs for ipv4 and similar are narrowing down the argument type to non-empty-string. But these functions aren't just about specifying a non-empty-string, they're specifying non-empty-string + some other criteria that we can't express.

But when ImpossibleCheckTypeHelper sees that a non-empty-string is narrowed down to non-empty-string, it considers it to be "always true". Implementing 1)-4) by itself doesn't solve this, but I think that once we have 1)-4) landed in phpstan-src, these extensions for str_contains and ipv4 can be implemented by supplying this expression to specifyTypesInCondition:

$str !== '' && SOME_FAUX_FUNCTION($str)

I believe this would still narrow down the type of $str to non-empty-string, but would not report false positives about non-empty-string being already non-empty-string.

Anyway, I already sinked too much time into TypeSpecifier and ImpossibleCheckTypeHelper over the last month, and I want to take care of other things too, so feel free to try this idea yourself :)

[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.