Fix false positives on non-existing-offset's

[staabm]

fixes some false positives we see with reportPossiblyNonexistentGeneralArrayOffset.

basic idea is to infer a expression type for $array[$last] after $last = array_key_last($array); based on the iterable-type of a non-empty $array.

refs phpstan/phpstan#11020

---

this PR fixes code like

	/**
	 * @param list<string> $hellos
	 */
	public function last(array $hellos): string
	{
		if ($hellos !== []) {
			$last = array_key_last($hellos);
			return $hellos[$last];
		}
        return '';
	}

	/**
	 * @param list<string> $hellos
	 */
	public function works(array $hellos): string
	{
		if ($hellos === []) {
			return 'nothing';
		}

		$count = count($hellos) - 1;
		return $hellos[$count];
	}

but not like (in which no intermediate variable is used)

	/**
	 * @param list<string> $hellos
	 */
	public function last(array $hellos): string
	{
		if ($hellos !== []) {
			return $hellos[array_key_last($hellos)];
		}
        return '';
	}

	/**
	 * @param list<string> $hellos
	 */
	public function works(array $hellos): string
	{
		if ($hellos === []) {
			return 'nothing';
		}

		return $hellos[count($hellos) - 1];
	}

[phpstan-bot]

This pull request has been marked as ready for review.

[jhoffland]

Thanks for fixing the issue I encountered. In the description are some specific cases mentioned which are not covered by this fix. What would be required to fix those cases? I could help.

[leongersen]

It seems to me that array_key_first and array_key_last (as well as count) could add information to the array they are called with.

Currently, there is ArrayKeyLastDynamicReturnTypeExtension, which determines the return type for array_key_last, but does not modify the information about the array itself.

Would it make sense to have a FunctionTypeSpecifyingExtension for array_key_last as well, which augments the information about the array with the result determined by ArrayKeyLastDynamicReturnTypeExtension,
very similar to what ArrayKeyExistsFunctionTypeSpecifyingExtension does?

ArrayKeyExistsFunctionTypeSpecifyingExtension extends the array type with a HasOffsetType, which is also what array_key_first and array_key_last do.

---

This would also be an option for many more functions, such as array_search, which also proves a key exists. There is ArraySearchFunctionTypeSpecifyingExtension, which would need to specify that the array is an intersection with HasOffsetType for the type determined by ArraySearchFunctionDynamicReturnTypeExtension.

https://phpstan.org/r/a35121d2-fc59-48c9-982b-41c1850b3bf5

[staabm]

It seems to me that array_key_first and array_key_last (as well as count) could add information to the array they are called with.

thats exactly what this PR is doing.

Would it make sense to have a FunctionTypeSpecifyingExtension for array_key_last as well, which augments the information about the array with the result determined by ArrayKeyLastDynamicReturnTypeExtension,
very similar to what ArrayKeyExistsFunctionTypeSpecifyingExtension does?

I think thats not possible, because a FunctionTypeSpecifyingExtension can only work on the FuncCall and its arguments.
for the array_key_last, array_key_first and count the $offset we want to add information about is not part of the FuncCall, but the assignment arround it. thats why I hacked it directly into the TypeSpecifier

ArrayKeyExistsFunctionTypeSpecifyingExtension extends the array type with a HasOffsetType, which is also what array_key_first and array_key_last do.

for HasOffsetType we need a ConstantStringType or ConstantIntegerType type, which we don't know at static analysis time for array_key_first, array_key_last and count.

phpstan.org/r/a35121d2-fc59-48c9-982b-41c1850b3bf5

this array_search examples produces the same errors this PR is trying to fix for the other functions, when reportPossiblyNonexistentGeneralArrayOffset: true - so we could do the same fix for it as suggested here. you don't see them in the playground because the necessary config switch is not available there.

[leongersen]

Thanks for your answer, I appreciate the insights!

Could you clarify the following for me?

I think thats not possible, because a FunctionTypeSpecifyingExtension can only work on the FuncCall and its arguments.

Is that not enough to ask the scope for the return type ($offset), which would be provided by the DynamicReturnTypeExtension?

[staabm]

Is that not enough to ask the scope for the return type ($offset), which would be provided by the DynamicReturnTypeExtension?

its enough to calculate the return-type of the FuncCall:
$last = array_key_last($array);
but to put the result of array_key_last($array); onto $array as $array[$last] we need to know the expression which the result is assigned to.

thats also the reason why my current approch doesn't work for $array[array_key_last($array)] - because there is no assignment in between. we could still support such a expression, but it needs simliar code at a different location of the TypeSpecifier. Thats why I sent this PR over for review now to check back with ondrey whether I am on the right track at all.

[staabm]

This would also be an option for many more functions, such as array_search, which also proves a key exists. There is ArraySearchFunctionTypeSpecifyingExtension, which would need to specify that the array is an intersection with HasOffsetType for the type determined by ArraySearchFunctionDynamicReturnTypeExtension.

@leongersen thanks to some great playground changes by ondrej, the opt-in only rule errors now show up:

https://phpstan.org/r/c519d102-52d4-41c5-8681-4b29be8b1381

[leongersen]

I've reported another issue on this topic. This one isn't due to array-key related functions, so I'm wondering if this fits within the solution-direction chosen here.

phpstan/phpstan#12406

[VincentLanglet]

Would it work for

$array[count($array) - 1]

and other calls, by adding those conditions in NonexistentOffsetInArrayDimFetchCheck ?

[VincentLanglet]

You may prefer to use === rather than in_array check (maybe better perf ?)

[staabm]

Yes, see #3766 (comment)

[mvorisek]

"count - 1" is definitely the most common usecase.

However "count - N" is quite common too - can this usecase be supported as well when the "count" is higher than "N"?

[staabm]

Count -N only works if the length of the array is known at static analysis time. Not sure how common/useful/likely this is in practice

Count -1 works pretty good because we know about non-emptiness in a lot more cases

[mvorisek]

I mean https://phpstan.org/r/779c2b7e-977c-4df6-ae27-970c1a147529 - in this case, the minimal count is known.

[staabm]

I think your example will be fixed by #3709

//cc @herndlm

[herndlm]

Yes, that should be covered there indeed