Skip to content

Rework ArrayReplaceFunctionReturnTypeExtension#3958

Merged
ondrejmirtes merged 2 commits intophpstan:1.12.xfrom
VincentLanglet:arrayReplace
Jul 17, 2025
Merged

Rework ArrayReplaceFunctionReturnTypeExtension#3958
ondrejmirtes merged 2 commits intophpstan:1.12.xfrom
VincentLanglet:arrayReplace

Conversation

@VincentLanglet
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet commented Apr 21, 2025 •
edited
Loading

Close phpstan/phpstan#12828

I basically copy the ArrayMergeFunctionReturnTypeExtension with the following change

phpstan-src/src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php

Line 87 in 4c41073

$keyType instanceof ConstantIntegerType ? null : $keyType,

$keyType instanceof ConstantIntegerType ? null : $keyType, is replaced by $keyType because

  • array_merge reindex numeric keys
  • array_replace keep all the existing keys

phpstan-src/src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php

Line 106 in 4c41073

if (!(new IntegerType())->isSuperTypeOf($keyType)->yes()) {

if (!(new IntegerType())->isSuperTypeOf($keyType)->yes()) { is replaced by if (!$argType->isList()->yes()) { because of the same reason.

@VincentLanglet VincentLanglet marked this pull request as ready for review April 22, 2025 08:44
@phpstan-bot
Copy link
Copy Markdown
Collaborator

phpstan-bot commented Apr 22, 2025

This pull request has been marked as ready for review.

@VincentLanglet VincentLanglet mentioned this pull request Apr 22, 2025
Closed
@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Apr 22, 2025 •
edited
Loading

I know I'm maybe a bit fast suggesting this, but we could create a Type::mergeArray() or Type::replaceArray() with a proper $preserveKeys or $reindex arg or whatever and then us it in both return type extensions. And maybe I should have done this already the last time I touched the merge one. Anyway, can be done in a follow-up too IMO, so this comment should not be considered a blocker IMO 😊 and I can volunteer to do this.

herndlm
herndlm reviewed Apr 22, 2025
View reviewed changes
@VincentLanglet
Copy link
Copy Markdown
Contributor Author

VincentLanglet commented Apr 22, 2025

Anyway, can be done in a follow-up too IMO, so this comment should not be considered a blocker IMO 😊 and I can volunteer to do this.

Yeah, I would prefer this to be done in a follow up PR to separate (and validate separately by ondrej)

  • The new implementation
  • The refactoring

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Apr 28, 2025

/cc @herndlm Waiting for LGTM from you here :)

@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Apr 28, 2025

I guess this is fine. And also brings value of course. Let's see if we can refactor array_merge and array_replace stuff a bit as follow-up I'd say :)

ondrejmirtes
ondrejmirtes requested changes May 13, 2025
View reviewed changes
src/Type/Php/ArrayReplaceFunctionReturnTypeExtension.php
/** @var array<int|string, ConstantIntegerType|ConstantStringType> $keyTypes */
$keyTypes = [];
foreach ($argType->getConstantArrays() as $constantArray) {
foreach ($constantArray->getKeyTypes() as $keyType) {
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes May 13, 2025

Choose a reason for hiding this comment

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

I don't think this is what you want. Let's say you have array{foo: int}|array{bar: string}.

You'll end up with two values in $keyTypes and you'll create a flat array out of them. Also you'll overwrite a value if two or more arrays in a union share the same key but with different type.

Copy link
Copy Markdown
Contributor Author

@VincentLanglet VincentLanglet May 13, 2025

Choose a reason for hiding this comment

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

I'm not sure to understand your point.

This is the current behavior we have with array_replace vs array_merge
https://phpstan.org/r/4ec3a6a9-2645-4565-a81e-577b518eb7c3

Since both function are working almost the same way, I copied and adapt the logic from ArrayMergeFunctionReturnTypeExtension

phpstan-src/src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php

Lines 74 to 94 in 84e2316

if ($allConstant->yes()) {
$newArrayBuilder = ConstantArrayTypeBuilder::createEmpty();
foreach ($argTypes as $argType) {
/** @var array<int|string, ConstantIntegerType|ConstantStringType> $keyTypes */
$keyTypes = [];
foreach ($argType->getConstantArrays() as $constantArray) {
foreach ($constantArray->getKeyTypes() as $keyType) {
$keyTypes[$keyType->getValue()] = $keyType;
}
}
foreach ($keyTypes as $keyType) {
$newArrayBuilder->setOffsetValueType(
$keyType instanceof ConstantIntegerType ? null : $keyType,
$argType->getOffsetValueType($keyType),
!$argType->hasOffsetValueType($keyType)->yes(),
);
}
}
return $newArrayBuilder->getArray();

In which we're doing

foreach ($argType->getConstantArrays() as $constantArray) {
					foreach ($constantArray->getKeyTypes() as $keyType) {

With this PR, we're now getting

array{bar?: int, foo: 'thing'}

with both array_merge and array_replace

@ondrejmirtes ondrejmirtes merged commit 3eb0372 into phpstan:1.12.x Jul 17, 2025
452 checks passed
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Jul 17, 2025

Thank you.

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

Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

2 more reviewers

@samsonasik samsonasik samsonasik left review comments

@herndlm herndlm herndlm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@VincentLanglet @phpstan-bot @herndlm @ondrejmirtes @samsonasik