Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 70 additions & 5 deletions src/Type/Constant/ConstantArrayType.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -762,6 +770,7 @@
continue;
}

$arrayHasChanged = true;
if (in_array($i, $optionalKeys, true)) {
continue 2;
}
Expand All @@ -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
Copy link
Copy Markdown
Member

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.

*/
private static function isListAfterUnset(array $newKeyTypes, array $newOptionalKeys, TrinaryLogic $arrayIsList, bool $unsetOptionalKey): TrinaryLogic
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might be able to kill the mutation with somethig like

https://phpstan.org/r/53bdb42a-d790-4576-aa47-62e33eb40865

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.4, ubuntu-latest)

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator": @@ @@ */ private static function isListAfterUnset(array $newKeyTypes, array $newOptionalKeys, TrinaryLogic $arrayIsList, bool $unsetOptionalKey): TrinaryLogic { - if (!$unsetOptionalKey || $arrayIsList->no()) { + if (!$unsetOptionalKey || !$arrayIsList->yes()) { return TrinaryLogic::createNo(); }

Check warning on line 830 in src/Type/Constant/ConstantArrayType.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.3, ubuntu-latest)

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator": @@ @@ */ private static function isListAfterUnset(array $newKeyTypes, array $newOptionalKeys, TrinaryLogic $arrayIsList, bool $unsetOptionalKey): TrinaryLogic { - if (!$unsetOptionalKey || $arrayIsList->no()) { + if (!$unsetOptionalKey || !$arrayIsList->yes()) { return TrinaryLogic::createNo(); }
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
Expand Down
203 changes: 203 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-14177.php
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
So technically it could be true, and PHPStan should be aware it's false as soon as we add back an element.

But I agree it's harder.

I removed the comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -1145,4 +1145,10 @@ public function testPr4375(): void
$this->analyse([__DIR__ . '/data/pr-4375.php'], []);
}

public function testBug14177(): void
{
$this->treatPhpDocTypesAsCertain = true;
$this->analyse([__DIR__ . '/data/bug-14177.php'], []);
}

}
27 changes: 27 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/bug-14177.php
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,
);
}
}
Loading