Add "DynamicFunctionReturnTypeExtension" for "array_replace"

[voku]

discussion: phpstan/phpstan#5440

[staabm]

Which case doesn‘t work? It seems all tests pass?

[staabm]

Is this union handling covered by a test?

i am wondering why only the last type of a union is taken into account

[voku]

I don't understand it, I only copy&past this code. :-/

I will try to add a test for that, but I also don't understand the "unpack" part here. Whenever we unpack "...", then we need to use a second loop for all possible arguments, or not?

[voku]

Which case doesn‘t work? It seems all tests pass?

array_combine is working as expected all other cases, are not what I expected. array_merge and array_replace results looks wrong for me, because some indexes are doublicate in the array and so there should be 4 or 2, but not both. Or? 🤔

[staabm]

all other cases, are not what I expected. array_merge and array_replace results looks wrong for me

does this mean the tests added with the PR don‘t assert the values you would expect? I am wondering why the testsuite is green.

[staabm]

Regarding the general idea of array_replace:

I would expect phpstan to know that the returned array will have the same key-types as the 1st arg but the value types being a union of the value type of all args?

I would leave the arg-unpack case as a next iteration open for a followup PR and instead try to cover the most important use-cases first

[staabm]

since you mentioned array_combine is doing what you expect.

In case you didn't find it yet, the corresponding array_combine sources are here: https://github.com/phpstan/phpstan-src/blob/master/src/Type/Php/ArrayCombineFunctionReturnTypeExtension.php

[voku]

since you mentioned array_combine is doing what you expect.

In case you didn't find it yet, the corresponding array_combine sources are here: https://github.com/phpstan/phpstan-src/blob/master/src/Type/Php/ArrayCombineFunctionReturnTypeExtension.php

I added support for array shapes, but we need to combine the different arrays the same way as array_merge / array_replace would do it. Any idea how to achieve that?

EDIT: logic is implemented

@ondrejmirtes what do you think about the changes, or is there already code for combining arrays, somewhere in the codebase? 🤔 What is with the "+" operator, do we need to do something similar for that?

[voku]

INFO: The failed tests seem to come from master.

[voku]

@ondrejmirtes I added some more tests here, but I don't know if we need more tests here? Do you have some edge cases that should be tested in mind?

[voku]

I think this will also fix:

• False positive with array_merge phpstan#2567

[staabm]

I think this will also fix:

could you verify that by adding a unit test?

[staabm]

would it be easier to have separate PRs for the 2 extensions?
if its too much work, wait for input from ondrey ;)

[voku]

would it be easier to have separate PRs for the 2 extensions?
if its too much work, wait for input from ondrey ;)

done: #641

[ondrejmirtes]

Thank you!

[ondrejmirtes]

Unfortunately I had to revert this extension: d28f785

It's because of performance impact on real-world projects. The extension itself may not be the fault but it proves that having more ConstantArrayType instances during analysis leads to a slowdown.

The only way I see is to build a benchmarking mode into PHPStan itself that would compare the analysis times of project files and point out which file slowed down after this commit. Then isolate what exactly in that file causes it and try to optimize that. Unfortunately it's not my priority right now.

[staabm]

@voku do you think you could re-do this PR without using the ConstantArrayTypeBuilder part - so we at least get the "non-empty-array" parts of it (similar to what we have right now with array_merge in https://github.com/phpstan/phpstan-src/blob/b105b9ebe3496c1c1f7ad9757a1a143ffedb88a7/src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php)?

that way your work could at least close phpstan/phpstan#5327 and we could have a more in-detail look regarding the array-shape related stuff with a focus on performance in a separate PR

[voku]

@staabm here I added a simply copy of the "array_merge"-extension but for "array_replace": #696