Print literals from their value, not their source spelling, when building expression keys

[phpstan-bot]

Summary

$searchParams['key'] and $searchParams["key"] refer to the same array offset, but PHPStan
narrowed them independently: narrowing established through the double-quoted spelling was invisible
at the single-quoted one and vice versa. Running a formatter that rewrites quotes therefore changed
the analysis result, which is what the reporter hit.

The cause is in PHPStan\Node\Printer\Printer, whose printed output doubles as the expression key
under which MutatingScope tracks types. It inherits nikic/php-parser's Standard printer, which
faithfully reproduces the source spelling of a literal — exactly the wrong property for a key that
is supposed to identify an expression. The fix makes the printer derive literals from their value.

Changes

All in src/Node/Printer/Printer.php:

• pScalar_String() — print a String_ from $node->value, ignoring the kind and docLabel
  attributes. The canonical form mirrors ConstantStringType::export(): single quotes normally,
  double quotes with escapes when the value contains control characters.
• pScalar_Int() — always print the decimal form instead of honouring the kind attribute
  (PHP_INT_MIN keeps Standard's (-9223372036854775807-1) form, since it cannot be written as a
  literal).
• pScalar_InterpolatedString() — always print the "..." form instead of honouring the heredoc
  kind.
• pExpr_ConstFetch() — lowercase a single-part, non-relative true, false or null.

Analogous cases probed and found already correct, so no change was made and no test was kept:

• Float literals — pScalar_Float() is already value-based, so $a[1.5], $a[1.50] and $a[15e-1]
  already agreed (kept as a guard case in the new printer test, since it is the same code path).
• Curly-brace member access — $o->{'p'} / $o->{"p"} / $o->p, and the method-call equivalents,
  are already normalized by the existing pObjectProperty() override.
• Variable variables with a constant name (${'a'}) and a leading-\ on a constant name
  (\PHP_INT_MAX) already resolve to the same thing as their plain spelling.

Deliberately left alone: class, function and method name case (c::$p, $c->GET(),
STRVAL(1)) does produce a distinct expression key, but every one of those spellings is already
reported by a dedicated rule (class.nameCase, function.nameCase, method.nameCase,
staticMethod.nameCase), and normalizing case in the printer would degrade error messages that
quote the expression back to the user. true/false/null are the one case-insensitive spelling
with no such rule, which is why they are normalized here.

Two rule-test expectations were updated because their messages quote the printed expression:
tests/PHPStan/Rules/Keywords/DeclareStrictTypesRuleTest.php (\true → true) and
tests/PHPStan/Rules/Arrays/DuplicateKeysInLiteralArraysRuleTest.php ((null, NULL) →
(null, null)).

Root cause

MutatingScope keys its type table by the pretty-printed form of an expression
(ScopeOps::nodeKey() → ExprPrinter::printExpr()). Two spellings of the same expression must
print identically or narrowing established under one is simply not found under the other.

nikic/php-parser's Standard printer is built for round-tripping source, so it reproduces the
literal as written: pScalar_String() and pScalar_InterpolatedString() branch on the kind
attribute (single-quoted / double-quoted / heredoc / nowdoc), pScalar_Int() branches on the
numeric base, and pExpr_ConstFetch() prints the Name verbatim. Every literal kind that carries
such a spelling attribute was therefore affected by the same pattern — the key encodes syntax where
it should encode value. The fix is to print each of them from the value the node holds, which is
the same thing the pre-existing pObjectProperty() override already does for $obj->{'n'}.

Note that the bug was masked whenever the array was already typed as an array: narrowing
$a['k'] also refines $a itself with a HasOffsetValueType, and reading $a["k"] then resolves
through that offset regardless of the expression key. It only surfaced where no such array type
exists — a mixed variable, as in the reported snippet.

Deriving the string form from the value also keeps expression keys newline-free (a heredoc key used
to embed real newlines), so Printer::p()'s print cache now applies to them too.

Test

• tests/PHPStan/Analyser/nsrt/bug-15060.php — the reporter's playground snippet, asserting that
  the single- and double-quoted reads agree after isset(), after truthiness, after is_array()
  and after is_array() && truthy. Extended with the analogous spellings: "\x74est" /
  nowdoc / heredoc for strings, 0x1 / 01 / 0b1 for ints (with $m[10] pinned to mixed so a
  key collapse would be caught), heredoc for interpolated strings, and TRUE / True / NULL /
  FALSE. 14 assertions in this file fail without the fix.
• tests/PHPStan/Node/Printer/ExprPrinterTest.php — a new unit test asserting directly that
  equivalent spellings print to the same expression key, that genuinely different expressions
  ($a[1] vs $a['1'], 'a\nb' vs "a\nb", FOO vs foo) still print differently, and that a
  heredoc key contains no newline. 9 of its 17 cases fail without the fix.

Fixes phpstan/phpstan#15060

[staabm]

do we need a similar fix for

"$key.value" vs $key . '.value'

?

[phpstan-bot]

Pushed. Here's my answer to the review.

Reply to staabm

I checked, and the gap is real but I don't think we should close it here.

It reproduces, symmetrically:

function f($m, string $key): void {
	if (is_array($m["$key.value"]) && $m["$key.value"]) {
		assertType('non-empty-array<mixed, mixed>', $m["$key.value"]);
		assertType('mixed', $m[$key . '.value']);   // ← narrowing not visible
	}
}

…and the same the other way round.

But normalizing it isn't a safe extension of this PR. Everything in this PR normalizes spelling of one node — same AST node, different source attribute. Interpolation vs. concatenation is two different AST shapes, and collapsing them in the printer breaks in ways that are worse than the bug. InterpolatedString is a Scalar, so it's absent from php-parser's precedenceMap and printed atomically. I prototyped the naive version:

-"$a$b"      => -$a . $b     ← same key as (-$a) . $b
"$a$b" ** 2  => $a . $b ** 2 ← same key as $a . ($b ** 2)
"$a"         => $a           ← same key as the uncast variable

Those are false collisions — PHPStan would hand one expression a type established for a genuinely different one. That's a worse failure mode than the missing narrowing. Fixing them means routing InterpolatedString through the precedence machinery and special-casing the single-part form (as '' . $a), and then that concat form also reaches every error message that quotes the expression back to the user — the same objection that made me leave name case alone.

The formatter angle is already covered. The original issue was pint rewriting quotes. No formatter rewrites concat ↔ interpolation, but several do rewrite between interpolation syntaxes (php-cs-fixer's explicit_string_variable, Rector's SimpleToComplexStringVariableRector). Those already work — php-parser's pEncapsList always emits the {$...} form:

$a["$b"] / $a["{$b}"] / $a["${b}"]  →  $a["{$b}"]
$a["$b[k]"] / $a["{$b['k']}"]       →  $a["{$b['k']}"]
$$v / ${$v}                         →  ${$v}

Two commits pushed:

• 59dd58703 — pins those interpolation spellings in ExprPrinterTest (guard tests; they pass without any source change).
• dee330f87 — records the reasoning above on pScalar_InterpolatedString, so the next person asking finds the answer.

Happy to open a follow-up issue for the concat case if you'd rather track it, but I'd keep it out of this PR.

[staabm]

Ping

[phpstan-bot]

I'll report once the checks finish. Summary of what I did so far:

The scoping suggestion doesn't hold up. The literal isn't only a problem in $a['k'] — it's a problem anywhere a literal is part of an expression key. Measured on /tmp/t1.php:

if (is_array($b->get('key')) && $b->get('key')) {
	\PHPStan\dumpType($b->get('key'));   // non-empty-array<mixed, mixed>
	\PHPStan\dumpType($b->get("key"));   // before the fix: mixed
}

Before 41a1c4c3e the second dump is mixed; after it, both agree. Scoping to ArrayDimFetch::$dim would leave that case — and every other literal-in-a-key position — still broken, which is the same bug the issue reports.

The cost concern is real in two spots, and I fixed both rather than narrowing the scope. Committed as 580dcfb41:

• pScalar_String() was calling addcslashes() to build a fully escaped copy of every string literal purely to compare it against the original. Replaced with preg_match('/[\x00-\x1f]/', ...) — same condition, no allocation, and it short-circuits on the first control character.
• pExpr_ConstFetch() was calling Name::getParts(), which explode()s the name, then getFirst(), which scans it again. Replaced with toLowerString() plus three string comparisons: a qualified name lowercases to something containing a backslash and can never equal true/false/null, so the part-counting was never needed. Only the Name\Relative guard has to stay, since it holds just the part after namespace\.

Microbenchmark, best of 5 × 300k calls, comparing each override against Standard's implementation of the same method:

node	PHPStan	Standard	delta
'someArrayKey'	263 ns	256 ns	+7 ns
"someArrayKey"	263 ns	586 ns	−323 ns
1234	102 ns	218 ns	−116 ns
"x{$k}"	791 ns	947 ns	−156 ns
true	272 ns	242 ns	+30 ns

The overrides are cheaper than Standard for every spelling except the single-quoted string and the keyword, where they're within noise — Standard branches on the kind attribute and then does the same escaping work. And Printer::p() caches each expression's printed form on the node, so an override runs at most once per literal node per parse.

End to end, analysing src/Type/Constant src/Type/Accessory src/Rules/Comparison at level 8 with a cleared result cache, 3 runs each: 11.46 s / 11.66 s / 13.12 s without the overrides vs 11.56 s / 11.76 s / 12.52 s with them — no measurable difference.

I also added an ExprPrinterTest case pinning that $a[Foo\true] stays distinct from $a[true], since the rewritten condition is what makes that hold. ($a[namespace\true] is not distinct in the global namespace — the name resolver turns it into \true, which genuinely is the same constant.)

[staabm]

@SanderMuller I am not sure I like this approach very much. maybe you can find a alternative fix.

[SanderMuller]

@SanderMuller I am not sure I like this approach very much. maybe you can find a alternative fix.

Will look into it!

[SanderMuller]

maybe you can find a alternative fix

I think there is one, and there's a concrete reason to want it.

Why the current approach bites. Printer's output does double duty: it is the expression key MutatingScope tracks types under, and the text rules quote back in error messages. Canonicalizing it for the key therefore rewrites messages. The PR updates two rule tests for exactly that (\true → true, (null, NULL) → (null, null)), but the effect reaches further — NoopRule quotes the expression and has no test covering it:

"double quoted";
0x1F;
01;
<<<EOT
heredoc body
EOT;

line	2.2.x	this PR
1	Expression ""double quoted""	Expression "'double quoted'"
2	Expression "0x1f"	Expression "31"
3	Expression "01"	Expression "1"
4	Expression "<<<EOT…"	Expression "'heredoc body'"

Telling someone who wrote 0x1F that the problem is 31 is the part I would not want to ship.

The alternative: canonicalize the key, leave the display alone. printExpr() stays faithful to the source for the 16 sites that quote expressions back to users. A new ExprPrinter::printExprKey(), backed by an ExprKeyPrinter carrying exactly the four normalizations this PR already has, is used by the 19 identity sites — ScopeOps::nodeKey(), TypeSpecifier, AssignHandler, EqualityTypeSpecifyingHelper, BinaryOpHandler and the two constant-condition bucket helpers. Printer is final (and phpstan.finalClass enforces that), so the shared virtual-node printers move to an abstract BasePrinter and both concrete printers stay final.

I built it and ran the gates. bug-15060 fails without it and passes with it; the full suite is green (21302 tests, 96881 assertions); self-analysis and phpcs are clean; and both rule tests pass with their expectations reverted to the current 2.2.x values, i.e. no message changes at all. NoopRule output is byte-identical to 2.2.x. On a doctrine/symfony codebase over 5 interleaved rounds there is no measurable CPU difference (medians 150.4 s / 144.5 s / 148.2 s for 2.2.x, this PR and the split, against a 27 s spread within a single build) and peak memory is identical.

Two costs, because they cut the other way:

1. It is bigger, not smaller — roughly 150 lines of new logic across 12 files, against +81 in a single file here. If the size is what bothered you, this does not answer it.
2. It needs a turbo-ext change. ScopeOps is #[ShadowedByTurboExtension], and pt_node_printed_expr() in support.cpp calls printExpr by name and reads the phpstan_cache_printer attribute — so without a matching change there (plus make bump-turbo) the native and PHP key computations would diverge. I have not implemented or tested that part; the extension is not loaded on my machine. This PR as it stands needs no C++ change at all, since the native side already calls the printer it modifies — a real point in its favour.

So there are three options, and which one is right depends on what actually bothers you:

• the split — messages preserved, costs more code and a turbo mirror change
• this PR as it stands — smallest, but changes messages, 0x1F → 31 included
• a middle path — keep the single printer and drop only pScalar_Int(), the one normalization that makes a message unrecognisable; $a[0x1] and $a[1] would still get separate keys, but that is not the case that was reported

Happy to push the split as a PR if you want to look at it properly, turbo side included. Which axis matters most to you?

[staabm]

instead of adjusting the Printer, could we normalize ArrayDimFetch with String_ keys into single quotes using a NodeVisitor ?

[phpstan-bot]

Done — pushed as 2582373b1.

What changed

Reworked the fix the way you asked: the Printer is back to its pre-PR state, and normalization now happens at parse time in src/Parser/ArrayOffsetSpellingVisitor.php, scoped to ArrayDimFetch::$dim.

It rewrites only the spelling attribute of the offset literal — never a subnode — so rules see the same AST and format-preserving printing (the fixable path) is untouched:

• String_ → KIND_SINGLE_QUOTED, except when the value holds control characters, where KIND_DOUBLE_QUOTED is used. That's ConstantStringType::export()'s canonical form, and it keeps the key newline-free so Printer::p()'s print cache still applies. (Always-single-quoted would put a real newline into the key for $a["a\nb"] — a regression against today.)
• InterpolatedString → KIND_DOUBLE_QUOTED, so a heredoc offset and the "..." form agree.
• Int_ → KIND_DEC, so $a[1]/$a[0x1]/$a[01]/$a[0b1] agree.

This also settles the error-message objection

Both rule-test expectations the printer overrides had forced are reverted to their original values and pass: \true stays \true, (null, NULL) stays (null, NULL). NoopRule output is byte-identical to 2.2.x outside array offsets:

line	before	now
"double quoted";	Expression ""double quoted""	unchanged
0x1F;	Expression "0x1f"	unchanged
<<<EOT…	Expression "<<<EOT…"	unchanged
\true;	Expression "\true"	unchanged
$a["key"];	Expression "$a["key"]"	Expression "$a['key']"

The last row is the one remaining message change, and it's inherent to canonicalizing the offset.

What the narrower scope gives up

Two cases the printer overrides covered are no longer fixed, and I want to be explicit rather than bury it:

• $b->get('key') vs $b->get("key") — literals outside an array offset still get distinct keys.
• $a[TRUE] vs $a[true] — normalizing a ConstFetch needs a Name replacement (a structural AST change), not an attribute tweak, so I left it out. The pExpr_ConstFetch override and its test section are gone.

Neither is the reported case; #15060 is fixed.

Verification

• bug-15060.php: 11 assertions fail with the visitor disabled, pass with it.
• ExprPrinterTest: 6 of 18 cases fail with the visitor disabled, pass with it. Dropped the true/false/null groups to match the new scope.
• make tests — 21248 tests, 96784 assertions, 0 failures.
• make phpstan (cleared result cache) — no errors. make cs — clean.
• The issue's own snippet now reports Comparison operation ">" between int<1, max> and 0 is always true for the double-quoted read.

One note: the new class needs composer dump-autoload to land in vendor/attributes.php before #[AutowiredService] picks it up — CI does that, but it'll bite anyone testing locally from a stale autoload.

[staabm]

rename this class into ArrayOffsetNormalizingVisitor

[phpstan-bot]

Renamed and pushed as d7b38a53c.

What changed

src/Parser/ArrayOffsetSpellingVisitor.php → src/Parser/ArrayOffsetNormalizingVisitor.php, class renamed to match. Git records it as a pure rename (R) — no other edits.

No registration change was needed: RichParser collects its visitors by interface (#[AutowiredExtensions(of: NodeVisitor::class)] behind the phpstan.parser.richParserNodeVisitor tag), so #[AutowiredService] on any NodeVisitor is enough and nothing referenced the old name by string. I did re-run composer dump-autoload so vendor/attributes.php picks up the new class name — worth knowing for anyone testing locally from a stale autoload, same caveat as when the class was first added.

Verification

• make tests — 21230 tests, 96755 assertions, 0 failures (97 skipped).
• make phpstan (cleared result cache) — no errors.
• make cs — clean.
• End-to-end on the issue's own snippet, mixing the two spellings across the isset/truthy/is_array chain and the count() read: still reports Comparison operation ">" between int<1, max> and 0 is always true, confirming the visitor is live in the container under its new name rather than silently dropped.

[staabm]

I like this way more. @SanderMuller wdyt?

it does not cover all possible cases, but I think it will cover the most common case (mixing single, double quotes)

[staabm]

PR title and description need an update to reflect the PRs current state

[phpstan-bot]

Static analysis and coding standards are clean. The full suite is still running; I'll report when it finishes.

[SanderMuller]

Agreed, this is the better shape, and I think the tradeoff you name is the right one to take.

I ran it. $m['key'] and $m["key"] now agree, as do $m[1] and $m[0x1]. Full suite green (21281 tests), self-analysis clean.

Two things make it better than the printer version beyond the smaller diff. Error messages survive: the only change is inside offsets, where $a["dq"] is reported as $a['dq'], the same characters in different quotes. The printer version reported source 0x1F; back as Expression "31", which was the part I did not want to ship. And it needs no turbo-ext change: ScopeOps is #[ShadowedByTurboExtension] and the native pt_node_key calls printExpr by name, so a printer-side fix has to be mirrored in support.cpp and bumped, whereas a visitor normalizes the node before analysis and the native path gets the canonical spelling for free.

I also checked the fixer, since the visitor mutates kind on a node the printer later sees. On a file with `ls`, $a["double"], $a[0x1F] and a heredoc offset, --fix rewrote only the backtick line and left every offset untouched, so format-preserving printing still reproduces the original source.

On the gap: confirmed, $b->get('key') and $b->get("key") still diverge. So do $m[1] and $m["1"], even though PHP reads both as the same offset, if anyone wants a follow-up target. Offsets are where quote rewriting actually bites, so I would take this now.

On performance, the visitor runs an instanceof per node, so I checked it too: three interleaved rounds on a doctrine/symfony codebase give 138.8s median on the base against 139.4s with the visitor, against a within-build spread of about 3s. No measurable cost.