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

Add return type extension for array_column() #948

Merged
merged 1 commit into from Jan 29, 2022

Conversation

jlherren
Copy link
Contributor

Note that when the iterable value type of the given array is a ConstantArrayType with optional keys, then it will not attempt to produce a precise result. I think it's not worth supporting this and would either produce useless types, or unnecessarily complex types.

@ondrejmirtes
Copy link
Member

There's some noise in the CI pipeline which I need to look into, but here are some real issues found with your extension: https://github.com/phpstan/phpstan-src/runs/4977783162?check_suite_focus=true

@jlherren
Copy link
Contributor Author

jlherren commented Jan 28, 2022

Mmh, yes I see the problem:

/**
 * @return VarTagValueNode[]
 */
public function getVarTagValues(string $tagName = '@var'): array
{
    return array_column(
        array_filter($this->getTagsByName($tagName), static function (PhpDocTagNode $tag): bool {
            return $tag->value instanceof VarTagValueNode;
        }),
        'value'
    );
}

Reported error:

Error: Method PhpDocNode::getVarTagValues() should return array<VarTagValueNode> but returns array<int, PhpDocTagValueNode>.

This code is obviously correct, but teaching PHPStan to correctly recognize this is obviously not possible at the moment. The previously inferred return type was array. I'm not sure what to do here (short of rewriting this code as a foreach).

@ondrejmirtes
Copy link
Member

What if we first ran array_column to extract the value of nodes, and only after that we ran array_filter? That could preserve the type.

@@ -92,6 +92,9 @@ public function or(self ...$operands): self

public static function extremeIdentity(self ...$operands): self
{
if ($operands === []) {
throw new ShouldNotHappenException();
}
$operandValues = array_column($operands, 'value');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the current behaviour with an empty array? If the code doesn't crash then this is a BC break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min([]) and max([]) cause fatal errors on PHP >= 8. On PHP < 8 it causes a warning and returns false, which gets coerced to 0 when given to create(). See https://3v4l.org/RS767

@jlherren
Copy link
Contributor Author

What if we first ran array_column to extract the value of nodes, and only after that we ran array_filter? That could preserve the type.

This would indeed work! I didn't even know PHPStan recognizes array_filter() with instanceof in the callback.

@ondrejmirtes
Copy link
Member

Can you please send a PR to phpdoc-parser that switches those calls around? :) Thanks.

@ondrejmirtes ondrejmirtes merged commit e8509b7 into phpstan:master Jan 29, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@BitScout
Copy link

Hi, 21 days ago, so right after the release of v1.4.4 (22 days ago) the daily build on ekino/phpstan-banned-code for PHP 7.4 broke. Could it be possible that this is due to this pull request?

phpstan analyze complains:

Line 16: Property Example::$myData (array) does not accept
array<array<string, string>|int|string|null, array<string, array<string, string>|string|null>>.

which in a simplified version means that phpstan thinks that array_column will return an
array<array|int|string|null, array>, in other words an array that can have arrays as its keys.

Here is an example file to reproduce the (potential) problem:

<?php

class Example
{
    /**
     * @var array<mixed>
     */
    protected $myData = [];

    /**
     * @param array<int, array<string, string|array<string,string>|null>> $data
     */
    public function __construct(array $data)
    {
        $x = array_column($data, null, 'type');
        $this->myData = $x;
    }
}

$inputData = [
    [
        'type' => 'one',
        'foo' => null,
    ],
    [
        'type' => 'two',
        'foo' => 'bar',
    ],
    [
        'type' => 'three',
        'foo' => [
            'alpha' => 'bravo',
            'charlie' => 'delta',
        ],
    ],
];

$example = new Example($inputData);

and this is the phpstan.neon.dist used:

parameters:
	level: 8
	paths:
		- src
		- tests

	checkGenericClassInNonGenericObjectType: false

includes:
	- extension.neon
	- vendor/phpstan/phpstan-phpunit/extension.neon
	- vendor/phpstan/phpstan-phpunit/rules.neon

Is something wrong about the example code? Did I misunderstand something?

@ondrejmirtes
Copy link
Member

@BitScout Please open a new issue (Bug report).

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