Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong inference about array from PHPStan #8087

Closed
VincentLanglet opened this issue Sep 30, 2022 · 14 comments
Closed

Wrong inference about array from PHPStan #8087

VincentLanglet opened this issue Sep 30, 2022 · 14 comments
Labels
Milestone

Comments

@VincentLanglet
Copy link
Contributor

Bug report

I suspect that some optimisation about array is creating an issue
@ondrejmirtes @rvanvelzen

Code snippet that reproduces the problem

https://phpstan.org/r/7aacecc5-cd35-4ec4-98ae-c70b4635e1ef

Doing things on one key of the array is automatically changing the way phpstan is inferring the other key (and the whole array).

Expected output

No errors.

@VincentLanglet
Copy link
Contributor Author

https://phpstan.org/r/4b292f19-c957-4967-89b6-31cdc3b31529

dumped type should be

Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', non-empty-array)

and not

Dumped type: non-empty-array<string, non-empty-array>&hasOffsetValue('uses', non-empty-array)

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Oct 4, 2022

The code started to fail with this commit: phpstan/phpstan-src@8f44183

I'm not sure I can do more to help

@ondrejmirtes ondrejmirtes added this to the Easy fixes milestone Oct 5, 2022
@VincentLanglet
Copy link
Contributor Author

You marked this as easy fix. Any recommendation about how solving this @ondrejmirtes ?

@herndlm
Copy link
Contributor

herndlm commented Oct 6, 2022

Just guessing, but I'd closely look at the array_unique. There's an extension that specifies the arg as non-empty-array and maybe that's done incorrectly. E.g. maybe isIterableAtLeastOnce incorrectly returns yes or it needs an additional check to ensure it's an iterable/array or the offset exists for sure or so. The type for data['uses'] changes from mixed to non-empty-array because of it and that must be related somehow
It could also be around/outside the extension, I'd still start there though

@VincentLanglet
Copy link
Contributor Author

Not sure it's related to array_unique since I can reproduce this without it
https://phpstan.org/r/2e29c835-f87b-4540-b2d4-e7d5543f1a0e

Or even https://phpstan.org/r/8ed2fee6-9069-448e-bc4a-cd09aeffe784

@herndlm
Copy link
Contributor

herndlm commented Oct 7, 2022

If you play around with your last example, which you surely did already I suppose, you see that the ArrayDimFetch append statement is most likely related, see https://phpstan.org/r/921492b3-3535-419e-8e25-8e13fee8bf1d
Maybe such expressions should only modify the HasOffsetValueType and not the general array, which should stay mixed then. but could be tricky, I'm still guessing and don't know how exactly it behaved before and why it worked. I'd step-debug this next, you'll most likely find yourself in NodeScopeResolver or MutatingScope soon :)

@VincentLanglet
Copy link
Contributor Author

If you play around with your last example, which you surely did already I suppose, you see that the ArrayDimFetch append statement is most likely related, see https://phpstan.org/r/921492b3-3535-419e-8e25-8e13fee8bf1d Maybe such expressions should only modify the HasOffsetValueType and not the general array, which should stay mixed then. but could be tricky, I'm still guessing and don't know how exactly it behaved before and why it worked. I'd step-debug this next, you'll most likely find yourself in NodeScopeResolver or MutatingScope soon :)

I debugged a little, issue is here:
https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Analyser/NodeScopeResolver.php#L3352

here, I try to override the array with a wrong value
https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Analyser/NodeScopeResolver.php#L3455

the value is computed here:
https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Analyser/NodeScopeResolver.php#L3436-L3450

the issue is with the call

$valueToWrite = $offsetValueType->setOffsetValueType($offsetType, $valueToWrite, $i === 0)

it's done on an IntersectionType.

The new type is computed in ArrayType::setOffsetValueType then. The call is made on array<string, mixed> with the keyType 'uses' and the value type array{'', ''}. And if dump the type of the newly created array:
https://github.com/phpstan/phpstan-src/blob/1.9.x/src/Type/ArrayType.php#L307-L310
I get

array<string, array{'', ''}>

This is because $unionValues is false during the call ; which is because of $i === 0. (in my case the key is 1).

Then, I can fix my personal issue this way phpstan/phpstan-src#1808
but I assume @ondrejmirtes put this $i === 0 for some reason, since some other tests are failing.

Maybe this code is talking to you... Do you have any recommendation @herndlm ?

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Oct 12, 2022

Seems like the $i === 0 check was added to have the following behavior

	public function doFoo(int $i)
	{
		$array = [];

		$array[$i]['bar'] = 1;
		$array[$i]['baz'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz: 2}>', $array);
	}

But this seems to be a little buggy because we also have the following test passing

	public function doFoo(int $i, int $j)
	{
		$array = [];

		$array[$i]['bar'] = 1;
		$array[$i]['baz'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz: 2}>', $array);

		$array[$j]['toto'] = 1;
		$array[$j]['tata'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz: 2, toto: 1, tata: 2}>', $array);
	}

which is not true.

But I have difficulties to understand how should I fix this.

For the record, psalm (https://psalm.dev/r/7f28c720dd) consider it's

non-empty-array<int, array{bar: 1, baz?: 2}>
non-empty-array<int, array{bar: 1, baz?: 2, toto?: 1, tata?: 2}>

which is not true either but is the same that I have with the change phpstan/phpstan-src#1808.

I'm not sure how hard it is to differentiate

$array = [];
$array[$i]['bar'] = 1;
$array[$i]['baz'] = 2;

and

$array = [];
$array[$i]['bar'] = 1;
$i++;
$array[$i]['baz'] = 2;

But to me it would be ok having a less precise array shape, and promote instead to write code like this
https://phpstan.org/r/6589bbdf-83d4-439f-8049-98d86245a055

@phpstan-bot
Copy link
Contributor

@VincentLanglet After the latest push in 1.9.x, PHPStan now reports different result with your code snippet:

@@ @@
-13: Comparison operation ">" between int<1, max> and 0 is always true.
+No errors

@phpstan-bot
Copy link
Contributor

@VincentLanglet After the latest push in 1.9.x, PHPStan now reports different result with your code snippet:

@@ @@
 10: Dumped type: array<string, mixed>
-15: Dumped type: non-empty-array<string, non-empty-array>&hasOffsetValue('uses', non-empty-array)
-17: Comparison operation ">" between int<1, max> and 0 is always true.
+15: Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', non-empty-array)
Full report
Line Error
10 Dumped type: array<string, mixed>
15 Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', non-empty-array)

@phpstan-bot
Copy link
Contributor

@VincentLanglet After the latest push in 1.9.x, PHPStan now reports different result with your code snippet:

@@ @@
 10: Dumped type: array<string, mixed>
-15: Dumped type: non-empty-array<string, non-empty-array<''>>&hasOffsetValue('uses', non-empty-array<''>)
-17: Comparison operation ">" between int<1, max> and 0 is always true.
+15: Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', non-empty-array<''>)
Full report
Line Error
10 Dumped type: array<string, mixed>
15 Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', non-empty-array<''>)

@phpstan-bot
Copy link
Contributor

@VincentLanglet After the latest push in 1.9.x, PHPStan now reports different result with your code snippet:

@@ @@
 10: Dumped type: array<string, mixed>
-15: Dumped type: non-empty-array<string, array{'', ''}>&hasOffsetValue('uses', array{'', ''})
-17: Comparison operation ">" between 2 and 0 is always true.
+15: Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', array{'', ''})
Full report
Line Error
10 Dumped type: array<string, mixed>
15 Dumped type: non-empty-array<string, mixed>&hasOffsetValue('uses', array{'', ''})

@ondrejmirtes
Copy link
Member

Fixed by phpstan/phpstan-src#1808

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants