Skip to content

Fix bug in array-shape subtraction #3308

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.

Sign up for GitHub

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

Draft
wants to merge 8 commits into
base: 1.11.x
Choose a base branch
from
Draft

Fix bug in array-shape subtraction #3308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de109b6
Fix bug in array-shape subtraction
staabm Aug 9, 2024
499d9ef
more tests
staabm Aug 9, 2024
21d99b4
fix
staabm Aug 9, 2024
f894e7b
fix
staabm Aug 9, 2024
282252a
more assertions
staabm Aug 9, 2024
09f3526
Update bug11488.php
staabm Aug 9, 2024
2f692c6
Update TypeCombinatorTest.php
staabm Aug 12, 2024
07a2469
cs
staabm Aug 12, 2024
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
19 changes: 19 additions & 0 deletions src/Type/Constant/ConstantArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,25 @@ public function isSuperTypeOf(Type $type): TrinaryLogic
$results[] = $isValueSuperType;
}

if (
$this->isList->yes()
&& $type->isList->yes()
&& count($this->keyTypes) !== count($type->keyTypes)
&& count($type->optionalKeys) === 0
) {
$keepSeparate = true;
foreach ($this->valueTypes as $valueType) {
if ($valueType->isConstantValue()->yes()) {
$keepSeparate = false;
break;
}
}
Comment on lines +396 to +402
Copy link
Contributor Author

@staabm staabm Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we somehow need to identify which constant arrays we can/want to keep separte (in our case tagged unions), without running into "is always false" comparison false positives like

list<array{string, 1|2|3|4|5|6|7}> and list<array{string, 1|2|3|4|5|6|7, int}> will always evaluate to false

I think my approach is wrong, but I have no better idea atm

Copy link
Member

Choose a reason for hiding this comment

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

Any change to isSuperTypeOf logic needs tests in TypeCombinatorTest::dataUnion and dataIntersect. So please express there what you're trying to achieve, only when reviewing that I can decide if it makes sense or not.

Copy link
Contributor Author

@staabm staabm Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I could not figure out a dataUnion and dataIntersect test.

but what made sense to me was a test like
subtract array{mixed} from array{mixed, string|null, mixed}|array{mixed} should lead to array{mixed, string|null, mixed}


if ($keepSeparate) {
return TrinaryLogic::createNo();
}
}

return TrinaryLogic::createYes()->and(...$results);
}

Expand Down
64 changes: 64 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug11488.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

namespace Bug11488;

use function PHPStan\Testing\assertType;

class Foo
{
/**
* @param array{mixed}|array{0: mixed, 1?: string|null} $row
*/
protected function testOptionalKeys(array $row): void
{
if (count($row) === 1) {
assertType('array{0: mixed, 1?: string|null}', $row);
} else {
assertType('array{0: mixed, 1?: string|null}', $row);
}

if (count($row) !== 1) {
assertType('array{0: mixed, 1?: string|null}', $row);
} else {
assertType('array{0: mixed, 1?: string|null}', $row);
}

if (count($row) !== 2) {
assertType('array{0: mixed, 1?: string|null}', $row);
} else {
// should be array{0: mixed, 1: string|null}
assertType('array{mixed, mixed}', $row);
Comment on lines +29 to +30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think thats another bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed via #3309

}

if (count($row) !== 3) {
// should be array{0: mixed, 1?: string|null}
assertType('array{0: mixed, 1?: mixed}', $row);
} else {
assertType('*NEVER*', $row);
}
}

/**
* @param array{mixed}|array{mixed, string|null, mixed} $row
*/
protected function test(array $row): void
{
if (count($row) !== 1) {
assertType('array{mixed, string|null, mixed}', $row);
} else {
assertType('array{mixed}', $row);
}

if (count($row) !== 2) {
assertType('array{mixed, string|null, mixed}|array{mixed}', $row);
} else {
assertType('*NEVER*', $row);
}

if (count($row) !== 3) {
assertType('array{mixed}', $row);
} else {
assertType('array{mixed, string|null, mixed}', $row);
}
}
}
43 changes: 43 additions & 0 deletions tests/PHPStan/Type/TypeCombinatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4876,6 +4876,49 @@ public function dataRemove(): array
ObjectShapeType::class,
'object{foo?: int}',
],
[
new UnionType([
new ConstantArrayType(
[
new ConstantIntegerType(0),
],
[
new MixedType(true),
],
[0],
[],
true,
),
new ConstantArrayType(
[
new ConstantIntegerType(0),
new ConstantIntegerType(1),
new ConstantIntegerType(2),
],
[
new MixedType(true),
new UnionType([new StringType(), new NullType()]),
new MixedType(true),
],
[0],
[],
true,
),
]),
new ConstantArrayType(
[
new ConstantIntegerType(0),
],
[
new MixedType(true),
],
[0],
[],
true,
),
ConstantArrayType::class,
'array{mixed, string|null, mixed}',
],
];
}

Expand Down
Loading