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

Add "DynamicFunctionReturnTypeExtension" for "array_replace" + fix for "array_merge" #696

Closed
wants to merge 3 commits into from

Conversation

voku
Copy link
Contributor

@voku voku commented Oct 2, 2021

This Extension is a copy of "ArrayMergeFunctionDynamicReturnTypeExtension"

⇾ requested here: #622

INFO: Currently we are getting wrong types from "ArrayMergeFunctionDynamicReturnTypeExtension" because PHP handles array-keys different here: https://3v4l.org/YfKYq

  • if the key is numeric, then the value will be added
  • if the key is non-numeric, then the value will be replaced

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different to ArrayMergeFunctionDynamicReturnTypeExtension? Are they going to differ in the future?

$argType = $argType->getIterableValueType();
if ($argType instanceof UnionType) {
foreach ($argType->getTypes() as $innerType) {
$argType = $innerType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this part - the last inner type in the union will win in the $argType variable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the same code is in ArrayMergeFunctionDynamicReturnTypeExtension - it also puzzles me there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, you tell me :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the last pull request we already tried to process every parameter. I could re-add it, so we can see if it works, but the performance was bad, so that I will not add anything related to array shapes, ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try and see what happens :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-added the fixes and added a limit for the array. I also added the fix for "array_merge" here, so we can see the differences between the implementations in one diff.

@voku
Copy link
Contributor Author

voku commented Oct 6, 2021

How is this different to ArrayMergeFunctionDynamicReturnTypeExtension? Are they going to differ in the future?

If we check the type of the key, then we could implement the correct behavior in the future.

@voku voku changed the title Add "DynamicFunctionReturnTypeExtension" for "array_replace" Add "DynamicFunctionReturnTypeExtension" for "array_replace" + fix for "array_merge" Oct 16, 2021
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a regression test for phpstan/phpstan#5846 here and I'll take a look :) Thank you.

@voku
Copy link
Contributor Author

voku commented Nov 6, 2021

Please add a regression test for phpstan/phpstan#5846 here and I'll take a look :) Thank you.

I re-added some changes, even more tests and added some checks to prevent performance issues, but maybe we need to adjust the consts "MAX_*" settings? I already tested the performance with a private project and blackfire.io and for me, it looks good, but as airways it depends on the tested code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants