Please reproduce and fix the same problem with Empty_.

hmm interessting.. it does not reproduce with Empty_

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

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.

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

This pull request has been marked as ready for review.

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

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

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

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
}

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

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

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.

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.

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

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.

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

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

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.

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

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

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.

I will have a look 👀

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.

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.

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