Specify non-empty-string for callables via `is_string()`

[herndlm]

Closes https://github.com/phpstan/phpstan/issues/8153

See https://github.com/phpstan/phpstan/issues/8153#issuecomment-1275992641 and the next comment for how I came to this. I'm still not a 100% sure though if this really belongs here. It does make sense to me and https://phpstan.org/r/2d18e426-7943-41fd-af29-4e94d14225fd shows that phpstan ensures that '' is not a callable.

[ondrejmirtes]

Yeah, I think it's the wrong place for sure.

This is where $s !== '' is handled for pure strings: https://github.com/phpstan/phpstan-src/blob/4b01b522c1e41b836677882ba738c05985358007/src/Type/StringType.php#L163-L174

We need for it to work when $s is callable&string too, meaning the logic in IntersectionType::tryRemove is probably wrong? Or maybe it's not even called?

[herndlm]

We need for it to work when $s is callable&string too, meaning the logic in IntersectionType::tryRemove is probably wrong? Or maybe it's not even called?

it's not called correctly at the moment. Because the whole expression '' !== $foo always evaluates to false the Identical specification is basically aborted in https://github.com/phpstan/phpstan-src/blob/1.8.8/src/Analyser/TypeSpecifier.php#L216 and '' is never specified as sureNotType for the callable type. It just ends up with a never as sureNotType which does not help :/

[herndlm]

but you're also right :) even if the type is specified, it doesn't work yet. so there are 2 things here. I'll try to tackle one by one..

[herndlm]

oh, actually the "removal does not work" is again related to the whole expression evaluating to a constant boolean. then TypeCombinator::remove() also early exits and doesn't touch the string type in https://github.com/phpstan/phpstan-src/blob/1.8.8/src/Type/TypeCombinator.php#L64

[herndlm]

everything starts working of course if (new ConstantStringType(''))->isSuperTypeOf(new CallableType()) answers with maybe, but that feels also very wrong, doesn't it? it should be no IMO. it will of course also lead to the whole expression resolving to a boolean instead of a constant boolean and therefore Strict comparison using !== between \'\' and callable(): mixed&non-empty-string will always evaluate to true. does not get reported anymore 🤔

[ondrejmirtes]

Super-interesting situation :) This should work too (arguably even callable&string should be accepted as non-empty-string): https://phpstan.org/r/3448a72a-dcb9-4736-aa83-c16d68f9acc8

This probably needs a hack in IntersectionType, maybe this?

public function isNonEmptyString(): TrinaryLogic
{
	if ($this->isCallable()->yes() && $this->isString()->yes()) {
		return TrinaryLogic::createYes();
	}   
	return $this->intersectResults(static fn (Type $type): TrinaryLogic => $type->isNonEmptyString());
}

[ondrejmirtes]

Have you seen my comment? The fix you're trying is different to what I suggested :)

[herndlm]

Have you seen my comment? The fix you're trying is different to what I suggested :)

sorry, race-condition. yeah, I'm already trying it out :)

[herndlm]

unfortunately your fix is not enough to make the type specifier + type removal work which is needed here I think :/

[ondrejmirtes]

Alright, so if I try my suggestion with the tests from this PR, I get these failures:

1) PHPStan\Rules\Comparison\StrictComparisonOfDifferentTypesRuleTest::testBug8153
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'
+'16: Strict comparison using !== between '' and callable(): mixed&string will always evaluate to true.
 '

/Users/ondrej/Development/phpstan/src/Testing/RuleTestCase.php:182
/Users/ondrej/Development/phpstan/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php:607

2) PHPStan\Rules\Methods\CallMethodsRuleTest::testBug8153
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'
+'18: Parameter #1 $foo of method Bug8153\HelloWorld::nonEmptyStringParameter() expects non-empty-string, callable(): mixed&string given.
 '

/Users/ondrej/Development/phpstan/src/Testing/RuleTestCase.php:182
/Users/ondrej/Development/phpstan/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php:2666

3) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/Users/ondrej/Development/phpstan/tests/PHPStan/Analyser/data/bug-8153.php:17" ('type', '/Users/ondrej/Development/php...53.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\IntersectionType Object (...), 17)
Expected type callable(): mixed&non-empty-string, got type callable(): mixed&string in /Users/ondrej/Development/phpstan/tests/PHPStan/Analyser/data/bug-8153.php on line 17.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'callable(): mixed&non-empty-string'
+'callable(): mixed&string'

/Users/ondrej/Development/phpstan/src/Testing/TypeInferenceTestCase.php:94
/Users/ondrej/Development/phpstan/tests/PHPStan/Analyser/NodeScopeResolverTest.php:1071

The only one I'm bothered by is 2) PHPStan\Rules\Methods\CallMethodsRuleTest::testBug8153, the rest is expected. This one is worth an investigation :)

[herndlm]

ah ok, got you. I was kind of focused also on the NodeScopeResolverTest for example. the thing is.. StrictComparisonOfDifferentTypesRules has, again, something like this that will not work here because the whole expression results in a constant boolean

		$nodeType = $scope->getType($node);
		if (!$nodeType instanceof ConstantBooleanType) {
			return [];
		}

it could be also adapted in a similar fashion to check left and right types I guess, but hmm

[ondrejmirtes]

That's totally fine, callable&string is already a non empty string, we don't need it to include AccessoryNonEmptyString 😊 The only part that bothers me is why it's not accepted as a non empty string - it should be.