Vscode-intelephense: array_merge of exploded string throws error

[bryanburchfield]

When doing an array_merge() of an array created from explode(), an error is shown: "Expected type 'array'. Found 'string[]|false'."

To Reproduce
$array1 = explode(',', 'item1,item2,item3');
$array2 = ['thing1', 'thing2', 'thing3'];
$array3 = array_merge($array1, $array2); // this shows an error

Platform and version
Windows 10. Intelephnese 1.3.0

Screen shot

[KapitanOczywisty]

For me this is what I've always wanted :smiley:

Technically it's not a bug, but irl if delimiter is not empty, you'll never get false. This is edge case, maybe should not be checked unless there is strict_types=1

[jfcherng]

Technically it's not a bug, but irl if delimiter is not empty, you'll never get false.

Yes, technically this kind of checking is "correct" behavior.

It's super tedious when you are playing with built-in functions like explode, substr, json_encode, preg_*, ... because they most of them never throw an exception but return false when something weird happens. Honestly, I have never seen people checking if, for example, substr returns false or not. Now you have to check every of them.

Again, this is technically "correct" behavior, but I wonder what's the best way to eliminate all of them elegantly from your code without disabling other nice type checkings.

• Checking every substr (and other common functions that doesn't returns false 99.9% of the time)?
• Wrapping every "problematic" function to throw an exception when returns false?
• ?

[KapitanOczywisty]

For example psalm is checking if string is not empty or if it's variable (and assumes it's correct) https://psalm.dev/r/396c6b7149 , but this is way beyond what extension is doing now.

Personally I'd just cast as array or string, because I'm doing that anyway in many cases, when author fixes type unions I will be happy user, even with this. I like control over type, but I'm aware that it's not popular view. PHP devs likes chaos :)

Probably go to option is more relaxed check for buid-in (phpstorm-stubs) functions, maybe not entirely disabled but e.g. ignore bool (and less likely null) when variable is an union with correct type. For crazy users (like me) option or linked with strict_types=1.

Other option may be to add some hard-coded exceptions or keep information in union type from where it comes from (stubs / user) and skip notice if not necessary.

[jfcherng]

Personally I'd just cast as array or string,

Thanks. I think I will be happy with this in most of cases.

PHP devs likes chaos :)

Not me. But I check it in another way. For example, I may check the begin and length for invalid situations for substr (today, I just know it checks it automatically and returns false). And intelephense may probably never know these.

For crazy users (like me) option or linked with strict_types=1.

Literally me.

[bmewburn]

In 1.2 union types satisfied a type declaration if at least one of the types in the union was equivalent. In 1.3 it's more strict in that all of the union types must match the type declaration. IMO if a function signature says it returns string[]|false then these return types should be handled. Though, I get it that not everyone wants to code this way. I think linking the 1.3 type checking to strict_types=1 and is a reasonable solution here.

[KapitanOczywisty]

@jfcherng I wasn't expecting that assert with is_* will work. I'll use this too.

$array1 = explode(',', 'item1,item2,item3');
$array2 = ['thing1', 'thing2', 'thing3'];
assert(is_array($array1));
$array3 = array_merge($array1, $array2); // and now - without errors

[jfcherng]

@KapitanOczywisty Thanks. assert can be great since it can be disabled or with custom handler :smiley:

[jchlu]

It seems to me, the incorrect return type from explode is being reported and that is what causes intelephense to report the "problem".
explode returns array|false - never string but is reporting through vscode as string[]|false which, when the result is used as an input parameter to a function expecting an array (like array_merge in this example) the warning is shown.
See difference between explode and array_merge return types below:

[KapitanOczywisty]

@jchlu missread your message. string[] is compatible with array as for extension goes

No errors in code:

/** @var string[] */
$arrayOfString = ['abc'];
/**
 * @param array $array 
 * @return void 
 */
function anyArray(array $array):void{ var_dump($array); }
anyArray($arrayOfString);
// It's even allowing incompatible arrays:
/**
 * @param int[] $array 
 * @return void 
 */
function intArray(array $array):void{ var_dump($array); }
intArray($arrayOfString);

[ducalex]

While this behavior might be technically correct, I'd argue that it isn't helpful in most instances. Sure if no type in the union matches I want to know. But take for example explode(' ', 'a b c'), it will never return false and it seems silly to always have to check its return type anyway.

It would be nice if intelephense could deduce the return type based on the inputs one day. But in the meantime a setting to return to 1.2 behavior seems important to me.

[KapitanOczywisty]

@ducalex This is only one case. Consider fopen - with that is not entirely clear if function will succeed judging only by parameters. In general this is good check, but not every user may appreciate it. I don't mind adding (array) to explode, because in other cases this saves me a bit of headache, If I forgot to check false somewhere.

You can always use psalm even in vscode which can handle explode example.

[ducalex]

Naturally some things need to be checked.

But forcing us to do (array)array_map('function', (array)array_filter((array)explode(' ', (string)implode( (array)str_replace('a', 'b', (array)$input))); everywhere else Isn't a solution.

[KapitanOczywisty]

@ducalex real-life example? I'll fix it for ya:
array_map('function', array_filter((array)explode(' ', implode( (array)str_replace('a', 'b', $input)))));
and str_replace may be fine without cast when #737 will be fixed. You may also want to work a bit on your coding style.

[ducalex]

@KapitanOczywisty This was obviously not a real life example. You've made it clear since the first post that you're in love with the change. I don't want to convince you that you are wrong and I am right, I'm just trying to explain that version 1.2 did not behave that way and neither does PHPStorm (although it can be configured to do so) and that is why, I think, there needs to be a setting to accommodate both requirements.

[KapitanOczywisty]

@ducalex More or less we're agreeing that should be way to opt-out.

[Oliver-Saer]

Have downgraded to 1.2.3 for the time being as the latest version is unusable for me.

Throwing errors all over the place as I'm working with API values which are stored in an associative array. Currently, Intelliphense believes all of these values to be of type 'array' and spits out errors when feeding into type hinted functions which are not of type 'array'.

[bmewburn]

@Ollie-Saer-WBS , this looks like a separate issue. The issue here is that all types in a union type must satisfy a type declaration. Please open a new issue with a small code example.

[Oliver-Saer]

@bmewburn Will do, thanks!

[bmewburn]

I'm going to roll this back to the 1.2 behaviour by default and have an opt-in for stricter behaviour.

[bmewburn]

Added #830 to track strict mode