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

Dynamic array key access does not work if key is optional even after isset() #7000

Closed
Seldaek opened this issue Apr 7, 2022 · 8 comments · Fixed by phpstan/phpstan-src#1307
Labels

Comments

@Seldaek
Copy link
Contributor

Seldaek commented Apr 7, 2022

Bug report

This shows it best.

https://phpstan.org/r/5bd531ea-0a41-4be8-bbf9-3ca42e8d0bbe

It seems to be a "regression" of the 1.5.4 release which is unsurprising given it fixed a lot of issues related to arrays. But I do believe the behavior is incorrect and hope it can be fixed.

Expected output

No error

@Seldaek Seldaek changed the title Dynamic array key access does not check if key is optional even after isset() Dynamic array key access does not work if key is optional even after isset() Apr 7, 2022
@rajyan
Copy link
Contributor

rajyan commented Apr 8, 2022

Confirmed that it was broken by this change
https://github.com/phpstan/phpstan-src/pull/1154/files

but also confirmed that type of $composer in isset truthy scope haven't changed between before and after this commit. 'array{require?: array<string, string>, require-dev?: array<string, string>}'
This is correct see phpstan/phpstan-src#1187 (comment)

I think it means this commit caused a regression but place to fix is somewhere related to truthy scope of isset.
I'm currently working on TypeSpecifier to fix this issue!

@rajyan
Copy link
Contributor

rajyan commented Apr 8, 2022

I think I have narrowed the scope to fix
https://phpstan.org/r/674596ea-a3a7-4577-baef-af7f13533582
offset with non UnionType is working.
so markOffsetRequired https://github.com/phpstan/phpstan-src/blob/f584311ffe55bc33ecd97d972e367600c1aeb088/src/Type/Constant/ConstantArrayType.php#L916
with UnionType seems not working.

this is wrong

@rajyan
Copy link
Contributor

rajyan commented Apr 8, 2022

https://phpstan.org/r/9496a732-ceff-43ce-ad47-b30af5a9baa4
It works if I add another offsetType... looks wired
also If I delete the foreach
https://phpstan.org/r/54a66692-b591-4473-a2d7-650940c1cc07
the result changes

related to foreach maybe?

@rajyan
Copy link
Contributor

rajyan commented Apr 10, 2022

I think the type in isset truthy scope can be improved as array<'require'|'require-dev', array<string, string>>&hasOffset('require'|'require-dev')>

I’m not sure why it is only working when there is a following foreach
https://phpstan.org/r/9496a732-ceff-43ce-ad47-b30af5a9baa4

@ondrejmirtes
Copy link
Member

@rajyan This type isn't a solution. You're losing the array shape and the error should not be reported only for $composer[$linkType], but should be still reported for $composer[$foo] where $foo is also 'require'|'require-dev'.

@rajyan
Copy link
Contributor

rajyan commented Apr 10, 2022

@ondrejmirtes
Thanks. I think I’ve finally understood the difficulty of this issue.

https://phpstan.org/r/441c0694-0e3b-404f-a25e-f85522e1780f
If my understanding is correct, this example should express what you are saying.

By the way, there seems to be a bug with foreach scope. Type changes when I add offset 'test’, type changes if I remove the foreach, and also the type is the (wrong) one like I commented. Maybe it was working previously with this bug.

@rajyan
Copy link
Contributor

rajyan commented Apr 21, 2022

Maybe a duplicate of #6508
I’m still interested in fixing this issue:) I’ll try it after finishing phpstan/phpstan-src#1234

rajyan added a commit to rajyan/phpstan-src that referenced this issue Apr 24, 2022
rajyan added a commit to rajyan/phpstan-src that referenced this issue May 12, 2022
ondrejmirtes added a commit to phpstan/phpstan-src that referenced this issue May 12, 2022
* add regression tests for phpstan/phpstan#7000

* specify expression even it doesn't change types

* skip UnionType offset because it's hard to solve in some cases

* skip specified expressions in NonexistentOffsetInArrayDimFetchRule

* fix tests

* add regression tests

* fix useless call in foreach

* fix issue-7190 too

* Avoided duplicate namespace

Co-authored-by: Ondřej Mirtes <ondrej@mirtes.cz>
@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 Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants