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 arrays with union value-types in implode() #3772

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 5, 2025

Primary motivation: get rid of instanceof ConstantScalarType

closes phpstan/phpstan#11854

@staabm staabm marked this pull request as ready for review January 5, 2025 13:12
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

}
}

if (count($strings) > InitializerExprTypeResolver::CALCULATE_SCALARS_LIMIT) {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally you'd count the combinations before generating them, it can explode as well.

}

$strings[] = new ConstantStringType(implode($separatorType->getValue(), $arrayValues));
if (count($strings) + count($arrayValues, COUNT_RECURSIVE) > InitializerExprTypeResolver::CALCULATE_SCALARS_LIMIT) {
Copy link
Member

Choose a reason for hiding this comment

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

You're not counting the combinations properly. This is how I verified it:

diff --git a/src/Type/Php/ImplodeFunctionReturnTypeExtension.php b/src/Type/Php/ImplodeFunctionReturnTypeExtension.php
index 76afc963c..e45e0146a 100644
--- a/src/Type/Php/ImplodeFunctionReturnTypeExtension.php
+++ b/src/Type/Php/ImplodeFunctionReturnTypeExtension.php
@@ -22,6 +22,7 @@ use PHPStan\Type\TypeCombinator;
 use function count;
 use function implode;
 use function in_array;
+use function iterator_count;
 use const COUNT_RECURSIVE;
 
 final class ImplodeFunctionReturnTypeExtension implements DynamicFunctionReturnTypeExtension
@@ -124,10 +125,16 @@ final class ImplodeFunctionReturnTypeExtension implements DynamicFunctionReturnT
 				$arrayValues[] = $constScalars;
 			}
 
+			var_dump(count($strings) + count($arrayValues, COUNT_RECURSIVE));
+
 			if (count($strings) + count($arrayValues, COUNT_RECURSIVE) > InitializerExprTypeResolver::CALCULATE_SCALARS_LIMIT) {
 				return null;
 			}
 
+			var_dump(iterator_count(CombinationsHelper::combinations($arrayValues)));
+
+			var_dump('---');
+
 			$combinations = CombinationsHelper::combinations($arrayValues);
 			foreach ($combinations as $combination) {
 				$strings[] = new ConstantStringType(implode($separatorType->getValue(), $combination));

The output:

int(33)
int(8)
string(3) "---"
int(41)
int(8)
string(3) "---"

Expected output:

int(8)
int(8)
string(3) "---"
int(8)
int(8)
string(3) "---"

@staabm staabm force-pushed the impl branch 2 times, most recently from 29caa26 to 9a991c8 Compare January 5, 2025 17:32
@ondrejmirtes ondrejmirtes merged commit 65be2b2 into phpstan:1.12.x Jan 7, 2025
452 checks passed
@ondrejmirtes
Copy link
Member

Thank you!


/** @param array{0: 1, 1: 'a'|'b', 3?: 'c'|'d', 4?: 'e'|'f', 5?: 'g'|'h', 6?: 'x'|'y'} $constArr */
public function constArrays6($constArr) {
assertType("string", implode('', $constArr));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this too generic type made me think about the general case and triggerd #3774

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

Successfully merging this pull request may close these issues.

3 participants