phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

verify constants via array_key_exists

Open voku opened this issue 4 years ago • 12 comments

issue: https://github.com/phpstan/phpstan/issues/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 avatar Aug 22 '21 00:08 voku

@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.

voku avatar Aug 22 '21 23:08 voku

@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 avatar Aug 25 '21 20:08 ondrejmirtes

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

ondrejmirtes avatar Aug 25 '21 20:08 ondrejmirtes

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

ondrejmirtes avatar Aug 25 '21 20:08 ondrejmirtes

@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?

voku avatar Aug 25 '21 22:08 voku

@voku There's unionWith method on SpecifiedTypes:https://github.com/phpstan/phpstan-src/blob/73406572636be4ae9c63ae892cef9e733181116d/src/Analyser/SpecifiedTypes.php#L103-L104

ondrejmirtes avatar Aug 26 '21 08:08 ondrejmirtes

@voku There's unionWith method on SpecifiedTypes:

https://github.com/phpstan/phpstan-src/blob/73406572636be4ae9c63ae892cef9e733181116d/src/Analyser/SpecifiedTypes.php#L103-L104

@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 avatar Aug 27 '21 00:08 voku

@ondrejmirtes Do you have some extra tests in mind?

voku avatar Aug 29 '21 11:08 voku

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

Can you maybe give feedback here, thanks. 😊

voku avatar May 13 '22 05:05 voku

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 :/

herndlm avatar May 13 '22 06:05 herndlm

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

ondrejmirtes avatar May 13 '22 06:05 ondrejmirtes

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

voku avatar May 13 '22 07:05 voku

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.

ondrejmirtes avatar Oct 16 '22 10:10 ondrejmirtes