Normalize double and single quotes in offset keys, to remember them with the same node-key

[phpstan-bot]

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.

[staabm]

add support for

function stringConcet($m, string $k): void
{
	if (is_array($m[$k.".value"]) && $m[$k.".value"]) {
		assertType('non-empty-array<mixed, mixed>', $m["$k.value"]);
		assertType('non-empty-array<mixed, mixed>', $m[$k.".value"]);
	}
}

[phpstan-bot]

Pushed as aa5ed10cd.

What changed

ArrayOffsetNormalizingVisitor now normalizes the two ways of building an offset, not just the spelling of a single literal.

Interpolation is sugar for concatenation — every part is cast to string, the results are joined — and string concatenation is associative. So an offset that is an InterpolatedString or a Concat is flattened into its list of operands, adjacent literals are merged, and the list is rebuilt as one left-associative Concat:

written	expression key
$a["$k.value"] / $a[$k . ".value"] / $a["{$k}.value"]	$a[$k . '.value']
$a["$k$j.x"] / $a[$k . $j . '.x'] / $a[$k . ($j . '.x')] / $a["$k" . "$j" . '.x']	$a[$k . $j . '.x']
$a['a' . 'b']	$a['ab']
$a["x$k"] / $a["x{$k}"] / heredoc x$k	$a['x' . $k]

Your snippet is in bug-15060.php as stringConcat(), plus the reverse direction (interpolation establishing the narrowing, concat reading it), nesting, and constant folding.

One case deliberately left alone: a one-part interpolation. "$k" is a string cast and $k is not — they are the same offset only when $k already holds a string, so collapsing them would be a false collision. "$k" and "{$k}" still agree with each other. There's a test pinning that $m[$i] stays separate from $m["$i"].

Why an AST rewrite and not a printer change

This is the objection I raised against normalizing interpolation earlier — -"$a$b" printing as -$a . $b, "$a$b" ** 2 as $a . $b ** 2. That was about overriding pScalar_InterpolatedString() to return concat text while the printer still thought it was a Scalar. Replacing the node instead routes it through the normal precedence machinery, so $a[-("$a$b")] prints -"{$a}{$b}" and nothing collides. BacktickRule already builds the same InterpolatedStringPart → String_ → Concat shape, so it's an established transformation here.

The one visible consequence

Inside an offset, an interpolation part that can't be cast to string is now reported by InvalidBinaryOperationRule instead of InvalidPartOfEncapsedStringRule:

$m["$o text"];   // before: Part $o (object) of encapsed string cannot be cast to string.
                 // now:    Binary operation "." between object and ' text' results in an error.

Still an error, different message and identifier (encapsedStringPart.nonString → binaryOp.invalid). No test in the suite covered that combination. It's recorded in the class docblock.

Verification

• bug-15060.php: 10 assertions fail with the previous visitor, pass with this one.
• New tests/PHPStan/Parser/ArrayOffsetNormalizingVisitorTest.php — 28 cases pinning the canonical key for every spelling above, through the container's parser and ExprPrinter, so it also guards that the visitor stays registered. 10 fail without the change.
• make tests — 21258 tests, 96783 assertions, 0 failures (97 skipped).
• make phpstan (cleared result cache) — no errors. make cs — clean.
• --fix on PHP 8.5: rewriting a backtick next to an offset ($m["$k.value"] . \ls`) and *inside* one ($m["x" . `ls` . "$k.value"]) changed only the backtick — "$k.value", 'a' . 'b', "x{$k}y"and0x1F` all came back byte-identical. The synthesized nodes carry the token positions of the source they stand for, which is what makes that hold.
• No turbo-ext change: this is still a parser visitor, so the native path gets the canonical AST before analysis.

The PR title/description are still the quote-only ones — suggest Normalize array offset spelling and shape so equivalent offsets share one expression key, since it now covers int spellings, interpolation and concatenation too.