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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive with template covariant #9161

Closed
enumag opened this issue Apr 6, 2023 · 7 comments 路 Fixed by phpstan/phpstan-src#2464
Closed

False positive with template covariant #9161

enumag opened this issue Apr 6, 2023 · 7 comments 路 Fixed by phpstan/phpstan-src#2464

Comments

@enumag
Copy link
Contributor

enumag commented Apr 6, 2023

Bug report

I'm not 100% sure but I don't think there is much of a difference between toArray and ToPairs regarding type variance, right?

Or In case the error is correct then how should I fix it? 馃

Code snippet that reproduces the problem

https://phpstan.org/r/ccc12749-6dfd-4e0d-abf5-54dcd11e6fd6

Expected output

no error

@ondrejmirtes
Copy link
Member

Summoning @jiripudil to judge it here 馃槅

@jiripudil
Copy link
Contributor

jiripudil commented Apr 10, 2023

Hi, referencing phpstan/phpstan-src#2054 for context. I've stumbled upon a very similar error in phpstan's integration tests there. The issue is that the array shape / tuple type must be invariant because there's nothing that prevents you from writing into it:

/**
 * @param Map<int, Fruit>
 */
function doMagicWithFruit(Map $fruits)
{
    [$firstPair] = $fruits->toPairs();
    $firstPair[1] = new Apple();
}

The covariant template type makes it possible to pass a Map<int, Orange> into the magic function, but then, in the implementation, you're mixing apples and oranges. If there was a readonly-array shape type preventing the modification, it could safely be covariant.

Plain arrays (and, transitively, lists) do not behave so strictly type-wise (#8880 (comment)), and therefore do not need to be so strict variance-wise. I have my reservations about that (see my previous comment in the linked issue), but that's a different (and more complicated) discussion.

@ondrejmirtes
Copy link
Member

Writing into arrays does not matter if they're not passed by reference, right? And references are they own beasts which are generally hands-off by the static analyser because they bring a lot of mess with themselves.

@jiripudil
Copy link
Contributor

Writing into arrays does not matter if they're not passed by reference, right?

Oh, yeah, right! But in that case, I'd say this is indeed a bug. ConstantArrayType is still an array and has the same copy-on-write mechanism, so it too can safely be covariant in its keys and values because no change can modify the original value and its type 馃

@ondrejmirtes
Copy link
Member

I'd be really grateful if you could fix it and test it 馃槉 Thank you.

@ondrejmirtes
Copy link
Member

Fixed: phpstan/phpstan-src#2464

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants