Skip to content
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

Support non-empty-array in InArrayFunctionTypeSpecifyingExtension #1108

Merged
merged 1 commit into from Jun 17, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 23, 2022

@staabm staabm marked this pull request as ready for review March 23, 2022 16:32
@staabm staabm marked this pull request as draft March 23, 2022 16:33

$specifiedTypes = new SpecifiedTypes([], []);
if ($context->true()) {
$arrayType = TypeCombinator::intersect($arrayType, new NonEmptyArrayType());
Copy link
Contributor Author

@staabm staabm Mar 23, 2022

Choose a reason for hiding this comment

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

it seems this intersection alters some array-types, so they no longer get reported as TRUE by the ImpossibleTypeChecker.. still investigating..

Copy link
Contributor Author

@staabm staabm Mar 23, 2022

Choose a reason for hiding this comment

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

had to add a lot of checks, to make the test-suite - especially the ImpossibleTypeChecker - happy, see e.g. for errors which happen, without these checks

...
1) PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRuleTest::testImpossibleCheckTypeFunctionCall
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 194: Call to function method_exists() with $this(CheckTypeFunctionCall\FinalClassWithMethodExists) and 'doBar' will always evaluate to false.
 209: Call to function property_exists() with $this(CheckTypeFunctionCall\FinalClassWithPropertyExists) and 'fooProperty' will always evaluate to true.
 212: Call to function property_exists() with $this(CheckTypeFunctionCall\FinalClassWithPropertyExists) and 'barProperty' will always evaluate to false.
-235: Call to function in_array() with arguments int, array{'foo', 'bar'} and true will always evaluate to false.
-244: Call to function in_array() with arguments 'bar'|'foo', array{'baz', 'lorem'} and true will always evaluate to false.
-248: Call to function in_array() with arguments 'bar'|'foo', array{'foo', 'bar'} and true will always evaluate to true.
-252: Call to function in_array() with arguments 'foo', array{'foo'} and true will always evaluate to true.
-256: Call to function in_array() with arguments 'foo', array{'foo', 'bar'} and true will always evaluate to true.
 320: Call to function in_array() with arguments 'bar', array{}|array{'foo'} and true will always evaluate to false.
-336: Call to function in_array() with arguments 'baz', array{0: 'bar', 1?: 'foo'} and true will always evaluate to false.
-343: Call to function in_array() with arguments 'foo', array{} and true will always evaluate to false.
 360: Call to function array_key_exists() with 'a' and array{a: 1, b?: 2} will always evaluate to true.
 366: Call to function array_key_exists() with 'c' and array{a: 1, b?: 2} will always evaluate to false.
 560: Call to function is_string() with mixed will always evaluate to false.

C:\dvl\Workspace\phpstan-src-staabm\src\Testing\RuleTestCase.php:131
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRuleTest.php:47

2) PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRuleTest::testImpossibleCheckTypeFunctionCallWithoutAlwaysTrue
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 105: Call to function is_numeric() with 'blabla' will always evaluate to false.
 194: Call to function method_exists() with $this(CheckTypeFunctionCall\FinalClassWithMethodExists) and 'doBar' will always evaluate to false.
 212: Call to function property_exists() with $this(CheckTypeFunctionCall\FinalClassWithPropertyExists) and 'barProperty' will always evaluate to false.
-235: Call to function in_array() with arguments int, array{'foo', 'bar'} and true will always evaluate to false.
-244: Call to function in_array() with arguments 'bar'|'foo', array{'baz', 'lorem'} and true will always evaluate to false.
 320: Call to function in_array() with arguments 'bar', array{}|array{'foo'} and true will always evaluate to false.
-336: Call to function in_array() with arguments 'baz', array{0: 'bar', 1?: 'foo'} and true will always evaluate to false.
-343: Call to function in_array() with arguments 'foo', array{} and true will always evaluate to false.
 366: Call to function array_key_exists() with 'c' and array{a: 1, b?: 2} will always evaluate to false.
 560: Call to function is_string() with mixed will always evaluate to false.
 571: Call to function is_callable() with mixed will always evaluate to false.

C:\dvl\Workspace\phpstan-src-staabm\src\Testing\RuleTestCase.php:131
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRuleTest.php:256

3) PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRuleTest::testBugInArrayDateFormat
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 '39: Call to function in_array() with arguments 'a', non-empty-array<int, 'a'> and true will always evaluate to true.
 43: Call to function in_array() with arguments 'b', non-empty-array<int, 'a'> and true will always evaluate to false.
-47: Call to function in_array() with arguments int, array{} and true will always evaluate to false.
 '
...

I am pretty sure, there is some kind of check which should be done instead, but I did not find it yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm maybe there is some kind of cache invalidated, because the type is altered within the type-specifying-extension?

e.g. intersecting array{'foo', 'bar'} with NonEmptyArrayType does not change the type itself, but it feels like since we get a new object this has some kind of side-effect on the ImpossibleTypeChecker..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

investigation still goes on...

before this PR, phpstan took this path to return false

if (count($sureTypes) === 1 && count($sureNotTypes) === 0) {
$sureType = reset($sureTypes);
if ($isSpecified($sureType[0])) {
return null;
}
if ($this->treatPhpDocTypesAsCertain) {
$argumentType = $scope->getType($sureType[0]);
} else {
$argumentType = $scope->getNativeType($sureType[0]);
}
/** @var Type $resultType */
$resultType = $sureType[1];
$isSuperType = $resultType->isSuperTypeOf($argumentType);
if ($isSuperType->yes()) {
return true;
} elseif ($isSuperType->no()) {
return false;

which produced the errors, now missing with this PR applied. after this change we have more then 1 sureType, and therefore the code takes a different path.

Copy link
Contributor Author

@staabm staabm Mar 25, 2022

Choose a reason for hiding this comment

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

I am getting the feeling, that a TypeSpecifyingExtension cannot specify types for different expressions in one run.

I cannot find any other TypeSpecifyingExtension wich uses unionWith. also every other TypeSpecifyingExtension which I checked in the codebase always specifies its type for a single arg.


starting wit 1cb657f I changed the TypeSpecifyingExtension to drop these hopefully unneeded checks for a change in the ImpossibleTypeChecker

Comment on lines 269 to 271
$types = TypeCombinator::union(
...array_column($sureNotTypes, 1),
);
Copy link
Contributor Author

@staabm staabm Mar 25, 2022

Choose a reason for hiding this comment

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

instead of union togehter all types within $sureNotTypes, we now take the $expr into account, and union only these refering to the same expr.

adding this here to support the change in InArrayFunctionTypeSpecifyingExtension regresses some other parts.
I am not sure how this should progress further and therefore wait for a first round of review from ondrey.

@rvanvelzen
Copy link
Contributor

The changes in #1430 should make it possible to finish this as well :)

@ondrejmirtes ondrejmirtes changed the base branch from 1.6.x to 1.7.x June 17, 2022 08:17
@ondrejmirtes ondrejmirtes marked this pull request as ready for review June 17, 2022 08:17
@ondrejmirtes
Copy link
Member

I've rebased it to see how the code works :)

@ondrejmirtes
Copy link
Member

Wow, looks like it works in the first try :)

@ondrejmirtes ondrejmirtes merged commit 5b2d7b6 into phpstan:1.7.x Jun 17, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the in-array branch June 17, 2022 08:39
@staabm
Copy link
Contributor Author

staabm commented Jun 17, 2022

Thank you guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants