Fix phpstan/phpstan#14390: Unconsistant behavior for offsetAccess.notFound by phpstan-bot · Pull Request #5310 · phpstan/phpstan-src

phpstan-bot · 2026-03-27T14:26:07Z

Summary

When using $obj->property[array_key_first($obj->property)] after checking the array is non-empty, PHPStan incorrectly reported "Offset string might not exist on non-empty-array". The same code worked fine with local variables. This fix extends the special-case offset validation to handle any expression type, not just variables.

Changes

Modified src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php:
- Injected ExprPrinter dependency
- Replaced instanceof Variable + name comparison with ExprPrinter::printExpr() comparison for the array_key_first/array_key_last, array_rand, and count()-1 special cases
Updated tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php to pass the new ExprPrinter dependency
Added tests/PHPStan/Rules/Arrays/data/bug-14390.php regression test

Root cause

The NonexistentOffsetInArrayDimFetchRule had special-case logic to suppress false positives for patterns like $arr[array_key_first($arr)] on non-empty arrays. However, it compared expressions by checking that both were Variable nodes with the same name. This meant property fetches like $sample->fields[array_key_first($sample->fields)] were not recognized as the same expression, causing false positive errors.

The fix uses ExprPrinter to compare the printed representation of expressions, which works for any expression type (variables, property fetches, static property fetches, etc.).

Test

Added regression test tests/PHPStan/Rules/Arrays/data/bug-14390.php that covers:

Property access with array_key_first() on non-empty array (the reported bug)
Property access with array_key_last() on non-empty array
Variable access with array_key_first() (existing working case)
Property access with intermediate variable (existing working case)

Fixes phpstan/phpstan#14390

staabm

the PR reminded me that ondrej once said "using instanceof Variable" most of the time is wrong, because if you use it your logic likely misses other expressions :-).

change looks good

src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php

tests/PHPStan/Rules/Arrays/data/bug-14390.php

staabm

usually expr-printing can be slow. but since we use it only after some very specific AST based instanceof checks I think this is a solid improvement

…essions - The special-case logic in NonexistentOffsetInArrayDimFetchRule that suppresses false positives for $arr[array_key_first($arr)] only matched Variable AST nodes - Changed expression comparison to use ExprPrinter so it works with property fetches, static property fetches, and any other expression type - Also fixed the same limitation in the array_rand and count()-1 special cases - New regression test in tests/PHPStan/Rules/Arrays/data/bug-14390.php

Addresses review feedback: the ExprPrinter changes for array_rand and count()-1 special cases were not covered by tests. Adds test cases for property access with array_rand() and count()-1 on non-empty arrays. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

… checks Covers array_key_first, array_key_last, array_rand, and count()-1 special cases with static properties, as requested in review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

VincentLanglet

Without modifying the code, can you tell me

If there is other place where the same logic (exprPrinter) is used and/or if another strategy is used
If there is other place where the Variable comparison should be replaced by an exprPrinter check

phpstan-bot · 2026-03-28T14:28:43Z

The GitHub token doesn't have permission to comment on the upstream repo. Since this is a research-only review (no code changes requested), let me summarize the findings here:

VincentLanglet's Review - Research Findings

1. Where `ExprPrinter` is used for expression comparison / alternative strategies

Direct printExpr() comparison: Only in NonexistentOffsetInArrayDimFetchRule.php (this PR's 3 changes).
MutatingScope::getNodeKey(): The primary expression comparison strategy. It uses printExpr() internally but adds extra context (file position markers, void closure markers). Used extensively for expression type tracking.
Other rules use ExprPrinter only for error messages, not comparison (DuplicateKeysInLiteralArraysRule, NoopRule, etc.).
Object identity (===): Used in NullsafeOperatorHelper — different use case (same instance check after transformation).

2. Candidates where `instanceof Variable` + name comparison could be replaced

Location	Pattern	Candidate?
`src/Type/Php/ArrayCombineHelper.php:102`	`$firstArg instanceof Variable && $secondArg instanceof Variable && $firstArg->name === $secondArg->name`	Strongest candidate — same class of bug, `array_combine($obj->keys, $obj->keys)` wouldn't be recognized
`src/Analyser/ExprHandler/AssignHandler.php:1190`	`isSameVariable()` helper, Variable-only	Possible candidate, depends on callers
`src/Analyser/RicherScopeGetTypeHelper.php:32-39`	`$x === $x` always true, Variable-only	Already has separate PropertyFetch handling below, less impactful
`src/Analyser/ExprHandler/BinaryOpHandler.php:104-113`	`$x == $x` always true, Variable-only	Same consideration — identity semantics for properties need care (side effects)

The reviewer's question is informational — no code changes were requested. The analysis is complete. Since I can't post to the upstream PR, the findings are documented above. Would you like me to try pushing these findings some other way?

VincentLanglet · 2026-03-28T18:58:36Z

tests/PHPStan/Rules/Arrays/data/bug-14390.php

+
+function doWithMethods(WithMethods $withMethods) {
+	echo $withMethods->pureMethod()[array_key_first($withMethods->pureMethod())];
+	echo $withMethods->impureMethod()[array_key_first($withMethods->impureMethod())];


This line should expect an error because $withMethods->impureMethod() might not return the same array in the two calls even if exprPrinter->printExpr gives the same result.

So you have to find a better fix.

The background make phpstan task also completed successfully (exit 0) — confirming no static analysis errors with the fix.

… calls ExprPrinter comparison alone is insufficient because impure method calls like `$obj->impureMethod()` may return different values on each invocation, even though the printed expression is identical. Add isDeterministicExpr() to verify the expression is safe to compare: variables, property fetches, static property fetches, and pure method/static calls are deterministic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

VincentLanglet

For MethodCall and StaticCall, this doesn't check the params.

An example $foo->pureMethod($foo->impureMethod())[array_key_first($foo->pureMethod($foo->impureMethod()))]

If we start supporting complex expression, we should support dimFetch like

$sample->fields[1][array_key_first($sample->fields[1])];

be aware that if the key is non determinist like

$sample->fields[$i++][array_key_first($sample->fields[$i++])];

or

$sample->fields[$this->getImpure()][array_key_first($sample->fields[$this->getImpure()])];

this should still report an error

…rministicExpr - Check that MethodCall/StaticCall arguments are also deterministic, not just the method purity. E.g. $foo->pureMethod($foo->impureMethod()) is not deterministic because the impure argument may produce different values. - Support ArrayDimFetch expressions (e.g. $nested[$key][array_key_first(...)]), checking both the array and the dim key for determinism. - Add test cases for pure method with impure args, deterministic dim fetch, and non-deterministic dim fetch with impure method key. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

phpstan-bot · 2026-03-28T19:30:12Z

Pushed. Here's what was done to address VincentLanglet's review:

Method args determinism check: isDeterministicExpr now checks that all arguments to MethodCall and StaticCall are also deterministic via a new areDeterministicArgs() helper. This catches cases like $foo->pureMethod($foo->impureMethod()) where the method is pure but an argument is impure.
ArrayDimFetch support: Added handling for Expr\ArrayDimFetch in isDeterministicExpr — it checks both the array expression and the dimension key are deterministic. This supports $nested[$key][array_key_first($nested[$key])] while correctly rejecting $nested[$this->getImpure()][...].
Test cases added: Pure method with impure args (expects error), deterministic dim fetch with a variable key (no error), and non-deterministic dim fetch with impure method key (expects error).

VincentLanglet · 2026-03-28T19:36:50Z

@staabm While I find the fix correct, I also think this fix might be too "complicated".
This might need a human to find a better way to replace isDeterministic method by something reusable.
I would be tempted to let/close this and look for easier fix

staabm requested changes Mar 28, 2026

View reviewed changes

src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php Outdated Show resolved Hide resolved

src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php Outdated Show resolved Hide resolved

staabm reviewed Mar 28, 2026

View reviewed changes

tests/PHPStan/Rules/Arrays/data/bug-14390.php Show resolved Hide resolved

staabm approved these changes Mar 28, 2026

View reviewed changes

phpstan-bot and others added 3 commits March 28, 2026 13:06

Add test coverage for static property access patterns in array offset…

3bb9cdb

… checks Covers array_key_first, array_key_last, array_rand, and count()-1 special cases with static properties, as requested in review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

staabm force-pushed the create-pull-request/patch-yi96520 branch from 1f27903 to 3bb9cdb Compare March 28, 2026 12:06

staabm requested a review from VincentLanglet March 28, 2026 12:06

VincentLanglet reviewed Mar 28, 2026

View reviewed changes

staabm added 2 commits March 28, 2026 19:27

test pure/impure methods

978a497

impureMethod expects error

a09c9df

VincentLanglet reviewed Mar 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix phpstan/phpstan#14390: Unconsistant behavior for offsetAccess.notFound#5310

Fix phpstan/phpstan#14390: Unconsistant behavior for offsetAccess.notFound#5310
phpstan-bot wants to merge 7 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-yi96520

phpstan-bot commented Mar 27, 2026

Uh oh!

staabm left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

staabm left a comment

Uh oh!

VincentLanglet left a comment

Uh oh!

phpstan-bot commented Mar 28, 2026

Uh oh!

VincentLanglet Mar 28, 2026

Uh oh!

phpstan-bot Mar 28, 2026

Uh oh!

VincentLanglet left a comment

Uh oh!

phpstan-bot commented Mar 28, 2026

Uh oh!

VincentLanglet commented Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

phpstan-bot commented Mar 27, 2026

Summary

Changes

Root cause

Test

Uh oh!

staabm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

staabm left a comment

Choose a reason for hiding this comment

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Mar 28, 2026

VincentLanglet's Review - Research Findings

1. Where ExprPrinter is used for expression comparison / alternative strategies

2. Candidates where instanceof Variable + name comparison could be replaced

Uh oh!

VincentLanglet Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

phpstan-bot Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Mar 28, 2026

Uh oh!

VincentLanglet commented Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. Where `ExprPrinter` is used for expression comparison / alternative strategies

2. Candidates where `instanceof Variable` + name comparison could be replaced