Skip to content

Require every receiver union member to have the property when resolving native type of PropertyFetch and StaticPropertyFetch - #6813

Open
phpstan-bot wants to merge 1 commit into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-jczzhmq
Open

phpstan-bot wants to merge 1 commit into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-jczzhmq

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

Since 2.3.0, the native type of a property fetch on a nullable receiver (?Dekking $data; $data->amount) was the property's native type (float) rather than *ERROR* as in 2.2.17. With treatPhpDocTypesAsCertain: false this made $data->amount === null report as always false, even though the fetch gives null at runtime when $data is null. The same regression affected union receivers where only some members declare the property, and static property fetches on an expression ($data::$prop).

Changes

  • src/Analyser/ExprHandler/PropertyFetchHandler.php: on the native-type path, return ErrorType unless hasInstanceProperty() is yes for the whole receiver type. Before that check, null is removed only when the receiver is part of a nullsafe chain, which short-circuits anyway.
  • src/Analyser/ExprHandler/StaticPropertyFetchHandler.php: on the native-type path, return ErrorType when the class expression is nullable outside a nullsafe chain, or when the fetched-on type does not have the static property for all members.
  • turbo-ext/src/PropertyFetchHandler.cpp, turbo-ext/src/StaticPropertyFetchHandler.cpp: same changes in the native mirrors.
  • Probed and found already correct (same as 2.2.17): method calls and static method calls on nullable receivers, class constant fetches, ArrayDimFetch on nullable arrays, ?-> property fetches, dynamic-name property fetches ($d->{'amount'}, $d::${'s'}; these go through the same fixed code path).

Root cause

Commit 34bc963 ("Fetch handlers read operand results") switched the native-type path from PropertyReflectionFinder to MutatingScope::getInstancePropertyReflection() / getStaticPropertyReflection(). That finder required hasInstanceProperty()/hasStaticProperty() to be yes on the whole receiver type. The new MutatingScope methods filter a union down to the members that have the property, so Dekking|null resolved to Dekking::$amount. That filtering is fine for the PHPDoc type, where rules report the bad fetch separately. It is wrong for the native type, which must describe what can actually happen at runtime.

Test

  • tests/PHPStan/Analyser/nsrt/bug-15424.php: assertNativeType() / assertType() checks for:
    • instance and static fetches on a nullable receiver
    • a union receiver where one member lacks the property
    • a union receiver where all members have it
    • nullsafe chains
    • a non-nullable receiver
  • tests/PHPStan/Rules/Comparison/data/bug-15424.php + StrictComparisonOfDifferentTypesRuleTest::testBug15424 (treatPhpDocTypesAsCertain: false): the issue's reproducer; only the non-nullable receiver comparison is reported.

Both tests fail without the fix.

Fixes phpstan/phpstan#15424

…ng native type of `PropertyFetch` and `StaticPropertyFetch`

- PropertyFetchHandler: the native-type path used MutatingScope::getInstancePropertyReflection(),
  which keeps only the union members that have the property, so `?Foo` / `Foo|Bar` receivers got
  the property's native type. Now it returns ErrorType unless the whole receiver type (with null
  removed only on a nullsafe chain) has the property, as 2.2 did via PropertyReflectionFinder.
- StaticPropertyFetchHandler: same for `$expr::$prop` - a nullable class expression outside a
  nullsafe chain, or a union with members lacking the static property, gives ErrorType.
- Ported both changes to the turbo-ext C++ mirrors (PropertyFetchHandler.cpp,
  StaticPropertyFetchHandler.cpp); analysis output is identical with the extension on and off.
- Probed method calls, static method calls, class constants, array dim fetch, nullsafe and
  dynamic-name property fetches against 2.2.17: they already match, so no changes there.
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.

Property fetch on a nullable receiver has a non-nullable native type since 2.3.0

1 participant