Skip to content

Do not treat conditional-expression entries about a property fetch as proof it is initialized - #6183

Merged
staabm merged 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-mjsjnrk
Aug 6, 2026
Merged

Do not treat conditional-expression entries about a property fetch as proof it is initialized#6183
staabm merged 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-mjsjnrk

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

$this->foo ?? null was reported as nullCoalesce.unnecessary ("Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.") when the typed property $this->foo had only been conditionally initialised earlier in the same scope — even though it may still be uninitialised at that point.

The check that decides whether the left side of ?? is always set now no longer treats the mere presence of conditional-expression entries about a property fetch as proof that the property is initialised.

Changes

  • src/Analyser/IssetabilityResolution.php — removed the !$link->hasConditionalExpressionsOfFetch() clause from the property branch of isSet(), restoring exact parity with MutatingScope::issetCheck().
  • src/Analyser/IssetabilityLinkInfo.php — removed the now-unused hasConditionalExpressionsOfFetch constructor parameter, factory argument and getter.
  • src/Analyser/IssetabilityDescriptor.php — stopped computing isset($scope->getConditionalExpressions()[$scope->getNodeKey($propertyFetch)]).
  • tests/PHPStan/Rules/Variables/data/bug-15046.php, tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php — regression test.

Analogous cases that reproduced the same false positive and are fixed by the same change (all covered by the new test data file):

  • static properties (self::$answer ?? null)
  • a plain bool condition instead of a !== null narrowing
  • ??= ($this->answer ??= null)
  • conditional assignment via a ternary
  • conditional assignment inside a switch branch
  • nested chains ($this->inner->deep ?? null)

Probed and found already correct, so no test was kept for them:

  • isset() and empty() on a conditionally initialised property — these render through Rules\IssetCheck::doCheck(), which never consulted conditional expressions.
  • The inferred types of $this->answer ?? null, isset($this->answer) and empty($this->answer) — computed by MutatingScope::issetCheck(), which also never consulted conditional expressions.
  • Conditional assignment inside try/catch, while and for — these do not produce conditional-expression entries.
  • Hooked and promoted-readonly properties — still correctly reported as always set.

Root cause

IssetabilityResolution::isSet() is the fold that NullCoalesceRule::checkUnnecessaryNullCoalesce() uses to decide whether the left side of ?? is always set. Its property branch normally answers "maybe" for a typed property with no default value, because such a property can be uninitialised. That branch was skipped whenever $scope->getConditionalExpressions() held entries keyed by the property fetch, documented with the rationale that such entries "exist only when the fetch was narrowed in an evaluated condition — and evaluating a condition READS the fetch, which would have thrown on an uninitialized typed property".

That premise is wrong. AssignHandler also records conditional expressions for a property that is assigned inside a conditional branch, so:

if ($my_answer !== null) {
    $this->answer = $my_answer;
}
// scope now holds: if $my_answer=int then $this->answer is int
echo $this->answer ?? null; // ← reported as unnecessary

produced exactly the entries the clause interpreted as "witnessed initialisation", and the fold answered "always set".

The clause was introduced in ac92f19c8 ("Resolve isset/empty/?? chains from handler-built descriptors", released in 2.2.8) when MutatingScope::issetCheck()'s chain walk was reimplemented on top of IssetabilityDescriptor; neither MutatingScope::issetCheck() nor Rules\IssetCheck has ever had it. Dropping it restores the three implementations to the same semantics. The scenario the clause was meant to cover still works: reading a property inside a condition puts a real ExpressionTypeHolder for the fetch into the scope, which the pre-existing hasExpressionTypeOfFetch() check already handles.

Test

tests/PHPStan/Rules/Variables/data/bug-15046.php contains the reporter's playground snippet verbatim plus the analogous constructs listed above, and two control cases that must keep reporting: a property assigned in both branches of an if/else (nullCoalesce.initializedProperty) and a fetch inside a second if where the same condition holds again (nullCoalesce.property).

Without the fix the test fails with seven nullCoalesce.unnecessary false positives (lines 14, 34, 50, 66, 80, 98, 122); with the fix only the two control errors remain.

Full make tests and make phpstan are green.

Fixes phpstan/phpstan#15046

… proof it is initialized

- `IssetabilityResolution::isSet()` bailed out of the "typed property may be
  uninitialized" branch whenever the scope held conditional-expression entries
  about the property fetch, on the assumption that such entries only come from
  reading the fetch inside an evaluated condition. They are also created by
  `AssignHandler` when a property is assigned inside a conditional branch, so a
  conditionally initialized property was reported as always set.
- Dropped the `hasConditionalExpressionsOfFetch` clause from
  `IssetabilityResolution::isSet()`, restoring exact parity with
  `MutatingScope::issetCheck()` and `Rules\IssetCheck::doCheck()`, neither of
  which ever consulted conditional expressions.
- Removed the now-unused `hasConditionalExpressionsOfFetch` plumbing from
  `IssetabilityLinkInfo` and its computation in `IssetabilityDescriptor`.
- The scenario the clause was meant to support (property read inside a
  condition) keeps working, because reading a property leaves a real expression
  type in the scope, which `hasExpressionTypeOfFetch()` already covers.
- Same root cause reproduced and fixed for the analogous constructs: static
  properties, plain boolean conditions, `??=`, ternary assignment, `switch`
  branches and nested `$this->inner->deep` chains. Probed and found already
  correct: `isset()`/`empty()` (they go through `Rules\IssetCheck`, which lacks
  the clause), the inferred types of `??`/`isset()` (computed by
  `MutatingScope::issetCheck()`), assignments inside `try`/`catch` and loops,
  and hooked/promoted-readonly properties.
@staabm
staabm requested a review from VincentLanglet August 6, 2026 06:24
@staabm
staabm merged commit b88ca9c into phpstan:2.2.x Aug 6, 2026
753 of 756 checks passed
@staabm
staabm deleted the create-pull-request/patch-mjsjnrk branch August 6, 2026 07:54
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.

Incorrect nullCoalesce.unnecessary error on a class property that was conditionally initialised

3 participants