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

array_is_list makes the type less specific. #8046

Open
mj4444ru opened this issue Jun 2, 2022 · 7 comments
Open

array_is_list makes the type less specific. #8046

mj4444ru opened this issue Jun 2, 2022 · 7 comments
Assignees
Labels
easy problems Issues that can be fixed without background knowledge of Psalm enhancement good first issue Help wanted

Comments

@mj4444ru
Copy link

mj4444ru commented Jun 2, 2022

https://psalm.dev/r/ffab419792

The test1 function passes the test successfully.

Errors are generated for the test2 function because array_is_list turns the $arr type into a non-empty-list<array<array-key, string>|string>.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ffab419792
<?php

/**
 * @param array{0: string, 1: string[]} $arr
 */
function test1(array $arr): string {
    [$a0, $a1] = $arr;
    
    return $a0 . implode('', $a1);
}

/**
 * @param array{0: string, 1: string[]} $arr
 */
function test2(array $arr): string {
    if (!array_is_list($arr)) {
        throw new Exception();
    }
    
    [$a0, $a1] = $arr;
    
    return $a0 . implode('', $a1);
}
Psalm output (using commit 3a24488):

ERROR: PossiblyInvalidArgument - 22:30 - Argument 2 of implode expects array<array-key, null|object{__tostring()}|scalar>, possibly different type array<array-key, string>|string provided

ERROR: PossiblyInvalidOperand - 22:12 - Cannot concatenate with a array<array-key, string>|string

@AndrolGenhald
Copy link
Collaborator

I'm leaving this open as an enhancement, but your check in test2 is pointless. Destructuring works based on keys rather than order: https://3v4l.org/ZuKtg

Now that destructuring with keys is supported (since PHP 7.1, the same version the short [] destructuring syntax was introduced), normal destructuring is functionally identical to using numbered keys starting from 0.

@mj4444ru
Copy link
Author

mj4444ru commented Jun 2, 2022

I'm leaving this open as an enhancement, but your check in test2 is pointless. Destructuring works based on keys rather than order: https://3v4l.org/ZuKtg

It seems meaningless to you, because you do not know the logic.

In this case, I have to make sure that the data passed from the external source has 3 elements with keys 0, 1, 2 in ascending order. I do it like this:

if (!array_is_list($arr) || count($arr) !== 3) {
    throw new InvalidFormatException();
}

@AndrolGenhald
Copy link
Collaborator

It seems meaningless to you, because you do not know the logic.

It is meaningless as given in the example, I was just trying to be helpful by pointing it out since it can be somewhat unintuitive. If you have other code where it's not meaningless that's fine, it is something that I think is worth improving in Psalm.

@orklah
Copy link
Collaborator

orklah commented Jun 2, 2022

If I'm not mistaken, the fix should be here:

$array_types[] = new TNonEmptyList($type->type_params[1]);

We're trying to reconcile array{0: string, 1: string[]} with list<mixed>, we retrieved the "GenericArrayType" (should look like array<0|1, string|string[]> I believe), then, we're creating a TNonEmptyList who'll be rendered as non-empty-list<string|string[]>. All that is correct, but we lost that we had 2 definitely set keys. TNonEmptyList has a $count property who should be set to 2 in that case.

@orklah orklah added easy problems Issues that can be fixed without background knowledge of Psalm Help wanted good first issue labels Jun 2, 2022
AndrolGenhald added a commit to AndrolGenhald/psalm that referenced this issue Jun 2, 2022
@AndrolGenhald AndrolGenhald self-assigned this Jun 2, 2022
AndrolGenhald added a commit to AndrolGenhald/psalm that referenced this issue Jun 2, 2022
@AndrolGenhald
Copy link
Collaborator

I actually went a step further to make this possible.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ec9a06bb45
<?php
/**
 * @param mixed $arr
 * @psalm-assert-if-true list $arr
 */
function is_list($arr): bool
{
    return is_array($arr) && array_is_list($arr);
}
/** @var array{0: int, 1: bool, 2: string} */
$list = [1, true, "string"];
assert(is_list($list));
function takesString(string $_str): void {}
takesString($list[2]);
Psalm output (using commit 3a24488):

INFO: PossiblyUndefinedIntArrayOffset - 14:13 - Possibly undefined array offset '2' is risky given expected type 'int'. Consider using isset beforehand.

ERROR: InvalidScalarArgument - 14:13 - Argument 1 of takesString expects string, bool|int|string provided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy problems Issues that can be fixed without background knowledge of Psalm enhancement good first issue Help wanted
Projects
None yet
Development

No branches or pull requests

3 participants