phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

add callback types for array_uintersect etc

Open schlndh opened this issue 1 year ago • 5 comments

This is a fixed version of https://github.com/phpstan/phpstan-src/pull/1571 (TK of array-key as you requested). Fixes https://github.com/phpstan/phpstan/issues/7707

Unfortunately, it doesn't cover all variants due to the limitations of the stubs and the weird interface of the functions. E.g.

var_dump(array_uintersect(
	['a' => 1, 'b' => 2],
	['c' => 1, 'd' => 2],
	['c' => 1, 'd' => 2],
	['c' => 1, 'd' => 2],
	// values comparison
	static function (int $a, int $b): int {
		return $a <=> $b;
	}
));

results in:

Parameter #5 ...$rest of function array_uintersect expects array|(callable(mixed, mixed): int), Closure(int, int): int<-1, 1> given.
    💡 • Type #2 from the union: Type int of parameter #1 $a of passed callable needs to be same or wider than parameter type mixed of accepting callable.
• Type #2 from the union: Type int of parameter #2 $b of passed callable needs to be same or wider than parameter type mixed of accepting callable.

But at least it's a step in the right direction.

schlndh avatar Aug 02 '24 11:08 schlndh

Please try to open a separate PR with an alternative approach:

  1. Try writing FunctionParameterClosureTypeExtension for one of these functions instead of introducing generics into their stubs (we have PregReplaceCallbackClosureTypeExtension in the repo so you can see how it works)
  2. You can shuffle the signature parameters in ParametersAcceptorSelector so that the number of parameters will match the number of passed arrays and so that the last parameter/argument always has the correct name (so that the FunctionParameterClosureTypeExtension can simply ask about the $value_compare_func parameter and adjust its callable signature.

In theory this should get rid of all false positives. If this alternative PR is not successful then I'll merge this one.

ondrejmirtes avatar Aug 03 '24 09:08 ondrejmirtes

@ondrejmirtes ~It turns out that neither approach works consistently out of the box.~

It seems that FunctionParameterClosureTypeExtension only narrows down the type inside the closure. It doesn't narrow it down for the purpose of CallFunctionParametersRule: https://phpstan.org/r/8ff9bc8f-4162-4ccf-bf5c-8b3d2061e68d

EDIT: I accidentally tested it without bleeding edge. So the rest of the comment is not true: https://phpstan.org/r/1bff567d-d9a1-4906-9fe0-83705f7854ab

~On the other hand, the reflection based approaches (this and V2) don't (sufficiently) narrow down the parameters inside of the closure. E.g.~

<?php namespace ParamsArrayIntersectUassoc;

use function PHPStan\Testing\assertType;

var_export(array_uintersect_assoc(
	['a' => 'a', 'b' => 'b'],
	['c' => 'c', 'd' => 'd'],
	function (mixed $a, mixed $b): int {
		assertType("'a'|'b'|'c'|'d'", $a);
		assertType("'a'|'b'|'c'|'d'", $b);

		return 1;
	},
));

Results in:

  9      Expected type 'a'|'b'|'c'|'d', actual: string  
  10     Expected type 'a'|'b'|'c'|'d', actual: string  

So I suppose that should be fixed first. I'll try to have a look (no promises).

schlndh avatar Aug 23 '24 11:08 schlndh

@ondrejmirtes I created a proof-of-concept PR which applies FunctionParameterClosureTypeExtension in FunctionCallParametersCheck. Please let me know whether this is a direction that you're comfortable with, so that I don't get too far down a wrong path again.

schlndh avatar Aug 25 '24 11:08 schlndh

@ondrejmirtes I had another look at this, but I'm not convinced that FunctionParameterClosureTypeExtension is the way to go:

  • selectFromArgs does not know the function name. So it's not possible to modify the signatures of these functions there (without further changes).
  • FunctionParameterClosureTypeExtension doesn't get the argument for which it's being called, so without receiving the hacked ParameterReflection it cannot do anything.

Would you be willing to move forward with the simple stub approach instead? I'd expect 2 array arguments to be the most common usage, so at least it's something (array_udiff is already handled this way).

Or perhaps even the "variadic in the middle" approach. Though you're right that it's very unexpected, so it could definitely lead to random bugs (I also forgot to look for other places which might need handling - e.g. NodeScopeResolver). On the other hand matching arguments to parameters is quite complicated in general, so probably not many people attempt to do that themselves.

schlndh avatar Aug 30 '24 11:08 schlndh

This pull request has been marked as ready for review.

phpstan-bot avatar Sep 13 '24 11:09 phpstan-bot

Thank you.

ondrejmirtes avatar May 05 '25 09:05 ondrejmirtes