Fix constant-string handling in union-types #2134

staabm · 2022-12-21T09:00:08Z

closes phpstan/phpstan#8568
closes phpstan/phpstan#8562

src/Type/UnionType.php

staabm · 2022-12-21T09:10:53Z

src/Type/UnionTypeHelper.php

@@ -70,6 +70,10 @@ public static function getConstantStrings(array $types): array
 			$strings[] = $type->getConstantStrings();
 		}

+        if ($strings === []) {


php <= 7.3 doesn't like array_merge with an variadic empty array

7) PHPStan\Rules\Properties\TypesAssignedToPropertiesRuleTest::testTypesAssignedToStaticPropertiesExpressionNames ArgumentCountError: array_merge() expects at least 1 parameter, 0 given

ondrejmirtes

Please show and fix the problems with the other methods as well. It's also about getArrays and getConstantArrays. Feel free to write unit tests.
Do no longer call UnionTypeHelper at all. Obviously the implementation has to be different for UnionType and IntersectionType, this just complicates and obfuscates things.
Please also verify these methods work correctly in IntersectionType. As I said - do not call UnionTypeHelper in UnionType/IntersectionType for getArrays, getConstantArrays, getConstantStrings at all.

src/Type/IntersectionType.php

ondrejmirtes · 2022-12-21T19:57:07Z

src/Type/UnionType.php

-		return UnionTypeHelper::getConstantArrays($this->getTypes());
+		$constantArrays = [];
+		foreach ($this->getTypesOfClass(ConstantArrayType::class) as $type) {
+			$constantArrays[] = $type->getConstantArrays();


This logic is wrong. For example if I have (array{1?: int, 2?: string}&non-empty-array)|(array{Foo, Bar}, these two arrays should still be returned.

But if I have array{1?: int, 2?: string}|int, this method should return an empty array.

Basically - the function should return something if all the types in the union return a non-empty array from getConstantArrays. If one of them returns an empty array, an empty array should be returned. The getTypesOfClass method is backwards - it returns doing instanceof *Type which is something that no longer works.

ondrejmirtes · 2022-12-21T20:04:22Z

src/Type/UnionType.php

 	}

 	public function getConstantStrings(): array
 	{
-		return UnionTypeHelper::getConstantStrings($this->getTypes());
+		$strings = [];
+		foreach ($this->getTypesOfClass(ConstantStringType::class) as $type) {


Same logic as with getConstantArrays.

You could test this with:

''|numeric-string - should return empty array

Create a fake denormalized type and test that getConstantStrings returns an array of 'foo' and 'bar':

new UnionType([ new IntersectionType([new ConstantStringType('foo'), new AccessoryLiteralStringType()]), new IntersectionType([new ConstantStringType('bar'), new AccessoryLiteralStringType()]), ])

(In the future there could be a valid intersection like this - for example if we add nominal types and people are able to define new types like email-string, we could have 'ondrej@mirtes.cz'&email-string.

thx for the detailled feedback. I couldn't have come up with such tests on my own.

ondrejmirtes

Otherwise 👍

src/Type/UnionType.php

ondrejmirtes · 2022-12-29T13:02:47Z

Pushed some fixes. There's one remaining failing test (PHPStan\Analyser\AnalyserIntegrationTest::testBug7903). It seemed familiar - I get the same error when I try to do this change:

diff --git a/src/Type/IntersectionType.php b/src/Type/IntersectionType.php
index 913e5a455..e3d745274 100644
--- a/src/Type/IntersectionType.php
+++ b/src/Type/IntersectionType.php
@@ -551,7 +551,12 @@ class IntersectionType implements CompoundType

 	public function getOffsetValueType(Type $offsetType): Type
 	{
-		return $this->intersectTypes(static fn (Type $type): Type => $type->getOffsetValueType($offsetType));
+		$result = $this->intersectTypes(static fn (Type $type): Type => $type->getOffsetValueType($offsetType));
+		if ($this->isOversizedArray()->yes()) {
+			return TypeUtils::toBenevolentUnion($result);
+		}
+
+		return $result;
 	}

 	public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type

So it seems like the test case is very brittle. Can you please investigate what types are involved on top of 1.9.x (so that there are no errors) and what types are involved in your PR, so that we can decide what should be done there?

Thanks.

staabm · 2022-12-31T12:35:49Z

src/Analyser/MutatingScope.php

@@ -4200,15 +4200,15 @@ private function generalizeVariableTypeHolders(

 			$variableTypeHolders[$variableExprString] = new ExpressionTypeHolder(
 				$variableTypeHolder->getExpr(),
-				self::generalizeType($variableTypeHolder->getType(), $otherVariableTypeHolders[$variableExprString]->getType()),
+				self::generalizeType($variableTypeHolder->getType(), $otherVariableTypeHolders[$variableExprString]->getType(), 0),


I printed the types after generalization at this line in this PR vs. 1.9.x-dev at 9128321

most types are identical. the only different are:

- string(71) "non-empty-array<literal-string&non-falsy-string, mixed>&oversized-array" + string(206) "non-empty-array<literal-string&non-falsy-string, non-empty-array<int|string, 0|(non-empty-array<int|string, 0|(non-empty-array<int|string, *ERROR*>&oversized-array)|float>&oversized-array)>>&oversized-array"

so after this PR we no longer get MixedType and therefore we get the errors

staabm · 2022-12-31T13:50:07Z

src/Analyser/MutatingScope.php

-				$type = $temp;
-			}
+			$type = $temp;
+			$arrays = TypeUtils::getAnyArrays($type);


I think our problem is that getAnyArrays does not return arrays for the types in question because we are working with a union of intersection types

and TypeUtils::getAnyArrays does not recursivly check the intersections within unions

phpstan-bot · 2022-12-31T14:00:51Z

This pull request has been marked as ready for review.

ondrejmirtes · 2023-01-02T12:15:15Z

Thank you.

herndlm · 2023-01-02T12:46:59Z

yeah, thx for fixing this! :)

staabm commented Dec 21, 2022

View reviewed changes

src/Type/UnionType.php Outdated Show resolved Hide resolved

staabm commented Dec 21, 2022

View reviewed changes

staabm force-pushed the stop-on-constant branch from 37482dd to f49a35f Compare December 21, 2022 09:11

ondrejmirtes requested changes Dec 21, 2022

View reviewed changes

staabm force-pushed the stop-on-constant branch 3 times, most recently from d1958cd to 62f2afa Compare December 21, 2022 19:00

ondrejmirtes requested changes Dec 21, 2022

View reviewed changes

staabm force-pushed the stop-on-constant branch from 95ff433 to 434e55d Compare December 25, 2022 17:44

ondrejmirtes requested changes Dec 26, 2022

View reviewed changes

src/Type/UnionType.php Show resolved Hide resolved

staabm force-pushed the stop-on-constant branch 4 times, most recently from 29273e7 to 31269b3 Compare December 29, 2022 10:08

staabm marked this pull request as draft December 29, 2022 12:43

ondrejmirtes force-pushed the stop-on-constant branch from 3070b29 to 918ff36 Compare December 29, 2022 12:59

staabm and others added 10 commits December 31, 2022 10:45

Fix constant-string handling in union-types

e7ef43d

fix php <=7.3

4c8ddf9

added regression test

fa19cac

remove UnionTypeHelper usages

fdfde48

simplify

d1d9b44

Delete test.php

3948597

fix getArrays

c392ef8

nope

c07a033

Another nope

7d59e37

fix

7d779e9

staabm force-pushed the stop-on-constant branch from 918ff36 to 7d779e9 Compare December 31, 2022 09:48

staabm commented Dec 31, 2022

View reviewed changes

staabm marked this pull request as ready for review December 31, 2022 13:59

recurse into union-inner Intersections

58d0472

staabm force-pushed the stop-on-constant branch from 66f63d3 to 58d0472 Compare December 31, 2022 14:01

ondrejmirtes merged commit 463995c into phpstan:1.9.x Jan 2, 2023

staabm deleted the stop-on-constant branch January 2, 2023 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix constant-string handling in union-types #2134

Fix constant-string handling in union-types #2134

staabm commented Dec 21, 2022 •

edited

staabm Dec 21, 2022 •

edited

ondrejmirtes left a comment

ondrejmirtes Dec 21, 2022

ondrejmirtes Dec 21, 2022

ondrejmirtes Dec 21, 2022

staabm Dec 25, 2022

ondrejmirtes left a comment

ondrejmirtes commented Dec 29, 2022

staabm Dec 31, 2022 •

edited

staabm Dec 31, 2022 •

edited

phpstan-bot commented Dec 31, 2022

ondrejmirtes commented Jan 2, 2023

herndlm commented Jan 2, 2023

Fix constant-string handling in union-types #2134

Fix constant-string handling in union-types #2134

Conversation

staabm commented Dec 21, 2022 • edited

staabm Dec 21, 2022 • edited

Choose a reason for hiding this comment

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes Dec 21, 2022

Choose a reason for hiding this comment

ondrejmirtes Dec 21, 2022

Choose a reason for hiding this comment

ondrejmirtes Dec 21, 2022

Choose a reason for hiding this comment

staabm Dec 25, 2022

Choose a reason for hiding this comment

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes commented Dec 29, 2022

staabm Dec 31, 2022 • edited

Choose a reason for hiding this comment

staabm Dec 31, 2022 • edited

Choose a reason for hiding this comment

phpstan-bot commented Dec 31, 2022

ondrejmirtes commented Jan 2, 2023

herndlm commented Jan 2, 2023

staabm commented Dec 21, 2022 •

edited

staabm Dec 21, 2022 •

edited

staabm Dec 31, 2022 •

edited

staabm Dec 31, 2022 •

edited