refactor $conditionalExpressions

[rajyan]

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

  phpstan-src/src/Analyser/MutatingScope.php

  Lines 4784 to 4786 in 0b3bfeb

  	if ($mergedVariableHolders[$name]->getType()->equals($holder->getType())) {
  	continue;
  	}

  
  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

phpstan-src/src/Analyser/MutatingScope.php

Lines 4737 to 4741 in 0b3bfeb

	foreach (array_keys($holders) as $key) {
	if (!array_key_exists($key, $otherHolders)) {
	continue 2;
	}
	}

[rajyan]

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]

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

[ondrejmirtes]

Thank you!

[ondrejmirtes]

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

1. 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.
2. 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]

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

[ondrejmirtes]

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]

Thank you! I look into it.