str split return non empty

[VincentLanglet]

Since str_split('', 42) is [0 => ''], I think the list is never empty.

[bitwise-operators]

In the 8.0 delta, the old version for mb_str_split should still list the |false return option, and the same applies for the older changes and the historical file.

And it looks like that change for str_split is missing from the 8.0 delta altogether

The |false option was one of the 8.0 changes (when most internal functions went over to throwing Exceptions). This is not correctly mentioned in the current version of the PHP docs for str_split, but if you check the Github history, you'll see it used to return array|false. (mb_str_split does correctly list the change in the docs)

[bitwise-operators]

Also, for this reason, values of zero or lower are allowed in older versions (7.4 and before). That's when it will return false.

While this is probably unwanted behaviour, it is legal code.

So I would only add the positive-int requirement to PHP 8 and later.

[weirdan]

In the 8.0 delta, the old version for mb_str_split should still list the |false return option

Is that for the case when an invalid encoding was passed?

And it looks like that change for str_split is missing from the 8.0 delta altogether

That's because it returned false for negative lengths only (unlike mb_str_split). But since we're requiring a positive-int (the change I requested in this PR), there won't be any false returns even for older PHP versions.

[weirdan]

While this is probably unwanted behaviour, it is legal code.

It generates warnings (https://3v4l.org/FMlWJ), so I wouldn't call that legal code.

[weirdan]

@VincentLanglet given that mb_str_split() returns false for invalid encodings (https://3v4l.org/rcGeq) and Psalm can't detect that it makes sense to revert the return type for mb_str_split to false|non-empty-list<string>, but keep $length as positive-int

[bitwise-operators]

In the 8.0 delta, the old version for mb_str_split should still list the |false return option

Is that for the case when an invalid encoding was passed?

And it looks like that change for str_split is missing from the 8.0 delta altogether

That's because it returned false for negative lengths only (unlike mb_str_split). But since we're requiring a positive-int (the change I requested in this PR), there won't be any false returns even for older PHP versions.

But by omitting the |false in PHP <=7.4, people who correctly put in boilerplate code to check for a falsey return will get a Psalm warning. So even with the positive-int value for length, the |false should be appended to the return type for both functions.

While this is probably unwanted behaviour, it is legal code.
It generates warnings (https://3v4l.org/FMlWJ), so I wouldn't call that legal code.

Fair enough, it won't hurt if Psalm warns people of wrong inputs.

[VincentLanglet]

But by omitting the |false in PHP <=7.4, people who correctly put in boilerplate code to check for a falsey return will get a Psalm warning. So even with the positive-int value for length, the |false should be appended to the return type for both functions.

You don't need the falsey check if you pass a positive-int.
So instead of a falsey check after, it's better to do a positive-int check before.

But I agree we could create such issues by removing the false value.

@VincentLanglet given that mb_str_split() returns false for invalid encodings (https://3v4l.org/rcGeq) and Psalm can't detect that it makes sense to revert the return type for mb_str_split to false|non-empty-list, but keep $length as positive-int

What about str_split ? Should I add false back or not ?

[bitwise-operators]

You don't need the falsey check if you pass a positive-int.

Point is, the function CAN return false, so a programmer should be able to check for it without needing @psalm-suppress. Maybe length is supplied through user input, maybe there is another reason for it.

I don't think a static analyzer should mark boilerplate code as incorrect, unless it absolutely is (as it is from PHP 8.0 onward).

[VincentLanglet]

@bitwise-operators I'm making this PR because I'm calling array_rand on the result of mb_str_split and getting an error because the array must be non-empty.

array_rand([], 1)

is not a forbidden call. It emits a warning and return null.

But psalm decided to use non-empty-array to describe the parameter.

'array_rand' => ['int|string|array<int,int>|array<int,string>', 'array'=>'non-empty-array', 'num'=>'int']

So the decision has already been taken long time ago and should be on the str_split PR.

[weirdan]

Point is, the function CAN return false

We don't want to end up in a situation where we both require to pass positive-int (e.g. by validating it with >0) and then also require the user to check the result for false. So it's either int -> (false|array) or positive-int -> array. positive-int -> false|array just doesn't make sense. And since int -> (false|array) results in warnings, which means users likely already have >0 in place, the choice is pretty clear.

[weirdan]

Thanks!