Reproduce a weird bug with closures

[canvural]

Hi 👋🏽

Since some time (actually over a a year or two) there is a bug that effects Larastan. Some of the reported ones I could find:

• When() method expects callable Builder with specific model but Model given larastan/larastan#1214
• When method do not supports clousure with whereIn expression larastan/larastan#1937
• Wrong return type on whereRaw larastan/larastan#1956

And I and other people always suggested to remove the typehint inside the closure. Because that made the error go away, and PHPStan still did know the type.

But when I tried to reproduce it inside the PHPStan I couldn't do it. Until a couple days ago! So I reproduced the failing test, and started looking into the possible fix. But no luck 🫤

Here is a sample code:

SubModel::getBuilder()->methodWithCallback(function (Builder $builder, $value) {
	return $builder->someMethod();
});

Couple of things need to happen for this bug to happen:

• The parameter of the closure needs to be generic.
• The method call inside the closure (someMethod) needs to come from an extension. If someMethod was native method of Builder, bug doesn't happen.

I step debugged it and my current understanding is that the closure body itself is first analyzed by ClosureReturnTypeRule In this case there is no generic information for Builder $builder for PHPStan to infer, so it gets the upper bound Model

Later in the analysis the MethodParameterClosureTypeExtension kicks in, replaces the closure with correct parameter type. Then it knows the real return type.

Side note: MethodParameterClosureTypeExtension does not cause this. The linked issues have the problem without that extension, but I just couldn't reproduce it here.

So this is where I got so far and kinda blocked. Any clues where I can look further more?

[canvural]

Tests seem to pass in CI 🤯 But locally it fails

[ondrejmirtes]

This is exactly what I need to ruin my weekend thinking about this, thanks 😀 /jk of course 😊

[canvural]

This is exactly what I need to ruin my weekend thinking about this, thanks 😀 /jk of course 😊

heheh. It's a challenging one.

One thing I just now discovered, I wasn't running the test locally 🤦🏽 I was using ./bin/phpstan analyse -c tests/PHPStan/Analyser/weird-bug-config.neon tests/PHPStan/Analyser/data/weird-bug.php --debug -vvv -l9 to test it. I could move the test file to som e RuleTestCase test, but I think it's still possible to reproduce it if you check out the PR code.

[ondrejmirtes]

To be clear: You don't want this error to happen:

Anonymous function should return WeirdBug\Builder<WeirdBug\Model> but returns WeirdBug\Builder<WeirdBug\SubModel>.

Because the return type of the closure shouldn't be limited, right? Because MethodParameterClosureTypeExtension returns new CallableType but doesn't say what the return type should be, which falls back to MixedType.

[ondrejmirtes]

I guess this is the same bug as these that popped after that bugfix I did in Verona:

• Closure required to no return statement phpstan#11099
• Template name is wrongly propagated to the type phpstan#11079

The problem is that we need as precise Closure return type as possible for generics for example. If we have callable(): T as a parameter and then use T in @return, we really want T to be inferred precisely.

But we don't need the exact type for example in ClosureReturnTypeRule or FunctionCallParametersCheck. Only the type required by involved signatures should be enforced there.

The fact that this is enforced inconsistently is another issue that should be addressed.

[canvural]

To be clear: You don't want this error to happen:

Anonymous function should return WeirdBug\Builder<WeirdBug\Model> but returns WeirdBug\Builder<WeirdBug\SubModel>.

Because the return type of the closure shouldn't be limited, right? Because MethodParameterClosureTypeExtension returns new CallableType but doesn't say what the return type should be, which falls back to MixedType.

In this case, yes. It should respect the return type of the closure. Also if I add a return type to the closure in MethodParameterClosureTypeExtension, that should be respected.

I guess this is the same bug as these that popped after that bugfix I did in Verona:

Possible. But like I said this error was happening a year ago too. But maybe that bugfix made it become more clear.

[canvural]

Just realized I hit this bug again, while debugging an issue. Trying to understand what is going on, and what can be the potential solution also gave me a headache 😅

Would be cool if anyone can gave me a direction to look at least.

[canvural]

@ondrejmirtes I pushed a possible fix for this. My thinking was that we should compare the return type of the closure with the return type of the callable the closure is passed into. This fixes the test case I created and does not fail any other test case locally 🤞🏽

[ondrejmirtes]

Hi, I've thought about this but I think we should make sure that $scope->getAnonymousFunctionReturnType() already has the correct information. I thought that instead of your changes in src/, this would achieve it:

diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php
index 0e7b7c328..0cdaf854b 100644
--- a/src/Analyser/MutatingScope.php
+++ b/src/Analyser/MutatingScope.php
@@ -1242,6 +1242,7 @@ final class MutatingScope implements Scope
 
 			$callableParameters = null;
 			$arrayMapArgs = $node->getAttribute(ArrayMapArgVisitor::ATTRIBUTE_NAME);
+			$inParameter = null;
 			if ($arrayMapArgs !== null) {
 				$callableParameters = [];
 				foreach ($arrayMapArgs as $funcCallArg) {
@@ -1522,6 +1523,17 @@ final class MutatingScope implements Scope
 				}
 			}
 
+			if (
+				$inParameter !== null
+				&& !$inParameter->getType()->isCallable()->no()
+			) {
+				$inParameterCallableReturnTypes = [];
+				foreach ($inParameter->getType()->getCallableParametersAcceptors($this) as $callableParametersAcceptor) {
+					$inParameterCallableReturnTypes[] = $callableParametersAcceptor->getReturnType();
+				}
+				$returnType = self::intersectButNotNever($returnType, TypeCombinator::union(...$inParameterCallableReturnTypes));
+			}
+
 			foreach ($parameters as $parameter) {
 				if ($parameter->passedByReference()->no()) {
 					continue;

But it doesn't because for some reason, $callableParametersAcceptor->getReturnType() is mixed.

Also, there's a problem in always trusting the "passedToType callable's return type" because it might be incompatible with the native return type of the closure. Which I also tried to solve ($returnType already contains the native derived type).

[canvural]

but I think we should make sure that $scope->getAnonymousFunctionReturnType() already has the correct information

But that would cause unexpected issues I think. When analyzing the closure body we need precise types, but only for the ClosureReturnTypeRule we are not interested in precise type. So we need to make this distinction somewhere.

because it might be incompatible with the native return type of the closure

Other rules like argument.type would catch those, no? No other test failed currently also. So I'm guessing it should be fine.

[canvural]

@ondrejmirtes Is there no way the current solution can be accepted? If not, I can start digging into returning correct type from $scope->getAnonymousFunctionReturnType() already and maybe try to build on your attempt.

[ondrejmirtes]

@canvural I must admin I don't understand the problem entirely. I'm not sure what's the right type to return from getAnonymousFunctionReturnType and why would only this one specific rule be interested in a different answer. It just seemed fishy to me and I think it should be fixed on a different layer so that we don't need to fix one specific rule, but everyone will get the same value.

[canvural]

@ondrejmirtes From my understanding the issue comes from analyzing the closure at different points. That inconsistency needs to be fixed I guess somehow. Closure is analyzed when it's an argument of a method/function call, and at this point some extensions like the *ParameterClosureTypeExtension are also taken into account. But then later when we want to get the type of the closure, the closure itself is analyzed in isolation kinda, and the extensions like *ParameterClosureTypeExtension are not taken into account.

It's also possible I did not understand how stuff works internally. I'll try to look into it again later.