Cannot access offset on mixed when using isset()

[noemi-salaun]

Bug report

On PHPStan level 9

When using isset($data['foo']) with $data being mixed, PHPStan complains about "Cannot access offset 'foo' on mixed.".

Also, when checking for value type, like is_string($json['Foo']['Bar']), the value type is not narrowed.

Code snippet that reproduces the problem

        public function sayHello(mixed $json): string
	{
		if (!isset($json['Foo']['Bar']) || !is_string($json['Foo']['Bar'])) {
			throw new \Exception('Bad JSON');
		}

		return $json['Foo']['Bar'];
	}

https://phpstan.org/r/9b18a65f-f28d-4374-8167-1e94703d9707

Expected output

PHPStan should not report an issue when trying to access offset inside isset, and after validating offset with isset, we should be able to assert the type

Did PHPStan help you today? Did it make you happy in any way?

[rvanvelzen]

On level 9 you need to check any mixed value explicitly: https://phpstan.org/r/1dd7ca88-4866-47ba-876c-2fb717584927

[noemi-salaun]

I don't think your additional checks bring any more strictness to the code. The goal is to write better code, not bloated code with useless checks.

Is there any case where your checks can detect error that a simple isset cannot?

In my exemple, we don't care if the intermediate item are arrays, we just need to access an offset and its value to be a string. I don't know in what case it can trigger an error with the simple check.

So if the code is simpler, more readable, more maintenable, more perfomant and as strict, shouldn't it be supported by quality tools?

sorry if my formulations seem aggressive or mean, I'm not native English, I try above all to be as clear as possible in my sentences

[herndlm]

The initial problem is $json itself. With mixed it can also be null, or an object, or a resource,.. you get the point. And accessing an offset of e.g. stdClass will fail. And the same then applies to subarrays basically. This is ugly and annoying though, yeah..
So to keep your code simple you'd either have to narrow it down to an array first, or you use an array as parameter type instead and let phpstan also enforce that an array of arrays is passed, which affectively moves the "problem" to the outer layers of your app where data gets in.

See also https://phpstan.org/writing-php-code/narrowing-types and you can do type-narrowing also via assertion (libraries).

[rajyan]

Example of what herndlm is mentioning.
https://3v4l.org/lFcCq

mixed is too wide, we need to know that $json is not an object at least, because accessing a non offset accessible object would throw an fatal error (even inside of isset)

Although, I think maybe we can have some improvements with these issues
#8069 (comment)
This is a related one, and I remember there are lot of issues related to this.

[rajyan]

phpstan/phpstan-src#1312 (comment)
I've investigated this behavior before, it throws fatal error if the type is closure or a object (without ArrayAccess).

[rajyan]

Also, when checking for value type, like is_string($json['Foo']['Bar']), the value type is not narrowed.

By the way, type of $json['Foo']['Bar'] is narrowed
https://phpstan.org/r/405df969-8f5a-43bb-97c1-e7cf29f63a8a

[rajyan]

@noemi-salaun
If you know that the input $json is a multi dim array at least, you can add a @param mixed[][] $json to slightly narrow the type
https://phpstan.org/r/1f812851-e1c6-4430-9d02-bc23815ffd35

[rajyan]

I have an idea to mitigate these issues. Let's create something like isOffsetAccessThrowsFatalError(): bool to separate whether offset access throws or not.
For example BooleanType is isOffsetAccessible->no, but doesn't throw
https://3v4l.org/bAejd
while closure and object does, as mentioned above.

Then, relax the condition of this line to
https://github.com/phpstan/phpstan-src/blob/6d2a87afef54085c7b8263c4f8bff107d75a55e8/src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php#L63-L65

$scope->isUndefinedExpressionAllowed($node) && ($isOffsetAccessible->yes() || !isOffsetAccessThrowsFatalError()))

We can make StrictMixedType as isOffsetAccessThrowsFatalError() => false, because lot of users are expecting this behavior. Leaving the current strict behavior to strict rules.

[noemi-salaun]

Oh you are right, I tested isset($foo['bar']) on null, string or non existant variable and that didn't fatal error. But on object it does. In my case I know it can't be an object because it came from a json_decode with associativeArray as true but it's not expressed in the resulting type.

Should I close the issue because it's not a false positive? Or I keep it open and rename it for an improvement?

[herndlm]

Unfortunately json_decode with associativeArray as true is currently resolved to mixed~stdClass "mixed without stdClass", see https://phpstan.org/r/82dcf5d7-14f8-40db-9023-5ae9e0644cce

This was discussed a couple of times before, it's a bit complicated, but most likely the last time we wanted to change this was in my PR where Ondrej explained the situation in phpstan/phpstan-src#1283 (comment)

[hydrastro]

Quite related to the question but not that much, I'd wanted to share with you my thoughts and struggles about a problem I was facing, hoping to help others and hoping that others may help me, so we can all write better code! yay

On level 9 you need to check any mixed value explicitly: https://phpstan.org/r/1dd7ca88-4866-47ba-876c-2fb717584927

You can also specify/assert them rather than check them.

I got quite a few headaches dealing with this problem with mixed data.

I have a Config class that retrieves polymorphic data from the config files.
So of course my method Config->get(string $config_name, mixed $fallback) returns mixed.
In my code I do check that the retrieved configuration is properly structured, and I do that either in assert statements, so I can turn them off in production and gain performance, or in if blocks.

@noemi-salaun useless checks are okay in assertions

I can see these options for dealing with the code having mixed types:

1. Decomposing and refactoring the code, ending up having a LOT of classes.
   Personally, I dislike this approach because it adds up to the code way too many layers of useless abstractions which I call abstractions hell.
2. Using if statements for checking useless things, in order to make phpstan shut up.
   This slows down the code in a stupid way. And also you'd have to add PHPDoc comments because if statements aren't enough to inver the type
3. Using assertions, which is my preferred option since they can be turned off.
4. @php-stan-ignore-next-line. lol

Soooo....
if anyone is facing the problem I had, here's how I solved it

I

Just

Started

Using

Assertions

(and PHPDoc comments when necessary)

Old code:

$language = $Config->getConfig("language", "english"); // returns mixed|null if it can't find the config.
if(is_string($language)) {
    $something->foo($language);
}

Bad code:

$language = $Config->getConfig("language", "english");
/*
 * @var string $language Language.
 */
$something->foo($language);

New code:

$language = $Config->getConfig("language", "english");
assert(is_string($language));
$something->foo($language);

@noemi-salaun In your example though, as @herndlm said, I'd rather change the parameter $json, or something in the caller, because your code feels a bit dirty.
But if you're confident with it, here's the solution with assertions:

<?php declare(strict_types = 1);

class HelloWorld
{
	public function sayHello(mixed $json): string
	{
		assert(
			is_array($json) &&
			array_key_exists("Foo", $json) &&
			is_array($json["Foo"]) &&
			array_key_exists("Bar", $json["Foo"]) &&
			is_string($json["Foo"]["Bar"])
			);

        return $json['Foo']['Bar'];
	}
}

[ondrejmirtes]

@hydrastro You can also make PHPStan understand that $Config->getConfig("language", "english") is already string.

You can use:

• Conditional return types
• Generics
• Dynamic return type extensions
• Stub files

All is described in PHPStan documentation on phpstan.org.

[hydrastro]

@hydrastro You can also make PHPStan understand that $Config->getConfig("language", "english") is already string.

You can use:

* Conditional return types

* Generics

* Dynamic return type extensions

* Stub files

All is described in PHPStan documentation on phpstan.org.

Thanks for the reply @ondrejmirtes
I don't really like conditional return types, in my opinion they just feel a bit too much for my purposes and I'd rather keep logic out of phpdocs as much as possible.
Generics are interesting but I think they're a bit too much for my project. I'd rather keep things cleaner with normal phpdocs ... so no @templates and other phpstan features, and then in the future rework on the code when generics will be implemented in PHP, if they'll ever be.
I find assertions awesome for this purpose.. you can even edit their behaviour.
Do you think that there are any reasons to not use them more?

In the next few days I'll work on my code and see if I'll find any.
Happy coding you all

[staabm]

assertions will protect your logic only at static analysis time. they won't protect your production system (and e.g. therefore don't sanitize your user input) which leads to security problems.

assertions are not meant to be enabled in production systems.

[hydrastro]

assertions will protect your logic only at static analysis time. they won't protect your production system (and e.g. therefore don't sanitize your user input) which leads to security problems.

assertions are not meant to be enabled in production systems.

Absolutely! Input sanitization shouldn't absolutely be done with assertions

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.