Add better propagation of truthy and falsey types

[jlherren]

No description provided.

[jlherren]

This now allows

if (!$something) { if ($something) { } }

to correctly report the inner condition having constant type, among many other situations.

I'm not 100% happy with the approach of representing truthy types via mixed~{all falsey types}, but it works for now. In general I found it a bit difficult to work with the current type system, where it is not possible to build arbitrary differences, for example it's not possible to represent that something is string but not 'foo'. Perhaps replacing SubtractableType (which works on only some classes) with a more generic DifferentType could be useful.

[ondrejmirtes]

Perhaps replacing SubtractableType (which works on only some classes) with a more generic DifferentType could be useful.

Yes! The main obstacle right now is that there are many instances of $foo instanceof *Type which would be broken by introducing a DifferentType like this.

We'd first have to gather all usecases why people do $foo instanceof *Type, express them as methods on Type interface, and then we'd be able to create a DifferentType.

I see this more of a PHPStan 2.0 thing rather than an immediate thing. I plan to release PHPStan 1.0 some time during winter 2020/21 that will be mostly the same from internals perspective as 0.12.x.

But we can gradually introduce new Type methods in minor versions as I did with Type::isArray() recently (that was very beneficial and helped remove a lot of bugs!), and in 2.0 we can make most instances of $foo instanceof *Type illegal through a custom rule, and take advantage of that.

Another feature that this will enable is an easy implementation of more complex type variable bounds like @template T of int|string. Right now we have specific TemplateType implementations like TemplateObjectType, but we could have a general one that would support all bounds.

[jlherren]

I noticed this PR causes some weird things to happen, for example:

// top-level scope where $a and $c are not defined
if ($a || $c) {
	if ($a) {
	}
}

Produces:

 ------ ----------------------------------- 
  Line   test.php                           
 ------ ----------------------------------- 
  3      Variable $a might not be defined.  
  3      Variable $c might not be defined.  
  4      If condition is always false.      
  4      Variable $a might not be defined.  
 ------ ----------------------------------- 

The third one is not desirable. It comes from the merging a scope where $a does not exist and a scope where $a is false (right side of ||). The type certainty is maybe, but that doesn't prevent it from being reported in the second if.

I tried something like the following, but that breaks at least some tests, so I'm not sure what's the best approach here.

diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php
index 9b28a1ce..17ebcb5f 100644
--- a/src/Analyser/NodeScopeResolver.php
+++ b/src/Analyser/NodeScopeResolver.php
@@ -1339,6 +1339,15 @@ class NodeScopeResolver
                        if ($expr->name instanceof Expr) {
                                return $this->processExprNode($expr->name, $scope, $nodeCallback, $context->enterDeep());
                        }
+                       if (is_string($expr->name)) {
+                               $hasVariable = $scope->hasVariableType($expr->name);
+                               // If variable does not exist, create it to avoid problems with merging scopes
+                               if ($hasVariable->no()) {
+                                       $scope = $scope->assignVariable($expr->name, new MixedType(), TrinaryLogic::createMaybe());
+                               } elseif ($hasVariable->maybe()) {
+                                       $scope = $scope->assignVariable($expr->name, $scope->getVariableType($expr->name), TrinaryLogic::createMaybe());
+                               }
+                       }
                } elseif ($expr instanceof Assign || $expr instanceof AssignRef) {
                        if (!$expr->var instanceof Array_ && !$expr->var instanceof List_) {
                                $result = $this->processAssignVar(

[ondrejmirtes]

I rebased it and made one small change: 0da3eb4

Thank you, I like it a lot!

[ondrejmirtes]

About #339 (comment) - I'll look into it on master.

[ondrejmirtes]

Yeah, so the problem was created because nonexistent variables in global scope always have certainty "maybe" and MutatingScope::getVariableType returns mixed for them. But for merging scopes, the mixed type wasn't there because the variable wasn't in the $ourVariableTypeHolders array. So the subtracted type won, but the variable should have stayed mixed.

The second array should also probably be iterated over and the variables missing in it should be added, but I wasn't able to write a failing test for that scenario, so maybe it's not going to be a problem.

Fixed: ebce7de

[ondrejmirtes]

This PR found a bug in Composer :) composer/composer#9277