WIP: Ternary Expression TypeNarrower for Nullables #14

mhsdesign · 2023-04-09T14:41:37Z

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

- 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, by looking into its members

`nullableString ? nullableString : "fallback"` will be a string

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.

src/TypeSystem/Type/UnionType/UnionType.php

grebaldi · 2023-04-20T14:06:19Z

src/TypeSystem/Resolver/TernaryOperation/TernaryOperationTypeResolver.php

+        if ($conditionNode->root instanceof IdentifierNode && $rootType instanceof UnionType && $rootType->isNullable()) {
+            $trueExpressionTypeResolver = new ExpressionTypeResolver(
+                scope: new ShallowScope(
+                    $conditionNode->root->value,
+                    $rootType->withoutNullable(),
+                    $this->scope
+                )
+            );
+
+            $falseExpressionTypeResolver = new ExpressionTypeResolver(
+                scope: new ShallowScope(
+                    $conditionNode->root->value,
+                    NullType::get(),
+                    $this->scope
+                )
+            );
+
+            return UnionType::of(
+                $trueExpressionTypeResolver->resolveTypeOf($ternaryOperationNode->true),
+                $falseExpressionTypeResolver->resolveTypeOf($ternaryOperationNode->false)
+            );
+        }
+


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:

The ternary condition must consist of exactly one identifier

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.

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!!!

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.

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

…oleans around > "it must be true though" https://www.youtube.com/watch?v=7EmboKQH8lM&t=4407s

This might be error-prone but why not ^^

…vanced type inference

…rerContext

…s indeed associative (0 cost in production) Sadly we cant annotate the param in the constructor to tell psalm, that we only want to accept string keys. At least i have found no way

…ndling

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! 👍

src/TypeSystem/Type/UnionType/UnionType.php

src/TypeSystem/Inferrer/InferredTypes.php

grebaldi · 2023-04-24T15:09:36Z

src/TypeSystem/Inferrer/InferredTypes.php

+
+use PackageFactory\ComponentEngine\TypeSystem\TypeInterface;
+
+class InferredTypes


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.

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

src/TypeSystem/Inferrer/InferredTypes.php

src/TypeSystem/Inferrer/TypeInferrer.php

src/TypeSystem/Inferrer/TypeInferrerContext.php

src/TypeSystem/Scope/TernaryBranchScope/TernaryBranchScope.php

mhsdesign · 2023-04-25T07:33:32Z

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.

…rom code review

… to null

@fcool

remove Truthiness from api thanks @fcool for the discussion

grebaldi · 2023-04-30T14:19:21Z

src/TypeSystem/Scope/TernaryBranchScope/TernaryBranchScope.php

 use PackageFactory\ComponentEngine\TypeSystem\ScopeInterface;
 use PackageFactory\ComponentEngine\TypeSystem\TypeInterface;

 final class TernaryBranchScope implements ScopeInterface
 {
-    private function __construct(
+    public function __construct(


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 …

But you've made it public 🧐

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

mhsdesign · 2023-05-13T09:21:16Z

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.

TASK: Put domain logic inside constructor of UnionType

7efcfd3

mhsdesign force-pushed the task/simpleNullableHandling branch from 3074b07 to ebb28e6 Compare April 9, 2023 14:47

TASK: Simple Nullable Handling

2ffe099

- 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, by looking into its members

mhsdesign force-pushed the task/simpleNullableHandling branch from ebb28e6 to 2ffe099 Compare April 9, 2023 14:54

TASK: Infer types in arms of ternary

6293145

`nullableString ? nullableString : "fallback"` will be a string

mhsdesign force-pushed the task/simpleNullableHandling branch from e0a9241 to 6293145 Compare April 9, 2023 15:37

mhsdesign changed the title ~~TASK: Simple Nullable Handling~~ TASK: Nullable Type Handling Apr 9, 2023

mhsdesign added 3 commits April 9, 2023 20:31

TASK: Revert 7efcfd3

2a08474

TASK: Union test that all members are deduplicated

5e4d6ce

TASK: Union test isNullable and withoutNullable

5c08a4e

grebaldi requested changes Apr 20, 2023

View reviewed changes

mhsdesign added 10 commits April 21, 2023 20:31

TASK: UnionType rename to containsNull, withoutNull

8f0ba11

TASK: Introduce TernaryBranchScope

8a34ee1

TASK: Type inference for null comparison in ternary

1b476e5

TASK: UnionType RequiresAtLeastOneMember

57ee20c

TASK: TernaryBranchScope introduce static factories and dont throw bo…

3487bd9

…oleans around > "it must be true though" https://www.youtube.com/watch?v=7EmboKQH8lM&t=4407s

TASK: Make type inference in TernaryBranchScope more explicit

1348ac1

TASK: Adjust naming of $nonNullable to $typeWithoutNull

492e05c

TASK: Solve #7 rudimentary

0c7061f

This might be error-prone but why not ^^

TASK: Introduce TypeInferrer inspired by phpstan to support future ad…

22df4ba

…vanced type inference

TASK: Cleanup InferredTypes and extract duplicated logic to TypeInfer…

0adb4c0

…rerContext

mhsdesign force-pushed the task/simpleNullableHandling branch from 0fef930 to 0adb4c0 Compare April 23, 2023 09:16

mhsdesign added 2 commits April 23, 2023 11:32

TASK: Remove @phpstan-ignore-next-line by asserting that an array i…

f61b896

…s indeed associative (0 cost in production) Sadly we cant annotate the param in the constructor to tell psalm, that we only want to accept string keys. At least i have found no way

Merge remote-tracking branch 'origin/main' into task/simpleNullableHa…

6a8bd00

…ndling

grebaldi requested changes Apr 24, 2023

View reviewed changes

mhsdesign added 4 commits April 26, 2023 07:49

TASK: UnionType::getIterator use yield from

d835fa5

TASK: Rename Inferrer to Narrower and apply further suggestions f…

88c6fd6

…rom code review

TASK: Narrower handle boolean literal comparisons

e0a913a

TASK: Narrower null comparison against any expression that resolves…

aaf6c49

… to null

TASK: Add ExpressionTypeNarrowerTest

560c97d

mhsdesign force-pushed the task/simpleNullableHandling branch from 138e5b5 to 560c97d Compare April 26, 2023 07:15

mhsdesign added 3 commits April 26, 2023 09:48

TASK: Don't narrow nullableString === true as string

d00a194

TASK: Narrow nullableString && true

530b155

TASK: Correct namespace

331cdda

mhsdesign force-pushed the task/simpleNullableHandling branch from 414ae16 to 331cdda Compare April 26, 2023 08:09

mhsdesign mentioned this pull request Apr 26, 2023

TASK Binary expression ast with only two operands: left and right #16

Merged

mhsdesign added 4 commits April 29, 2023 18:17

Merge remote-tracking branch 'origin' into task/simpleNullableHandling

bd28d87

TASK: Adjust to BinaryOperationNode api change

1027e16

TASK: Apply suggestions from code review

884b895

remove Truthiness from api thanks @fcool for the discussion

TASK: ExpressionTypeNarrower support UnaryOperationNode

0121247

grebaldi reviewed Apr 30, 2023

View reviewed changes

mhsdesign changed the title ~~TASK: Nullable Type Handling~~ WIP: Ternary Expression TypeNarrower for Nullables May 13, 2023

mhsdesign marked this pull request as draft May 13, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Ternary Expression TypeNarrower for Nullables #14

WIP: Ternary Expression TypeNarrower for Nullables #14

mhsdesign commented Apr 9, 2023 •

edited

Loading

grebaldi left a comment

grebaldi Apr 20, 2023

mhsdesign Apr 21, 2023

mhsdesign Apr 22, 2023 •

edited

Loading

mhsdesign Apr 22, 2023 •

edited

Loading

grebaldi left a comment

grebaldi Apr 24, 2023

mhsdesign Apr 29, 2023

mhsdesign commented Apr 25, 2023

grebaldi Apr 30, 2023

mhsdesign Apr 30, 2023

grebaldi Apr 30, 2023

mhsdesign Apr 30, 2023

mhsdesign commented May 13, 2023 •

edited

Loading


		use PackageFactory\ComponentEngine\TypeSystem\TypeInterface;

		class InferredTypes

WIP: Ternary Expression TypeNarrower for Nullables #14

Are you sure you want to change the base?

WIP: Ternary Expression TypeNarrower for Nullables #14

Conversation

mhsdesign commented Apr 9, 2023 • edited Loading

grebaldi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign Apr 22, 2023 • edited Loading

Choose a reason for hiding this comment

mhsdesign Apr 22, 2023 • edited Loading

Choose a reason for hiding this comment

grebaldi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Apr 25, 2023

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

mhsdesign commented May 13, 2023 • edited Loading

mhsdesign commented Apr 9, 2023 •

edited

Loading

mhsdesign Apr 22, 2023 •

edited

Loading

mhsdesign Apr 22, 2023 •

edited

Loading

mhsdesign commented May 13, 2023 •

edited

Loading