Fix !isset() with Variable

[staabm]

Another split out of #2657

implements the

Because for $foo with certainty maybe and not-nullable, the only !isset($foo) is that the variable does not exist, so the certainty needs to be no.

part of #2657 (review)

---

closes phpstan/phpstan#3632
closes phpstan/phpstan#8190
closes phpstan/phpstan#8366
closes phpstan/phpstan#8659
closes phpstan/phpstan#9580
closes phpstan/phpstan#10064
closes phpstan/phpstan#10088

[staabm]

I am not sure about this condition

[staabm]

hopefully its OK to do the variable stuff in a separate PR.
whats missing is ArrayDimFetch, PropertyFetch, StaticPropertyFetch which could be done in a followup?

[staabm]

the carbon error seems valid

the larastan errors seem unrelated, as they also show up in other open PRs

the presta shop error looks fishy

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

Also this is the place to test all the different scenarios like I described in the original PR: #2657 (review)

So if we're doing !isset($a):

1. If the variable is nullable, it means that after this PR it might be null or it might not exist
2. If the variable is not nullable, it means that after this PR it does not exist anymore

etc...

[staabm]

I have more tests and a small fix open todo

[staabm]

one interessting failling drupal case:

https://github.com/drupal/drupal/blob/2700c5afb6c3936041db413872eea82dc0bd4fe4/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php#L138C84-L154

------ ------------------------------------------------------------------- 
  Line   core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php  
 ------ ------------------------------------------------------------------- 
  152    Variable $result might not be defined.                             
 ------ ------------------------------------------------------------------- 

reduced it to

function doBar(bool $view, bool $edit): void {
	foreach (['view' => $view, 'edit' => $edit] as $operation => $result) {
		assertVariableCertainty(TrinaryLogic::createYes(), $result);

		$result_text = !isset($result) ? 'null' : ($result ? 'true' : 'false'); // Variable $result in isset() always exists and is not nullable.

		assertVariableCertainty(TrinaryLogic::createYes(), $result); // Expected variable certainty Yes, actual: Maybe
		assertType('bool', $result); // Variable $result might not be defined.
	}
}

-> I think its great that PHPStan realized that since $result can never be null and therefore the isset() is actually dead code
-> since we turn the certainty to NO via !isset() we get a misleading consequential false-positve error

[ondrejmirtes]

Hi, I'm putting this on hold for a while until this problem is resolved: #2666 (comment)

[ondrejmirtes]

Alright, one more problem this PR should try to fix is:

Expected No, actual certainty of variable $shortcut_id is Maybe in /Users/ondrej/Development/phpstan/tests/PHPStan/Analyser/data/bug-10088.php on line 22.

Not sure why it does not work now.

[ondrejmirtes]

The problem in Drupal should be fixed, the code in Carbon should still fail because it was wrong from the start.

[ondrejmirtes]

Did my commit in forward fashion on 1.10.x, only added more tests here.

[ondrejmirtes]

Results look better but there's something wrong with !empty in Drupal: https://github.com/phpstan/phpstan-src/actions/runs/6756144616/job/18365350671

Source: https://github.com/drupal/drupal/blob/2700c5afb6c3936041db413872eea82dc0bd4fe4/core/modules/book/book.module#L123

[staabm]

I guess the problem is related that the variable is only defined via dim fetches

(Maybe I can have a look tonight)

[ondrejmirtes]

That shouldn't matter, right? Also I found out it already fails on 1.10.x so maybe I broke it with my recent commits.

[staabm]

It shouldn't - but its a notable difference of the case at hand vs working unit tests

[staabm]

Another notable difference of the drupal case: the dim-fetch based declarations are wrapped in a property-fetch based isset()

[staabm]

added a reduced test for the drupal case. looks like a certainty issue of regular variables (which was already contained in 1.10.40)

[ondrejmirtes]

Hi, my plan for today is to make 1.10.x releasable again by reverting all related changes and picking those commits into this PR. Right now it’s pretty broken and we need to do this properly in order not to endanger the stability. So we’re gonna focus the changes here and make sure the CI is green.

I also need to work on other stuff like PHP 8.3 and PHPStan 1.11.

[staabm]

I see thanks. If there is anything I can do to support you in other areas, feel free to note.

[staabm]

Results look better but there's something wrong with !empty in Drupal: phpstan/phpstan-src/actions/runs/6756144616/job/18365350671

Source: drupal/drupal@2700c5a/core/modules/book/book.module#L123

I also found the cause for this problem:

function book_node_links_alter(array &$links, NodeInterface $node) {
  \PHPStan\dumpType(isset($node->book['depth'])); // Dumped type: false 
    if (isset($node->book['depth'])) {
          $book_links['book_printer'] = [
          ];
    }
    \PHPStan\Testing\assertVariableCertainty(\PHPStan\TrinaryLogic::createMaybe(), $book_links); // Expected variable certainty Maybe, actual: No

    if (!empty($book_links)) {
      $links['book'] = [
        '#links' => $book_links,
      ];
    }
}

I think everything is working as expected - as far as this PR is a concern. we conclude the isset() expression is always FALSE, therefore we don't take the IF-truethy-scope into the IF-final-scope which means the variable has a certainty of NO after the if (isset($node->book['depth'])) { block

[staabm]

Alright, one more problem this PR should try to fix is:

Expected No, actual certainty of variable $shortcut_id is Maybe in /Users/ondrej/Development/phpstan/tests/PHPStan/Analyser/data/bug-10088.php on line 22.

Not sure why it does not work now.

I think the cause for this problem is, that the scope before entering the if ($link_mode === "add") { statement is

$scope->debug();

array (
  '$shortcut_id (Maybe)' => '1',
  '$link_mode (Yes)' => '\'add\'|\'remove\'',
  'native $shortcut_id' => '1',
  'native $link_mode' => '\'add\'|\'remove\'',
  'condition about $shortcut_id #1' => 'if $link_mode=\'remove\' then $shortcut_id is 1 (Yes)',
)

which means we don't know about a conditional type and certainty for the ELSE case of the ternary $link_mode = isset($shortcut_id) ? "remove" : "add";

in the end it feels like a possible future improvement..?

[ondrejmirtes]

I hope you don't mind the forcepushes. If you have some commits locally and I forcepush the base under your feet, can solve it easily with git rebase --onto.

[staabm]

really nice, thank you. I was looking at it for hours and coulnd't make sense of it.

[clxmstaab]

I have a test-case locally for the conditon bug. let me push it into the PR

[ondrejmirtes]

One part didn't make much sense to me so I changed it:

• 6e72a32
• e718636

The change about PHPDoc certainty is probably a bug in merging scopes, I don't care much about that right now.

I will be merging this soon. Thank you!

[staabm]

thanks for your support!