psalm icon indicating copy to clipboard operation
psalm copied to clipboard

closure parameter are analysed without the function context ( generics ) when used with userland functions.

Open azjezz opened this issue 4 years ago • 23 comments

The following example shows the issue: https://psalm.dev/r/3e6b2451c3

both map and array_map act the same way, however, psalm complains about missing @param type declaration for the closure with user land implementations, but not builtin array_map.

This results in useless type declaration being added ( see https://psalm.dev/r/15e391e947 ) to satisfy psalm.

azjezz avatar May 23 '21 19:05 azjezz

I found these snippets:

https://psalm.dev/r/3e6b2451c3
<?php

/**
 * @template T
 * @template Ts
 * @param list<T> $a
 * @param callable(T): Ts $c
 * @return list<Ts>
 */
function map(array $a, callable $c): array {
  $res = [];  
  foreach($a as $v) { $res[] = $c($v); }
  return $res;
}

/** @var list<array{a: string, b: int}> $input */

array_map(function(array $in): string {
    return  $in['a'];
}, $input);

map($input, function(array $in): string {
    return  $in['a'];
});
Psalm output (using commit 350df11):

INFO: PossiblyUndefinedStringArrayOffset - 23:13 - Possibly undefined array offset '"a"' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: MixedReturnStatement - 23:13 - Could not infer a return type

INFO: MixedInferredReturnType - 22:34 - Could not verify return type 'string' for /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php:22:373:-:closure
https://psalm.dev/r/15e391e947
<?php

/**
 * @template Tk
 * @template Tv
 * @template Ts
 * @param array<Tk, Tv> $a
 * @param callable(Tv): Ts $c
 * @return array<Tk, Ts>
 */
function map(array $a, callable $c): array {
  $res = [];  
  foreach($a as $k => $v) { $res[$k] = $c($v); }
  return $res;
}

/** @var list<array{a: string, b: int}> $input */

array_map(function(array $in): string {
    return  $in['a'];
}, $input);

map($input,
    /**
     * @param array{a: string, b: int} $in
     */
    function(array $in): string {
      return  $in['a'];
    }
 );
Psalm output (using commit 350df11):

No issues!

psalm-github-bot[bot] avatar May 23 '21 19:05 psalm-github-bot[bot]

https://psalm.dev/r/3ec04d7b36

azjezz avatar May 23 '21 19:05 azjezz

I found these snippets:

https://psalm.dev/r/3ec04d7b36
<?php

/**
 * @template Tk
 * @template Tv
 * @template Ts
 * @param array<Tk, Tv> $a
 * @param callable(Tv): Ts $c
 * @return array<Tk, Ts>
 */
function map(array $a, callable $c): array {
  $res = [];  
  foreach($a as $k => $v) { $res[$k] = $c($v); }
  return $res;
}

/** @var list<array{a: string, b: int}> $input */

array_map(function(array $in): string {
    return  $in['a'];
}, $input);

map($input, function(array $in): string {
    return  $in['a'];
});
Psalm output (using commit 350df11):

INFO: PossiblyUndefinedStringArrayOffset - 24:13 - Possibly undefined array offset '"a"' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: MixedReturnStatement - 24:13 - Could not infer a return type

INFO: MixedInferredReturnType - 23:34 - Could not verify return type 'string' for /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php:23:410:-:closure

psalm-github-bot[bot] avatar May 23 '21 19:05 psalm-github-bot[bot]

Psalm has some special behaviour for array_map because it knows the type of the first array is unpacked into the argument for the second.

Imagine that the params are flipped: https://psalm.dev/r/5db81b6045

Now Psalm must know to scan the second argument first to get the param type so it can then use the correct types on the first. This is infeasible (and Hack does not do this either AFAIK).

muglug avatar May 23 '21 20:05 muglug

I found these snippets:

https://psalm.dev/r/5db81b6045
<?php

/**
 * @template T
 * @template Ts
 * @param list<T> $a
 * @param callable(T): Ts $c
 * @return list<Ts>
 */
function map(callable $c, array $a): array {
  $res = [];  
  foreach($a as $v) { $res[] = $c($v); }
  return $res;
}

/** @var list<array{a: string, b: int}> $input */

array_map(function(array $in): string {
    return  $in['a'];
}, $input);

map(function(array $in): string {
    return  $in['a'];
}, $input);
Psalm output (using commit de1bb95):

INFO: PossiblyUndefinedStringArrayOffset - 23:13 - Possibly undefined array offset '"a"' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: MixedReturnStatement - 23:13 - Could not infer a return type

INFO: MixedInferredReturnType - 22:26 - Could not verify return type 'string' for /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php:22:365:-:closure

psalm-github-bot[bot] avatar May 23 '21 20:05 psalm-github-bot[bot]

This case should be handled at least for when the generic argument is provided before the closure.

currently a lot of projects i maintain contain these redundant parameters, and in most cases, the generic argument appears before the closure

Hack does not do this either AFAIK

it does!

image

azjezz avatar May 23 '21 20:05 azjezz

Hack also handles this when the arguments are flipped:

image

azjezz avatar May 23 '21 20:05 azjezz

Psalm has some special behaviour for array_map

can this special behavior be replicated for other functions using a plugin? i would like to do so in php-standard-library/psalm-plugin

azjezz avatar May 23 '21 21:05 azjezz

I stand very corrected!

muglug avatar May 23 '21 23:05 muglug

can this special behavior be replicated for other functions using a plugin

No, but once I figure out how Hack is doing it this will work in Psalm.

Given this code:

function maptwice<T1, T2, T3>((function(T2):T3) $c2, (function(T1):T2) $c1, vec<T1> $a): vec<T3> {
  $res = vec[];
  foreach($a as $v) { $res[] = $c2($c1($v)); }
  return $res;
}

function foo(vec<shape('a' => string, 'b' => int)> $input): vec<int> {
  return maptwice(
    ($in) ==> $in + 3,
    ($in) ==> $in['b'],
    $input
  );
}

The process could go like this:

  • Figure out how many times we'll need to scan the argument list by creating a map of templates: [T2 => T3, T1 => T2] implies a chain of length two, so argument list will have to be scanned three times
  • first two times we suppress errors (like we do for loops) while gathering better and better templates
  • the third time we turn errors back on, using the templating knowledge we built up in the previous two steps

muglug avatar May 23 '21 23:05 muglug

Equivalent PHP: https://psalm.dev/r/2767b16320

muglug avatar May 24 '21 00:05 muglug

I found these snippets:

https://psalm.dev/r/2767b16320
<?php

/**
 * @template T1
 * @template T2
 * @template T3
 * @param Closure(T2):T3 $c2
 * @param Closure(T1):T2 $c1
 * @param list<T1> $a
 * @return list<T3>
 */
function maptwice(Closure $c2, Closure $c1, array $a): array {
  	$res = [];
  	foreach($a as $v) { $res[] = $c2($c1($v)); }
  	return $res;
}

/**
 * @param list<array{a: string, b: int}> $input
 * @return array<int>
 */
function foo(array $input): array {
  	return maptwice(
    	fn($in) => $in + 3,
    	fn($in) => $in['b'],
    	$input
  	);
}
Psalm output (using commit 67413c8):

INFO: MixedOperand - 24:17 - Left operand cannot be mixed

INFO: MissingClosureParamType - 24:9 - Parameter $in has no provided type

INFO: MissingClosureReturnType - 24:6 - Closure does not have a return type, expecting mixed

INFO: MixedArrayAccess - 25:17 - Cannot access array value on mixed variable $in

INFO: MissingClosureParamType - 25:9 - Parameter $in has no provided type

INFO: MissingClosureReturnType - 25:6 - Closure does not have a return type, expecting mixed

INFO: MixedReturnTypeCoercion - 23:11 - The type 'list<mixed>' is more general than the declared return type 'array<array-key, int>' for foo

INFO: MixedReturnTypeCoercion - 20:12 - The declared return type 'array<array-key, int>' for foo is more specific than the inferred return type 'list<mixed>'

psalm-github-bot[bot] avatar May 24 '21 00:05 psalm-github-bot[bot]

@azjezz I believe this has been fixed, probably by a recent PR by @klimick . Can you confirm your use case is covered?

orklah avatar Jan 25 '22 17:01 orklah

No, the example provided by @muglug is still failing ( https://psalm.dev/r/90597ecafa ), so i think this problem still persists

azjezz avatar Jan 25 '22 19:01 azjezz

I found these snippets:

https://psalm.dev/r/90597ecafa
<?php

/**
 * @template T1
 * @template T2
 * @template T3
 * @param Closure(T2):T3 $c2
 * @param Closure(T1):T2 $c1
 * @param list<T1> $a
 * @return list<T3>
 */
function maptwice(Closure $c2, Closure $c1, array $a): array {
  	$res = [];
  	foreach($a as $v) { $res[] = $c2($c1($v)); }
  	return $res;
}

/**
 * @param list<array{a: string, b: int}> $input
 * @return array<int>
 */
function foo(array $input): array {
  	return maptwice(
    	static fn($in) => $in + 3,
    	static fn($in) => $in['b'],
    	$input
  	);
}
Psalm output (using commit 8ab0eec):

INFO: MixedOperand - 24:24 - Left operand cannot be mixed

INFO: MissingClosureReturnType - 24:6 - Closure does not have a return type, expecting mixed

INFO: MixedArrayAccess - 25:24 - Cannot access array value on mixed variable $in

INFO: MissingClosureReturnType - 25:6 - Closure does not have a return type, expecting mixed

INFO: MixedReturnTypeCoercion - 23:11 - The type 'list<mixed>' is more general than the declared return type 'array<array-key, int>' for foo

INFO: MixedReturnTypeCoercion - 20:12 - The declared return type 'array<array-key, int>' for foo is more specific than the inferred return type 'list<mixed>'

psalm-github-bot[bot] avatar Jan 25 '22 19:01 psalm-github-bot[bot]

@klimick is this expected? Will #7471 be able to solve that?

orklah avatar Jan 25 '22 19:01 orklah

@klimick is this expected? Will #7471 be able to solve that?

Expected. Inference of this kind is hard to implement for me. And functions with reverse arg ordering in PHP is rare. It is not worth implementing it in Psalm. In my opinion.

However, these rare cases can be covered with hook from #7471

klimick avatar Jan 25 '22 20:01 klimick

Great! Thanks for the diagnosis!

orklah avatar Jan 25 '22 21:01 orklah

It is not worth implementing it in Psalm. In my opinion.

I disagree, this problem currently results in the need of writing useless docblocks from the end user perspective to explain things to psalm.

As shown above, this is already supported by other type checkers such as hh ( hack ).

azjezz avatar Jan 25 '22 22:01 azjezz

The following example shows the issue: https://psalm.dev/r/3e6b2451c3

Your first example has no issues now. Psalm can infer callable args at now. But only in left to right order.

can this special behavior be replicated for other functions using a plugin?

Currently I work at new plugin hook here #7471.

writing useless docblocks

With new hook we will able to write plugin for function with weird params ordering. We'll can implement behavior like array_map has.

Look at test: https://github.com/klimick/psalm/blob/f7da5a8d55d5a0e8b15a6f28a914042750dd0aa7/tests/Config/PluginTest.php#L1053-L1074 custom_array_map similar to maptwice

klimick avatar Jan 25 '22 22:01 klimick

I found these snippets:

https://psalm.dev/r/3e6b2451c3
<?php

/**
 * @template T
 * @template Ts
 * @param list<T> $a
 * @param callable(T): Ts $c
 * @return list<Ts>
 */
function map(array $a, callable $c): array {
  $res = [];  
  foreach($a as $v) { $res[] = $c($v); }
  return $res;
}

/** @var list<array{a: string, b: int}> $input */

array_map(function(array $in): string {
    return  $in['a'];
}, $input);

map($input, function(array $in): string {
    return  $in['a'];
});
Psalm output (using commit 1cc9d1c):

No issues!

psalm-github-bot[bot] avatar Jan 25 '22 22:01 psalm-github-bot[bot]

Still doesn't work when the closure parameter is first.

Broken example: https://psalm.dev/r/16e9d5fb4f "Fixed" example: https://psalm.dev/r/28cded7a0d

Swapping the arguments is not always possible (e.g. varargs) or desirable.

PHPStan is fine with it as-is: https://phpstan.org/r/f7924a67-a216-4621-af5e-64ed76663816

shira-374 avatar Jun 09 '24 10:06 shira-374

I found these snippets:

https://psalm.dev/r/16e9d5fb4f
<?php

/**
 * @template T
 */
interface Example
{
    /**
     * @template TOther
     *
     * @param callable(T|TOther, T|TOther):int $comparator
     * @param Example<TOther> $other
     * @return Example<T>
     */
	function intersectUsing(callable $comparator, Example $other): Example;
}

/**
 * @param Example<array{name: string, value: int}> $first
 * @param Example<array{name: string, value: int}> $second
 */
function foo(Example $first, Example $second): void
{
    $first->intersectUsing(
        function (array $a, array $b) {
            /**
             * @psalm-check-type $a = array{name: string, value: int}
             * @psalm-check-type $b = array{name: string, value: int}
             */           
            return \strcmp($a['name'], $b['name']);
        },
        $second
    );
}
Psalm output (using commit 16b24bd):

INFO: PossiblyUndefinedStringArrayOffset - 30:28 - Possibly undefined array offset ''name'' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: PossiblyUndefinedStringArrayOffset - 30:40 - Possibly undefined array offset ''name'' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: MixedArgument - 30:28 - Argument 1 of strcmp cannot be mixed, expecting string

INFO: MixedArgument - 30:40 - Argument 2 of strcmp cannot be mixed, expecting string

ERROR: CheckType - 30:13 - Checked variable $a = array{name: string, value: int} does not match $a = array<array-key, mixed>

ERROR: CheckType - 30:13 - Checked variable $b = array{name: string, value: int} does not match $b = array<array-key, mixed>
https://psalm.dev/r/28cded7a0d
<?php

/**
 * @template T
 */
interface Example
{
    /**
     * @template TOther
     *
     * @param Example<TOther> $other
     * @param callable(T|TOther, T|TOther):int $comparator
     * @return Example<T>
     */
	function intersectUsing(Example $other, callable $comparator): Example;
}

/**
 * @param Example<array{name: string, value: int}> $first
 * @param Example<array{name: string, value: int}> $second
 */
function foo(Example $first, Example $second): void
{
    $first->intersectUsing(
        $second,
        function (array $a, array $b) {
            /**
             * @psalm-check-type $a = array{name: string, value: int}
             * @psalm-check-type $b = array{name: string, value: int}
             */           
            return \strcmp($a['name'], $b['name']);
        }
    );
}
Psalm output (using commit 16b24bd):

No issues!

psalm-github-bot[bot] avatar Jun 09 '24 10:06 psalm-github-bot[bot]