Skip to content

verify constants via array_key_exists#636

Closed
voku wants to merge 3 commits intophpstan:1.5.xfrom
voku:array_key_exists_fix
Closed

verify constants via array_key_exists#636
voku wants to merge 3 commits intophpstan:1.5.xfrom
voku:array_key_exists_fix

Conversation

@voku
Copy link
Copy Markdown
Contributor

@voku voku commented Aug 22, 2021

issue: phpstan/phpstan#4174

I looked into ArrayKeyExistsFunctionTypeSpecifyingExtension, but it seems like I didn't fully understand the logic of the specifying classes. In this case we want to specify parameter 1 or/and 2, can we currently do that?

@voku voku force-pushed the array_key_exists_fix branch from aaeda50 to e7e6ec6 Compare August 22, 2021 21:54
@voku voku changed the title Constant enumerations: add failing tests verify constants via array_key_exists Aug 22, 2021
@voku voku force-pushed the array_key_exists_fix branch from e7e6ec6 to f69fe84 Compare August 22, 2021 21:57
@voku voku marked this pull request as ready for review August 22, 2021 21:57
@voku voku force-pushed the array_key_exists_fix branch from f69fe84 to 6098f94 Compare August 22, 2021 23:14
@voku
Copy link
Copy Markdown
Contributor Author

voku commented Aug 22, 2021

@ondrejmirtes is there already a option to run multiple FunctionTypeSpecifyingExtension per function? I added a simple skip method, but I don't know if there is a better way.

@ondrejmirtes
Copy link
Copy Markdown
Member

@voku Hi, I don't think it's necessary to support multiple FunctionTypeSpecifyingExtension for the same function. Everything can be written in a single extension.

@ondrejmirtes
Copy link
Copy Markdown
Member

Please verify that this also fixes: https://phpstan.org/r/2561c442-f952-470d-a6a5-1bc7a74b99ad

@ondrejmirtes
Copy link
Copy Markdown
Member

Or probably doesn't have to, maybe it's a totally different issue.

@voku
Copy link
Copy Markdown
Contributor Author

voku commented Aug 25, 2021

@voku Hi, I don't think it's necessary to support multiple FunctionTypeSpecifyingExtension for the same function. Everything can be written in a single extension.

@ondrejmirtes I really tried it, but I failed. How can I combine different SpecifiedTypes for different parameters in one FunctionTypeSpecifyingExtension class? Can you please give me a hint?

@ondrejmirtes
Copy link
Copy Markdown
Member

@voku There's unionWith method on SpecifiedTypes:

/** @api */
public function unionWith(SpecifiedTypes $other): self

@voku voku force-pushed the array_key_exists_fix branch 2 times, most recently from 6a19e68 to 2e5129a Compare August 27, 2021 00:36
@voku
Copy link
Copy Markdown
Contributor Author

voku commented Aug 27, 2021

@voku There's unionWith method on SpecifiedTypes:

/** @api */
public function unionWith(SpecifiedTypes $other): self

@ondrejmirtes Thanks for the hint. I tried it, but it's not working as expected for different parameters?! But I re-think about it and for a simple check for array-shapes, we do not need multiple specified types at all: If we have array-shapes, then we do not need to specify the $array anymore, and we can specify the value of $key of array_key_exists($key, $array).

@voku
Copy link
Copy Markdown
Contributor Author

voku commented Aug 29, 2021

@ondrejmirtes Do you have some extra tests in mind?

@voku voku force-pushed the array_key_exists_fix branch 4 times, most recently from 4725a4a to cfc1d9a Compare October 17, 2021 19:42
@voku voku force-pushed the array_key_exists_fix branch 2 times, most recently from d3e4852 to 18e1a40 Compare November 6, 2021 18:40
@voku
Copy link
Copy Markdown
Contributor Author

voku commented May 13, 2022

@ondrejmirtes this week I had again a bug in my code, where this fix could prevent it.

Can you maybe give feedback here, thanks. 😊

@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented May 13, 2022

I did not read through all comments, but I think we need to be careful that this doesn't become another in_array in regard of impossiblechecktype adaptions needed. Preferably none should be needed I guess :/

@ondrejmirtes
Copy link
Copy Markdown
Member

Yes, that's what I worry about here. ImpossibleCheckTypeHelper should get thinner over time, not thicker.

@voku
Copy link
Copy Markdown
Contributor Author

voku commented May 13, 2022

Maybe we can only use the "array_key_exists"-extension improvement? 🤔

@ondrejmirtes
Copy link
Copy Markdown
Member

Hi, I'm cleaning up old and stale PRs. Please send a new PR if you're still interested, thanks.

I haven't merged this because I don't particularly feel great about the changes here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants