An optional non-list key makes isList Maybe, not No - #6025
Conversation
e27c6d8 to
9fa048d
Compare
| public function narrowing(array $optStr): void | ||
| { | ||
| if (array_is_list($optStr)) { | ||
| assertType('list{}&list', $optStr); |
There was a problem hiding this comment.
Before this fix array{a?: string} had isList = No, so this branch was *NEVER* (unreachable). With isList now Maybe, the empty-array realisation survives the is list intersection, so the branch is reachable and narrows to the empty list.
| } 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); |
There was a problem hiding this comment.
The a? here (rather than array{a: string}) is pre-existing and unrelated to this fix: array_is_list()'s false branch is not refined. It stays a? for sealed shapes too — e.g. even array{0: string, a?: string} keeps a? in the false branch on current stable.
|
Update: I folded the The only remaining difference between the two PRs is the projection of an invalid sealed list-shape in |
|
This pull request has been marked as ready for review. |
702067f to
25d81d1
Compare
e3e6293 to
291f84b
Compare
|
This pull request has been marked as ready for review. |
91450e0 to
501c39d
Compare
8928a85 to
d9f6883
Compare
| // A sealed shape with an optional key past the gap at 1 (2? here): key 2 can | ||
| // never appear in a list, so makeList() drops it and projects to the prefix. | ||
| BleedingEdgeToggle::withBleedingEdge(true, function (): void { | ||
| $array = $this->buildShape([[0, new IntegerType(), false], [2, new StringType(), true]]); | ||
| $this->assertSame(TrinaryLogic::createMaybe()->describe(), $array->isList()->describe()); | ||
| $this->assertSame('array{int}', $array->makeList()->describe(VerbosityLevel::precise())); | ||
| }); | ||
|
|
||
| // Unsealed: extras may fill the gap, so key 2 is reachable and every key is kept. | ||
| BleedingEdgeToggle::withBleedingEdge(false, function (): void { | ||
| $array = $this->buildShape([[0, new IntegerType(), false], [2, new StringType(), true]]); | ||
| $this->assertSame(TrinaryLogic::createMaybe()->describe(), $array->isList()->describe()); | ||
| $this->assertSame('array{0: int, 2?: string}', $array->makeList()->describe(VerbosityLevel::precise())); | ||
| }); |
There was a problem hiding this comment.
can we use a data-provider to ease reading the test? atm its hard to see where the diff is between with/without bleeding edge.
same in the other test
There was a problem hiding this comment.
Done. Both tests are now data providers. testMakeListProjectsSealedShapeButKeepsUnsealedKeys has one row per sealedness: the input shape is the same, only the expected makeList() projection differs. testMergeWithRecomputesListnessOfSealedShape has one named row per {bleeding edge, shapes, expected isList}, so you can compare the with and without cases row by row. The test method applies the toggle itself, because data providers run before the container enables bleeding edge. I re-verified by manual mutation that the rows kill the mutants of the $naiveIsList->or(infer…) combinator in both mergeWith() and legacyMergeWith().
|
|
||
| public function testMergeWithTreatsNumericStringKeyAsIntWhenRecomputingListness(): void | ||
| { | ||
| BleedingEdgeToggle::withBleedingEdge(true, function (): void { |
There was a problem hiding this comment.
do we need a BleedingEdgeToggle::withBleedingEdge(false,...) variant of this test?
There was a problem hiding this comment.
Yes, added. inferIsListFromShape() has two call sites: mergeWith() (bleeding edge, sealed by the explicit-never unsealed marker) and legacyMergeWith() (unsealed === null). The test is now a data provider with both variants. The new withBleedingEdge(false) row goes through legacyMergeWith() and fails with Maybe instead of Yes without the toArrayKey() normalization, so it covers the second call site.
A ConstantArrayType whose only list-incompatible keys are optional can
still be a list when those keys are absent (`array{a?: string}` admits
`[]`), so its isList must be Maybe, not No. Previously any string /
negative / gap key forced No regardless of optionality, which made
`array_is_list()` report "always false" on such shapes.
- ConstantArrayTypeBuilder: an optional list-incompatible key degrades
isList Yes to Maybe (No stays No) via markNonListKey().
- ArrayType::isSuperTypeOf(): a possibly-empty constant array always
admits `[]`, a subtype of every array type, so the relationship is at
worst Maybe — never a definite No. This lets the `($value is list)`
conditional of array_is_list() resolve to bool for possibly-empty
shapes instead of false.
- ConstantArrayType::makeList(): now that gap/string optional keys yield
isList Maybe, intersecting a sealed such shape with list keeps only the
contiguous 0..m prefix (the keys that can actually appear in a list)
instead of collapsing to *NEVER*.
Closes phpstan/phpstan#14938
…ergeWith with a numeric-string key
01f9c5a to
dc9a7e6
Compare
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
I like the new data-provider tests a lot more. thank you
3 participants
A
ConstantArrayTypewhose only list-incompatible keys are optional can still be a list when those keys are absent —array{a?: string}admits[], andarray_is_list([]) === true. So itsisListmust be Maybe, not No. Previously any string / negative / gap key forcedisList = Noregardless of optionality, soarray_is_list()was reported as always false on such shapes.This is the dual of #12725 (there, shapes that also admit non-list values wrongly report
isListyes; here, shapes that also admit list values wrongly report no).Parts:
ConstantArrayTypeBuilder— an optional list-incompatible key degradesisListYes→Maybe (No stays No) via a newmarkNonListKey(), replacing four unconditionalcreateNo()sites.ArrayType::isSuperTypeOf()— a possibly-empty constant array always admits[], which is a subtype of every array type, so the relationship is at worst Maybe, never a definite No. This lets the($value is list ? true : false)conditional ofarray_is_list()resolve toboolfor possibly-empty shapes (e.g.array{a?: string}) instead offalse.ConstantArrayType::makeList()— now that gap/string optional keys yieldisListMaybe, intersecting a sealed such shape withlistkeeps only the contiguous0..mprefix (the keys that can actually appear in a list) rather than collapsing to*NEVER*. Unsealed extras may fill the gaps, so there every key is kept.ConstantArrayType::mergeWith()/legacyMergeWith()— merging widens keys present in only one side into optional keys, so the result can admit list realizations neither input did (array{a: 1}|array{b: 2}→array{a?: 1, b?: 2}admits[]). The list-ness of a sealed merged shape is now recomputed from the shape viainferIsListFromShape()instead of the too-strict$this->isList->and($otherArray->isList); two pure lists still merge into a list. This is the same merge-side fix as DegradeisListtomaybefor optional non-list keys in array shapes and shape merges #6026, folded in here — thanks to the parallel analysis there. (The one deliberate difference from DegradeisListtomaybefor optional non-list keys in array shapes and shape merges #6026 ismakeList()above: this PR projects invalid sealed list-shapes to their valid form, e.g.list{0: string, 1: int, 2?: string, 4?: string}→list{0: string, 1: int, 2?: string}, rather than keeping the unreachable key; see the tradeoff note on DegradeisListtomaybefor optional non-list keys in array shapes and shape merges #6026.)The existing
array-shape-list-optional.phpexpectations change accordingly: an invalid sealed list-shape likelist{0: string, 1: int, 2?: string, 4?: string}now resolves to its valid projectionarray{0: string, 1: int, 2?: string}.Closes phpstan/phpstan#14938