-
Notifications
You must be signed in to change notification settings - Fork 585
An optional non-list key makes isList Maybe, not No #6025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.2.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| <?php | ||
|
|
||
| namespace Bug14938; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| class Foo | ||
| { | ||
|
|
||
| /** | ||
| * @param array{a?: string} $optStr | ||
| * @param array{0: int, a?: string} $listPlusOptStr | ||
| * @param array{-1?: string} $optNeg | ||
| * @param array{0: int, 5?: string} $listPlusGap | ||
| * @param array{a: string} $reqStr | ||
| * @param array{1: string} $gapReq | ||
| * @param array{0?: string} $optZero | ||
| */ | ||
| public function doFoo( | ||
| array $optStr, | ||
| array $listPlusOptStr, | ||
| array $optNeg, | ||
| array $listPlusGap, | ||
| array $reqStr, | ||
| array $gapReq, | ||
| array $optZero, | ||
| ): void | ||
| { | ||
| // An optional non-list key might be absent, so the array can still be | ||
| // a list ([] / the list prefix) — array_is_list() is not decidable. | ||
| assertType('bool', array_is_list($optStr)); | ||
| assertType('bool', array_is_list($listPlusOptStr)); | ||
| assertType('bool', array_is_list($optNeg)); | ||
| assertType('bool', array_is_list($listPlusGap)); | ||
|
|
||
| // A required non-list key is always present, so it is never a list. | ||
| assertType('false', array_is_list($reqStr)); | ||
| assertType('false', array_is_list($gapReq)); | ||
|
|
||
| // Only an optional key 0 keeps it a guaranteed list ([] and [v]). | ||
| assertType('true', array_is_list($optZero)); | ||
| } | ||
|
|
||
| /** | ||
| * @param array{a?: string} $optStr | ||
| */ | ||
| public function narrowing(array $optStr): void | ||
| { | ||
| if (array_is_list($optStr)) { | ||
| assertType('list{}&list', $optStr); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this fix |
||
| } else { | ||
| // array_is_list()'s false branch is not narrowed, so `a` stays optional | ||
| // here rather than being refined to array{a: string}. | ||
| assertType('array{a?: string}', $optStr); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| } | ||
| } | ||
|
|
||
| public function mergedShapes(): void | ||
| { | ||
| // Merging a list with a shape that adds a string key: the empty-of-the-extra | ||
| // realization is still a list, so array_is_list() is not decidable. | ||
| $a = [0 => 'z']; | ||
| if (rand(0, 1)) { | ||
| $a['y'] = 1; | ||
| } | ||
| assertType("array{0: 'z', y?: 1}", $a); | ||
| assertType('bool', array_is_list($a)); | ||
|
|
||
| // Two pure lists merge into a list (optional keys stay a suffix). | ||
| $b = [0 => 'z']; | ||
| if (rand(0, 1)) { | ||
| $b[1] = 'w'; | ||
| } | ||
| assertType("array{0: 'z', 1?: 'w'}", $b); | ||
| assertType('true', array_is_list($b)); | ||
|
|
||
| // Shapes disjoint except for the empty array still admit the empty list. | ||
| $c = []; | ||
| if (rand(0, 1)) { | ||
| $c['a'] = 1; | ||
| } | ||
| assertType('array{}|array{a: 1}', $c); | ||
| assertType('bool', array_is_list($c)); | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line makes me feel that we might need to handle shapes with a key which php auto converts to int (e.g.
"1").please add a test for such example. usually a fix would need
toArrayKey()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — fixed.
inferIsListFromShape()now normalizes each key withtoArrayKey()before deciding whether it continues the list, so a numeric-string key like"1"(which PHP stores as the integer key1) is treated as an integer.In practice keys reaching this method are already normalized (the builder calls
toArrayKey()insetOffsetValueType(), and PHPDocarray{"1": …}resolves toarray{1: …}), so it's a defensive fix — but a directly-constructedConstantArrayTypecan carry an un-normalized"1"key, which is what the new testtestMergeWithTreatsNumericStringKeyAsIntWhenRecomputingListnessexercises: mergingarray{0: string, '1'?: string}yieldsisList = yes(it fails without thetoArrayKey()call, givingmaybe).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed up: the same
instanceof ConstantIntegerTypepattern in the adjacentisListAfterUnset()(thepreserveListCertaintypath ofunsetOffset()) had the same numeric-string-key gap, so I normalized there too — matching the two fixes #6026 landed for this. AddedtestUnsetOffsetTreatsNumericStringKeyAsIntWhenRecomputingListness(unsetting the optional tail oflist{'0': string, 1?: int}keepsisList = yes; it regresses to a broken result without thetoArrayKey()call).