Fix usage of context->truthy

[VincentLanglet]

Closes phpstan/phpstan#3013
Closes phpstan/phpstan#7686

I tried to work on this issue to understand more TypeSpecifyingExtension,
I discovered like explained in phpstan/phpstan#3013 (comment) that we don't get the same behavior with

if (in_array($bar, $foo, true) === true)

than with

if (in_array($bar, $foo, true))

I started a PR to solve this, it seems like replacing $context->truthy() by $context->true() solve the issue.
It's because we get

if (in_array($bar, $foo, true)) (
     $context->true();
     $context->truthy();
} else {
     $context->false();
     $context->falsey();
}

but

if (in_array($bar, $foo, true) === true) (
     $context->true();
     $context->truthy();
} else {
     $context->truthy();
     $context->false();
     $context->falsey();
}

Then I started to look at another type specifying extensions I worked on, the array_key_exists one, and I was able to reproduce the same issue by using array_key_exists($foo, $bar) === true). And again I solved it by changing $context->truthy() by $context->true().

There is 8 others TypeSpecifyingExtensions with $context->truthy() about method which can only return false or true:

• ClassExistsFunctionTypeSpecifyingExtension
• DefinedConstantTypeSpecifyingExtension
• FunctionExistsFunctionTypeSpecifyingExtension
• IsAFunctionTypeSpecifyingExtension
• IsSubclassOfFunctionTypeSpecifyingExtension
• MethodExistsTypeSpecifyingExtension
• PropertyExistsTypeSpecifyingExtension
• StrContainingTypeSpecifyingExtension

And I wonder if there is not the same issue for them.
(Also there is 7 truthy calls (and 3 falsey calls) in the TypeSpecifier and I don't know if similar issues can occur in it.)

Since I don't really know if I'm going in the right direction, I'd like your opinion @ondrejmirtes
(cc @staabm if you can help me)

[herndlm]

This feels weird, shouldn't truthy include the true context in such cases? What I mean: truthy is kind of like a super type of true and should the fix be maybe somewhere else?

[staabm]

I have the same feeling as @herndlm

[ondrejmirtes]

I've long suspected it's not intuitive and that it's actually switched. Would be nice to have some unit tests for TypeSpecifierContext to verify that.

[VincentLanglet]

This feels weird, shouldn't truthy include the true context in such cases? What I mean: truthy is kind of like a super type of true

Indeed, truthy is a super type type of true @herndlm.

But, let's say in_array could return true, false, 1 or 0.
We have

if (in_array($bar, $foo, true)) (
     // `true` possible
     // `1` possible
     // so context is 0011, therefore true and truthy
} else {
     // `false` possible
     // `0` possible
     // so context is 1100, therefore false and falsey
}

and

if (in_array($bar, $foo, true) === true) (
     // `true` possible
     // so context is 0001, therefore true
} else {
     // `1` possible
     // `false` possible
     // `0` possible
     // so context is 1110 therefore truthy, false and falsey
}

or

if (in_array($bar, $foo, true) === false) (
     // `false` possible
     // so context is 0100, therefore false
} else {
     // `1` possible
     // `true` possible
     // `0` possible
     // so context is 1011 therefore falsey, true and truthy
}

and should the fix be maybe somewhere else?

This would be a great way, but I'm not sure how.

The fact is context->truthy and context->falsey doesn't make a lot of sens for method which only returns true or false values and not truthy/falsey values. So maybe PHPStan should have a way to filter out those context.

Would be nice to have some unit tests for TypeSpecifierContext to verify that.

Sure, but is there a way to write php code and assert the context inside ?
Is it possible to implement a assertContext method in the same way that

if ($function->getName() === 'PHPStan\\Testing\\assertType') {
     return $this->processAssertType($node->getArgs(), $scope);
}

for instance ?

[VincentLanglet]

There is a return null if true === array_key_exists('host', $redirectUrlParts) so the key shouldn't exist anymore.

[VincentLanglet]

I edited all my example, because I made a little mistake. The error is in the fact that when doing

=== true

check, both path (if and else) are truthy.

[VincentLanglet]

Thinking more about this, I'm hesitating between two point of view about the context:

1. The current context behavior is valid:

if (in_array($bar, $foo, true) === true) (
     // `true` possible
     // so context is 0001, therefore true
} else {
     // `false` possible
     // `truthy not true` possible
     // `falsy` possible
     // so context is 1110, therefore truthy, false and falsey
}

Because it reports the context without knowing anything about the return values of the function.

Therefore, it the type specifying method to care about. For example changing

public function isFunctionSupported(FunctionReflection $functionReflection, FuncCall $node, TypeSpecifierContext $context): bool
	{
		return strtolower($functionReflection->getName()) === 'in_array'
			&& !$context->null();
	}

to

public function isFunctionSupported(FunctionReflection $functionReflection, FuncCall $node, TypeSpecifierContext $context): bool
	{
		return strtolower($functionReflection->getName()) === 'in_array'
			&& ($context->false() || $context->true());
	}

2. Le context should at sometime care about the return type values of the function, so we'll get

if (in_array($bar, $foo, true) === true) (
     // `true` possible
     // so context is 0001, therefore true
} else {
     // `false` possible
     // so context is 0100, therefore false
}
// truthy not true and falsey not false are impossible

at some point when working on the context.

The more I think about it and the more I think the (1) solution is the right one, it's just that the right context check must be used. So if you agree with me @ondrejmirtes, I'll keep doing this kind of fix for all the extensions

[phpstan-bot]

This pull request has been marked as ready for review.

[VincentLanglet]

I have made all the truthy->true changes in typeSpecifyingFunction about function which only returns boolean.

I tried to see if it fix some bugs and if so, I added test. So far it fixes for

• in_array
• array_key_exists
• defined

I tried to reproduce the same potential bugs for others functions with type specifying function but wasn't able to find one for them (like is_a, is_array, method_exists, ...). I don't know why such bug existed for some functions and not others.

With my level of understanding of context/typeSpecifier, I'm not sure to be able to do more.

[ondrejmirtes]

It might have also fixed phpstan/phpstan#7686, please look into that. Thank you.

[VincentLanglet]

It might have also fixed phpstan/phpstan#7686, please look into that. Thank you.

Indeed, it's also solved. I added a non regression test.

[staabm]

Awesome 👏

[ondrejmirtes]

I'll keep doing this kind of fix for all the extensions

Yes, please do, I'd really appreciate it!

[ondrejmirtes]

Thank you!