[TypeInfo] Enforce stricter types sanity to avoid edge cases #57943

skalpa · 2024-08-07T23:08:29Z

Q	A
Branch?	7.2
Bug fix?	yes
New feature?	yes
Deprecations?	no
License	MIT

This was originally supposed to try and improve how nullability is handled, but taking care of a couple more issues on the same branch really made my life easier. Hopefully it will be fine. At the end the main improvements are:

A new NullableTypeInterface to ensure only types actually concerned with nullability have to deal with it
getTypeIdentifier is now an abstract method on Type and every type consumer can rely on it
The validation of unions and intersections was improved to more closely follow the PHP type system and forbid the creation of invalid types.

There are tests missing, but I'm pushing now so @stof and @mtarld can give me their comments.

Composite types validation

Intersections

Intersections can only contain class-types. Consequently, trying to create an intersection from the following types will now throw an exception:

Builtins/non-objects: new IntersectionType(Type::int(), Type::float())
Unions: new IntersectionType(Type::object('Foo'), Type::union(Type::object('Bar'), Type::object('Baz'))) (an union can still be made of intersections, but an intersection cannot contains unions, as per the DNF rules).

Note: the tests were loaded with intersections of scalars, this one was fun to take care of. 🤨

Unions

Unions can be made of almost any other types, except unions (that's the logic that was already implemented). I only added an additional obvious test to prevent creating unions that include void or never.

Other checks must be missing, but I'd be for following the PHP rules here and left it there for now.

`Type::getTypeIdentifier()`

getTypeIdentifier() is now an abstract method of Type. It is implemented as follow in children classes:

BuiltinType: unchanged
ObjectType: returns OBJECT
Wrapped types (collection/generic): returns the wrapped type identifier
IntersectionType: returns OBJECT
UnionType: if all subtypes have the same type identifier, return it. Otherwise, MIXED

This is already exploited by the composite type classes to allow the following:

To validate whether all subtypes of an intersection represent object types, automatically supporting generics and collections.
To determine if a type is "just null" 😉 in UnionType::asNonNullable(), by testing whether TypeIdentifier::NULL === $type->getTypeIdentifier()

NullableTypeInterface

As I started to discuss on #57630, nullability is not a global concern as there are only 3 nullable cases: null, mixed, and unions that contain a nullable type.

Therefore, I moved isNullable() and asNonNullable() to a new NullableTypeInterface that is only implemented by BuiltinType and UnionType.

I also fixed Type::nullable() and UnionType::asNonNullable() to ensure they don't create new types if the current is already nullable (or not).

stof

This PR means that the nullable check becomes $t instanceof NullableTypeInterface && $t->isNullable() instead of $t->isNullable(). I don't think this is a DX improvement.

stof · 2024-08-08T17:27:42Z

src/Symfony/Component/TypeInfo/Type/IntersectionType.php

+        if (\count($types) < 2) {
+            throw new InvalidArgumentException(\sprintf('"%s" expects at least 2 types.', self::class));
+        }
+        // Only accept non-composite object types, except the builtin 'object'


what about mixed ?

Fully agree about the interface, not sure why I finally decided to add it. Removed and re-added the methods to the Type class (might have messed the diff for now, will fix it, sorry).

mixed is not allowed as a member of an intersection. mixed, never and void are standalone types in PHP, treated very differently from the others: they cannot be used in unions or intersections.

I agree we want to stay as flexible as possible, but we also want to avoid nightmarish edge cases like this current behaviour:

$type = Type::generic(Type::mixed(), Type::int()); dump($type->isNullable()); // true $nonNullable = $type->asNonNullable(); dump($nonNullable->isNullable()); // true

stof · 2024-08-08T17:29:37Z

src/Symfony/Component/TypeInfo/Type/CompositeTypeInterface.php

+    /**
+     * @return list<Type>
+     */
+    public function getTypes(): array;


what is the benefit of this interface ? The various composite types we have right now have different method for what the type composing them mean. So I don't see any use case for that interface.

It's Mathias'. I just didn't copy/paste it from his PR and forgot to commit his comments/credits (fixed). You're incorrect, both composite types actually have common methods. I added them to the interface to make it clearer, and improved the docblocks. Basically, composite types provide methods to query their subtypes/parts.

stof · 2024-08-08T17:30:50Z

src/Symfony/Component/TypeInfo/TypeIdentifier.php

+    /**
+     * Whether this type can be used in unions.
+     */
+    public function isComposable(): bool


The method name does not reflect what it does (using it in intersection would also be composing it)

You are not allowed to use primitive types as members of an intersection, only class names.

However, I agree about the name/doc. I improved the description and refactored/renamed the method to isStandalone(), flipping the logic (it now returns true for mixed, never and void.
This is meant to allow us to recognize standalone types (as the PHP engine calls them), as they are very specific.

stof · 2024-08-08T17:32:21Z

src/Symfony/Component/TypeInfo/Type/UnionType.php

@@ -19,17 +20,71 @@
 * @author Mathias Arlaud <mathias.arlaud@gmail.com>
 * @author Baptiste Leduc <baptiste.leduc@gmail.com>
 *
- * @template T of Type
+ * @template T of BuiltinType|ObjectType|GenericType|CollectionType|IntersectionType


Restricting the bounds here without restricting them in the static factory method does not make sense.

Agreed. Sorry. Fixed.

stof · 2024-08-08T17:35:46Z

src/Symfony/Component/TypeInfo/Type/CompositeTypeTrait.php

+     * @param callable(T): bool $callable
+     * @return list<T>
+     */
+    public function filter(callable $callable): array


What does filtering means in this context ? This is not clear as public API IMO.

Improved the description. It basically calls array_filter($this->getTypes(), $callable). This is used to extract subtypes that satisfy a specific predicate:

// Only builtin types (mixed, int, etc...) $type->filter(fn (Type $t) => $t instanceof BuiltinType); // Only intersections $type->filter(fn (Type $t) => $t instanceof IntersectionType); // All integers, supporting generics like int<10,20> $type->filter(fn (Type $t) => TypeIdentifier::INT === $t->getTypeIdentifier()); // All objects (including intersections, class types, generics and "object") $type->filter(fn (Type $t) => TypeIdentifier::OBJECT === $t->getTypeIdentifier()); // All objects except "object" $type->filter(fn (Type $t) => TypeIdentifier::OBJECT === $t->getTypeIdentifier() && !$t instanceof BuiltinType); // All bools (thas is bool, true or false) $type->filter(fn (Type $t) => $t->getTypeIdentifier()->isBool()); // Everything except "null" $type->filter(fn (Type $t) => TypeIdentifier::NULL !== $t->getTypeIdentifier());

stof · 2024-08-08T17:37:31Z

src/Symfony/Component/TypeInfo/Type/ObjectType.php

    }

-    public function getTypeIdentifier(): TypeIdentifier
+    public function getBaseType(): BuiltinType|self


I would suggest avoiding to create useless diffs like that (by avoiding to swap methods). It makes git blame a lot more useful.

Yes, sorry, it's because I went back and forth. This is still a draft, I'll clean up everything at some point.

This is still experimental and I move methods or purposely break BC and like to keep related methods together, so this may happen again, although I will still try and avoid it, fully get your comment and apologize.

skalpa · 2024-08-08T21:42:44Z

@stof Removing the interface got rid of my biggest BC break and made sure all the tests (including tests that are part of the Serializer) turned green again. That should allow us to determine whether the changes have any consequence on real world uses.

skalpa · 2024-08-15T14:04:04Z

@stof Thank you for your help on what was obviously a work in progress. I was able to come up with something better, thanks in part to your comments.

@mtarld I am closing this PR for now, will try to post something for you tonight.

carsonbot added Status: Needs Review Bug Feature labels Aug 7, 2024

carsonbot added this to the 7.2 milestone Aug 7, 2024

skalpa changed the title ~~Add NullableTypeInterface and improve types comparison~~ [TypeInfo] Add NullableTypeInterface and improve types comparison Aug 7, 2024

skalpa force-pushed the type-info/nullability branch from e88ea3f to d0d90c8 Compare August 8, 2024 00:17

Add NullableTypeInterface and improve types comparison

cbd04ea

skalpa force-pushed the type-info/nullability branch from d0d90c8 to cbd04ea Compare August 8, 2024 01:22

skalpa mentioned this pull request Aug 8, 2024

[TypeInfo] Redesign Type methods and nullability #57630

Merged

Guard against more invalid types

6f1d882

stof reviewed Aug 8, 2024

View reviewed changes

skalpa added 2 commits August 8, 2024 19:10

Remove NullableTypeInterface

832d197

Added methods to CompositeTypeInterface and improved doc

d150681

skalpa changed the title ~~[TypeInfo] Add NullableTypeInterface and improve types comparison~~ [TypeInfo] Enforce stricter types sanity to avoid edge cases Aug 8, 2024

skalpa closed this Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TypeInfo] Enforce stricter types sanity to avoid edge cases #57943

[TypeInfo] Enforce stricter types sanity to avoid edge cases #57943

skalpa commented Aug 7, 2024

stof left a comment

stof Aug 8, 2024

skalpa Aug 8, 2024

stof Aug 8, 2024

skalpa Aug 8, 2024

stof Aug 8, 2024

skalpa Aug 8, 2024

stof Aug 8, 2024

skalpa Aug 8, 2024

stof Aug 8, 2024

skalpa Aug 8, 2024

stof Aug 8, 2024

skalpa Aug 8, 2024

skalpa commented Aug 8, 2024

skalpa commented Aug 15, 2024

[TypeInfo] Enforce stricter types sanity to avoid edge cases #57943

[TypeInfo] Enforce stricter types sanity to avoid edge cases #57943

Conversation

skalpa commented Aug 7, 2024

Composite types validation

Intersections

Unions

Type::getTypeIdentifier()

NullableTypeInterface

stof left a comment

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

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

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

Choose a reason for hiding this comment

skalpa commented Aug 8, 2024

skalpa commented Aug 15, 2024

`Type::getTypeIdentifier()`