Skip to content

Do not match conditional expression guards when ConstantArrayType has extra keys#5640

Open
phpstan-bot wants to merge 6 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-4lgn6f2
Open

Do not match conditional expression guards when ConstantArrayType has extra keys#5640
phpstan-bot wants to merge 6 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-4lgn6f2

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Closes phpstan/phpstan#14595

Summary

When a function had multiple if blocks that conditionally appended to an array, PHPStan could incorrectly narrow an unrelated array offset type. The issue required: (1) array offsets accessed from a typed array parameter, (2) conditional logic creating type guards, and (3) multiple if blocks that grew a local array variable.

Root cause

In MutatingScope::filterBySpecifiedTypes Pass 2 (supertype match), the guard-matching logic used isSuperTypeOf to check whether the current type of a variable satisfies a conditional expression guard. For ConstantArrayType, isSuperTypeOf uses structural subtyping — array{0: 'foo'}->isSuperTypeOf(array{0: 'foo', 1: 'bar'}) returns Yes because the subtype has all required keys. This is correct for method signatures and type acceptance, but too permissive for guard matching: a 2-element array should not match a guard expecting a 1-element array.

This caused a chain reaction: narrowing $options to a specific shape fired a conditional expression that narrowed $instructions, and then a stale guard on $instructions (with fewer keys than the actual type) incorrectly fired, narrowing $options['multiple'] from 0|1|2 to 1.

Fix

After the existing isSuperTypeOf check in Pass 2, add a targeted check for ConstantArrayType guards: verify that the current type's constant arrays do not have keys absent from the guard. If any current array has a key that the guard array lacks, skip the conditional expression.

This preserves structural subtyping for all other contexts (accepts, method signatures, general isSuperTypeOf usage) while preventing incorrect guard matching.

Test plan

  • New regression test tests/PHPStan/Analyser/nsrt/bug-14595.php reproduces the exact issue from the bug report
  • All 12,049 existing tests pass (no regressions)
  • make phpstan passes (0 errors)
  • make cs-fix applied (early-exit style)

… extra keys

In filterBySpecifiedTypes Pass 2 (supertype match), isSuperTypeOf with
structural subtyping for ConstantArrayType allowed a current type with
extra keys to satisfy a guard expecting fewer keys. This caused
incorrect type narrowing through a chain of conditional expressions.

Closes phpstan/phpstan#14595
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

This is not related to extra key because this can be reproduced with

/**
 * @param array<mixed> $data
 * @param array{
 *     multiple: 0|1|2
 *  } $options
 */
function formulaire_edition(array $data, array $options, bool $total): void {
    $instructions = [ ];
    $instructions[] = "foo";
    if ($options['multiple'] != 1 || $total) {
        $instructions[] = "bar";
	}
    \PHPStan\dumpType($options['multiple']); // 0|1|2 (ok)
    if (!$total) {
        $instructions[] = "baz";
	}
    \PHPStan\dumpType($options['multiple']); // 0|1|2 (ok)
    if (!$total) {
        $instructions[] = "qux";
	}
    \PHPStan\dumpType($options['multiple']); // should be: 0|1|2
}

But notice

/**
 * @param array<mixed> $data
 * @param 0|1|2 $options
 */
function formulaire_edition(array $data, int $options, bool $total): void {
    $instructions = [ ];
    $instructions[] = "foo";
    if ($options != 1 || $total) {
        $instructions[] = "bar";
	}
    \PHPStan\dumpType($options); // 0|1|2 (ok)
    if (!$total) {
        $instructions[] = "baz";
	}
    \PHPStan\dumpType($options); // 0|1|2 (ok)
    if (!$total) {
        $instructions[] = "qux";
	}
    \PHPStan\dumpType($options); // should be: 0|1|2
}

works fine

The bug also reproduces when `total` is a separate `bool` parameter
instead of a key in the `$options` array. The existing fix handles
this case because the extra-keys check applies to `$instructions`
(the guard condition variable that grows), not to `$options`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's a summary of the investigation and response to the review:

The reviewer's observation is correct — the bug reproduces with their variant where total is a separate bool parameter (not a key in $options). However, the existing fix already handles their case.

The reason: the "extra keys" check in the fix applies to $instructions (the guard condition variable that grows via $instructions[] = ...), not to $options. When $instructions grows from array{'foo'} to array{'foo', 'bar', 'baz'}, the current type has keys absent from the guard — that's what the check catches, regardless of whether $options has one key or two.

I verified this by:

  1. Checking the reviewer's case on main (without fix) → line 27 incorrectly shows 1
  2. Checking the reviewer's case on this branch (with fix) → correctly shows 0|1|2
  3. Running the full test suite (12,049 tests pass) and make phpstan (0 errors)

I added the reviewer's variant as an additional regression test to cover this scenario explicitly.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

I don't fully understand this fix. Can't the issue happen in other case than constantArray ? Can't the fix be simpler or more generic ?

And what if hasOffsetValueType return Maybe ?

Instead of checking individual keys with hasOffsetValueType(), compare
key counts. Since isSuperTypeOf() already verified all guard keys exist
in the current type, any extra keys manifest as a higher key count.

This addresses the reviewer's concern about hasOffsetValueType()
returning Maybe and simplifies the logic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's the summary for the reviewer's questions:

1. "Can't the issue happen in other cases than ConstantArray?"

No. The issue is specific to ConstantArrayType::isSuperTypeOf() structural subtyping, which ignores extra keys in the subtype (it only iterates the super type's keys). For other types, narrowing (e.g. stringnon-empty-string) produces a true subtype where the guard should fire — the narrowed value IS a string. Arrays are unique because they "grow" (adding keys = fundamentally different program state) rather than "narrow". The reviewer's own example confirmed this: replacing $options array access with a plain int parameter eliminates the bug.

2. "Can't the fix be simpler or more generic?"

Simplified the fix to use key count comparison instead of per-key hasOffsetValueType() checks. Since isSuperTypeOf() already verified all guard keys exist in the current type, any extra keys manifest as a higher key count — no need to check individual keys. A fully generic fix (e.g. bidirectional isSuperTypeOf) would break legitimate guard matching for type narrowing (e.g. string guard wouldn't match non-empty-string current type).

3. "What if hasOffsetValueType returns Maybe?"

Eliminated this concern entirely by switching to key count comparison, which doesn't use hasOffsetValueType() at all.

All 12,049 tests pass, make phpstan reports 0 errors, and make cs-fix is clean.

Comment thread src/Analyser/MutatingScope.php Outdated
$hasExtraKeys = false;
foreach ($specifiedExpressions[$holderExprString]->getType()->getConstantArrays() as $currentConstantArray) {
if (count($currentConstantArray->getKeyTypes()) > $guardKeyCount) {
$hasExtraKeys = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hasExtraKeys var is unneeded, you could just continue 3

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Both review comments addressed:

  1. Removed $hasExtraKeys flag — replaced with continue 3 directly from the inner loop, eliminating the flag variable and the separate if check.

  2. Support multiple guard constant arrays — changed count($guardConstantArrays) !== 1 to count($guardConstantArrays) === 0 (only skip when there are no constant arrays), and compare current key counts against max() across all guard constant arrays instead of only handling the single-array case.

All 12,049 tests pass and make phpstan reports 0 errors.

Comment thread src/Analyser/MutatingScope.php Outdated
}

$guardConstantArrays = $conditionalTypeHolder->getType()->getConstantArrays();
if (count($guardConstantArrays) !== 1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we only support single constant array ?

Address review feedback:
- Use `continue 3` directly instead of a flag variable
- Support multiple guard constant arrays by comparing against the max
  key count, instead of skipping when count !== 1

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread src/Analyser/MutatingScope.php
… key counts

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Function max() should not be referenced via a fallback global name, but via a use statement.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Added use function max; to the import list in MutatingScope.php so max() is referenced via a proper use function statement instead of a fallback global name. Tests (12,049) and make phpstan both pass.

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.

2 participants