Skip to content

Add early termination for NeverType match expression #1078

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

Jump to bottom
Closed
rajyan wants to merge 3 commits into from
Closed

Add early termination for NeverType match expression #1078

rajyan wants to merge 3 commits into from

Conversation

@rajyan
Copy link
Contributor

@rajyan rajyan commented Mar 17, 2022

ondrejmirtes
ondrejmirtes requested changes Mar 17, 2022
View reviewed changes
src/Analyser/NodeScopeResolver.php Outdated
Copy link
Member

@ondrejmirtes ondrejmirtes Mar 17, 2022

Choose a reason for hiding this comment

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

I'm not sure about this fix. What should be done instead is that these situations should be caught by the if ($exprType instanceof NeverType && $exprType->isExplicit()) { instead.

"Implicit" NeverType is for example when you try to intersect two incompatible types (string&int). "Explicit" mixed is when you have a throw expression, function with @return never and similar.

This means that the explicitness of "NeverType" gets lost when we have a match expression with "explicit never" arms, and that's something that should be fixed instead.

Copy link
Contributor Author

@rajyan rajyan Mar 17, 2022

Choose a reason for hiding this comment

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

Thank you for the review and informative examples. I'll try fixing in that way!

@rajyan rajyan marked this pull request as draft March 17, 2022 13:54
@ondrejmirtes ondrejmirtes force-pushed the 1.5.x branch 3 times, most recently from ddd20b4 to 95d480b Compare March 18, 2022 19:53
@rajyan
Copy link
Contributor Author

rajyan commented Mar 20, 2022 •
edited
Loading

The cause is here,

phpstan-src/src/Type/TypeCombinator.php

Lines 134 to 137 in 51e2df3

if ($types[$i] instanceof NeverType) {
unset($types[$i]);
continue;
}

phpstan-src/src/Type/TypeCombinator.php

Line 304 in 51e2df3

return new NeverType();

but not sure how to fix yet.

Also, the comment says that it's transforming A | never to A

phpstan-src/src/Type/TypeCombinator.php

Line 272 in 51e2df3

// transform A | never to A

but NeverType is not handled there.

I think it can be handled like the comment says, and never | never can be NeverType with $type->isExplicit() && $type->isExplicit().

But if I fix like that, test fails here with array{Never, Never}

phpstan-src/tests/PHPStan/Analyser/data/bug-1516.php

Lines 1 to 37 in 86fd620

<?php
namespace Bug1516;
use function PHPStan\Testing\assertType;
class FlowNodeManager
{
public function _test() : void
{
$out = [];
$a =
[
'foof' => 'barr',
'ftt' => []
];
foreach ($a as $k => $b) {
$str = 'toto';
assertType('\'toto\'|array{}', $out[$k]);
if (is_array($b)) {
// $out[$k] is redefined there before the array_merge
assertType('\'toto\'|array{}', $out[$k]);
$out[$k] = [];
assertType('array{}', $out[$k]);
$out[$k] = array_merge($out[$k], []);
assertType('array{}', $out[$k]);
} else {
// I think phpstan takes this definition as a string and takes no account of the foreach
$out[$k] = $str;
assertType('\'toto\'', $out[$k]);
}
}
}
}
.

@rajyan
Copy link
Contributor Author

rajyan commented Mar 20, 2022

Have to take more time breaking and fixing TypeCombinator to understand it deeper...

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 21, 2022

Hi, I've took the liberty to push a change here. Some tests still fail but I like it beter. Before TypeCombinator removed NeverType from $types so the fact the it's an explicit NeverType got lost.

Right now we leave NeverType in there so if TypeCombinator::union() is called with multiple explicit NeverType instances, the explicitness is preserved.

But some tests currently fail and I don't know why. Please debug that, thanks :)

Please read more about what's going on in TypeCombinator here: https://phpstan.org/developing-extensions/type-system (especially https://phpstan.org/developing-extensions/type-system#type-normalization).

@rajyan
Copy link
Contributor Author

rajyan commented Mar 22, 2022 •
edited
Loading

@ondrejmirtes
Hi, thank you for looking into this PR.
The failing test were from NeverType handling, which TypeCombinator says that

phpstan-src/src/Type/TypeCombinator.php

Line 268 in c4ac24b

// transform A | never to A

but not transformed actually.

By adding NeverType union handling c4ac24b, all tests passed except this one.

phpstan-src/tests/PHPStan/Analyser/data/bug-1516.php

Lines 1 to 37 in 86fd620

<?php
namespace Bug1516;
use function PHPStan\Testing\assertType;
class FlowNodeManager
{
public function _test() : void
{
$out = [];
$a =
[
'foof' => 'barr',
'ftt' => []
];
foreach ($a as $k => $b) {
$str = 'toto';
assertType('\'toto\'|array{}', $out[$k]);
if (is_array($b)) {
// $out[$k] is redefined there before the array_merge
assertType('\'toto\'|array{}', $out[$k]);
$out[$k] = [];
assertType('array{}', $out[$k]);
$out[$k] = array_merge($out[$k], []);
assertType('array{}', $out[$k]);
} else {
// I think phpstan takes this definition as a string and takes no account of the foreach
$out[$k] = $str;
assertType('\'toto\'', $out[$k]);
}
}
}
}

This one failed because it has a different reason related to array_merge (the test is green if array_merge is removed).
Empty array for ConstantArray was changed to and explicit NeverType in this commit c2b9e71

but the handling for ArrayMergeFunctionDynamicReturnTypeExtension

phpstan-src/src/Type/Php/ArrayMergeFunctionDynamicReturnTypeExtension.php

Lines 58 to 60 in f241051

if ($keyType instanceof NeverType && !$keyType->isExplicit()) {
return new ConstantArrayType([], []);
}

is expecting a implicit NeverType

I hope this fix is in the right direction. I'll add a regression test for NeverType (and maybe for ArrayMergeFunctionDynamicReturnTypeExtension ?) if it looks good to you!

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 22, 2022

The thing is - NeverType is already handled by TypeCombinator::compareTypesInUnion() by these lines:

		if ($b->isSuperTypeOf($a)->yes()) {
			return [null, $b];
		}

		if ($a->isSuperTypeOf($b)->yes()) {
			return [$a, null];
		}

The root issue was that some types wrongly responded to isSuperTypeOf with NeverType. So I addressed the root issue and merged it: 99951ac

Thank you.

@rajyan rajyan deleted the fix/issue-6251 branch March 22, 2022 09:43
@rajyan
Copy link
Contributor Author

rajyan commented Mar 22, 2022

Thank you.
Sorry I couldn't get to the root issue...

This was referenced Mar 22, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Match interpretation doesn't identify return type correctly Match statements throwing exceptions not interpreted correctly

2 participants

@rajyan @ondrejmirtes