Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive TooFewArguments with non-empty-list and spread operator #7603

Open
dsech opened this issue Feb 7, 2022 · 13 comments
Open

False positive TooFewArguments with non-empty-list and spread operator #7603

dsech opened this issue Feb 7, 2022 · 13 comments

Comments

@dsech
Copy link

dsech commented Feb 7, 2022

https://psalm.dev/r/09a02c2b5f

The issue exists in versions >=4.19.0

Looks to be related to the non-empty-list typehint, because there is no error reported if the variables are declared: https://psalm.dev/r/5f60f9ef1b

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/09a02c2b5f
<?php

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

func(...[...$existingItems, $newItem]);
Psalm output (using commit faad966):

ERROR: TooFewArguments - 12:1 - Too few arguments for func - expecting item2 to be passed
https://psalm.dev/r/5f60f9ef1b
<?php

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

$existingItems = ['item1'];
$newItem = 'item2';

func(...[...$existingItems, $newItem]);
Psalm output (using commit faad966):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Feb 7, 2022

Psalm doesn't have a concept of 'list with at least N elements, N > 1'. So inner splat results in non-empty-list<string> and that does not guarantee two elements required by the function: https://psalm.dev/r/3cdda0aa21

@psalm-github-bot
Copy link

I found these snippets:

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

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

$args = [...$existingItems, $newItem];
/** @psalm-trace $args */;
func(...$args);
Psalm output (using commit faad966):

INFO: Trace - 13:26 - $args: non-empty-list<string>

ERROR: TooFewArguments - 14:1 - Too few arguments for func - expecting item2 to be passed

@weirdan
Copy link
Collaborator

weirdan commented Feb 7, 2022

Basically, we can't do much here given the confines of the type system we currently have in Psalm.

@orklah
Copy link
Collaborator

orklah commented Feb 7, 2022

This could become an improvement though. There are already places in Psalm's code where we check for min_count (for example, TypeCombiner). The goal is to check a combined array is never empty. It could make sense to store min_count on non-empty-arrays / list for this. We already have assertions for that and everything...

@dsech
Copy link
Author

dsech commented Feb 7, 2022

Psalm doesn't have a concept of 'list with at least N elements, N > 1'. So inner splat results in non-empty-list<string> and that does not guarantee two elements required by the function: https://psalm.dev/r/3cdda0aa21

Thanks, I see that now. Before 4.19 there was no error reported, so I thought psalm was able to track the min size of the list.

@psalm-github-bot
Copy link

I found these snippets:

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

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

$args = [...$existingItems, $newItem];
/** @psalm-trace $args */;
func(...$args);
Psalm output (using commit faad966):

INFO: Trace - 13:26 - $args: non-empty-list<string>

ERROR: TooFewArguments - 14:1 - Too few arguments for func - expecting item2 to be passed

@orklah
Copy link
Collaborator

orklah commented Feb 7, 2022

I completely reworked this check here: #7348

It was unable to check for named arguments and it forced me to start from scratch. I may have missed some checks Psalm was making in order to reduce the amount of false positives though.

For example, I could add something in here that prevent emitting this issue when we have a non-empty-array with no known count.

This would probably reduce false positives (as well as removing some possible bug warning for unckecked arrays...). What do you think @dsech @weirdan ?

Maybe we should only do that when we have a variadic argument on the end? (For all other cases, I think we can expect a count() === X to be made before the call...)

@dsech
Copy link
Author

dsech commented Feb 8, 2022

Ideally I'd love to see psalm keep track of min and max size of the array/list (obviously).
And I'm not sure if any other solution will properly fix this false positive, without causing other issues and/or false negatives.

Personally I'm good with just leaving a @suppress annotation until psalm can properly detect the code is safe.

@weirdan
Copy link
Collaborator

weirdan commented Nov 23, 2022

We can express the unbounded list with N guaranteed elements in Psalm 5 as list{T,T,T,...<int<0,max>, T>}: https://psalm.dev/r/80dd0babf0

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/80dd0babf0
<?php

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

$z = [...$existingItems, $newItem];
assert(count($z) >= 2);
/** @psalm-trace $z */;

func(...$z);
Psalm output (using commit f846906):

INFO: Trace - 14:23 - $z: list{string, string, ...<int<0, max>, string>}

@dsech
Copy link
Author

dsech commented Jan 30, 2023

It looks like this is fixed, because the assert from the above snippet is not necessary anymore: https://psalm.dev/r/80dd0babf0

But (not sure if relevant to this issue) the size >= 2 information is lost after calling func: https://psalm.dev/r/18a8f67489
or when removing the semicolon after the @psalm-trace docblock: https://psalm.dev/r/90e8d5a001
which I think is wrong.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/80dd0babf0
<?php

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

$z = [...$existingItems, $newItem];
assert(count($z) >= 2);
/** @psalm-trace $z */;

func(...$z);
Psalm output (using commit d600a35):

ERROR: RedundantCondition - 13:1 - Type list{string, string, ...<int<0, max>, string>} for $z is always has-at-least-2

INFO: Trace - 14:23 - $z: list{string, string, ...<int<0, max>, string>}
https://psalm.dev/r/18a8f67489
<?php

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

$z = [...$existingItems, $newItem];
/** @psalm-trace $z */;

func(...$z);

/** @psalm-trace $z */
Psalm output (using commit d600a35):

INFO: Trace - 13:23 - $z: list{string, string, ...<int<0, max>, string>}

INFO: Trace - 17:23 - $z: non-empty-list<string>
https://psalm.dev/r/90e8d5a001
<?php

function func(string $item1, string $item2, string ...$otherItems): array {
    return [$item1, $item2, ...$otherItems];
}

/**
 * @var non-empty-list<string> $existingItems
 * @var string $newItem
 */

$z = [...$existingItems, $newItem];
/** @psalm-trace $z */

func(...$z);
Psalm output (using commit d600a35):

INFO: Trace - 15:1 - $z: non-empty-list<string>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants