Improve `in_array` extension to get rid of related impossible check adaptions #1514

herndlm · 2022-07-13T18:26:35Z

Almost all of the problems were solved by adding the $offsetValueType information to HasOffset. This way it is now possible to e.g. specify a type non-empty-array<int, int>&hasOffset(int, 17) which tells us that the generic array consists of ints AND has at least one entry with the value 17. (but if the type is dumped/printed the hasOffset is not showing the offset value to e.g. not mess up baselines..).

…daptions

src/Type/Accessory/HasOffsetType.php

src/Type/Php/InArrayFunctionTypeSpecifyingExtension.php

herndlm · 2022-07-13T18:31:15Z

tests/PHPStan/Analyser/data/in-array.php

@@ -26,7 +28,7 @@ public function doFoo(
 			return;
 		}

-		if (in_array($r, $strings, true)) {
+		if (in_array($r, $moreStrings, true)) {


I had to do that because $strings was already type-specified from previous conditionals in the same scope which messed the result up here since types are specified better now.

tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php

herndlm · 2022-07-13T18:33:27Z

src/Type/Accessory/HasOffsetType.php

 		return $type->isOffsetAccessible()
-			->and($type->hasOffsetValueType($this->offsetType));
+			->and($type->hasOffsetValueType($this->offsetType))
+			->and($this->offsetValueType->isSuperTypeOf($type->getOffsetValueType($this->offsetType)));


not sure if isSuperTypeOf and friends are really correct, e.g. I did not touch isSubTypeOf or accept at all

src/Type/Constant/ConstantArrayType.php

herndlm · 2022-07-13T20:59:36Z

tests/PHPStan/Analyser/data/in-array-non-empty.php

@@ -14,15 +14,15 @@ class HelloWorld
 	public function sayHello(array $array): void
 	{
 		if(in_array("thing", $array, true)){
-			assertType('non-empty-array<int, string>', $array);
+			assertType('non-empty-array<int, string>&hasOffset(mixed)', $array);


this is obviously weird. I'd prefer to rather have an hasOffsetValue('thing') I guess. But on the other hand, the HasOffset with both the offset and offset value might be useful somewhere else?
Also, mixed is potentially confusing here, isn't it? :/

maybe print offset and offsetValue type within the bracket, like
&hasOffset(mixed, 'thing') ?

that would be easy to do and makes sense, yeah. I did not do it yet because I was scared that it will lead to too many changes in e.g. baselines. but we can't think always about that, right? I'll check it out :)

src/Type/Php/InArrayFunctionTypeSpecifyingExtension.php

herndlm · 2022-07-14T05:03:10Z

This is obviously not ready yet. I'll keep thinking about and come back to it. E.g. generic array usage should be fixed with hasoffset only, while constant array most likely needs special handling to e.g. make optional entries non-optional for example.

If you ask me if this PR is going to be finished soon, I'd answer maybe. Feels a bit like https://xkcd.com/612/ right now :D

The array shape should be of a different type I'd say, but I'm surprised this worked at all tbh

…alue is there + document tests

herndlm · 2022-07-14T10:18:46Z

tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php

+			[
+				'Call to function in_array() with arguments \'abstract methods\'|\'constructor\'|\'destructor\'|\'final methods\'|\'magic methods\'|\'private constants\'|\'private methods\'|\'private properties\'|\'private static…\'|\'private static…\'|\'protected abstract…\'|\'protected constants\'|\'protected final…\'|\'protected methods\'|\'protected properties\'|\'protected static…\'|\'protected static…\'|\'protected static…\'|\'protected static…\'|\'public abstract…\'|\'public constants\'|\'public final methods\'|\'public methods\'|\'public properties\'|\'public static…\'|\'public static final…\'|\'public static…\'|\'public static…\'|\'static constructors\'|\'static methods\'|\'static properties\', array{0: \'final methods\'|\'private static…\'|\'protected final…\'|\'public abstract…\'|\'public constants\'|\'public final methods\'|\'public static…\'|\'static constructors\'|\'static properties\', 1: \'abstract methods\'|\'private methods\'|\'protected abstract…\'|\'protected constants\'|\'protected final…\'|\'protected static…\'|\'protected static…\'|\'public properties\'|\'public static final…\', 2?: \'private constants\'|\'private static…\'|\'protected abstract…\'|\'protected properties\'|\'protected static…\'|\'public abstract…\'|\'public static…\'|\'public static final…\'|\'static methods\', 3?: \'constructor\'|\'private properties\'|\'protected static…\'|\'protected static…\'|\'public static…\', 4?: \'destructor\'|\'protected static…\'|\'protected static…\'|\'public static…\', 5?: \'protected methods\'|\'public methods\'|\'public static…\', 6?: \'protected methods\'|\'protected static…\', 7?: \'private methods\'|\'private static…\', ...} and true will always evaluate to true.',
+				132,
+			],


this is showing up because in_array_key is specifying constant types I think which it shouldn't. I created phpstan/phpstan#7621 for that

herndlm · 2022-07-14T10:21:26Z

src/Type/Php/InArrayFunctionTypeSpecifyingExtension.php

+		if (
+			$arrayType instanceof ConstantArrayType && !$arrayType->isEmpty()
+			&& count(TypeUtils::getConstantScalars($needleType)) === 0 && $arrayValueType->isSuperTypeOf($needleType)->yes()
+		) {
+			// Avoid false-positives with e.g. a string needle and array{'self', string} as haystack
+			// For such cases there seems to be nothing more that we can specify unfortunately
+			return $specifiedTypes;
+		}
+


this is an edge case I'd really like to not have here. But I didn't find a way to specify anything here, that doesn't get normalized away (e.g. $needle is 'self'|string which is string and the array must have a string value for sure, which it already has). There was initially no test case for this, but it was showing up as error in slevomat-cs.
any other ideas?

herndlm · 2022-07-14T10:24:58Z

src/Type/Php/InArrayFunctionTypeSpecifyingExtension.php

+		// If e.g. needle is 'a' and haystack non-empty-array<int, 'a'> we can be sure that this always evaluates to true
+		// Belows HasOffset::isSuperTypeOf cannot deal with that since it calls ArrayType::hasOffsetValueType and that returns maybe at max
+		if ($needleType instanceof ConstantScalarType && $arrayType->isIterableAtLeastOnce()->yes() && $arrayValueType->equals($needleType)) {
+			return $specifiedTypes;
+		}
+


I also dislike this edge case, maybe it is possible to adapt HasOffset::isSuperTypeOf somehow to figure this out.. Any ideas?

ondrejmirtes · 2022-07-14T10:55:14Z

I'm gonna stop you right there - you're trying to do too many things here - improve in_array(), and introduce value type into HasOffsetType.

These should be two separate PRs. Please send HasOffsetType separately as that's more difficult and going to have many implications we need to discuss.

herndlm · 2022-07-14T11:10:38Z

I totally agree, I'm quite unsure about HasOffsetType tbh, I was even thinking about a HasOffsetValueType, but not sure if that helps either.
If I start with HasOffsetType - do you have already ideas how and where roughly? And the most important thing - how to test it? UPDATE: It looks at least like I'm not using that for constant arrays and that could be improved first here maybe hmm

ondrejmirtes · 2022-07-14T11:28:36Z

There are some issues where I mention it and that should fix some things. If you can't think of any scenarios it'd fix then it's not probably worth it 😊

herndlm · 2022-07-14T11:48:33Z

thx for the explanations. I found at least one issue where it might help, yeah. well, I'm gonna step back and re-think it :) I was kinda forcing it I guess

Improve in_array extension to get rid of related impossible check a…

e994f69

…daptions

herndlm force-pushed the in-array-impossible-checks branch from 31c2a6c to e994f69 Compare July 13, 2022 18:26

herndlm commented Jul 13, 2022

View reviewed changes

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved

herndlm force-pushed the in-array-impossible-checks branch from b51b928 to e994f69 Compare July 13, 2022 19:26

Simplified things but extracted 2 remaining special cases

b02bd1b

herndlm commented Jul 13, 2022

View reviewed changes

src/Type/Php/InArrayFunctionTypeSpecifyingExtension.php Outdated Show resolved Hide resolved

herndlm added 4 commits July 14, 2022 09:15

Add 2 more test cases that are currently OK

12c1301

The array shape should be of a different type I'd say, but I'm surprised this worked at all tbh

Add another constant array test that rules out than optional offset v…

7b35446

…alue is there + document tests

Improve ConstantArrayType handling

7d3a859

Commit new slevomat errors cause by presumably another bug

46a92e1

herndlm commented Jul 14, 2022

View reviewed changes

herndlm closed this Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `in_array` extension to get rid of related impossible check adaptions #1514

Improve `in_array` extension to get rid of related impossible check adaptions #1514

herndlm commented Jul 13, 2022 •

edited

herndlm Jul 13, 2022 •

edited

herndlm Jul 13, 2022 •

edited

herndlm Jul 13, 2022 •

edited

staabm Jul 14, 2022

herndlm Jul 14, 2022

herndlm commented Jul 14, 2022 •

edited

herndlm Jul 14, 2022

herndlm Jul 14, 2022

herndlm Jul 14, 2022

ondrejmirtes commented Jul 14, 2022

herndlm commented Jul 14, 2022 •

edited

ondrejmirtes commented Jul 14, 2022

herndlm commented Jul 14, 2022

Improve in_array extension to get rid of related impossible check adaptions #1514

Improve in_array extension to get rid of related impossible check adaptions #1514

Conversation

herndlm commented Jul 13, 2022 • edited

herndlm Jul 13, 2022 • edited

Choose a reason for hiding this comment

herndlm Jul 13, 2022 • edited

Choose a reason for hiding this comment

herndlm Jul 13, 2022 • edited

Choose a reason for hiding this comment

staabm Jul 14, 2022

Choose a reason for hiding this comment

herndlm Jul 14, 2022

Choose a reason for hiding this comment

herndlm commented Jul 14, 2022 • edited

herndlm Jul 14, 2022

Choose a reason for hiding this comment

herndlm Jul 14, 2022

Choose a reason for hiding this comment

herndlm Jul 14, 2022

Choose a reason for hiding this comment

ondrejmirtes commented Jul 14, 2022

herndlm commented Jul 14, 2022 • edited

ondrejmirtes commented Jul 14, 2022

herndlm commented Jul 14, 2022

Improve `in_array` extension to get rid of related impossible check adaptions #1514

Improve `in_array` extension to get rid of related impossible check adaptions #1514

herndlm commented Jul 13, 2022 •

edited

herndlm Jul 13, 2022 •

edited

herndlm Jul 13, 2022 •

edited

herndlm Jul 13, 2022 •

edited

herndlm commented Jul 14, 2022 •

edited

herndlm commented Jul 14, 2022 •

edited