FNSR false positive: Call to method X on a separate line has no effect

[staabm]

closes phpstan/phpstan#13956

[staabm]

I think the bug does not reproduce in tests (but it does when running the repro script standalone), because in tests a method is filtered out because it is reported to have impure point, while it doesn't have a impure point when running standalone

phpstan-src/src/Rules/DeadCode/MethodWithoutImpurePointsCollector.php

Lines 34 to 36 in a951ac0

	if (count($node->getImpurePoints()) !== 0) {
	return null;
	}

[staabm]

applying this diff makes the test fail, with the same error we get when run standalone:

diff --git a/src/Rules/DeadCode/CallToMethodStatementWithoutImpurePointsRule.php b/src/Rules/DeadCode/CallToMethodStatementWithoutImpurePointsRule.php
index a04d151ad9..65d63f8d29 100644
--- a/src/Rules/DeadCode/CallToMethodStatementWithoutImpurePointsRule.php
+++ b/src/Rules/DeadCode/CallToMethodStatementWithoutImpurePointsRule.php
@@ -34,6 +34,11 @@ final class CallToMethodStatementWithoutImpurePointsRule implements Rule
                        }
                }
 
+               $methods['bug13956\foo'] =  [
+                       'addsuccess' => 'Bug13956\Foo::addSuccess',
+                       'getsuccessmessages' => 'Bug13956\Foo::getSuccessMessages',
+               ];
+
                $errors = [];
                foreach ($node->get(PossiblyPureMethodCallCollector::class) as $filePath => $data) {
                        foreach ($data as [$classNames, $method, $line]) {

[staabm]

interessting.. I can see that in

phpstan-src/src/Analyser/NodeScopeResolver.php

Line 819 in 6f4f744

$methodImpurePoints[] = new ImpurePoint(

a impure point is generated and appended to the by-ref array &$methodImpurePoints but when inspecting this line

phpstan-src/src/Analyser/NodeScopeResolver.php

Line 842 in 6f4f744

$methodReflection = $methodScope->getFunction();

I can see the array is still empty.

so I suspect either xdebug is lying/buggy or php-src somehow looses the by-ref appended array content.
maybe its working like fiber should work by design, but thats something I haven't enough experience with to judge.

[staabm]

hmm maybe the problem is, that we are appending to the array from inside a fiber..

but the actual array lives in the non-fiber execution context

because I can see the array beeing empty after the callable, even though it was appended to it in the meantime

---

repro:

➜  phpstan-src git:(testBug13956) ✗ cp tests/PHPStan/Rules/Methods/data/bug-13956.php .


➜  phpstan-src git:(testBug13956) ✗ PHPSTAN_FNSR=1 php bin/phpstan analyze bug-13956.php --debug         
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
/Users/staabm/workspace/phpstan-src/bug-13956.php
int(0)
int(0)
appending
int(0)
 ------ ----------------------------------------------------------------------------- 
  Line   bug-13956.php                                                                
 ------ ----------------------------------------------------------------------------- 
  30     Call to method Bug13956\Foo::addSuccess() on a separate line has no effect.  
         🪪  method.resultUnused                                                      
         at bug-13956.php:30                                                          
 ------ ----------------------------------------------------------------------------- 


                                                                                                                        
 [ERROR] Found 1 error                                                                                                  
                                                                                                                        

with

--- a/src/Analyser/NodeScopeResolver.php
+++ b/src/Analyser/NodeScopeResolver.php
@@ -816,6 +816,7 @@ class NodeScopeResolver
                                                ) {
                                                        return;
                                                }
+                                               echo "appending\n";
                                                $methodImpurePoints[] = new ImpurePoint(
                                                        $scope,
                                                        $node,
@@ -839,6 +840,7 @@ class NodeScopeResolver
                                        $gatheredReturnStatements[] = new ReturnStatement($scope, $node);
                                }, StatementContext::createTopLevel())->toPublic();
 
+                               var_dump(count($methodImpurePoints));
                                $methodReflection = $methodScope->getFunction();
                                if (!$methodReflection instanceof PhpMethodFromParserNodeReflection) {
                                        throw new ShouldNotHappenException();

[ondrejmirtes]

Fibers are not threads, they still work with the same objects and values, they are just "functions that can be paused and resumed later".

I'll look into it later, but caching of FileTypeMapper and releasing the current performance improvements is a priority 😊

[ondrejmirtes]

Alright, with this diff:

diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php
index 043ae8943c..0ae6a24889 100644
--- a/src/Analyser/NodeScopeResolver.php
+++ b/src/Analyser/NodeScopeResolver.php
@@ -798,7 +798,8 @@ class NodeScopeResolver
 				$gatheredYieldStatements = [];
 				$executionEnds = [];
 				$methodImpurePoints = [];
-				$statementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $methodScope, $storage, static function (Node $node, Scope $scope) use ($nodeCallback, $methodScope, &$gatheredReturnStatements, &$gatheredYieldStatements, &$executionEnds, &$methodImpurePoints): void {
+				$a = md5(uniqid());
+				$statementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $methodScope, $storage, static function (Node $node, Scope $scope) use ($nodeCallback, $a, $methodScope, &$gatheredReturnStatements, &$gatheredYieldStatements, &$executionEnds, &$methodImpurePoints): void {
 					$nodeCallback($node, $scope);
 					if ($scope->getFunction() !== $methodScope->getFunction()) {
 						return;
@@ -816,6 +817,8 @@ class NodeScopeResolver
 						) {
 							return;
 						}
+						var_dump('add:');
+						var_dump($a);
 						$methodImpurePoints[] = new ImpurePoint(
 							$scope,
 							$node,
@@ -823,6 +826,7 @@ class NodeScopeResolver
 							'property assignment',
 							true,
 						);
+						var_dump('------');
 						return;
 					}
 					if ($node instanceof ExecutionEndNode) {
@@ -839,6 +843,11 @@ class NodeScopeResolver
 					$gatheredReturnStatements[] = new ReturnStatement($scope, $node);
 				}, StatementContext::createTopLevel())->toPublic();
 
+				var_dump('below:');
+				var_dump($a);
+				var_dump(count($methodImpurePoints));
+				var_dump('------');
+
 				$methodReflection = $methodScope->getFunction();
 				if (!$methodReflection instanceof PhpMethodFromParserNodeReflection) {
 					throw new ShouldNotHappenException();

I figured out why it breaks with full analysis and not in test of a single rule. The output is this:

string(6) "below:"
string(32) "ef896eacffbd3bc19029f89a422d5b56"
int(0)
string(6) "------"
string(6) "below:"
string(32) "0befe615aa1f40df34c9979ac2fac1b1"
int(0)
string(6) "------"
string(4) "add:"
string(32) "0befe615aa1f40df34c9979ac2fac1b1"
string(6) "------"
string(6) "below:"
string(32) "4540a3c77f8fbbb0403f6d3deb05072d"
int(0)
string(6) "------"
string(4) "here"

So the callback is "paused" in a Fiber and the rest of the analysis continues. I'm surprised that more stuff doesn't break because of this. I'll have to think about what we can do about it.

[ondrejmirtes]

Here's a quick fix but it's stupid: 60c3118