-
Notifications
You must be signed in to change notification settings - Fork 574
Unsetting optional offset might still give a list. #5029
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
Changes from 9 commits
b5379a5
86e7f3f
d7d28b9
89f5a54
405eecf
f1d04af
77b49bf
1d76e96
faf726d
2a30bf1
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 |
|---|---|---|
|
|
@@ -741,7 +741,14 @@ | |
| $k++; | ||
| } | ||
|
|
||
| return new self($newKeyTypes, $newValueTypes, $this->nextAutoIndexes, $newOptionalKeys, TrinaryLogic::createNo()); | ||
| $newIsList = self::isListAfterUnset( | ||
| $newKeyTypes, | ||
| $newOptionalKeys, | ||
| $this->isList, | ||
| in_array($i, $this->optionalKeys, true), | ||
| ); | ||
|
|
||
| return new self($newKeyTypes, $newValueTypes, $this->nextAutoIndexes, $newOptionalKeys, $newIsList); | ||
| } | ||
|
|
||
| return $this; | ||
|
|
@@ -751,6 +758,7 @@ | |
| if (count($constantScalars) > 0) { | ||
| $optionalKeys = $this->optionalKeys; | ||
|
|
||
| $arrayHasChanged = false; | ||
| foreach ($constantScalars as $constantScalar) { | ||
| $constantScalar = $constantScalar->toArrayKey(); | ||
| if (!$constantScalar instanceof ConstantIntegerType && !$constantScalar instanceof ConstantStringType) { | ||
|
|
@@ -762,6 +770,7 @@ | |
| continue; | ||
| } | ||
|
|
||
| $arrayHasChanged = true; | ||
| if (in_array($i, $optionalKeys, true)) { | ||
| continue 2; | ||
| } | ||
|
|
@@ -770,21 +779,77 @@ | |
| } | ||
| } | ||
|
|
||
| return new self($this->keyTypes, $this->valueTypes, $this->nextAutoIndexes, $optionalKeys, TrinaryLogic::createNo()); | ||
| if (!$arrayHasChanged) { | ||
| return $this; | ||
| } | ||
|
|
||
| $newIsList = self::isListAfterUnset( | ||
| $this->keyTypes, | ||
| $optionalKeys, | ||
| $this->isList, | ||
| count($optionalKeys) === count($this->optionalKeys), | ||
| ); | ||
|
|
||
| return new self($this->keyTypes, $this->valueTypes, $this->nextAutoIndexes, $optionalKeys, $newIsList); | ||
| } | ||
|
|
||
| $optionalKeys = $this->optionalKeys; | ||
| $isList = $this->isList; | ||
| $arrayHasChanged = false; | ||
| foreach ($this->keyTypes as $i => $keyType) { | ||
| if (!$offsetType->isSuperTypeOf($keyType)->yes()) { | ||
| continue; | ||
| } | ||
| $arrayHasChanged = true; | ||
| $optionalKeys[] = $i; | ||
| $isList = TrinaryLogic::createNo(); | ||
| } | ||
| $optionalKeys = array_values(array_unique($optionalKeys)); | ||
|
|
||
| return new self($this->keyTypes, $this->valueTypes, $this->nextAutoIndexes, $optionalKeys, $isList); | ||
| if (!$arrayHasChanged) { | ||
| return $this; | ||
| } | ||
|
|
||
| $newIsList = self::isListAfterUnset( | ||
| $this->keyTypes, | ||
| $optionalKeys, | ||
| $this->isList, | ||
| count($optionalKeys) === count($this->optionalKeys), | ||
| ); | ||
|
|
||
| return new self($this->keyTypes, $this->valueTypes, $this->nextAutoIndexes, $optionalKeys, $newIsList); | ||
| } | ||
|
|
||
| /** | ||
| * When we're unsetting something not on the array, it will be untouched, | ||
| * So the nextAutoIndexes won't change, and the array might still be a list even with PHPStan definition. | ||
| * | ||
| * @param list<ConstantIntegerType|ConstantStringType> $newKeyTypes | ||
| * @param int[] $newOptionalKeys | ||
| */ | ||
| private static function isListAfterUnset(array $newKeyTypes, array $newOptionalKeys, TrinaryLogic $arrayIsList, bool $unsetOptionalKey): TrinaryLogic | ||
|
Contributor
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. you might be able to kill the mutation with somethig like
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. I don't think so, it will be an union of two ConstantArrayType ; and each one will be handled separatly by this method. Also, the tests are failing locally with the mutation ; dunno why it's reported as not caught then. |
||
| { | ||
| if (!$unsetOptionalKey || $arrayIsList->no()) { | ||
|
Check warning on line 830 in src/Type/Constant/ConstantArrayType.php
|
||
| return TrinaryLogic::createNo(); | ||
| } | ||
|
|
||
| $isListOnlyIfKeysAreOptional = false; | ||
| foreach ($newKeyTypes as $k2 => $newKeyType2) { | ||
| if (!$newKeyType2 instanceof ConstantIntegerType || $newKeyType2->getValue() !== $k2) { | ||
| // We found a non-optional key that implies that the array is never a list. | ||
| if (!in_array($k2, $newOptionalKeys, true)) { | ||
| return TrinaryLogic::createNo(); | ||
| } | ||
|
|
||
| // The array can still be a list if all the following keys are also optional. | ||
| $isListOnlyIfKeysAreOptional = true; | ||
| continue; | ||
| } | ||
|
|
||
| if ($isListOnlyIfKeysAreOptional && !in_array($k2, $newOptionalKeys, true)) { | ||
| return TrinaryLogic::createNo(); | ||
| } | ||
| } | ||
|
|
||
| return TrinaryLogic::createMaybe(); | ||
| } | ||
|
|
||
| public function chunkArray(Type $lengthType, TrinaryLogic $preserveKeys): Type | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace Bug14177Nsrt; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| class HelloWorld | ||
| { | ||
| /** | ||
| * @param list{0: string, 1: string, 2?: string, 3?: string} $b | ||
| */ | ||
| public function testList(array $b): void | ||
| { | ||
| if (array_key_exists(3, $b)) { | ||
| assertType('list{0: string, 1: string, 2?: string, 3: string}', $b); | ||
| } else { | ||
| assertType('list{0: string, 1: string, 2?: string}', $b); | ||
| } | ||
| assertType('list{0: string, 1: string, 2?: string, 3?: string}', $b); | ||
| } | ||
|
|
||
| public function placeholderToEditor(string $html): void | ||
| { | ||
| $result = preg_replace_callback( | ||
| '~\[image\\sid="(\\d+)"(?:\\shref="([^"]*)")?(?:\\sclass="([^"]*)")?\]~', | ||
| function (array $matches): string { | ||
| $id = (int) $matches[1]; | ||
|
|
||
| assertType('list{0: non-falsy-string, 1: numeric-string, 2?: string, 3?: string}', $matches); | ||
|
|
||
| $replacement = sprintf( | ||
| '<img src="%s"%s/>', | ||
| $id, | ||
| array_key_exists(3, $matches) ? sprintf(' class="%s"', $matches[3]) : '', | ||
| ); | ||
|
|
||
| assertType('list{0: non-falsy-string, 1: numeric-string, 2?: string, 3?: string}', $matches); | ||
|
|
||
| return array_key_exists(2, $matches) && $matches[2] !== '' | ||
| ? sprintf('<a href="%s">%s</a>', $matches[2], $replacement) | ||
| : $replacement; | ||
| }, | ||
| $html, | ||
| ); | ||
| } | ||
|
|
||
| public function placeholderToEditor2(string $html): void | ||
| { | ||
| $result = preg_replace_callback( | ||
| '~\[image\\sid="(\\d+)?"(?:\\shref="([^"]*)")?(?:\\sclass="([^"]*)")?\]~', | ||
| function (array $matches): string { | ||
| $id = (int) $matches[0]; | ||
|
|
||
| assertType('list{0: non-falsy-string, 1?: \'\'|numeric-string, 2?: string, 3?: string}', $matches); | ||
|
|
||
| $replacement = sprintf( | ||
| '<img src="%s"%s/>', | ||
| $id, | ||
| array_key_exists(2, $matches) ? sprintf(' class="%s"', $matches[2]) : '', | ||
| ); | ||
|
|
||
| assertType('list{0: non-falsy-string, 1?: \'\'|numeric-string, 2?: string, 3?: string}', $matches); | ||
|
|
||
| return array_key_exists(1, $matches) && $matches[1] !== '' | ||
| ? sprintf('<a href="%s">%s</a>', $matches[1], $replacement) | ||
| : $replacement; | ||
| }, | ||
| $html, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| class HelloWorld2 | ||
| { | ||
| /** | ||
| * @param list{0: string, 1: string, 2?: string, 3?: string} $b | ||
| */ | ||
| public function testUnset0OnList(array $b): void | ||
| { | ||
| assertType('true', array_is_list($b)); | ||
| unset($b[0]); | ||
| assertType('false', array_is_list($b)); | ||
| $b[] = 'foo'; | ||
| assertType('false', array_is_list($b)); | ||
| } | ||
|
|
||
| /** | ||
| * @param list{0: string, 1: string, 2?: string, 3?: string} $b | ||
| */ | ||
| public function testUnset1OnList(array $b): void | ||
| { | ||
| assertType('true', array_is_list($b)); | ||
| unset($b[1]); | ||
| assertType('false', array_is_list($b)); // Could be true | ||
|
Member
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. It could not. When you're unsetting an existing key, adding a new one will create a gap.
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. Cf https://3v4l.org/4GBOm#v8.3.30 It's still a list, and it change when we add back a new element But I agree it's harder. I removed the comments.
Member
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 problem is that the logic is unsound when passing the array around to function arguments. When we accept a list, it must mean that after appending a new item, it's still a list.
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. Yes. Maybe the right fix will be to introduce a DynamicReturnTypeExtension for array_is_list to handle this special case. Anyway, this PR should be ok to fix the issue 14177 so far. |
||
| $b[] = 'foo'; | ||
| assertType('false', array_is_list($b)); | ||
| } | ||
|
|
||
| /** | ||
| * @param list{0: string, 1: string, 2?: string, 3?: string} $b | ||
| */ | ||
| public function testUnset2OnList(array $b): void | ||
| { | ||
| assertType('true', array_is_list($b)); | ||
| unset($b[2]); | ||
| assertType('bool', array_is_list($b)); | ||
| $b[] = 'foo'; | ||
| assertType('bool', array_is_list($b)); // Could be false | ||
| } | ||
|
|
||
| /** | ||
| * @param list{0: string, 1: string, 2?: string, 3?: string} $b | ||
| */ | ||
| public function testUnset3OnList(array $b): void | ||
| { | ||
| assertType('true', array_is_list($b)); | ||
| unset($b[3]); | ||
| assertType('bool', array_is_list($b)); // Could be true | ||
| $b[] = 'foo'; | ||
| assertType('bool', array_is_list($b)); // Could be false | ||
| } | ||
|
|
||
| /** | ||
| * @param array{0: string, 1?: string, 2: string, 3?: string} $b | ||
| */ | ||
| public function testUnset0OnArray(array $b): void | ||
| { | ||
| assertType('bool', array_is_list($b)); | ||
| unset($b[0]); | ||
| assertType('false', array_is_list($b)); | ||
| $b[] = 'foo'; | ||
| assertType('false', array_is_list($b)); | ||
| } | ||
|
|
||
| /** | ||
| * @param array{0: string, 1?: string, 2: string, 3?: string} $b | ||
| */ | ||
| public function testUnset1OnArray(array $b): void | ||
| { | ||
| assertType('bool', array_is_list($b)); | ||
| unset($b[1]); | ||
| assertType('false', array_is_list($b)); | ||
| $b[] = 'foo'; | ||
| assertType('false', array_is_list($b)); | ||
| } | ||
|
|
||
| /** | ||
| * @param array{0: string, 1?: string, 2: string, 3?: string} $b | ||
| */ | ||
| public function testUnset2OnArray(array $b): void | ||
| { | ||
| assertType('bool', array_is_list($b)); | ||
| unset($b[2]); | ||
| assertType('false', array_is_list($b)); | ||
| $b[] = 'foo'; | ||
| assertType('false', array_is_list($b)); | ||
| } | ||
|
|
||
| /** | ||
| * @param array{0: string, 1?: string, 2: string, 3?: string} $b | ||
| */ | ||
| public function testUnset3OnArray(array $b): void | ||
| { | ||
| assertType('bool', array_is_list($b)); | ||
| unset($b[3]); | ||
| assertType('bool', array_is_list($b)); | ||
| $b[] = 'foo'; | ||
| assertType('bool', array_is_list($b)); // Could be false | ||
| } | ||
|
|
||
| /** | ||
| * @param list{0: string, 1: string, 2?: string, 3?: string} $a | ||
| * @param list{0: string, 1?: string, 2?: string, 3?: string} $b | ||
| * @param list{0: string, 1?: string, 2?: string, 3: string} $c | ||
| * @param 1|2 $int | ||
| */ | ||
| public function testUnsetNonConstant(array $a, array $b, array $c, int $int): void | ||
| { | ||
| assertType('true', array_is_list($a)); | ||
| assertType('true', array_is_list($b)); | ||
| assertType('true', array_is_list($c)); | ||
| unset($a[$int]); | ||
| unset($b[$int]); | ||
| unset($c[$int]); | ||
| assertType('false', array_is_list($a)); | ||
| assertType('bool', array_is_list($b)); | ||
| assertType('bool', array_is_list($c)); | ||
| } | ||
|
|
||
| /** | ||
| * @param list{0?: string, 1?: string, 2?: string, 3?: string} $a | ||
| * @param list{0: string, 1?: string, 2?: string, 3?: string} $b | ||
| */ | ||
| public function testUnsetInt(array $a, array $b, array $c, int $int): void | ||
| { | ||
| assertType('true', array_is_list($a)); | ||
| assertType('true', array_is_list($b)); | ||
| unset($a[$int]); | ||
| unset($b[$int]); | ||
| assertType('bool', array_is_list($a)); | ||
| assertType('false', array_is_list($b)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace Bug14177; | ||
|
|
||
| class HelloWorld | ||
| { | ||
| public function placeholderToEditor(string $html): void | ||
| { | ||
| $result = preg_replace_callback( | ||
| '~\[image\\sid="(\\d+)"(?:\\shref="([^"]*)")?(?:\\sclass="([^"]*)")?]~', | ||
| function (array $matches): string { | ||
| $id = (int) $matches[1]; | ||
|
|
||
| $replacement = sprintf( | ||
| '<img src="%s"%s/>', | ||
| $id, | ||
| array_key_exists(3, $matches) ? sprintf(' class="%s"', $matches[3]) : '', | ||
| ); | ||
|
|
||
| return array_key_exists(2, $matches) && $matches[2] !== '' | ||
| ? sprintf('<a href="%s">%s</a>', $matches[2], $replacement) | ||
| : $replacement; | ||
| }, | ||
| $html, | ||
| ); | ||
| } | ||
| } |
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.
I don't like these alignments. There should be just a single space between the type and variable.