Feature check array functions which require stringish values

[schlndh]

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]

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

[schlndh]

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.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

For backward compatibility, I'd like this approach:

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

[ondrejmirtes]

What's the reasoning behind this condition?

[schlndh]

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.

[ondrejmirtes]

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.

[schlndh]

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.

[ondrejmirtes]

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]

What if the index does not exist?

[schlndh]

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.

[ondrejmirtes]

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

[schlndh]

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.

[ondrejmirtes]

Thank you!