Make Flow Types fully compatible with Static Analysis Tools

[norberttech]

Flow comes with it's own internal type system:

• type_float()
• type_string()
• etc

The challange is that due to nullability being defined on a type level tools like phpstan can't properly narrow the type in methods like isValid(mixed $value) : bool
like it does for tools like webmozart/assert for example.

The goal is to also introduce Type::assert(mixed $value) : mixed so when we use it like this:

$floatValue = type_float()->assert($value); 
// $floatValue type is float, not float|null

the $floatValue is recognized as a proper float

Additionally we also want to get the same behavior for

$floatValue = caster()->to(type_float()->value($value);
// $floatValue type is float, not float|null

[norberttech]

PHPStan

Warning

My understanding of PHPStan internals is very limited, it was basically my first attempt to understand how to build a proper extensions. If I'm missing something, please let me know!

The first attempt to implement types narrowing was to use phpstan Custom Type Checking built in mechanism.

But it didn't work, PHPStan Playground, explanation below:

So then thought that maybe I can make it work through a custom extension which almost worked...

In theory we should be able to use:

• DynamicMethodReturnTypeExtension for Type::assert(mixed $value) : mixed`
• MethodTypeSpecifyingExtension for Type::isValid(mixed $value) : bool

And it works for scenarios like:

type_float(true)->assert($value); 

it works because in PHPStan extension context we can easily get from assert($value) to type_float(true) understanding that this particular float might be nullable so if $value type might be null we should keep it nullable, but for type_float(false) we can for sure say it wont be null and we can just take the type from type_float template implementation.

But nothing is simple...
In the following example Type is just an function argument and tracing it back is challenging (or maybe I just don't understand static analysis good enough)

function printFloat(float $value) : void
{
    echo "Float: " . $value . "\n";
}

function printString(string $value) : void
{
    echo "String: " . $value . "\n";
}

function randomFloat() : float|string
{
    return random_int(0, 1) ? 12.425 : '12.425';
}

$float = randomFloat();

if (type_float()->isValid($float)) {
    printFloat($float);
} else {
    printString($float);
}

Long story short, that $nullable property of Type is making it super pretty hard to implement a proper type narrowing :|

This might be an indication of a code smell and that putting nullability as a property of types is actually not the best design.

[norberttech]

Simplify Flow Types

Usually when things are getting to complicated it's a sign that there is something wrong with the design. One way of reducing the complexity of static analysis is to introduce a proper UnionType.

This is how libraries like azjezz/psl handle assert and coerce methods.

When as a user you expect to have a nullable float you simply create a union type:

union(float(), null())->assert().

Can Flow get UnionType?

The first problem that Flow has is that Type is used in Entry. So for example StringEntry will return StringType instance.

But when entry is created from null type has $nullable property set to true.

Then that type goes to Schema\Definition (that pretty much represents the column in a row).

To understand the problem we need to understand how schema is created.
Lets take a look at following example:

rows(
   row(int_entry('id', 1)
   row(int_entry('id', null)
)

We pretty much have here 2 rows with following types:

index	type	value
0	type_integer(nullable: false)	1
1	type_integer(nullable: true)	null

So the schema for those two rows is going to look like this:

schema(
  int_schema('id', nullable: true, metadata: null)
)

Schema of a dataset is nothing else then a merged schema of each Row in that dataset.
Why do we need to know on a schema definition level is entry accepts nulls?

Schema Validation needs it - if we define schema like this:

schema(
  int_schema('id', nullable: false, metdata: null)
)

Then flow needs to guarantee that not a single row has null in column id.

Move nullability to Schema\Definition

In theory we can move definition of nullability to schema definition.
This way we should be able to remove the $nullable property and nullable() : bool getter from the Type

Each entry would than have a Type and only for Entry::definition() : Definition it would need to pass $nullable = true when value of that entry is null.

This solves one problem but now we are getting to something more problematic. Logical Array Based Types.

Logical Array Based Types

Removing nullability from Type creates another complexity in types like List/Map/Structure

Just as a reminder:

• List - is a collection of elements of the same type - [1 , 2, 3, 4]
• Map - is a collection of key/value elements with a same type of Key and Value where Key can be only string/integer and value anything where each key is unique. - ['a' => 1, 'b' => 2]
• Structure - is an associative array of keys and values where each key needs to be unique and it must be string. - ['id' => 1, 'name' => 'Norbert', 'active' => true]

Now the tricky part is that each of the types inside of those types can be nullable...

For example list of nullable integers list<?int>

type_list(type_int(nullable: true), nullable: false)` 

that accepts values like [1, null, 2, null, 3, 4, 5]

Ok, but how that's problematic? It would just be an union type.

Data Storages

Most efficient data storages does not support union types as under the hood they store the data in binary format.

We don't have union types in databases, we dont have them in parquet etc.

This is the first complexity. The example of list<?int> from above with union types should look like this:

type_list(type_union(type_int(), type_null())) 

And in this case it's fine. But following example is already problematic:

type_list(type_union(type_int(), type_null(), type_string())) 

Technically speaking it's a correct (from a PHP perspective) construct, list<null|int|string> which on the other hand is not supported by any of the data storaegs we are dealing with.

Can't we just forbid union types with more than 2 types?

Well, yes but also no.

If the only purpose of that type system would be to support DataFrame then yes, we can easily constraint the union type to not accept union type on left/right type.

However it sounds a bit limiting and pretty much defeats the whole purpose of having union types in the first place.

Ideally I would like to extract Flow Types to a standalone package in order to be able to use it anywhere (even without flow) as an lighter alternative to azjezz/psl (which I will not make a dependency of Flow since it's simply too big).

One thing that comes to my mind is adding two types:

• union() a regular union type that can be built from many types (including union types)
• nullable() a decorator for any type except union and null - it would be pretty much a logical type based on union type. (In Flow vocabulary types like String or Object are Native but types like Json or DateTime are Logical since they expands the Native types)

The problem is that I can't limit type_list to accept only types that are not union.

One solution that comes to my mind is to prepare a TypeCompactor/TypeMerger that would be dedicated for Flow DataFrame and that would detect those kind of problematic situations that would result in creating a union type and throw an Exception.

That exception would be avoidable through:

• providing schema definition to extractor that will narrow types automatically based on the definitions
• explicit type casting in pipeline

This is pretty much what I figured out so far but I'm open for any ideas 🙏

[norberttech]

Type Interface

Currently type interface is a bit too much coupled with DataFrame. Now is the time to decouple them as DataFrame should just use Types.

There are few mechanisms that used Type::isCompatible/merge/isEqual/isSame - all of that logic should be moved to DataFrame specific classes.

For example Type::merge was designed specifically for DataFrame when in reality merge of different types should simply return union type instead of throwing exception (which is dataframe related behavior). This logic should be moved to DataFrame TypeCompactor class instead.

New Type Interface should be as slim as possible:

<?php

declare(strict_types=1);

namespace Flow\ETL\PHP\Type;

use Flow\ETL\Exception\InvalidTypeException;

/**
 * @template-covariant T of mixed
 */
interface Type
{
    /**
     * @throws InvalidTypeException
     *
     * @return T
     */
    public function assert(mixed $value) : mixed;

    /**
     * @phpstan-assert-if-true T $value
     */
    public function isValid(mixed $value) : bool;

    public function normalize() : array;

    public function toString() : string;
}

[norberttech]

We can also remove method Type::is(Type $type) : bool and Type::fromArray(array $data) : self as it was mostly used by internal Flow mechanisms

    /**
     * @return Type<T>
     */
    public static function fromArray(array $data) : self;

    /**
     * Checks if another type is the same as this type.
     *
     * @param Type<mixed> $type
     */
    public function is(self $type) : bool;

[norberttech]

Another thing I'm now noticing is that we can also remove float precision.
It was introduced in order to simplify mappings between different storages but with that new redesigned type system we are going to be able to add also:

• float8
• float32
• float64

Precision is going to be controlled through round() scalar function that takes precision as a second argument, other than Flow is not going to attempt to modify the precision of the floats.

Additionally we are also going to be able to introduce new Double type which I already took attempt in the past to implement but failed due to lack of #1331 which now is redundant due to integration with brick/math #1584

[norberttech]

After removing precision from the Floats we can pretty much deprecate Flow\ETL\PHP\Type\Caster and move all casting logic from Handlers into each type cast method:

<?php

declare(strict_types=1);

namespace Flow\ETL\PHP\Type;

use Flow\ETL\Exception\InvalidTypeException;
use Flow\ETL\Exception\CastingException;

/**
 * @template-covariant T of mixed
 */
interface Type
{
    /**
     * @param mixed value
     *
     * @return T
     * @throws CastingException
     */
    public function cast(mixed $value) : mixed;
}

This is the simplest way to convert one type into another and to make sure that the same time that static analysis sees given variable as a given type.

For example

$limit = type_integer()->cast($input->get('limit')); 

In this case we not only make sure that non numeric values that can't be casted to integer are going to throw exception (like null) but also that later if this value is passed to a function/method expecting integer that this will be a proper integer.

[andrewprofile]

Why does assert return a value?

In general, there are two errors(in strict mode):

Analyzing your example, I think it can be solved in many ways, but let's simplify it to begin with, so that it's easier to understand where the problem is: https://3v4l.org/Q3k2c

Quoting the documentation:

During the analysis of the method, PHPStan doesn’t understand that the type of was narrowed to because it doesn’t descend to called functions and symbols, it just reads their typehints and PHPDocs.

Static analysis stops at this point because you're trying to cast a null to a float type, and PHP can't do that, that's why I've simplified this example a bit, which generally boils down to the fact that this code won't always work correctly, just because, that you added there this if $float->nullable() === false (by the way, then we still have there a false-positive error:

),
this code for nullable will work because it simply won't get called, but it is enough that someone forgets about this if and throws an error from this simplified example(this is the case caught by static analysis, because it only checked the typehint). The problem here is not the argument of the assert function but what it returns, you have some obfuscation there because you have ?float, and it's actually a float|null union, but this union is only right when FloatType allows nullable and then this error in your example looks like false-positive, but in this case this function should return only float, not union float|null, thats why you can't really make this class so universal with one method that handles both cases. You need to have either two methods assert and assertOrNull, and for that you need two functions printFloat and printFloatOrNull, or simply handle both cases printFloatOrNull(float|null $val), because the assert method as it stands can always be float or null, which is why PHPStan can't be fooled.

I think you could also consider simply FloatType and NullableFloatType and apply the appropriate strategy depending on whether type_float(nullable: true|false)

https://3v4l.org/bedrv

Fixed interface for cast method:

     /**
     * @param mixed $value
     *
     * @return T
     * @throws CastingException
     */
    public function cast(mixed $value) : mixed;

https://3v4l.org/WodjF#v8.3.21

[andrewprofile]

For this:

function printFloat(float $value) : void
{
    echo "Float: " . $value . "\n";
}

function printString(string $value) : void
{
    echo "String: " . $value . "\n";
}

function randomFloat() : float|string
{
    return random_int(0, 1) ? 12.425 : '12.425';
}

$float = randomFloat();

if (type_float()->isValid($float)) {
    printFloat($float);
} else {
    printString($float);
}

https://3v4l.org/mNKis

[norberttech]

First of all, thanks for spending time to write that comment, I really appreciate it!

Let me answer some of the questions, and ask a few more.

---

Why does assert return a value?

Lets start here. In general the idea is to improve the UX.
Look at the example below, they both works with static analysis but one is just shorter.

$value = $input->get('float-value'); // lets assume this returns mixed 

function printFloat(float $float) : string
{
   echo (string) $float; 
}

// Option 1
if (type_float()->isValid($value)) {
   printFloat($value); // at this point thanks to isValid static analysis knows that $value is float
} else {
   throw new InvalidTypeException($value, float_type()); 
}

// Option 2

printFloat(type_float()->assert($value)); 

I personally much more prefer option 2 than 1. Shorter, cleaner and it simply just makes sense.
But if there is something wrong with this approach I would love to learn what is it.

---

I think you could also consider simply FloatType and NullableFloatType and apply the appropriate strategy depending on whether type_float(nullable: true|false)

Yes, that would work, my only problem with this approach is that I would need to pretty much duplicate every single type like that. Both, Logical and Native.

versus the other approach that I described above where I'm removing nullable property from all types and instead adding 2 new types:

• OptionalType::__construct(public Type $base)
• UnionType::__construct(private Type $left, private Type $right)

Why those 2 types IMO are better?

Because now there are clear rules about when something is optional, which means that if anywhere in the code I want to assert optional type I can do this:

type_optional(type_string())->assert($value);

or

type_string(nullable: true)->assert($value); //when nullable is true, function returns optional_type(type_string()) under the hood. 

So now when I want to check if value is optional I need to check $type instanceof OptionalType instead of checking all possible NullableXType classes.

---

Fixed interface for cast method:

Right, good catch! Actually @param can be skipped as it already declared as mixed.

I already fixed in my local codebase since I'm first writing the comment here and implementing it later. I noticed that bug in the code but never updated here :P

---

I hope that puts a bit more light on what I'm trying to achieve here and why.

The additional goal is to make extract types under standalone library flow-php/types that can be used without Flow, pretty much anywhere.

[andrewprofile]

I personally much more prefer option 2 than 1. Shorter, cleaner and it simply just makes sense.
But if there is something wrong with this approach I would love to learn what is it.

ok, it makes sense, but it seems to me that assert is a bit of an unfortunate name, because assert in programming is associated with something else, and here it now has side effects. ;)

$value = $input->get('float-value'); // lets assume this returns mixed 

function printFloat(float $float) : string
{
   echo (string) $float; 
}

// Option 1
if (type_float()->isValid($value)) {
   printFloat($value); // at this point thanks to isValid static analysis knows that $value is float
} else {
   throw new InvalidTypeException($value, float_type()); 
}

function printFloatOrNull(float|null $float) : string // in this case, it is up to the composition to handle all edge cases 
{
   echo (string) $float; 
}

// Option 2

printFloatOrNull(type_float()->assert($value)); 

As I mentioned earlier (https://3v4l.org/WodjF#v8.3.21) if you want to use a variant with assert then you also automatically have to keep the types consistent throughout the composition, otherwise for this case in one of its edge cases you will get a PHP error: Argument #1 ($val) must be of type float, null given, so the static analysis reports an error: Parameter #1 $val of function printFloat expects float, float|null given.

https://phpstan.org/r/53788c3c-b9cb-4559-b70a-58239cf65441

For compliance with the strict rule:

    /**
     * @param mixed $value
     * @return T|null // for PHPStan strict rule
     *
     * @throws \InvalidArgumentException
     */
    public function assert(mixed $value): mixed;

because we have this error: