Fix !isset() with ArrayDimFetch #2657

staabm · 2023-09-30T13:04:06Z

closes phpstan/phpstan#9908
closes phpstan/phpstan#8724
closes phpstan/phpstan#8659
closes phpstan/phpstan#8190
closes phpstan/phpstan#5128
closes phpstan/phpstan#9426
closes phpstan/phpstan#9580

src/Analyser/MutatingScope.php

staabm · 2023-09-30T14:16:34Z

I can see in the issue bot, that the change as is would resolve a lot of open issues.

I have no idea yet, how to express the "nullability" in the !isset() case.

ondrejmirtes

So if we're going to handle isset in falsy context, we need to understand what isset does. With !isset($foo) it means:

The variable might not exist
The variable might be null

We need to test these scenarios:

Variable with certainty maybe and nullable
Variable with certainty maybe and not nullable
Variable with certainty yes and nullable
Variable with certainty yes and not nullable

For !isset($foo['bar']), it means:

The key does not exist
The key exists but it's null

So various scenarious with array{foo: int} and array{foo: int|null} and also array{foo?: int} and array{foo?: int|null} need to be tested.

Plus for !isset($foo['bar']) it might also mean that anything in the chain might also not exist so the first 4) points about !isset($foo) apply to !isset($foo['bar']).

I think the main hurdle here is that SpecifiedTypes currently cannot express a command to unset a variable. 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.

But we can introduce some kind of UnsetExpr virtual node that would be in SpecifiedTypes and would instruct MutatingScope::filterBySpecifiedTypes to unset the variable from scope. That might or might not be the way to go.

tests/PHPStan/Analyser/data/falsy-isset.php

src/Analyser/ExpressionTypeHolder.php

src/Analyser/MutatingScope.php

ondrejmirtes

Also please write regression tests for all the issues the issue bot found (after the first original iteration), to make sure we don't break them during the review.

ondrejmirtes · 2023-10-04T13:39:30Z

src/Analyser/MutatingScope.php

 		$specifiedExpressions = [];
 		foreach ($typeSpecifications as $typeSpecification) {
 			$expr = $typeSpecification['expr'];
 			$type = $typeSpecification['type'];
+
+			if ($expr instanceof NotIssetExpr) {
+				$unsetExpr = $expr->getExpr();


Do this only for 'sure'=true.

moving it into the 'sure'=true branch regresses some tests. I think what you want to achieve is already taken care of in the TypeSpecifier side of this PR, as NotIssetExpr is only passed thru in the !$context->true()-case

phpstan-bot · 2023-10-04T16:17:47Z

This pull request has been marked as ready for review.

staabm · 2023-10-04T16:18:23Z

I am bit a lost atm .. will take a break and look again at it later.

src/Analyser/TypeSpecifier.php

staabm · 2023-10-04T19:40:19Z

the only remaining errors are 2 test expectations, which I am not sure wheter my expectations are wrong, or there is a open todo:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/data/falsy-isset.php:84" ('variableCertainty', '/home/runner/work/phpstan-src...et.php', PHPStan\TrinaryLogic Object (...), PHPStan\TrinaryLogic Object (...), 'a', 84)
Expected Maybe, actual certainty of variable $a is Yes in /home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/data/falsy-isset.php on line 84.
Failed asserting that false is true.

/home/runner/work/phpstan-src/phpstan-src/src/Testing/TypeInferenceTestCase.php:110
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:1369

2) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/data/falsy-isset.php:88" ('variableCertainty', '/home/runner/work/phpstan-src...et.php', PHPStan\TrinaryLogic Object (...), PHPStan\TrinaryLogic Object (...), 'a', 88)
Expected Maybe, actual certainty of variable $a is Yes in /home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/data/falsy-isset.php on line 88.
Failed asserting that false is true.

staabm · 2023-10-05T06:46:46Z

src/Analyser/TypeSpecifier.php

+						$newType = TypeTraverser::map($newType, static function (Type $type, callable $traverse) use ($dimType): Type {
+							if ($type instanceof UnionType || $type instanceof IntersectionType) {
+								return $traverse($type);
+							}
+
+							if ($type instanceof ConstantArrayType) {
+								$builder = ConstantArrayTypeBuilder::createFromConstantArray(
+									$type,
+								);
+								$builder->setOffsetValueType(
+									$dimType,
+									new NullType(),
+									true,
+								);
+								return $builder->getArray();
+							}
+
+							return $type->setOffsetValueType($dimType, new NullType());
+						});


instead of this TypeTraverser block, we could add a optional new arg to Type->setOffsetValueType()

staabm · 2023-10-05T06:47:47Z

src/Analyser/TypeSpecifier.php

+							$var->var,
+							$newType,
+							$context->negate(),
+							true,


I think there is no way arround override=true here, because we do not intersect/union the previous known array-shape, but just update a single offset of it.

ondrejmirtes

Please solve not only ArrayDimFetch but also standalone variables that I talk about here #2657 (review)

ondrejmirtes · 2023-10-05T12:23:14Z

src/Analyser/MutatingScope.php

+						$exprString = $this->getNodeKey($unsetExpr->var);
+
+						unset($scope->expressionTypes[$exprString]);
+						unset($scope->nativeExpressionTypes[$exprString]);


I don't understand this logic at all. Here you're saying that if we unset($var['foo']) and the $var is an empty array, you're going to unset $var !?

this is the implementation part of the tests which fail atm

the idea is to implement what was described in

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.

let me fix it

I am doing this array stuff here, because I think:

a array shape which only has a single offset, which gets checked with isset() on said offset will never have the variable defined in the falsey-branch

$a = ['foo' => 'abc]; if (isset($a['foo'])) { } else { // $a is always undefined }

since we don't want to go from YES to NO certainty, I think my above case is not possible and I should not try to support it?

$a = ['foo' => 'abc']; if (isset($a['foo'])) { } else { // $a should be NeverType here }

staabm · 2023-10-05T20:19:01Z

just pushed the implementation for variables and a bunch of tests.

main problem is still the certainty stuff, and I think most - if not all - remaining test-failures feel related to certainty

and mooooore regression tests

staabm · 2023-10-06T20:34:25Z

I have an idea how to fix the remaining problems, but just need a bit more time.
I will ping you when things are ready for another review

staabm · 2023-10-08T07:46:01Z

@ondrejmirtes I did a lot of debugging locally and have some changes planned, but I think the main problem I am facing atm is, that when the TypeSpecifier is invoked for the Isset_ expression, the falsey-scope no longer knows the maybe certainty of $a.

the scope in the falsey-branch already thinks $a has a yes certainty because of processing happing in NodeScopeResolver, before TypeSpecifier Isset_ processing kicks in

example:

use function PHPStan\Testing\assertVariableCertainty;

function maybeCertainNull(): void
{
	if (rand() % 2) {
		$a = ['bar' => null];
		if (rand() % 3) {
			$a = ['bar' => 'hello'];
		}
	}

	assertVariableCertainty(TrinaryLogic::createMaybe(), $a);
	if (isset($a['bar'])) {
		assertVariableCertainty(TrinaryLogic::createYes(), $a);
	} else {
		assertVariableCertainty(TrinaryLogic::createMaybe(), $a); // Expected variable certainty Maybe, actual: Yes 
	}

	assertVariableCertainty(TrinaryLogic::createMaybe(), $a);
}

thats a problem we have already before this PR, and I don't know whether fixing it here with all the other changes needed would be a good thing as this PR will be already pretty large without this topic.

do you have an idea how to fix that problem? I don't have time right now to also dive into this new side-quest.

staabm · 2023-10-08T09:35:59Z

after a break and a short walk, I realized how it could be fixed: see #2666

staabm · 2023-10-31T11:19:15Z

I feel I am close to something useful :-)

one TypeSpecifierTest is failling, because when merging left/right types the SpecifiedTypes intersection/union does not realize that __phpstanIssetExpr($array) and $array belong to the same expression and need to be merged

ondrejmirtes · 2023-10-31T13:33:26Z

I''d say that when you're creating new IssetExpr in TypeSpecifier, you also need to say at the same time, what it means for the expression, and specify its type too in the same operation.

ondrejmirtes · 2023-11-28T14:39:33Z

Maybe start a new PR instead of reviving this ancient one :)

staabm commented Sep 30, 2023

View reviewed changes

src/Analyser/MutatingScope.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Oct 1, 2023

View reviewed changes

staabm force-pushed the bug9908 branch from 9d22eea to ad8b8a9 Compare October 3, 2023 20:02

staabm commented Oct 3, 2023

View reviewed changes

tests/PHPStan/Analyser/data/falsy-isset.php Outdated Show resolved Hide resolved

tests/PHPStan/Analyser/data/falsy-isset.php Show resolved Hide resolved

clxmstaab force-pushed the bug9908 branch from 1d3b454 to b8fa971 Compare October 4, 2023 07:59

ondrejmirtes requested changes Oct 4, 2023

View reviewed changes

src/Analyser/ExpressionTypeHolder.php Outdated Show resolved Hide resolved

src/Analyser/MutatingScope.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Oct 4, 2023

View reviewed changes

staabm marked this pull request as ready for review October 4, 2023 16:17

staabm commented Oct 4, 2023

View reviewed changes

src/Analyser/TypeSpecifier.php Outdated Show resolved Hide resolved

staabm commented Oct 5, 2023

View reviewed changes

ondrejmirtes requested changes Oct 5, 2023

View reviewed changes

staabm force-pushed the bug9908 branch from 28744bd to 000e2d3 Compare October 5, 2023 20:09

staabm force-pushed the bug9908 branch from a592231 to 4ccec57 Compare October 6, 2023 18:08

staabm marked this pull request as draft October 6, 2023 20:34

staabm mentioned this pull request Oct 8, 2023

Fix variable certainty for ArrayDimFetch in falsey isset context #2666

Merged

staabm force-pushed the bug9908 branch 2 times, most recently from e32d61d to 139dbc2 Compare October 30, 2023 20:30

staabm added 4 commits October 31, 2023 12:00

Added regression test

9c1c44e

Fix !isset() with ArrayDimFetch

459fa95

fix test expectations

2f8eaa1

add falsy-isset tests

800a678

staabm and others added 16 commits October 31, 2023 12:00

Added regression test

6623061

Added regression test

6dfc1e3

Added regression test

aa6c58e

fix regression in bug-7224

8c73fb3

fix

08bb671

cs

f3672dd

fix collision

b0d8ee0

support regular variables

b7609eb

cs

270500e

fix

0665831

Added regression test

8ad159f

Added regression test

99580ee

Added regression test

700c744

fix

8c0eaf9

NotIssetExpr -> IssetExpr

4c4caab

move certainty into IssetExpr

2f75d65

staabm force-pushed the bug9908 branch from 2c7dc3d to 2f75d65 Compare October 31, 2023 11:01

This was referenced Nov 1, 2023

More precise falsey isset() #2708

Merged

Rewrite multi param isset() to and-chain #2709

Merged

staabm added 2 commits November 2, 2023 17:06

Discard changes to tests/PHPStan/Analyser/TypeSpecifierTest.php

9cfc551

Discard changes to src/Analyser/TypeSpecifier.php

2d0dbff

staabm mentioned this pull request Nov 2, 2023

Fix !isset() with Variable #2710

Merged

staabm added 3 commits November 28, 2023 15:24

Discard changes to src/Node/IssetExpr.php

f22efef

Discard changes to src/Node/Printer/Printer.php

0860ca3

Discard changes to src/Analyser/MutatingScope.php

389b651

staabm closed this Nov 28, 2023

staabm deleted the bug9908 branch November 28, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix !isset() with ArrayDimFetch #2657

Fix !isset() with ArrayDimFetch #2657

staabm commented Sep 30, 2023 •

edited

Loading

staabm commented Sep 30, 2023 •

edited

Loading

ondrejmirtes left a comment

ondrejmirtes left a comment

ondrejmirtes Oct 4, 2023

staabm Oct 4, 2023 •

edited

Loading

phpstan-bot commented Oct 4, 2023

staabm commented Oct 4, 2023 •

edited

Loading

staabm commented Oct 4, 2023

staabm Oct 5, 2023

staabm Oct 5, 2023

ondrejmirtes left a comment

ondrejmirtes Oct 5, 2023

staabm Oct 5, 2023

staabm Oct 5, 2023

staabm Oct 30, 2023

ondrejmirtes Oct 30, 2023

staabm commented Oct 5, 2023 •

edited

Loading

staabm commented Oct 6, 2023

staabm commented Oct 8, 2023 •

edited

Loading

staabm commented Oct 8, 2023

staabm commented Oct 31, 2023

ondrejmirtes commented Oct 31, 2023

ondrejmirtes commented Nov 28, 2023

Fix !isset() with ArrayDimFetch #2657

Fix !isset() with ArrayDimFetch #2657

Conversation

staabm commented Sep 30, 2023 • edited Loading

staabm commented Sep 30, 2023 • edited Loading

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

phpstan-bot commented Oct 4, 2023

staabm commented Oct 4, 2023 • edited Loading

staabm commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Oct 5, 2023 • edited Loading

staabm commented Oct 6, 2023

staabm commented Oct 8, 2023 • edited Loading

staabm commented Oct 8, 2023

staabm commented Oct 31, 2023

ondrejmirtes commented Oct 31, 2023

ondrejmirtes commented Nov 28, 2023

staabm commented Sep 30, 2023 •

edited

Loading

staabm commented Sep 30, 2023 •

edited

Loading

staabm Oct 4, 2023 •

edited

Loading

staabm commented Oct 4, 2023 •

edited

Loading

staabm commented Oct 5, 2023 •

edited

Loading

staabm commented Oct 8, 2023 •

edited

Loading