InvalidArgument doesn't work for callable args count

[kkmuffme]

https://psalm.dev/r/1b69254ca2

It correctly complains about InvalidArgument when the type is wrong.
But it should also give an error when I only pass 1 param, but the callable expects 2

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/1b69254ca2

<?php

/**
 * @param callable(string, string):void $callback
 * @return void
 */
function caller($callback) {}

/**
 * @param int $bar
 * @return void
 */
function foo( $bar ) {}

caller('foo');

/**
 * @param string $bar
 * @return void
 */
function bar( $bar ) {}

caller('bar');

Psalm output (using commit 61ef140):

ERROR: InvalidArgument - 15:8 - Argument 1 of caller expects callable(string, string):void, impure-callable(int):void provided

[vudaltsov]

I don't agree that this should have been fixed. It is valid to pass a callable with fewer parameters.

[vudaltsov]

See https://3v4l.org/bCe3Q.

[vudaltsov]

It's wrong only when passing a built-in function https://3v4l.org/GT86Q.

[orklah]

I'm open to arguments on that, but saying that php doesn't crash is not the most convincing.

Do you have use cases that require you to call the callback with more param than it requires?

Then it's a balance between:

• I'm a user that wants to call a callable with more params than it declares
• I'm a user that wants to be warned when I make a mistake by passing a callback that receives less than I passed

[kkmuffme]

The fix makes it consistent with psalm default behavior of TooFewArguments/TooManyArguments https://psalm.dev/r/748342d72e

Additionally, for me this was mostly an issue of stubs, where we only have the phpdoc & function declaration, not any code inside the function. Therefore it relies on the phpdoc.

Feel free to add a PR though, that fixes the behavior in cases where you pass x number of arguments in the callback, therefore overriding the phpdoc/stubs

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/748342d72e

<?php

/**
 * @param string $a
 * @param string $b
 * @return void
 */
function foo( $a, $b ) {}

foo( 'a', 'b', 'c' );
foo( 'a' );

Psalm output (using commit 1986c8b):

ERROR: TooManyArguments - 10:1 - Too many arguments for foo - expecting 2 but saw 3

ERROR: TooFewArguments - 11:1 - Too few arguments for foo - expecting b to be passed

[weirdan]

Do you have use cases that require you to call the callback with more param than it requires?

Yes, it's very common. E.g. a lot of methods accepting Closure(K,V) here: https://github.com/doctrine/collections/blob/2.0.x/src/ArrayCollection.php can actually be called with closures accepting K only if we don't care about the values.

[orklah]

Not sure how to proceed then. Should we keep it to help user find legit mistakes or should we remove the check for functions outside Callmap not to bother more advanced usages? Maybe a config?

[kkmuffme]

Definitely want to keep it as is, as it's super helpful for us :-)

[danog]

+1 for keeping it as it is, you can always provide an alternative callable in a union if needed: (callable(A, B): int)|(callable(A): int)

[vudaltsov]

you can always provide an alternative callable

No, you can't, because it will cause a TooManyArguments error: https://psalm.dev/r/b5116d9026.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/b5116d9026

<?php

/**
 * @param (callable(string, string): void)|(callable(string): void) $callback
 */
function caller(callable $callback): void
{
    $callback('a', 'b');
}

Psalm output (using commit 4932337):

ERROR: TooManyArguments - 8:5 - Too many arguments for  - expecting 1 but saw 2

[vudaltsov]

Definitely want to keep it as is, as it's super helpful for us :-)

How is it helpful if it's absolutely legal to pass a non-native function with less parameters?

[orklah]

@vudaltsov Would a new CallableTooManyArguments emitted especially for those cases that you could suppress for your whole project work for you?

[weirdan]

That would work I suppose. Or an alternative syntax (e.g. strict-callable()), but I would rather avoid that as it will result in a combinatorial explosion if we go that way (strict-pure-callable etc).

[danog]

Ooo, I love the strict-callable idea :D

[vudaltsov]

I don't think that strict-callable is a good idea, because it moves the responsibility towards the one who designs the contract, while the actual problem belongs to the user of the contract.

The one who defines the contract does not really care about the number of parameters, because it does not affect the result in any way. The user can always pass fn (string $a, string $b) => functionWithOneParameter($a) and that immediately converts "strict" to "loose", so there's nothing we are really checking here. As a result nobody will ever use strict-callable, because it does not improve the contract in any way.

I think that CallableTooManyArguments is a good compromise.