Skip to content

Re-check scalar types after integer range expansion in TypeCombinator::union #5660

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

Merged
staabm merged 5 commits into from
May 14, 2026
+113 −1
Merged

Re-check scalar types after integer range expansion in TypeCombinator::union #5660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
470212f
Re-check scalar types after integer range expansion in TypeCombinator…
phpstan-bot May 14, 2026
99c79c2
Add test cases for array<mixed> parameter to show superglobal-specifi…
phpstan-bot May 14, 2026
628e531
Remove testWithParam and merge bug-14610 test files into single nsrt …
phpstan-bot May 14, 2026
e65d557
another test
staabm May 14, 2026
81ad1bf
another test
staabm May 14, 2026
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
3 changes: 2 additions & 1 deletion src/Type/TypeCombinator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,9 @@ public static function union(Type ...$types): Type
[$a, $b] = $compareResult;
if ($a !== null) {
$types[$i] = $a;
array_splice($scalarTypeItems, $j--, 1);
array_splice($scalarTypeItems, $j, 1);
Copy link
Copy Markdown
Contributor

@staabm staabm May 14, 2026

Choose a reason for hiding this comment

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

do we need the same fix for the if ($b !== null) { block below?
if so, come up with tests currently failling and fix it

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot May 14, 2026

Choose a reason for hiding this comment

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

All tests pass (12066 tests, 79688 assertions) and PHPStan reports no errors.

Here's my analysis for the reviewer's question:

The $b !== null branch does NOT need the same fix. The issue in the $a !== null branch occurs because an IntegerRangeType in $types[$i] expands by absorbing a ConstantIntegerType scalar, and previously-checked scalars might now be adjacent to the expanded range. The $b !== null branch has the inverse relationship — the scalar $scalarTypeItems[$j] is supposed to absorb $types[$i]. But this path can never trigger the expanding-range scenario because:

  1. $scalarTypeItems contains only constant scalar types (ConstantIntegerType, ConstantStringType, etc.) — never IntegerRangeType, since IntegerRangeType::isConstantScalarValue() returns no and these are separated into $integerRangeTypes then merged back into $types.

  2. The [null, $b] return from compareTypesInUnion for the IntegerRangeType path (line 517-522) requires $b instanceof IntegerRangeType, which is never true for a scalar item.

  3. The other [null, $b] paths (SubtractableType, $b->isSuperTypeOf($a)) either don't apply to constant scalar types or return $b unchanged — the scalar doesn't grow.

  4. Since the scalar never expands, there's no need to re-check earlier $types entries.

I was unable to construct a test case that fails due to the $b !== null branch lacking a restart, which confirms the analysis.

No code changes are needed. Tests and static analysis both pass.

$scalarTypeItemsCount--;
$j = -1;
continue 1;
}
if ($b !== null) {
Expand Down
68 changes: 68 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-14610.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

namespace Bug14610;

use function PHPStan\Testing\assertType;

function test(): void
{
$value = 0;

if (isset($_SESSION['test'])) {
$value = rand(0,3);
if ($value == 1) {
}
}

assertType('int<0, 3>', $value);

if ($value == 0) {
assertType('array<mixed>', $_SESSION);
$result = isset($_SESSION['test']); // should not be reported as always exists
}
}

function testWithOtherSuperglobals(): void
{
$value = 0;

if (isset($_GET['key'])) {
$value = rand(0,3);
if ($value == 1) {
}
}

if ($value == 0) {
$result = isset($_GET['key']);
}
}

function testWithStrictComparison(): void
{
$value = 0;

if (isset($_SESSION['test'])) {
$value = rand(0,3);
if ($value === 1) {
}
}

if ($value === 0) {
$result = isset($_SESSION['test']);
}
}

function testWithDifferentKey(): void
{
$value = 0;

if (isset($_SESSION['test'])) {
$value = rand(0,3);
if ($value == 1) {
}
}

if ($value == 0) {
$result = isset($_SESSION['other']);
}
}
7 changes: 7 additions & 0 deletions tests/PHPStan/Rules/Variables/IssetRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,4 +574,11 @@ public function testBug14393(): void
]);
}

public function testBug14610(): void
{
$this->treatPhpDocTypesAsCertain = true;

$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14610.php'], []);
}

}
36 changes: 36 additions & 0 deletions tests/PHPStan/Type/TypeCombinatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1642,6 +1642,42 @@ public static function dataUnion(): iterable
IntegerType::class,
'int',
],
[
[
new UnionType([
new ConstantIntegerType(0),
IntegerRangeType::fromInterval(2, 3),
]),
new ConstantIntegerType(1),
],
IntegerRangeType::class,
'int<0, 3>',
],
[
[
new ConstantIntegerType(1),
new UnionType([
new ConstantIntegerType(0),
IntegerRangeType::fromInterval(2, 3),
]),
],
IntegerRangeType::class,
'int<0, 3>',
],
Comment on lines +1645 to +1666
Copy link
Copy Markdown
Contributor

@staabm staabm May 14, 2026

Choose a reason for hiding this comment

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

I think these tests make clear that a fix in TypeCombinator is valid (and the super-global example is just a "bad example" which is fixed by it at the same time)

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot May 14, 2026

Choose a reason for hiding this comment

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

All 12,068 tests pass (79,692 assertions) and make phpstan reports no errors.

The reviewer's comment is positive feedback — they're agreeing that the TypeCombinator unit tests clearly demonstrate the fix is valid on their own, independent of the superglobal scenario. No code changes are needed to address this comment.

[
[
new UnionType([
new ConstantIntegerType(0),
IntegerRangeType::fromInterval(2, 3),
]),
new UnionType([
new ConstantIntegerType(10),
new ConstantIntegerType(1),
]),
],
UnionType::class,
'10|int<0, 3>',
],
[
[
new MixedType(),
Expand Down
Loading