Fix incorrect analyzing of array_shift with non-empty-list property by staabm · Pull Request #4294 · phpstan/phpstan-src

staabm · 2025-09-07T17:29:30Z

closes phpstan/phpstan#13438
closes phpstan/phpstan#2888
closes phpstan/phpstan#3387
closes phpstan/phpstan#2560
closes phpstan/phpstan#2457
closes phpstan/phpstan#11846

phpstan-bot · 2025-09-07T17:50:44Z

This pull request has been marked as ready for review.

ondrejmirtes

I'm worried that other virtual nodes still not fire for this, like VariableAssignNode.
This is still not going to work on $this->foo[$x] for example. This will still have a false positive: https://phpstan.org/r/a6e895c1-0e8e-4e49-8cd9-e7a51655a506

Correct solution in these places is to construct proper $this->processAssignVar call. Which might be a bit complicated to get right but here's the easiest inspiration probably:

phpstan-src/src/Analyser/NodeScopeResolver.php

Lines 5382 to 5397 in 02116b7

    
           $result = $this->processAssignVar( 
        
           	$scope, 
        
           	$stmt, 
        
           	$argValue, 
        
           	new TypeExpr($byRefType), 
        
           	static function (Node $node, Scope $scope) use ($nodeCallback): void { 
        
           		if (!$node instanceof PropertyAssignNode && !$node instanceof VariableAssignNode) { 
        
           			return; 
        
           		} 
        
           		$nodeCallback($node, $scope); 
        
           	}, 
        
           	$context, 
        
           	static fn (MutatingScope $scope): ExpressionResult => new ExpressionResult($scope, false, false, [], []), 
        
           	true, 
        
           );

staabm · 2025-09-08T08:33:00Z

src/Analyser/NodeScopeResolver.php

 					$isArrayPop ? $arrayArgType->popArray() : $arrayArgType->shiftArray(),
 					$isArrayPop ? $arrayArgNativeType->popArray() : $arrayArgNativeType->shiftArray(),
 				);
+


I have also tried removing aboves $scope->invalidateExpression($arrayArg)->assignExpression($arrayArg, ...) but this leads to different native vs. phpdoc type results

--- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -2659,22 +2659,14 @@ final class NodeScopeResolver && count($expr->getArgs()) >= 1 ) { $arrayArg = $expr->getArgs()[0]->value; - $arrayArgType = $scope->getType($arrayArg); - $arrayArgNativeType = $scope->getNativeType($arrayArg); - $isArrayPop = $functionReflection->getName() === 'array_pop'; - $scope = $scope->invalidateExpression($arrayArg)->assignExpression( - $arrayArg, - $isArrayPop ? $arrayArgType->popArray() : $arrayArgType->shiftArray(), - $isArrayPop ? $arrayArgNativeType->popArray() : $arrayArgNativeType->shiftArray(), - ); $scope = $this->processAssignVar( $scope, $stmt, $arrayArg, - $arrayArg, + new TypeExpr($isArrayPop ? $arrayArgType->popArray() : $arrayArgType->shiftArray()), static function (Node $node, Scope $scope) use ($nodeCallback): void { if (!$node instanceof PropertyAssignNode && !$node instanceof VariableAssignNode) {

I'm fine with that as a temporary step.

Maybe we can experiment with a new TypeExpr-like class in another PR that would carry $type and $nativeType - two constructor arguments. A bunch of places that currently do new TypeExpr might get better behaviour from that.

after doing #4294 (comment) we get the same type changes, as I saw with the above suggestion.

so I went ahead and did this simplification in a53b7a9

if you are not fine with it, I can revert the changes (but it does not affect results of tests)

Maybe we can experiment with a new TypeExpr-like class in another PR that would carry $type and $nativeType - two constructor arguments

agree 👍

I don't want this simplification. We'd have to put the code back anyway after we do the "TypeExpr with native type" again. Please revert it.

ondrejmirtes · 2025-09-08T09:22:06Z

Issue bot jackpot 🎉

Isn't phpstan/phpstan#11846 also fixed?

staabm · 2025-09-08T09:23:31Z

Isn't phpstan/phpstan#11846

I wasn't sure, because the reporter wanted it to dump true.
but bool at least fixes the wrong false we had before.

so we are better but not perfect yet.

will add a test

ondrejmirtes · 2025-09-08T09:25:17Z

src/Analyser/NodeScopeResolver.php

+					$scope,
+					$stmt,
+					$arrayArg,
+					$arrayArg,


These arguments would mean that we're doing $foo = $foo; which is weird.

It might not influence anything but $assignedExpr should be new TypeExpr with the thing passed to 2nd parameter of assignExpression. In this case it's $isArrayPop ? $arrayArgType->popArray() : $arrayArgType->shiftArray() (but of course first save it into a variable).

ok, so it seems you are fine with what I suggested in #4294 (comment) ?

ondrejmirtes · 2025-09-08T09:25:48Z

src/Analyser/NodeScopeResolver.php

+					$scope,
+					$stmt,
+					$arrayArg,
+					$arrayArg,


For example here it should be new TypeExpr($arrayType)

ondrejmirtes · 2025-09-08T10:52:32Z

Perfect, thank you!

VincentLanglet · 2025-09-08T11:13:43Z

Good job @staabm you're really good to close issues by batch !

staabm marked this pull request as ready for review September 7, 2025 17:50

ondrejmirtes requested changes Sep 8, 2025

View reviewed changes

staabm commented Sep 8, 2025

View reviewed changes

clxmstaab force-pushed the bug13438 branch from e5bbd49 to 0d648a1 Compare September 8, 2025 08:58

ondrejmirtes requested changes Sep 8, 2025

View reviewed changes

clxmstaab force-pushed the bug13438 branch 2 times, most recently from 51555c7 to a53b7a9 Compare September 8, 2025 09:42

staabm force-pushed the bug13438 branch from a53b7a9 to baee813 Compare September 8, 2025 09:43

clxmstaab force-pushed the bug13438 branch 2 times, most recently from ca8bacf to 2f5e06a Compare September 8, 2025 10:28

staabm and others added 12 commits September 8, 2025 12:28

Fix incorrect analyzing of array_shift with non-empty-list property

aa8d779

fix array_push()/array_pop()

9d2f659

fix more functions

4998c74

added regression test

f37c06c

fix php 7.4 lint

4fe77cf

use MutatingScope->processAssignVar() instead

cf9bf2c

Added regression test

b872a73

Added regression test

15bb2f7

Added regression test

de0b94b

fix php 7.4 build

15e7f3a

Create bug-11846.php

6d77e92

use newType() as expr

4702c2b

staabm force-pushed the bug13438 branch from 2f5e06a to 4702c2b Compare September 8, 2025 10:28

ondrejmirtes merged commit 0242d48 into phpstan:2.1.x Sep 8, 2025
449 of 455 checks passed

staabm deleted the bug13438 branch September 8, 2025 10:58

staabm mentioned this pull request Sep 8, 2025

Implemented NativeTypeExpr #4302

Merged

hemberger mentioned this pull request Sep 12, 2025

False positive with array_pop on same line as array_unshift phpstan/phpstan#13510

Open

	$result = $this->processAssignVar(
	$scope,
	$stmt,
	$argValue,
	new TypeExpr($byRefType),
	static function (Node $node, Scope $scope) use ($nodeCallback): void {
	if (!$node instanceof PropertyAssignNode && !$node instanceof VariableAssignNode) {
	return;
	}

	$nodeCallback($node, $scope);
	},
	$context,
	static fn (MutatingScope $scope): ExpressionResult => new ExpressionResult($scope, false, false, [], []),
	true,
	);

Conversation

staabm commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phpstan-bot commented Sep 7, 2025

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

staabm Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

staabm Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Sep 8, 2025

Uh oh!

staabm commented Sep 8, 2025

Uh oh!

ondrejmirtes Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

staabm Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ondrejmirtes commented Sep 8, 2025

Uh oh!

VincentLanglet commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

staabm commented Sep 7, 2025 •

edited

Loading

staabm Sep 8, 2025 •

edited

Loading

ondrejmirtes Sep 8, 2025 •

edited

Loading

staabm Sep 8, 2025 •

edited

Loading