Feature check array functions which require stringish values#3132

Merged

ondrejmirtes merged 15 commits intophpstan:1.11.xfrom

schlndh:feature-checkArrayFunctionsWhichRequireStringishValues

Jun 10, 2024

Contributor

schlndh commented Jun 9, 2024

This is an attempt to detect values which are not castable to string in functions which expect an array of values castable to string (e.g. array_unique([new stdClass(), new stdClass()])).

I considered using __stringandstringable in functionMap, but then I saw this comment: #3110 (comment) . Fortunately, I found that ImplodeFunctionRule already does the same thing, so I just generalized it to work with all the relevant functions.

schlndh commented

View reviewed changes

tests/PHPStan/Rules/Functions/data/param-castable-to-string-functions.php Outdated

Comment on lines 59 to 80

Contributor Author

schlndh Jun 9, 2024

Should this be allowed? IMO yes (it could be prohibited by more opinionated rulesets - e.g. strict-rules). However, currently it is a bit inconsistent. See https://phpstan.org/r/266e4f9e-a4c9-4679-ba6f-299684a10f7e

array_fill_keys is allowed.
array_combine is allowed with PHP < 8, and prohibited afterwards. That looks like an oversight, because as far as I can tell the function behaves the same: https://3v4l.org/dvTNC

Contributor Author

schlndh commented Jun 9, 2024

I checked the integration test errors (https://github.com/phpstan/phpstan-src/actions/runs/9435637126/job/25989364652?pr=3132):

rector-src: AttributeGroup (1st error) and ComplexType (2nd error) don't have __toString().
showare: PermissionStruct doesn't have __toString().
The rest seem obvious.

The AnalyserIntegrationTest::testBug11147 also fails in 1.11.x, so that's unrelated.

schlndh marked this pull request as ready for review

June 9, 2024 10:51

Collaborator

phpstan-bot commented Jun 9, 2024

This pull request has been marked as ready for review.

ondrejmirtes requested changes

View reviewed changes

Member

ondrejmirtes left a comment

For backward compatibility, I'd like this approach:

Keep ImplodeFunctionRule
Deprecate ImplodeFunctionRule and disable it in bleeding edge (like NoopRule is)
Introduce new rule like you did, but enable it only in bleeding edge

ondrejmirtes requested changes

View reviewed changes

src/Rules/Functions/ParameterCastableToStringFunctionRule.php Outdated

Member

ondrejmirtes Jun 9, 2024

What's the reasoning behind this condition?

Contributor Author

schlndh Jun 9, 2024

I'm not sure what you're asking about exactly. These are functions which either only have one argument, or which have multiple arguments and only the first one has to be array of values castable to string.

src/Rules/Functions/ParameterCastableToStringFunctionRule.php Outdated

Member

ondrejmirtes Jun 9, 2024

I know you probably just copied this from ImplodeFunctionRule, but the assumption is wrong because of named arguments. Someone might call implode(array: $a, separator: $s). and this code would break.

Please write a test for this scenario. The scenario can be fixed by processing $args through ArgumentsNormalizer.

Contributor Author

schlndh Jun 9, 2024

Good catch - I didn't think of that, since I just modifed the ImplodeFunctionRule. There was also a similar issue when no arguments were passed etc.

However, it lead me down another rabbit hole: when I added support for named arguments, I had to do something with the Parameter #1 of function ..., since the number might now be incorrect. So I attempted to make it consistent with how FunctionCallParametersCheck names the parameters. But it feels like individual rules shouldn't deal with that at all, since then it will "never" be consistent. If this is already handled somehow, maybe you can point me in the right direction.

Member

ondrejmirtes Jun 9, 2024 •

edited

Loading

Yeah, other rules don't deal with this at all now. We could refactor FunctionCallParametersCheck to be more flexible and to use its internals in other rules, but I don't think it's necessary in this PR.

Another thing this would allow us is to deal with $arg–>unpack properly same way FunctionCallParametersCheck does - it actually looks into $type->getConstantArrays() and expands them:

phpstan-src/src/Rules/FunctionCallParametersCheck.php

Lines 114 to 162 in 38f3906

    
           if ($arg->unpack) { 
        
           	$arrays = $type->getConstantArrays(); 
        
           	if (count($arrays) > 0) { 
        
           		$minKeys = null; 
        
           		foreach ($arrays as $array) { 
        
           			$countType = $array->getArraySize(); 
        
           			if ($countType instanceof ConstantIntegerType) { 
        
           				$keysCount = $countType->getValue(); 
        
           			} elseif ($countType instanceof IntegerRangeType) { 
        
           				$keysCount = $countType->getMin(); 
        
           				if ($keysCount === null) { 
        
           					throw new ShouldNotHappenException(); 
        
           				} 
        
           			} else { 
        
           				throw new ShouldNotHappenException(); 
        
           			} 
        
           			if ($minKeys !== null && $keysCount >= $minKeys) { 
        
           				continue; 
        
           			} 
        
           			$minKeys = $keysCount; 
        
           		} 
        
           		for ($j = 0; $j < $minKeys; $j++) { 
        
           			$types = []; 
        
           			$commonKey = null; 
        
           			foreach ($arrays as $constantArray) { 
        
           				$types[] = $constantArray->getValueTypes()[$j]; 
        
           				$keyType = $constantArray->getKeyTypes()[$j]; 
        
           				if ($commonKey === null) { 
        
           					$commonKey = $keyType->getValue(); 
        
           				} elseif ($commonKey !== $keyType->getValue()) { 
        
           					$commonKey = false; 
        
           				} 
        
           			} 
        
           			$keyArgumentName = null; 
        
           			if (is_string($commonKey)) { 
        
           				$keyArgumentName = $commonKey; 
        
           				$hasNamedArguments = true; 
        
           			} 
        
           			$arguments[] = [ 
        
           				$arg->value, 
        
           				TypeCombinator::union(...$types), 
        
           				false, 
        
           				$keyArgumentName, 
        
           				$arg->getStartLine(), 
        
           			]; 
        
           		} 
        
           	} else {

ondrejmirtes requested changes

View reviewed changes

src/Rules/Functions/ParameterCastableToStringFunctionRule.php Outdated

Member

ondrejmirtes Jun 9, 2024

What if the index does not exist?

Contributor Author

schlndh Jun 10, 2024

I didn't think of that. But it seems that this cannot happen on non-empty $normalizedArgs, because the normalizer only fills optional parameter holes, if a required parameter is missing it seems to return null. I guess it doesn't hurt to be extra defensive and check it anyway.

src/Rules/Functions/ParameterCastableToStringFunctionRule.php Outdated

Member

ondrejmirtes Jun 9, 2024

You don't need to do this, on normalized arg they're available in an attribute.

Contributor Author

schlndh Jun 10, 2024

I didn't notice that. But due to the usage of $origNamedArgs for handling implode (I probably added that after you wrote the comment), I still need to have a map of named args.

mvorisek reviewed

View reviewed changes

src/Rules/Functions/ParameterCastableToStringFunctionRule.php Outdated Show resolved Hide resolved

schlndh added 15 commits

June 10, 2024 10:12


          check array_unique etc for values castable to string

0767e0c


          add tests for existing issues

256b04e


          skip unpacked arguments

b675ca5


          fix typo in test name

b6b191a


          exclude array_flip - it requires int|string

b4e864a


          fix phpstan issue

2f30aca


          restore ImplodeFunctionRule


          allow ParameterCastableToStringFunctionRule only in bleeding edge

dca2315


          fix calls with named arguments or with wrong no. of args

341c9dd


          attempt to consistently report parameter position/name

168b0c9


          fix implode argument naming

01d571c


          try to fix param names in test for older PHP versions

59bf672


          make test file namespace unique

2123f1d


          fix old PHP test fix

fedc8f0


          hack 3rd arg name of array_intersect for old PHP versions

58c4447

ondrejmirtes force-pushed the feature-checkArrayFunctionsWhichRequireStringishValues branch from 07ea6aa to 58c4447 Compare

June 10, 2024 08:12

ondrejmirtes approved these changes

View reviewed changes

Member

ondrejmirtes commented Jun 10, 2024

Thank you!

ondrejmirtes merged commit 7896011 into phpstan:1.11.x

This was referenced Jun 10, 2024

Check array_diff arguments phpstan/phpstan#11141

Closed

array_diff() with arrays containing non-stringable values not reported phpstan/phpstan#5848

Closed

array_combine should allow array of string or integer only phpstan/phpstan#3694

Closed

ondrejmirtes mentioned this pull request

Check array_fill_keys arguments phpstan/phpstan#11111

Closed

schlndh deleted the feature-checkArrayFunctionsWhichRequireStringishValues branch

June 10, 2024 14:22

schlndh mentioned this pull request

Check that values passed to array_sum/product are castable to number #3658

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet