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_map((a, b) => c), a, b) fails to infer the type of b in the closure #2772

Closed
Ocramius opened this issue Feb 8, 2020 · 10 comments
Closed
Labels

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Feb 8, 2020

Typically, array_map() is used with two parameters, one is the closure mapping the values, and one is the foldable passed to that closure.

When array_map(function ($a, $b) { return $c; }, $as, $bs) is used, then the type of $b fails to be inferred as the type of $bs items: https://psalm.dev/r/cdabfb49ce

When declaring the type of $b explicitly, the errors obviously go away: https://psalm.dev/r/c5f1c38c50

The code is obviously over-simplified: the reason why this issue exists, is because $b is often a complex nested structure.

@psalm-github-bot
Copy link

I found these snippets:

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

/** @psalm-return array<array<string, string>> */
function functionsToBeTested() : array
{
  $as = ['key'];
  $bs = ['not important'];
  
    return array_map(
        /** @psalm-suppress MissingClosureParamType closure type is omitted to show missing inference */
        function (string $a, $b) : array {
            return [$a => $b];
        },
        $as,
        $bs
    );
}
Psalm output (using commit b439a57):

INFO: MixedReturnTypeCoercion - 9:12 - The type 'list<non-empty-array<string, mixed>>' is more general than the declared return type 'array<array-key, array<string, string>>' for functionsToBeTested

INFO: MixedReturnTypeCoercion - 3:19 - The declared return type 'array<array-key, array<string, string>>' for functionsToBeTested is more specific than the inferred return type 'list<non-empty-array<string, mixed>>'
https://psalm.dev/r/c5f1c38c50
<?php

/** @psalm-return array<array<string, string>> */
function functionsToBeTested() : array
{
  $as = ['key'];
  $bs = ['not important'];
  
    return array_map(
        function (string $a, string $b) : array {
            return [$a => $b];
        },
        $as,
        $bs
    );
}
Psalm output (using commit b439a57):

No issues!

@Ocramius
Copy link
Contributor Author

Ocramius commented Feb 8, 2020

Actual example from the code in the downstream project: https://psalm.dev/r/f5715acb09

@psalm-github-bot
Copy link

I found these snippets:

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

final class BetterReflectionFunctionAbstract
{
}

final class BetterReflectionClass
{
    public function getMethod(string $method) : BetterReflectionFunctionAbstract
    {
        throw new \Exception('ignore');
    }
}

final class ClassReflector
{
    public function reflect(string $className) : BetterReflectionClass
    {
        throw new \Exception('ignore');
    }
}

final class FunctionReflector
{
    public function reflect(string $functionName) : BetterReflectionFunctionAbstract
    {
        throw new \Exception('ignore');
    }
}

/**
 * @psalm-return array<string, array{
 *   0: BetterReflectionFunctionAbstract,
 *   1: BetterReflectionFunctionAbstract,
 *   2: list<string>
 * }>
 */
function functionsToBeTested() : array
{
    $fromClassReflector = new ClassReflector();
    $toClassReflector   = new ClassReflector();
    $fromReflector      = new FunctionReflector();
    $toReflector        = new FunctionReflector();

    $functions = [
        'orderChanged'                       => [
            '[BC] CHANGED: Default parameter value for for parameter $a of orderChanged() changed from 1 to 3',
            '[BC] CHANGED: Default parameter value for for parameter $c of orderChanged() changed from 3 to 1',
        ],
    ];

    return array_merge(
        array_combine(
            array_keys($functions),
            array_map(
                function (string $function, array $errorMessages) use ($fromReflector, $toReflector) : array {
                    return [
                        $fromReflector->reflect($function),
                        $toReflector->reflect($function),
                        $errorMessages,
                    ];
                },
                array_keys($functions),
                array_values($functions)
            )
        ),
        [
            'C::changed1' => [
                $fromClassReflector->reflect('C')->getMethod('changed1'),
                $toClassReflector->reflect('C')->getMethod('changed1'),
                [
                    '[BC] CHANGED: Default parameter value for for parameter $a of C::changed1() changed from 1 to 2',
                ],
            ]
        ]
    );
}
Psalm output (using commit b439a57):

ERROR: InvalidReturnStatement - 52:12 - The type 'non-empty-array<string(C::changed1)|string(orderChanged), array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: non-empty-array<array-key, mixed>}>' does not match the declared return type 'array<string, array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: list<string>}>' for functionsToBeTested

ERROR: InvalidReturnType - 32:18 - The declared return type 'array<string, array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: list<string>}>' for functionsToBeTested is incorrect, got 'non-empty-array<string, array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: non-empty-array<array-key, mixed>}>'

@muglug muglug added the bug label Feb 8, 2020
@muglug muglug closed this as completed in 5f4d797 Feb 8, 2020
@muglug
Copy link
Collaborator

muglug commented Feb 8, 2020

For whatever reason the @psalm-suppress confuses things, when it's removed everything works: https://psalm.dev/r/3ca580553f

@psalm-github-bot
Copy link

I found these snippets:

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

/** @psalm-return array<array<string, string>> */
function functionsToBeTested() : array
{
  $as = ['key'];
  $bs = ['not important'];
  
    return array_map(
        function (string $a, $b) : array {
            return [$a => $b];
        },
        $as,
        $bs
    );
}
Psalm output (using commit 5f4d797):

No issues!

Ocramius added a commit to Roave/BackwardCompatibilityCheck that referenced this issue Feb 8, 2020
Added workarounds for vimeo/psalm#2772 and
moved from `Safe\array_combine` to just
`\array_combine` due to thecodingmachine/safe#185
@Ocramius
Copy link
Contributor Author

Ocramius commented Feb 8, 2020

Uhh, something else happened too: if you see the comment above (about the real-world use-case), @psalm-github-bot found:

Psalm output (using commit b439a57):

ERROR: InvalidReturnStatement - 52:12 - The type 'non-empty-array<string(C::changed1)|string(orderChanged), array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: non-empty-array<array-key, mixed>}>' does not match the declared return type 'array<string, array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: list<string>}>' for functionsToBeTested

ERROR: InvalidReturnType - 32:18 - The declared return type 'array<string, array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: list<string>}>' for functionsToBeTested is incorrect, got 'non-empty-array<string, array{0: BetterReflectionFunctionAbstract, 1: BetterReflectionFunctionAbstract, 2: non-empty-array<array-key, mixed>}>'

Now it no longer reports anything

@muglug
Copy link
Collaborator

muglug commented Feb 8, 2020

Now it no longer reports anything

Yeah, because now it's inferring the types properly, right? Or is the code incorrect?

@Ocramius
Copy link
Contributor Author

Ocramius commented Feb 8, 2020

No, the code is correct, I'm just wondering what changed on this side meanwhile (the snippet wasn't changed)

@muglug
Copy link
Collaborator

muglug commented Feb 8, 2020

That's due to this commit: 9115ffd2

Psalm now has a better idea the array type than it did before, so it uses that instead of the array param type.

@Ocramius
Copy link
Contributor Author

Ocramius commented Feb 8, 2020

\o/ thanks!

Ocramius added a commit to Roave/BackwardCompatibilityCheck that referenced this issue Dec 5, 2021
…-phpunit`

This change ensures that we don't use `@internal` symbols in `src/` (too dangerous,
for long-term usage), and makes the type checking much stricter, thanks to `psalm/plugin-phpunit`.

Since `vimeo/psalm` has been upgraded, and vimeo/psalm#2772
no longer applies, we can also remove a lot of manually written code that was safe
to clean up.
uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this issue Aug 6, 2024
Added workarounds for vimeo/psalm#2772 and
moved from `Safe\array_combine` to just
`\array_combine` due to thecodingmachine/safe#185
uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this issue Aug 6, 2024
…-phpunit`

This change ensures that we don't use `@internal` symbols in `src/` (too dangerous,
for long-term usage), and makes the type checking much stricter, thanks to `psalm/plugin-phpunit`.

Since `vimeo/psalm` has been upgraded, and vimeo/psalm#2772
no longer applies, we can also remove a lot of manually written code that was safe
to clean up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants