Skip to content

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

Closed
staabm wants to merge 4 commits intophpstan:2.1.xfrom
staabm:constant-array-set
Closed

Preserve constant array when setting a union of constant scalar keys#4528
staabm wants to merge 4 commits intophpstan:2.1.xfrom
staabm:constant-array-set

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Nov 6, 2025

closes #4520

@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm
Copy link
Contributor Author

staabm commented Nov 6, 2025

ok, I see now that this new fix no longer fixes issues, which were reported by issue-bot in the previous PR.
will look into it.

@staabm staabm force-pushed the constant-array-set branch from 8cbe785 to 7964b98 Compare November 6, 2025 17:04
@staabm staabm marked this pull request as draft November 6, 2025 17:04
@staabm staabm force-pushed the constant-array-set branch from 7964b98 to 95ac439 Compare February 15, 2026 07:51
@staabm staabm force-pushed the constant-array-set branch from 95ac439 to bbe90ae Compare February 16, 2026 09:04
@staabm
Copy link
Contributor Author

staabm commented Feb 16, 2026

@phpstan-bot please debug why some existing tests are failing and some errors reappear?

@phpstan-bot
Copy link
Collaborator

phpstan-bot commented Feb 16, 2026

Claude finished @staabm's task in 6m 14s —— View job


Debugging Test Failures

  • Review the PR changes
  • Analyze CI failures (6 failures found)
  • Analyze all 6 failures and identify root cause pattern
  • Check issue-bot error reappearances (issue-bot passed)
  • Write up debugging analysis

Root Cause Analysis

The PR adds union-splitting logic in both ConstantArrayType::setOffsetValueType() and setExistingOffsetValueType() (lines 696-708, 718-729 of src/Type/Constant/ConstantArrayType.php). When the offset type is a union of N constant scalars, instead of passing the union to the builder (which already handled unions), it now creates N separate constant arrays and returns their union.

The core problem: This interacts badly with loop analysis and multi-step array construction. Here's why:

Problem 1: Combinatorial explosion in loops

When an array is assigned in a loop with a union key type, each iteration compounds the number of union members:

// pr-4390.php: for ($i = 0; $i < 10; $i++) { $locations[$i] = []; ... }

The loop analyzer runs the body multiple times and merges results. With the old code, $locations[$i] with $i: int<0, 9> was handled by the builder as a single operation, producing non-empty-array<int<0, 9>, ...>.

With the new code, when int<0, 9> is resolved to 10 constant scalars, setOffsetValueType creates 10 separate array types. On the next loop iteration, UnionType::setOffsetValueType distributes across all 10 members, each producing 10 more → 100 arrays. This quickly exceeds ARRAY_COUNT_LIMIT (256) or causes the generalizer to kick in, producing a less precise result than before: int<0, max> instead of int<0, 9>.

CI failure: pr-4390.php line 16:

Expected: non-empty-array<int<0, 9>, ...>
Actual:   non-empty-array<int<0, max>, ...>

Problem 2: Loss of type precision in foreach with constant array keys

// bug-11846.php
$idList = [1, 2];
foreach ($idList as $id) {     // $id: 1|2
    $outerList[$id] = [];      // splits into 2 arrays
    array_push($outerList[$id], []);  // now operates on a union, loses track
}

With the old code, after the loop, $outerList was non-empty-array<1|2, array{array{}}>. Now, each assignment splits the array into a union of constant arrays. When the loop iterations are merged, the types degrade because:

  1. $outerList[$id] = [] creates array{1: array{}} | array{2: array{}} (both with the other key optional)
  2. array_push($outerList[$id], []) then needs to resolve $outerList[$id] on a union type, producing array{}|array{array{}} instead of array{array{}}

CI failures: bug-11846.php lines 16, 21, 25 — all showing degraded types.

Problem 3: New false positive errors (bug-7594, bug-4829, bug-12115)

These tests expected no errors but now produce errors because the union-splitting causes the array types to degrade during loop/conditional processing:

  • bug-7594: A loop builds $boolAbilities[$i] where $i ranges 0-17 (with skips). The union splitting causes the key type to widen from self::ABILITY_* to int<0, max>, making the result incompatible with the expected array<self::ABILITY_*, bool>.

  • bug-4829: Array built in a foreach with string keys. The splitting causes optional keys to appear (cat2?: 'blah2'), triggering Offset 'cat2' might not exist errors.

  • bug-12115: Nested array construction in a loop loses track of string index keys after splitting, producing non-empty-array<literal-string&non-falsy-string, int> instead of a constant array, triggering Offset might not exist.

Problem 4: Dramatically fewer errors detected (bug-7903)

Expected: 24 errors
Actual:   7 errors

The bug-7903.php test has a deeply nested constant array ($results) with many levels. The PR's union-splitting amplifies when doing nested array assignments like $results['ddddd'][$key][0][0]['oooo_rrrrr'] += ... where $key: 'bbbbb'|'eeeee'. The splitting cascades through nested dimensions, ultimately causing the array to degrade to a general type. Once degraded, offset access errors that would normally be caught are no longer reported.

Why the Issue-Bot Passed

The issue-bot tests passed because they test different patterns — likely not involving union-key assignments on constant arrays inside loops.

The Fundamental Issue

The ConstantArrayTypeBuilder::setOffsetValueType already handles union types internally (lines 209-264 of the builder). It iterates the constant scalars and updates each matching key's value type in-place, keeping a single constant array. The PR's approach of splitting at the ConstantArrayType level creates N separate arrays instead, which then compound multiplicatively when:

  1. UnionType::setOffsetValueType/setExistingOffsetValueType distributes across all union members (line 788-793 of UnionType.php)
  2. Loop iterations merge via TypeCombinator::union, adding more union members each time
  3. Nested array dimensions each apply splitting independently

Suggested Fix Direction

The union-splitting approach works well for the simple, non-loop case (like the new test set-constant-union-offset-on-constant-array.php). The challenge is preventing it from causing degradation in loops.

Possible approaches:

  1. Only split in setOffsetValueType, not setExistingOffsetValueType — existing offsets don't need splitting since the builder handles them
  2. Guard against splitting when the result would exceed a threshold — e.g., check if count($constantScalars) * count(existing_union_members) would be too large
  3. Move the splitting logic into the builder itself — have ConstantArrayTypeBuilder optionally return a union when all constant scalars are known, but only when it won't cause combinatorial blowup
  4. Only apply splitting when not inside a loop context — though this would require threading context information

The simplest approach might be option 2: add a guard that checks whether $this is already from a union context or whether the number of resulting arrays would exceed a reasonable limit.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants