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

false positive: Variable $variableDefinedWithinForeach might not be defined. #651

Closed
stesie opened this issue Dec 1, 2017 · 7 comments
Closed

Comments

@stesie
Copy link

stesie commented Dec 1, 2017

PHPStan since 0.9 doesn't detect early continue of foreach loop wih continue 2 in foreach/switch loop

Demo: https://phpstan.org/r/62fe427842b4f0c162961a5eaf75a158

<?php declare(strict_types = 1);

foreach (['foo', 'bar'] as $loopValue) {
        switch ($loopValue) {
        case 'foo':
                continue 2;

        case 'bar':
                $variableDefinedWithinForeach = 23;
                break;

        default:
                throw new \LogicException();
        }

        echo $variableDefinedWithinForeach;
}

The echo statement is actually only reachable via "bar"-case and there the variable in question is defined. PHPStan doesn't consider that continue 2 breaks the loop early and considers the variable undefined, hence triggering "might not be defined"

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Dec 1, 2017 via email

@ondrejmirtes ondrejmirtes added this to the 0.9.x milestone Dec 20, 2017
@muglug
Copy link
Contributor

muglug commented Jan 8, 2018

Hi, yes, support for break X and continue X is really hard to implement and
not yet properly supported.

If you want pointers, I think this covers pretty much everything? https://github.com/vimeo/psalm/blob/66fa08148805d4935218b9593885dd3e0af1394d/src/Psalm/Checker/Statements/Block/LoopChecker.php

Figuring out whether what a given set of statements do is in https://github.com/vimeo/psalm/blob/66fa08148805d4935218b9593885dd3e0af1394d/src/Psalm/Checker/ScopeChecker.php#L56 and was key for me in cracking the whole break/continue code.

A very brief sketch of the system of contexts it creates is here: vimeo/psalm#365 (comment), would be happy to go into more depth while the thing is still relatively fresh in my mind.

And finally, https://github.com/vimeo/psalm/blob/66fa08148805d4935218b9593885dd3e0af1394d/tests/LoopScopeTest.php contains a whole bunch of tests for this stuff. Allowed me to go pretty crazy with a refactor without worrying about regressions

@ondrejmirtes
Copy link
Member

@muglug I'm afraid that your code is not applicable here because PHPStan uses a completely different architecture. The whole logic of existing variables and their types after a series of conditions and loops is handled here:

https://github.com/phpstan/phpstan/blob/master/src/Analyser/NodeScopeResolver.php#L1188 (NodeScopeResolver::lookForAssignsInBranches))) and in this object (https://github.com/phpstan/phpstan/blob/master/src/Analyser/LookForAssignsSettings.php)

So I will have to figure out by myself but after the latest refactoring, I don't think it will be a big problem.

@o5
Copy link

o5 commented Jan 17, 2018

I've similar issue with switch and "might not be defined".

<?php declare(strict_types = 1);

$var = 'foo';
switch ($var) {
	case 'foo':
		$bar = 'foo';
		break;
	default:
		throw new \InvalidArgumentException(); // some comment
}

in_array($bar, [], true);
 ------ ------------------------------------- 
  Line   analyzed.php                         
 ------ ------------------------------------- 
  12     Variable $bar might not be defined.  
 ------ ------------------------------------- 

Demo: https://phpstan.org/r/bbcef57b6fecc961a357a9d50122838e

It works when I remove comment // some comment on line 9. It's very strange.

@o5
Copy link

o5 commented Jan 17, 2018

Sorry, I just tried it on master and there is probably fixed by 870e449.

@ondrejmirtes
Copy link
Member

Fixed: phpstan/phpstan-src@31fcad6

@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 Apr 30, 2021
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