Propagate array dim fetch narrowings to parent variable in BooleanAnd falsey and BooleanOr truthy intersectWith

[phpstan-bot]

Summary

When using isset($arr['key']) && is_string($arr['key']) with an early return, PHPStan failed to narrow the array type in the falsey branch, leading to false positives like "Cannot assign new offset to list<int>|string". The same issue affected the logically equivalent !isset($arr['key']) || !is_string($arr['key']) pattern in the truthy branch.

Root cause

In the falsey branch of BooleanAnd (and truthy branch of BooleanOr), TypeSpecifier computes narrowings for the left and right conditions independently and combines them via SpecifiedTypes::intersectWith. The left side (e.g. falsey isset) produces a sureType for the parent variable $test, while the right side (e.g. falsey is_string) produces a sureType for the array dim fetch $test['hi']. Since intersectWith only keeps expressions present in both sides, both narrowings were dropped — different expression keys meant the intersection was empty.

The conditional expression holder mechanism (processBooleanSureConditionalTypes / processBooleanNotSureConditionalTypes) couldn't help either, because it only processes Expr\Variable nodes and skips ArrayDimFetch.

Changes

• Added TypeSpecifier::propagateArrayDimFetchNarrowingsToParent() that enriches normalized SpecifiedTypes by computing a parent array variable's narrowed type from its child ArrayDimFetch sureType using HasOffsetValueType — mirroring the propagation that MutatingScope::specifyExpressionType already does when narrowings are applied to a scope
  • src/Analyser/TypeSpecifier.php
• Applied the propagation in two locations before intersectWith:
  • BooleanAnd falsey handling (line ~737)
  • BooleanOr truthy handling (line ~799)
• Skips propagation when the parent expression is itself an ArrayDimFetch to avoid over-narrowing in nested offset access cases

Analogous cases probed

• BooleanOr truthy (!isset() || !is_string()): was broken, now fixed
• is_int, is_array, is_float, is_bool: all work correctly (same code path)
• instanceof: works correctly (same code path)
• Integer dim ($arr[0]): works correctly
• array_key_exists: not fixed by this PR — its falsey narrowing produces different SpecifiedTypes (includes an unsetOffset sureType) that requires separate handling
• Nested ArrayDimFetch ($arr['a']['b']): pre-existing bug, not made worse by this PR (propagation is skipped for nested parents)
• PropertyFetch: not applicable (different narrowing mechanism)

Test

Added tests/PHPStan/Analyser/nsrt/bug-14566.php with five test functions covering:

1. The original reproducer: isset($test['hi']) && is_string($test['hi']) with early return
2. The BooleanOr equivalent: !isset($test['hi']) || !is_string($test['hi'])
3. is_int variant
4. is_array variant
5. instanceof variant

Fixes phpstan/phpstan#14566

[VincentLanglet]

Would it make more sens to modify TypeSpecifier::createForExpr instead in order to add a special case for $expr instanceof ArrayDimFetch ?

[phpstan-bot]

All tests pass and static analysis is clean. Now let me provide the response to the reviewer's question. There are no code changes needed since the reviewer's suggestion isn't architecturally feasible. Let me explain why in a clear way that can be posted to the PR.

The reviewer's suggestion to modify createForExpr doesn't work for this case. Here's the analysis:

Why createForExpr is not the right place:

The core issue is about when the actual narrowed type is available:

1. In the falsey context (which is where the bug manifests), createForExpr produces a sureNotType — e.g., for is_string($test['hi']) in falsey context, it creates sureNotTypes['$test['hi']'] = StringType (meaning "dim fetch is NOT string")

2. To propagate to the parent array, we need the actual narrowed type of the dim fetch (what it IS, not what it ISN'T). For example, if $test['hi'] had type string|array{0: 42} in scope, the narrowed type after "NOT string" is array{0: 42}. Only then can we compute HasOffsetValueType('hi', array{0: 42}) to narrow the parent.

3. This "NOT type → actual type" resolution happens in normalize(), which converts sureNotTypes to sureTypes via TypeCombinator::remove(scope_type, sureNotType).

4. Therefore, propagation to parent must happen after normalize(), which is exactly where the current code places it — between normalization and intersectWith.

Moving it to createForExpr would require duplicating the normalization logic (resolving sureNotTypes against scope) inside that method, which would be both less clean and incorrect in some edge cases (e.g., when multiple sureNotTypes for the same expression are combined before normalization).

The current placement — a post-normalization enrichment step right before intersectWith — is the architecturally correct location because intersectWith is where the information loss occurs (it drops expressions not present in both sides), and the propagation ensures the parent expression IS present in both sides.

Since there are no code changes to make (the existing implementation is correct and the reviewer's question is answered by this technical explanation), there's nothing to commit or push. The explanation above should be posted as a reply to the review comment.

[VincentLanglet]

I refactored into createForExpr to handle ArrayDimFetch with the latest commit

Analyse the benefit and the drawback compare to your solution, and propose suggestions/improvements, without implementing them yet

[phpstan-bot]

Both produce the same result for non-constant arrays. The original approach's propagateArrayDimFetchNarrowingsToParent also doesn't help here because for array<string, string|int>, getConstantArrays() is empty. Wait, the original approach doesn't use getConstantArrays() — it uses HasOffsetValueType. Let me re-read it.

Looking back at the original code: it checks $parentType instanceof MixedType || !$parentType->isArray()->yes() and uses TypeCombinator::intersect($parentType, new HasOffsetValueType($dimType, $type)). For array<string, string|int>, the intersect with HasOffsetValueType('hi', int) (the normalized dim fetch type after removing string) would produce array<string, string|int> & hasOffsetValue('hi', int). But the isset on the left side produces a sureType for $test = non-empty-array<string, string|int>, and the right side's propagation produces a sureType for $test = array<string, string|int> & hasOffsetValue('hi', int). When intersectWith unions them: non-empty-array<string, string|int> | (array<string, string|int> & hasOffsetValue('hi', int)) = array<string, string|int>. So the narrowing is still lost due to the union in intersectWith.

Actually, wait. Let me re-read intersectWith. It takes the union of sureTypes for the SAME expression key. The left side (isset falsey) produces a sureType for $test and the right side (is_string falsey propagated) also produces a sureType for $test. So they'd be unioned. And union(parent_from_left, parent_from_right) is indeed wider than either alone.

The behavior is the same for both approaches with non-constant arrays, so this isn't a practical differentiator.

Now I have a thorough understanding. The review asks for analysis only, not implementation. Let me write the analysis as the response to the PR.

Since the review asks me to "analyse the benefit and drawback" and "propose suggestions/improvements, without implementing them yet" — this is a pure analysis task with no code changes needed. There's nothing to commit or push.

Here's my analysis of the two approaches:

---

Analysis: createForExpr approach vs propagateArrayDimFetchNarrowingsToParent

How they work

Original (propagateArrayDimFetchNarrowingsToParent): Runs post-normalization, right before intersectWith. Iterates over already-normalized sureTypes, finds ArrayDimFetch entries, and computes the parent's narrowed type via TypeCombinator::intersect($parentType, new HasOffsetValueType($dimType, $narrowedDimType)). Adds the parent as an additional sureType so it survives intersectWith.

New (createForExpr): Emits a sureNotType for the parent at the point where the dim-fetch narrowing is first created. Decomposes the parent into getConstantArrays(), then checks which constant array variants can be eliminated entirely (offset value is fully covered by the type check in falsey context, or fully incompatible in truthy context). The eliminated variants are added as a sureNotType on the parent, which normalize() then resolves.

Benefits of createForExpr

1. Broader scope: The narrowing propagates everywhere create() is called — not just BooleanAnd falsey / BooleanOr truthy. This means standalone is_string($arr['key']) also emits parent narrowing information, which could improve narrowing in more contexts (e.g., ternary expressions, match arms).

2. Co-located logic: The parent narrowing is defined next to the child expression narrowing, making it easier to understand what createForExpr produces for an ArrayDimFetch.

3. Simpler call sites: BooleanAnd/BooleanOr code stays as normalize().intersectWith() without needing a post-processing step. No separate method to maintain.

4. Participates in unionWith for free: Since the sureNotType is part of the returned SpecifiedTypes, it works correctly with unionWith (truthy BooleanAnd path) too.

Drawbacks of createForExpr

1. Only handles constant arrays: The getConstantArrays() gate means non-constant-array parents (e.g., array<string, string|int>) get no parent narrowing. The original approach uses HasOffsetValueType which works for any array type. In practice both approaches produce the same result for non-constant arrays in this specific BooleanAnd falsey scenario (because intersectWith's union swallows the refinement), but HasOffsetValueType is the more general mechanism.

2. Cannot handle partial overlap: When a constant array has an offset value that's a union type (e.g., string|int) and the type check only covers part of it (is_string in falsey context), isSuperTypeOf($offsetValueType)->yes() returns no (it's maybe), so the constant array is not removed. The original approach handles this correctly because HasOffsetValueType('hi', int) (the narrowed dim type after removing string) intersects with the constant array to refine the offset value type rather than removing the whole variant.

   Concrete regression: array{}|array{hi: string|int}|array{hi: float} after if (isset($test['hi']) && is_string($test['hi'])) return; — the original approach produces array{}|array{hi: float}|array{hi: int} but the createForExpr approach produces array{}|array{hi: float}|array{hi: int|string} (fails to narrow string|int to int).

3. instanceof ConstantIntegerType: Uses $dimType instanceof ConstantIntegerType, which violates the project convention of not using instanceof to check types. (The original approach had the same issue but acknowledged it with // @phpstan-ignore phpstanApi.instanceofType.)

4. Missing toArrayKey() call: The original approach applies $dimType->toArrayKey() before checking the dim type, ensuring numeric strings are correctly coerced to integer keys. The createForExpr approach skips this, which could cause edge cases with numeric string dimensions.

Suggestions for improvement

1. Use HasOffsetValueType refinement instead of constant-array elimination: Instead of removing entire constant array variants via sureNotTypes, emit a sureType for the parent computed via TypeCombinator::intersect($parentType, new HasOffsetValueType($dimType, $narrowedType)). This requires knowing the narrowed dim type, which in a false context means computing TypeCombinator::remove($scope->getType($expr), $type) — essentially doing part of normalization inline. This would handle both partial overlap and non-constant arrays.

2. Alternatively, keep createForExpr placement but change the strategy: In the false context, compute $narrowedOffsetType = TypeCombinator::remove($offsetValueType, $type) for each constant array's offset value. If $narrowedOffsetType is never, remove that variant (as now). If it's different from $offsetValueType, build a refined constant array with the narrowed offset type and emit that as a sureType entry. This would handle partial overlap while keeping the createForExpr location.

3. Add toArrayKey(): Apply $dimType = $dimType->toArrayKey() before the constant integer / constant string check, matching the original approach.

4. Replace instanceof ConstantIntegerType: Use $dimType->isInteger()->yes() && $dimType->getConstantScalarValues() !== [] or similar pattern.

5. Consider whether broader scope is desirable: The createForExpr approach fires for every type specifier context. Verify whether emitting parent sureNotTypes in the truthy context (inside if (is_string($arr['key']))) causes any unintended interactions — the types are correct in my testing, but the broader blast radius warrants careful review.

[VincentLanglet]

Try the Use HasOffsetValueType refinement instead of constant-array elimination suggestion, and add a dedicated test which

• would have fail with the current implementation
• which does not fail with your suggestion

[ondrejmirtes]

@VincentLanglet Please try to replicate the problem outside of arrays and ArrayDimFetch, for example with nested property fetches. I'm still not sure why we need hardcoding for arrays and nothing else.

[phpstan-bot]

Done. Here's a summary of what was implemented:

Changed createForExpr in TypeSpecifier.php to use HasOffsetValueType refinement instead of constant-array elimination:

• Old approach: Removed entire constant array variants via sureNotTypes only when $type->isSuperTypeOf($offsetValueType)->yes() (full coverage). Couldn't handle partial overlap.
• New approach: For each constant array variant, computes the narrowed offset value type using TypeCombinator::remove() (false context) or TypeCombinator::intersect() (true context). If the result is never, drops the variant. If different from original, rebuilds the constant array with the refined value via setExistingOffsetValueType(). Emits the refined union as a sureType.

Added partial overlap test (fooPartialOverlap and fooPartialOverlapOr) that demonstrates the improvement: array{hi: string|int} after is_string($arr['hi']) in false context correctly narrows to array{hi: int} — this fails with the old approach.

Updated bug-7716 expectations: The new approach also correctly propagates comparison-based refinements (e.g., $array['foo'] > 1 → int<2, max>) to the parent array, making it consistent with the dim fetch's own narrowed type.

[VincentLanglet]

I feel like the issue is well defined with the example

/**
 * @param array{}|array{hi: 'hello'}|array{hi: array{0: 42, 1?: 42}} $test
 */
function foo(array $test): void {
	if (isset($test['hi'])) {
		if (is_string($test['hi'])) {
			return;
		}
	}
	
	dumpType($test['hi']);
}

/**
 * @param array{}|array{hi: 'hello'}|array{hi: array{0: 42, 1?: 42}} $test
 */
function foo2(array $test): void {
	if (isset($test['hi']) && is_string($test['hi'])) {
		return;
	}
	
	dumpType($test['hi']);
}

While the first one works correctly, the second doesn't. It does not seems to be related to array.

[phpstan-bot]

All passing. Here's a summary of what was done to address the review:

The reviewer's concern: The && vs nested if narrowing discrepancy isn't array-specific — it's a general issue affecting property fetches and other compound expressions too.

Investigation findings:

• Confirmed: property fetches like $c->x instanceof Foo && $c->y instanceof Foo have the same issue — nested if correctly narrows $c->y on a follow-up $c->x instanceof Foo check, but && doesn't
• Root cause: processBooleanSureConditionalTypes and processBooleanNotSureConditionalTypes only created conditional expression holders for Expr\Variable nodes, ignoring PropertyFetch, ArrayDimFetch, and StaticPropertyFetch

Changes made:

1. Extended conditional holders to non-Variable expressions (TypeSpecifier.php): Both processBooleanSure/NotSureConditionalTypes now accept PropertyFetch, ArrayDimFetch, and StaticPropertyFetch as conditions and targets via a new isTrackableExpression() helper. The self-reference check was simplified from Variable-name comparison to expression-string comparison.

2. Removed redundant checks detected by improved inference (NodeScopeResolver.php, ConstantArrayTypeBuilder.php): The extended holders correctly narrow types in PHPStan's own code, revealing genuinely redundant is_string() checks and dead count() === 0 branches.

3. Added tests for property fetch conditional narrowing in bug-14566.php.

The array-specific createForExpr propagation is kept because arrays need structural type propagation (parent type from child dim fetch narrowing via HasOffsetValueType), which is fundamentally different from the conditional holder mechanism. Property fetches don't have equivalent structural types but now benefit from conditional holders for follow-up checks.