Print expressions in a canonical form so the source kind attribute does not change the expression key - #6197
Closed
phpstan-bot wants to merge 1 commit into
Closed
Conversation
…does not change the expression key - `PHPStan\Node\Printer\Printer` now ignores the php-parser `kind` attribute, which only records how a node was spelled in the source. Expression keys (and therefore the scope's expression tracking) are now derived from what a node means, not from how it was typed. - `pScalar_String()`: single-quoted, double-quoted, heredoc and nowdoc strings with the same value print identically (double-quoted escaping is kept only for values containing control characters, so the printed form stays single-line and readable). - `pScalar_InterpolatedString()`: a heredoc with interpolation prints like the equivalent double-quoted string. - `pScalar_Int()`: hexadecimal, octal and binary literals print as decimal. - `pExpr_Array()` / `pExpr_List()`: `array(...)` and `list(...)` print as `[...]`. - `pExpr_Cast_Double()`: `(double)` and `(real)` print as `(float)`. - `pExpr_ConstFetch()`: `TRUE`/`FALSE`/`NULL` and `\true`/`\false`/`\null` print lowercase and unqualified - these are the only case-insensitive constant names in PHP. - Updated two rule test expectations that asserted the old, source-spelling-dependent output (`NULL` -> `null`, `\true` -> `true`). - Probed but deliberately left alone: class, method, function and property-hook name casing. PHP treats those case-insensitively too, but PHPStan already reports mismatched case as `class.nameCase` / `method.nameCase` / `staticMethod.nameCase` / `function.nameCase`, and lowercasing identifiers in the printer would degrade every error message that prints an expression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
$searchParams['key']and$searchParams["key"]are the same expression, but PHPStan tracked them as two unrelated ones: narrowing applied through one spelling was invisible through the other. The reporter hit this after runningpint, which rewrote double-quoted array keys to single-quoted ones and thereby changed the analysis result (their baselinedcount($x) > 0always-true errors disappeared).The fix makes
PHPStan\Node\Printer\Printerprint one canonical form per value, so the expression key no longer depends on how the expression was spelled in the source.Changes
All in
src/Node/Printer/Printer.php:pScalar_String()- print from the value alone instead of from thekindattribute, so'k',"k",<<<'K'/<<<Kheredocs holdingkall print as'k'. Values containing control characters keep the double-quoted escaped form so the printed key stays single-line (and stays cacheable byp()).pScalar_InterpolatedString()- always print the double-quoted form, so<<<K\nx$s\nKmatches"x$s".pScalar_Int()- normalizeKIND_HEX/KIND_OCT/KIND_BINto decimal, so$a[0x1],$a[0b1],$a[01]match$a[1].pExpr_Array()andpExpr_List()- always print the short syntax, soarray(...)matches[...]andlist(...)matches[...].pExpr_Cast_Double()- always print(float), so(double)and(real)match(float). ((integer)/(boolean)were already normalized by php-parser.)pExpr_ConstFetch()- printtrue/false/nulllowercase and unqualified.Test expectations updated for the new canonical output:
tests/PHPStan/Rules/Arrays/DuplicateKeysInLiteralArraysRuleTest.php-(null, NULL)->(null, null)tests/PHPStan/Rules/Keywords/DeclareStrictTypesRuleTest.php-\true given->true givenProbed and found already correct, so left unchanged:
Namequalification (Foo::Cvs\Foo::C, handled by the name resolver), float literals (1.0/1.00/1e0, normalized by value), numeric separators (1_000), magic constants (__LINE__),(integer)/(boolean)casts,${x}vs{$x}interpolation syntax, and$obj->{'n'}vs$obj->n(already normalized by the existingpObjectProperty()override).Probed, found broken, and deliberately not changed: casing of class, method, static-method and function names (
$f->M()vs$f->m(),\FOO::Cvs\Foo::C,STRLEN(...)vsstrlen(...)). PHP resolves these case-insensitively, but PHPStan already reports every one of them as aclass.nameCase/method.nameCase/staticMethod.nameCase/function.nameCaseerror, and lowercasing identifiers in the printer would make every error message that prints an expression display a lowercased class or method name.Root cause
php-parser records the source spelling of a node in its
kindattribute (and inString_'s heredoc/nowdoc label), andPrettyPrinter\Standardfaithfully reproduces it.PHPStan\Node\Printer\PrinterextendsStandardand is whatExprPrinter::printExpr()- and thereforeMutatingScope::getNodeKey()- uses to build expression keys. So two spellings of the same value produced two different keys, and everything keyed off them (narrowed types,isset/instanceof/is_*()specifications, invalidation) applied to only one of them.The pattern is "the printer must canonicalize meaning, not reproduce syntax", and it affected every node type whose printing consults
kind:Scalar\String_,Scalar\InterpolatedString,Scalar\Int_,Expr\Array_,Expr\List_andExpr\Cast\Double- plusExpr\ConstFetch, where the same problem comes from PHP's case-insensitivetrue/false/nullrather than from akindattribute. The class already had one instance of this fix (pObjectProperty(), normalizing$obj->{'n'}to$obj->n); this extends it to the rest of the family.The turbo extension does not reimplement the printer -
pt_node_printed_expr()inturbo-ext/src/support.cppcalls back intoExprPrinter::printExpr()- so no.cppmirror change is needed.Test
tests/PHPStan/Analyser/nsrt/bug-15060.phpcontains the reporter's playground sample (thedumpTypecalls turned intoassertType), asserting that$searchParams['test']and$searchParams["test"]get the same narrowed type afterisset(), a truthiness check, andis_array().The same file adds one function per analogous case found in step 4, each of which failed before the fix:
heredocAndNowdoc()-'test'vs"test"vs heredoc vs nowdocescapeSequences()-"a\nb"vs the equivalent heredocinterpolatedString()-"x$s"vs the equivalent heredocintegerBases()-1vs0x1vs0b1vs01arraySyntax()-[$s][0]vsarray($s)[0]doubleCast()-(float)vs(double)constantCase()-true/TRUE/\true,null/NULL,false/FALSEVerified by stashing the
Printer.phpchange: the test fails withmixedinstead ofarray<mixed, mixed>on 12 assertions, and passes with the fix.make tests(21230 tests) andmake phpstanare green.Fixes phpstan/phpstan#15060