WIP: Ternary Expression TypeNarrower for Nullables

[mhsdesign]

• Scopes will check if $typeReferenceNode->isOptional and if so, wrap the type in a union with null

• The TypeReferenceTranspiler can now transpile those simple unions

• Infer types in arms of ternary
  nullableString ? nullableString : "fallback"
  will be a string when resolving :D

[grebaldi]

Hi @mhsdesign,

thanks so much for your contribution! 💯

While reading the code, I've stumpled upon a conceptual problem that turned out to be a magnificent opportunity for improvement, so I'd like to leave this as a change request.

[grebaldi]

It pains me to object to this, because I really, really like the idea. Yet, I think the cost of introducing ShallowScope for this is too high.

My reasoning here goes as follows:

The problem that the code above attempts to solve is that of deduction of a non-nullable subtype from a nullable union type given its falsiness has been determined for a ternary expression branch. The conditions in line 57 further narrow the specificity of the problem thusly:

1. The ternary condition must consist of exactly one identifier
2. The type of the ternary condition must be a nullable union-type

I would say that this is a very specific case. (Indeed a tad too specific, but more on that later)

Now, to make the deduction visible to the ternary branches, their respective type resolvers are instantiated each with a ShallowScope instance that overrides the type that the identifier in the ternary condition points to.

I would say that ShallowScope is a very general abstraction.

The point of the scope-system is to give total control over the mode of type resolution to specific language constructs that in the mental model of the reader/writer intuitively invoke the notion of "scope". Conceptually, I would say this prohibits the existence of more abstract scope implementations that would have no correspondence to said mental model.

Fortunately, this could be remedied rather easily, by renaming ShallowScope to TernaryBranchScope. (Since in the above scenario ternary branches would carry deduced type information, I find it absolutely fair to say that there is such a thing as a TernaryBranchScope)

This leaves us with the problem of specificity introduced by the condition in line 57. Take these two examples:

(1)

component Example {
  a: ?string

  return a ? "a is not null" : "a is null"
}

(2)

component Example {
  a: ?string
  b: boolean

  return a && b ? "a is not null" : "a may yet be null"
}

In both cases, reader's intuition would be able to tell that a is not null in the true branch. But due to the limitation through the condition in line 57, the type system would acknowledge this in case (1) but not in case (2). I would argue that this runs counter reader's intuition, who would expect this behavior to be consistent.

The more I think about it, the more this deduction problem sounds like a job for the aforementioned TernaryBranchScope :)

TernaryBranchScope may very well be aware of the entire expression of the ternary condition. Then if asked for the type of an identifier, it could then and there deduce a narrower type.

So, to conclude this: I'd like to suggest the introduction of TernaryBranchScope (as a replacement for ShallowScope) in the scope of this PR. It should take on the task of deduction as I've described it, but for now it would be fine to keep the current limitations on that algorithm and leave a TODO-comment, so it may be refined later.

[mhsdesign]

Implemented :D But now im too tired to type things down that i encountered, see you tomorrow.

(And thanks for the tipp with TernaryBranchScope works like a charm and even better ^^)

Also i cant thank you enough for your most precise code reviews and comments - they are extremely helpful and well guiding me! Thank you really so much!!!

[mhsdesign]

yes youre right, my implementation is only limited for super simple things like foo ? foo : ""

i thought about how we can i implement a universal solution, which also handles any thinkable more complex case, but it seems like a hard challenge

for example given foo is a nullable string.

those expressions will be resolved as of now correctly:

foo ? foo : ""
foo !== null ? foo : ""

but here we loose it:

(foo) ? foo : ""
((foo)) ? foo : ""
foo !== null === true ? foo : ""
!foo ? "a" : "b"
!!foo ? "a" : "b"
foo !== nullVariable ? foo : ""
(foo !== null) ? foo : ""
foo && true ? foo : ""
(foo ? foo : foo) ? foo : ""
((foo ? foo : foo) ? foo : foo) ? foo : ""

I checked all the examples with typescript, and it does a good job (unsurprisingly). I even went as far as trying to have a look at the ts codebase for some inspiration, but i couldnt even find the right unit tests to start with xD

Some of the above are surely not a real world use case but its a simple demonstration how easy it is to confuse the implemented type resolution (or type inference?).

I think i need to find out while resolving the condition part, which variable participated and how it affected the truthyness. Im not really sure if this would be the right approach in the long run and if it can be easily implemented.

But for now im also quite happy with my current implementation as a start.

[mhsdesign]

Found the place where phpstan does it:
https://github.com/phpstan/phpstan-src/blob/07bb4aa2d5e39dafa78f56c5df132c763c2d1b67/src/Analyser/MutatingScope.php#L1616

// $node is a TernaryExpression with the fields: cond, if and else

$booleanConditionType = $this->getType($node->cond)->toBoolean();
if ($booleanConditionType->isTrue()->yes()) {
     return $this->filterByTruthyValue($node->cond)->getType($node->if);
}

if ($booleanConditionType->isFalse()->yes()) {
    return $this->filterByFalseyValue($node->cond)->getType($node->else);
}

return TypeCombinator::union(
     $this->filterByTruthyValue($node->cond)->getType($node->if),
     $this->filterByFalseyValue($node->cond)->getType($node->else),
);

interestingly phpstan's code is quite clever but not clever to not fail for the bait (foo ? foo : foo) ? foo : "":
https://phpstan.org/r/3b012ae8-4dd9-4901-95c0-45587f70134e

also psalm fails for the bait
https://psalm.dev/r/c536f4dab4

but psalm correctly works with your example:

/** @var bool $bool */
$bool = true;
$bar = $foo && $bool ? PHPStan\Testing\assertType("non-falsy-string", $foo) : PHPStan\Testing\assertType("string|null", $foo);

and i couldnt find another way (except the ternary thing) to confuse the type resolution - its really good.

---

with 22df4ba

i kind of stole the basic idea and structure of phpstan and it should be capable to implement in the future more complex inference - but first i need to build support for negation !foo

---

it came just to my mind that it will be fun to implement type inference for nullable struct members ... foo.bar ? 1 : 0 or even foo.bar?.buz ? 1 : 0

[grebaldi]

Hi @mhsdesign,

thanks so much for your adjustments. I'm sorry for the wave of comments I'm still leaving on this 😅 I have to admit, that I have trouble articulating my thoughts on this entire complex, but I hope I'm making sense in the comments.

Thanks again! 👍

[grebaldi]

Suggested change

	class InferredTypes
	class TypeMap

It might be sensible to move this class to a namespace PackageFactory\ComponentEngine\TypeSystem\Utility or so. It may have more uses than just this one.

[mhsdesign]

Still not sure ;) maybe another day ^^ It will fall all into place sometime...

[mhsdesign]

Hi @grebaldi ;)

No problem at all. Im happy for your feedback :D

Before addressing your comments id like to merge #16 and #19 as they both will affect this pr as well.
With those changes i want to try implementing a narrower for:

!foo ? 1 : 0 and !!foo ? 1 : 0
and
foo === null === true ? 1 : 0

that way i can see if my the approach is flexible enough to allow even more advanced narrowing.

Later my goal is also having a narrower for foo.bar ? 1 : 0... lets see.

[grebaldi]

Why that?

[mhsdesign]

Because one should use the static factories - but I guess this will make unit testing harder … for now fine imo? I like having private constructors at times …

[grebaldi]

But you've made it public 🧐

[mhsdesign]

Misread the diff 😂 its the 4 day in berlin 😅😂

[mhsdesign]

Btw @grebaldi, as discussed TypeChecking is for now mostly a tool for helping with the transpilation. You said that at the current stage we dont want and need 100% accuracy. So for the better of the project it might make sense to not merge this for now (even if i am happy with the code) because it increases the complexity quite a bit (adding the new TypeNarrower concept).

It was definitely fun implementing it so no bad blood ;)

I will extract soonish the non TypeNarrower related concepts into another PR and leave this WIP.