Skip to content

Preserve constant array when setting a union of constant scalar keys #4520

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.

Sign up for GitHub

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

Open
ondrejmirtes wants to merge 1 commit into
base: 2.1.x
Choose a base branch
from
Open

Preserve constant array when setting a union of constant scalar keys #4520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/Type/Constant/ConstantArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,20 @@ public function getOffsetValueType(Type $offsetType): Type

public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type
Copy link
Contributor

@staabm staabm Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

looking at this unionValues parameter, makes me feel we nearly everytime ignore it.

I wonder whether this bool represents the same 2 use-cases as setOffsetValueType() vs. setExistingOffsetValueType().

like unionValues=true is the same as setOffsetValueType()
and unionValues=false is the same as setExistingOffsetValueType()

Copy link
Member Author

@ondrejmirtes ondrejmirtes Nov 3, 2025

Choose a reason for hiding this comment

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

The unionValues parameter is some hack I came up with, there's no deeper thought in it and it's likely the proper solution is different.

Copy link
Contributor

@VincentLanglet VincentLanglet Nov 4, 2025

Choose a reason for hiding this comment

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

The unionValues parameter is some hack I came up with,

What is the purpose of this hack ?

Copy link
Contributor

@staabm staabm Nov 4, 2025

Choose a reason for hiding this comment

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

I did some research yesterday. it was introduced with 5d64483

{
if ($offsetType !== null) {
$constantScalars = $offsetType->getConstantScalarTypes();
$constantScalarsCount = count($constantScalars);
if ($constantScalarsCount > 1 && $constantScalarsCount < ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT) {
Copy link
Contributor

@staabm staabm Nov 4, 2025

Choose a reason for hiding this comment

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

Do we need similar handling for integer ranges with less than array-limit elements?

Copy link
Member Author

@ondrejmirtes ondrejmirtes Nov 4, 2025

Choose a reason for hiding this comment

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

Yes, that would make sense!

$arrays = [];
foreach ($constantScalars as $constantScalar) {
$builder = ConstantArrayTypeBuilder::createFromConstantArray($this);
$builder->setOffsetValueType($constantScalar, $valueType);
$arrays[] = $builder->getArray();
}

return TypeCombinator::union(...$arrays);
}
}
Comment on lines 696 to 709
Copy link
Contributor

@staabm staabm Nov 3, 2025

Choose a reason for hiding this comment

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

I wonder, whether this code should be moved into the impl of ConstantArrayTypeBuilder->setOffsetValueType()?

Copy link
Member Author

@ondrejmirtes ondrejmirtes Nov 3, 2025

Choose a reason for hiding this comment

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

It should be handled there but this code should not be copied, but adapted (it's not so easy to do it there). I just wanted to try a quick PoC first.

$builder = ConstantArrayTypeBuilder::createFromConstantArray($this);
$builder->setOffsetValueType($offsetType, $valueType);

Expand All @@ -701,6 +715,19 @@ public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $uni

public function setExistingOffsetValueType(Type $offsetType, Type $valueType): Type
{
$constantScalars = $offsetType->getConstantScalarTypes();
$constantScalarsCount = count($constantScalars);
if ($constantScalarsCount > 1 && $constantScalarsCount < ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT) {
$arrays = [];
foreach ($constantScalars as $constantScalar) {
$builder = ConstantArrayTypeBuilder::createFromConstantArray($this);
$builder->setOffsetValueType($constantScalar, $valueType);
$arrays[] = $builder->getArray();
}

return TypeCombinator::union(...$arrays);
}

$builder = ConstantArrayTypeBuilder::createFromConstantArray($this);
$builder->setOffsetValueType($offsetType, $valueType);

Expand Down
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/nsrt/bug-11716.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public function parse(string $glue): string
$seenGlues[$glue] = true;

assertType("'&'|'|'", $glue);
assertType("array{'|': bool, '&': bool}", $seenGlues);
assertType("array{'|': false, '&': true}|array{'|': true, '&': false}", $seenGlues);
} else {
assertType("''", $glue);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/nsrt/constant-array-type-set.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function doFoo(int $i)
/** @var 0|1|2|3 $offset3 */
$offset3 = doFoo();
$e[$offset3] = true;
assertType('non-empty-array<0|1|2|3, bool>', $e);
assertType('array{0: bool, 1: bool, 2: bool, 3?: true}', $e);

$f = [false, false, false];
/** @var 0|1 $offset4 */
Expand Down
20 changes: 20 additions & 0 deletions tests/PHPStan/Analyser/nsrt/set-constant-union-offset-on-constant-array.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace SetConstantUnionOffsetOnConstantArray;

use function PHPStan\Testing\assertType;

class Foo
{

/**
* @param array{foo: int} $a
*/
public function doFoo(array $a): void
{
$k = rand(0, 1) ? 'a' : 'b';
$a[$k] = 256;
assertType('array{foo: int, a: 256}|array{foo: int, b: 256}', $a);
}

}
Loading