refactor `$conditionalExpressions` by rajyan · Pull Request #1270 · phpstan/phpstan-src

rajyan · 2022-05-02T13:35:01Z

Working on phpstan/phpstan#3677 and half way done.

218bd36
If my understanding is correct, the logic in merge of conditionalExpressions in filterBySpecifiedTypes should be consistent with https://github.com/phpstan/phpstan-src/blob/0b3bfebebdeb3be2fbdcef5c2ba98a2885875228/src/Analyser/MutatingScope.php#L4784-L4786
so continue 2 should be continue. I'm not sure enough with the fix yet, but the fixed test looks better to me.
Also, this example (coalesce -> coalesce, coalesce -> ternary) is fixed with this change.

		if ($first || $second) {
			echo ($first ?? $second)->getValue();
			echo ($first ?? $second)->getValue();
		}
		if ($first || $second) {
			echo ($first ?? $second)->getValue();
			echo ($first ?: $second)->getValue();
		}

these cases are not fixed yet. (ternary -> ternary, ternary -> coalesce)

		if ($first || $second) {
			echo ($first ?: $second)->getValue();
			echo ($first ?? $second)->getValue();
		}
		if ($first || $second) {
			echo ($first ?: $second)->getValue();
			echo ($first ?: $second)->getValue();
		}

4b22f3c

While investigating this issue, I noticed some inconsistencies in keys of $conditionalExpressions.
I fixed to set key from $holder->getKey() like in

phpstan-src/src/Analyser/MutatingScope.php

Lines 4514 to 4515 in f8be122

    
           $holder = new ConditionalExpressionHolder($filteredConditions, $conditionalExpression->getTypeHolder()); 
        
           $newConditionalExpressions[$variableExprString][$holder->getKey()] = $holder;

, because it is used here https://github.com/phpstan/phpstan-src/blob/0b3bfebebdeb3be2fbdcef5c2ba98a2885875228/src/Analyser/MutatingScope.php#L4737-L4741

rajyan · 2022-05-02T13:39:30Z

tests/PHPStan/Rules/Properties/TypesAssignedToPropertiesRuleTest.php

This change makes sense to me. The error must be same with line 54.

phpstan-src/tests/PHPStan/Rules/Properties/data/properties-from-array-into-object.php

Lines 48 to 70 in d4b1163

public function create_simple_1(): self {

$self = new self();

$data = $this->data();

foreach($data as $property => $value) {

$self->{$property} = $value;

}

return $self;

}

public function create_complex(): self {

$self = new self();

foreach($this->data() as $property => $value) {

if ($property === 'test') {

if ($self->{$property} === null) {

$self->{$property} = new \stdClass();

}

} else {

$self->{$property} = $value;

}

rajyan · 2022-05-02T15:44:50Z

Checked for the failing tests and I think it's unrelated.

ondrejmirtes · 2022-05-02T16:06:28Z

Thank you!

ondrejmirtes · 2022-05-02T16:12:29Z

@rajyan I think there's opportunity for refactoring, most likely in multiple steps:

Instead of MutatingScope::$variableTypes and MutatingScope::$moreSpecificTypes, there should be only one thing - MutatingScope::$expressionTypes. But of course variable-specific methods like getVariableType() should be preserved.
I think there's a chance we don't need MutatingScope::$conditionalExpressions at all. Instead of ConditionalExpressionHolder, we could have something similar to PHPStan\Type\ConditionalType right inside MutatingScope::$expressionTypes that would resolve lazily when calling MutatingScope::getType(). One nice thing it'd solve is that right now ConditionalExpressionHolder is used only for variables on both sides - if $a is X, then $b is Y. But it could work for any expression.

There are also other issues in https://github.com/phpstan/phpstan/milestone/12 (potentially very hard to solve) that you might tackle (if you feel like it) after 1) and 2) :)

Thanks for considering!

rajyan · 2022-05-02T16:19:12Z

Thank you for your pointers.
I'm getting to understand these concepts and I think I can look for improvements:+1:

ondrejmirtes · 2022-05-04T15:44:54Z

FYI @rajyan I noticed there are some commented-out test cases in

phpstan-src/tests/PHPStan/Analyser/data/dependent-variable-certainty.php

Lines 261 to 480 in 9f1e793

    
           /*function (): void { 
        
           	$from = null; 
        
           	$to = null; 
        
           	if (rand(0, 1)) { 
        
           		$from = new \stdClass(); 
        
           		$to = new \stdClass(); 
        
           	} 
        
           	if (rand(0, 1)) { 
        
           		$from = new \stdClass(); 
        
           		$to = new \stdClass(); 
        
           	} 
        
           	if ($from !== null) { 
        
           		assertType('stdClass', $to); 
        
           	} 
        
           }; 
        
           function (bool $b): void { 
        
           	$from = null; 
        
           	$to = null; 
        
           	if ($b) { 
        
           		$from = new \stdClass(); 
        
           		$to = new \stdClass(); 
        
           	} 
        
           	if ($from !== null) { 
        
           		assertType('true', $b); 
        
           		assertType('stdClass', $from); 
        
           		assertType('stdClass', $to); 
        
           	} 
        
           }; 
        
           function (bool $b): void { 
        
           	$from = null; 
        
           	$to = null; 
        
           	if ($b) { 
        
           		$from = new \stdClass(); 
        
           		$to = new \stdClass(); 
        
           	} 
        
           	if ($b) { 
        
           		assertType('true', $b); 
        
           		assertType('stdClass', $from); 
        
           		assertType('stdClass', $to); 
        
           	} 
        
           }; 
        
           function (bool $b, bool $c): void { 
        
           	if ($b) { 
        
           		if ($c) { 
        
           			$foo = 'bla'; 
        
           		} 
        
           	} 
        
           	assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           	if ($b) { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		if ($c) { 
        
           			assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} 
        
           	} else { 
        
           		assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		if (!$c) { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} 
        
           	} 
        
           	if (!$b && !$c) { 
        
           		assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           	} 
        
           }; 
        
           function (bool $b, bool $c): void { 
        
           	if ($b && $c) { 
        
           		$foo = 'bla'; 
        
           	} 
        
           	assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           	if ($b) { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		if ($c) { 
        
           			assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} 
        
           	} else { 
        
           		assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		if (!$c) { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} 
        
           	} 
        
           	if (!$b && !$c) { 
        
           		assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           	} 
        
           }; 
        
           function (bool $b, bool $c, bool $d): void { 
        
           	if ($b) { 
        
           		if ($c) { 
        
           			$foo = 'bla'; 
        
           		} 
        
           	} 
        
           	if ($d) { 
        
           		$foo = 'ble'; 
        
           	} 
        
           	assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           	assertType('\'bla\'|\'ble\'', $foo); 
        
           	if ($b) { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		assertType('\'bla\'|\'ble\'', $foo); 
        
           		if ($c) { 
        
           			assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           			assertType('\'bla\'|\'ble\'', $foo); 
        
           			if (!$d) { 
        
           				assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           				assertType('\'bla\'', $foo); 
        
           			} else { 
        
           				assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           				assertType('\'ble\'', $foo); 
        
           			} 
        
           		} 
        
           	} else { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		assertType('\'ble\'', $foo); 
        
           		if (!$c) { 
        
           			assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           			assertType('\'ble\'', $foo); 
        
           			if (!$d) { 
        
           				assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           				assertType('*ERROR*', $foo); 
        
           			} 
        
           		} 
        
           	} 
        
           	if (!$b && !$c) { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		assertType('\'ble\'', $foo); 
        
           		if (!$d) { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           			assertType('*ERROR*', $foo); 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           			assertType('\'ble\'', $foo); 
        
           		} 
        
           	} 
        
           	if ($d) { 
        
           		assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           		assertType('\'bla\'|\'ble\'', $foo); 
        
           	} else { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		assertType('\'bla\'', $foo); 
        
           		if (!$b && !$c) { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           			assertType('*ERROR*', $foo); 
        
           		} 
        
           	} 
        
           }; 
        
           function (bool $b, bool $c, bool $d): void { 
        
           	if ($d) { 
        
           		$foo = 'ble'; 
        
           	} 
        
           	if ($b) { 
        
           		if ($c) { 
        
           			$foo = 'bla'; 
        
           		} 
        
           	} 
        
           	assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           	assertType('\'bla\'|\'ble\'', $foo); 
        
           	if ($b) { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		assertType('\'bla\'|\'ble\'', $foo); 
        
           		if ($c) { 
        
           			assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           			assertType('\'bla\'', $foo); 
        
           			if (!$d) { 
        
           				assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           				assertType('\'bla\'', $foo); 
        
           			} else { 
        
           				assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           				assertType('\'bla\'', $foo); 
        
           			} 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           			assertType('\'ble\'', $foo); 
        
           			if ($d) { 
        
           				assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           				assertType('\'ble\'', $foo); 
        
           			} else { 
        
           				assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           			} 
        
           		} 
        
           	} else { 
        
           		assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); 
        
           		assertType('\'ble\'', $foo); 
        
           		if ($d) { 
        
           			assertVariableCertainty(TrinaryLogic::createYes(), $foo); 
        
           			assertType('\'ble\'', $foo); 
        
           		} else { 
        
           			assertVariableCertainty(TrinaryLogic::createNo(), $foo); 
        
           		} 
        
           	} 
        
           };*/

that we should make work too as part of this feature :)

rajyan · 2022-05-05T23:57:13Z

Thank you! I look into it.

rajyan commented May 2, 2022

View reviewed changes

rajyan marked this pull request as draft May 2, 2022 13:42

rajyan added 2 commits May 2, 2022 22:57

fix

218bd36

make $conditionalExpressions key consistent in all usages

4b22f3c

rajyan force-pushed the fix/issue-3677 branch from 6df52d2 to 4b22f3c Compare May 2, 2022 13:58

rajyan marked this pull request as ready for review May 2, 2022 15:43

ondrejmirtes merged commit 1e95392 into phpstan:1.6.x May 2, 2022

rajyan deleted the fix/issue-3677 branch May 2, 2022 16:14

rajyan mentioned this pull request May 20, 2022

add failing tests #1330

Closed

rajyan mentioned this pull request Oct 22, 2022

Native types todos phpstan/phpstan#8191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor `$conditionalExpressions`#1270

refactor `$conditionalExpressions`#1270
ondrejmirtes merged 2 commits intophpstan:1.6.xfrom
rajyan:fix/issue-3677

rajyan commented May 2, 2022 •

edited

Loading

Uh oh!

rajyan May 2, 2022

Uh oh!

rajyan commented May 2, 2022

Uh oh!

ondrejmirtes commented May 2, 2022

Uh oh!

ondrejmirtes commented May 2, 2022

Uh oh!

rajyan commented May 2, 2022 •

edited

Loading

Uh oh!

ondrejmirtes commented May 4, 2022

Uh oh!

rajyan commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	$holder = new ConditionalExpressionHolder($filteredConditions, $conditionalExpression->getTypeHolder());
	$newConditionalExpressions[$variableExprString][$holder->getKey()] = $holder;

	public function create_simple_1(): self {
	$self = new self();

	$data = $this->data();

	foreach($data as $property => $value) {
	$self->{$property} = $value;
	}

	return $self;
	}

	public function create_complex(): self {
	$self = new self();

	foreach($this->data() as $property => $value) {
	if ($property === 'test') {
	if ($self->{$property} === null) {
	$self->{$property} = new \stdClass();
	}
	} else {
	$self->{$property} = $value;
	}

Conversation

rajyan commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajyan May 2, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan commented May 2, 2022

Uh oh!

ondrejmirtes commented May 2, 2022

Uh oh!

ondrejmirtes commented May 2, 2022

Uh oh!

rajyan commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented May 4, 2022

Uh oh!

rajyan commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rajyan commented May 2, 2022 •

edited

Loading

rajyan commented May 2, 2022 •

edited

Loading