Skip to content

Normalize dynamic member names that resolve to a constant string to bareword form when building expression keys#5919

Closed
phpstan-bot wants to merge 2 commits into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-pulcpze
Closed

Normalize dynamic member names that resolve to a constant string to bareword form when building expression keys#5919
phpstan-bot wants to merge 2 commits into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-pulcpze

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

Accessing or assigning a member through a dynamic name that is in fact a constant string — $this->{'v'}, $this->{$n} or $this->$n where $n = 'v' — did not affect the analysed type of the directly-accessed member $this->v. The reported playground assigned 5/6/7 through the curly/variable forms but PHPStan kept inferring the type from the first direct assignment (4).

The recently merged Printer normalization (#5916) already made the literal $obj->{'n'} form share an expression key with $obj->n. This change extends the same normalization to any member name that the scope can resolve to a single constant string (variables, class constants, concatenations, ...), and to static properties / static calls.

Changes

  • src/Analyser/MutatingScope.php
    • getNodeKey() rewrites the node through normalizeConstantMemberNames() before printing the expression key.
    • New normalizeConstantMemberNames() walks the access chain and normalizeConstantMemberName() resolves a name expression: when it is a single constant string, a bareword is emitted as VarLikeIdentifier, otherwise (non-bareword string) as a String_ node so the existing literal normalization applies. Handles PropertyFetch, NullsafePropertyFetch, MethodCall, NullsafeMethodCall, StaticPropertyFetch and StaticCall.
  • src/Node/Printer/Printer.php
    • Extracted the bareword pattern into the public BAREWORD_NAME_REGEX constant, reused by both the printer and the scope.

Root cause

Expression types in MutatingScope are keyed by the pretty-printed expression. $this->v prints to $this->v, but $this->$n / $this->{$n} print to $this->$n, so a write to one key was invisible to a read of the other, and narrowing stored under one key was not retrieved under the other. The fix centralizes the normalization in getNodeKey() (the single chokepoint used by reads via getType(), writes via specifyExpressionType(), and invalidateExpression()), so all syntactic spellings of a constant-named member collapse to the same key. Only the printed key is normalized; the original node is still used for reflection/type resolution.

Test

  • tests/PHPStan/Analyser/nsrt/bug-7851.php reproduces the issue's playground (instance property assigned through $this->{'v'}, $this->{$n}, $this->$n) and additionally covers the analogous constructs found while sweeping the family:
    • static property assignment through self::${$s},
    • narrowing through a dynamic instance property name carrying to the bareword/literal forms,
    • narrowing through a dynamic static property name,
    • constant-string method-call member resolution and narrowing carrying to the bareword/literal forms.
  • Verified the test fails on the current tree and passes with the fix; full test suite and PHPStan self-analysis are green.

Fixes phpstan/phpstan#7851

…areword form when building expression keys

- MutatingScope::getNodeKey() now rewrites member-access names that resolve to a single constant string into their bareword form before the expression is printed into a key, so that `$obj->$n`, `$obj->{$n}`, `$obj->{'n'}` and `$obj->n` (and the static / method-call equivalents) share one expression key and the same tracked type.
- Added MutatingScope::normalizeConstantMemberNames()/normalizeConstantMemberName() which handle PropertyFetch, NullsafePropertyFetch, MethodCall, NullsafeMethodCall, StaticPropertyFetch and StaticCall, recursing into the access chain. Bareword names are emitted as VarLikeIdentifier (accepted by every member-access node constructor); non-bareword constant strings fall back to a String_ node so they match the existing literal-curly normalization in Printer.
- Extracted the bareword pattern into Printer::BAREWORD_NAME_REGEX and reused it in both Printer::pObjectProperty() and the new scope normalization.
- This fixes assignments through dynamic property names (`$this->$n = 6`) not updating the bareword type, and narrowing through dynamic names not carrying to the bareword form, for both instance and static properties as well as method calls.

@VincentLanglet VincentLanglet left a comment

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.

What about $this->$foo with $foo behing an union of constant string ?

…a union-of-constant-strings member name

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@phpstan-bot

Copy link
Copy Markdown
Collaborator Author

I've addressed VincentLanglet's review about $this->$foo where $foo is a union of constant strings.

What I found

The reviewer's question exposed a real soundness bug on the write side. When assigning through a member name that resolves to a union of constant strings (e.g. $name of type 'a'|'b'), the assignment may target either member, but the previous narrowing of the concrete members was kept stale:

$this->a = 3;            // narrowed to 3
$name = $c ? 'a' : 'b';  // 'a'|'b'
$this->$name = 5;
// before: $this->a still reported as 3  ← unsound (might now be 5)
// after:  $this->a reported as int      ← sound

The original PR's getNodeKey() normalization only collapses the single constant-string case to a bareword key. For a union there's no single key to collapse to, so the assignment never reached $this->a/$this->b to invalidate them.

The read side and condition narrowing were already sound (reading $this->$name gives the union of member types int, and narrowing the union name doesn't leak onto a concrete member), so no change was needed there.

The fix

MutatingScope::assignExpression() now, for property and static-property assignments, invalidates one concrete member fetch per possible constant-string name (via the new constantMemberNameFetches() helper). The single-constant case stays handled by getNodeKey() and is intentionally excluded.

Verification

  • Added regression cases to bug-7851.php: union-named instance assignment, union-named static assignment, and a union-named read. Confirmed they fail without the fix (lines 60/70 reported stale 3) and pass with it.
  • make phpstan self-analysis: clean.
  • Full tests/PHPStan/Analyser/ suite: 2843 tests green.

Committed and pushed to create-pull-request/patch-pulcpze.

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.

Constant variable property access has no effect on analysed type

2 participants