Fix variable certainty for ArrayDimFetch in falsey isset context

[staabm]

triggered by #2657 (comment)

---

btw: I realized that - across the PHPStan codebase - we are sometimes use falsey and falsy which is something we could fix with PHPStan 2.0

[ondrejmirtes]

Please reproduce and fix the same problem with Empty_.

[staabm]

hmm interessting.. it does not reproduce with Empty_

[staabm]

Ohh even more interessting.. it does not reproduce in a NodeScopeResolverTest, but running the same code directly with phpstan in a separate file reproduces the problem for Empty_ -> added a new AnalyserIntegrationTest

[staabm]

after debugging deep in PHPStan internals, I think I am able to pinpoint the underlying issue.

while handling Isset_ or Empty_ there is some expression types storing and re-storing going on.

while this process a variable which was "maybe" before automatically got a "yes"-certainty, because assigning a type to a variable automatically made it certain.

I fixed this mechanism to also remember the previous certainty of the variable, not just the type and native-type.

[staabm]

regarding build errors

---

Carbon:

Error: Variable $date might not be defined.
 ------ -------------------------------------- 
  Line   tests/CarbonPeriod/ToArrayTest.php    
 ------ -------------------------------------- 
  100    Variable $date might not be defined.  
 ------ -------------------------------------- 

https://github.com/briannesbitt/Carbon/blob/0af369fba2435afe17a97cfa9a02a8e84d6c6640/tests/CarbonPeriod/ToArrayTest.php#L95-L100

I think this error could be valid, as we are probably not narrow the type of $date via assertInstanceOfCarbon() - which sounds like a carbon specific unit test utility to me

---

the efabrica-team/phpstan-latte error look wild.. I wonder whether this are related.

---

drupal

 ------ --------------------------------------------- 
  Line   core/modules/shortcut/shortcut.module        
 ------ --------------------------------------------- 
  294    Variable $shortcut_id might not be defined.  
  297    Variable $shortcut_id might not be defined.  
 ------ --------------------------------------------- 

https://github.com/drupal/drupal/blob/2700c5afb6c3936041db413872eea82dc0bd4fe4/core/modules/shortcut/shortcut.module#L280-L298

these look valid to me. it seems we are now detecting these mabye undefined while we were missing them before

------ ----------------------------------------------------------------------------------------------------------------------------- 
  Line   core/modules/views/src/Plugin/views/filter/NumericFilter.php                                                                 
 ------ ----------------------------------------------------------------------------------------------------------------------------- 
  426    Ignored error pattern #^Variable \$value might not be defined\.$# in                                                         
         path                                                                                                                         
         /home/runner/work/phpstan-src/phpstan-src/e2e/integration/repo/core/modules/views/src/Plugin/views/filter/NumericFilter.php  
         is expected to occur 2 times, but occurred 3 times.                                                                          
  432    Variable $value might not be defined.                                                                                        
 ------ ----------------------------------------------------------------------------------------------------------------------------- 

https://github.com/drupal/drupal/blob/2700c5afb6c3936041db413872eea82dc0bd4fe4/core/modules/views/src/Plugin/views/filter/NumericFilter.php#L423-L438

also looks valid to me

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

Did you look through the errors please?

• https://github.com/phpstan/phpstan-src/actions/runs/6453872440/job/17518378375
• https://github.com/phpstan/phpstan-src/actions/runs/6453872440/job/17518379195

[staabm]

I did please see the comment above. I think our comments might be overlapped

[ondrejmirtes]

Sorry, if there are 21 emails in the thread, I tend to overlook things :)

[ondrejmirtes]

So if we're currently preserving the certainty, how does PHPStan go from "maybe" to "yes" in this case?

// $foo is Maybe
if (isset($foo)) {
    // $foo is Yes
}

[staabm]

excellent question. I had to debug further and look into it even more to answer it ;-).

we get from maybe to yes while we specify non-nullability in

phpstan-src/src/Analyser/TypeSpecifier.php

Line 748 in 6252d21

$type = $this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope, $rootExpr);

just pushed a new commit which refactors this code a bit to make it more obvious

---

another interessting find: it seems we stop specifying types in isset(...) as soon as we encouter the first expression which is undefined

[staabm]

I decided to add new assertions for this old bug, while in the process of understanding the internals better
(IMO the new assertions better describe what the end user expected)

[ondrejmirtes]

Yeah, please clean this up as much as possible, and don't forget that both empty and ?? are very similar to isset and might have similar problems.

[ondrejmirtes]

we get from maybe to yes

I'd still like to get from maybe to yes for other specifiers, like is_string($foo) - if it's a string then we know the certainty is yes.

[staabm]

I see. do you talk about new features or something I need to make sure we don't regress?

[ondrejmirtes]

Not new features, but about this: https://phpstan.org/r/5f6093e0-0f6e-405a-9497-54263c6348b7

I worry that if you preserve certainty, there will be new error on line 12.

[staabm]

should we not have the error on line 15 or should we have one on line8?

https://phpstan.org/r/d3ba81f8-188e-47ea-805e-54302006bc7a

[ondrejmirtes]

We can't go from no to yes and from yes to no.

[ondrejmirtes]

Also an idea: in the other PR you introduced NotIssetExpr.

I think it should really be IssetExpr and applied in both truthy and falsy contexts.

[staabm]

We can't go from no to yes and from yes to no.

lets see if I understood you right ;)

b1981b0

Also an idea: in the other PR you introduced NotIssetExpr.

I think it should really be IssetExpr and applied in both truthy and falsy contexts.

I am fine with it. lets discuss it in the other PR when this one is merged

[ondrejmirtes]

Thank you! Please continue by rebasing and finishing #2657.

[ondrejmirtes]

Hi @staabm this PR caused failures in Drupal and Carbon:

• https://github.com/phpstan/phpstan/actions/runs/6697009618/job/18196030092
• https://github.com/phpstan/phpstan/actions/runs/6697009618/job/18196030700

Fortunately it's not released yet. Please look into them otherwise I'll have to revert this. Thanks.

[staabm]

I will have a look 👀

[staabm]

I had a first look into drupal problem - related source - and reduced it to:

<?php

use function PHPStan\Testing\assertType;
use function PHPStan\Testing\assertVariableCertainty;

function book_node_links_alter():void {
	if (isset($x)) {
		$book_links = 1;
	}

// Expected variable certainty Maybe, actual: No 
	assertVariableCertainty(\PHPStan\TrinaryLogic::createMaybe(), $book_links); 

	if (!empty($book_links)) {
		var_dump($book_links);
	}
}

I wonder why we have a NO certainty here.. is it expected?
its not something which regressed with this PR - its reproducible also with the latest stable release.

[staabm]

another finding: in the latest 1.10 release in this snippet we went from No to Yes.

function justEmpty(): void
{
	assertVariableCertainty(TrinaryLogic::createNo(), $foo);
	if (!empty($foo)) {
		assertVariableCertainty(TrinaryLogic::createYes(), $foo);
	}
	assertVariableCertainty(TrinaryLogic::createNo(), $foo);
}

this was changed with this PR here, but regressed the snippet in the above comment.

[staabm]

to sum up my investigation of all mentioned failures caused by this PR:

IMO there is only 1 regression, and thats the one reduced above.
I think we need to question whether we are sure about "never go from No to Yes" in certainty, or we need to compensate NO certainty in the rule which emits "Undefined variable: $book_links" when nested in empty() or isset()

all remaining new failures are valid