feat: improve union type narrowing during mapping #493

romm · 2024-03-12T22:29:39Z

The algorithm used by the mapper to narrow a union type has been greatly improved, and should cover more edge-cases that would previously prevent the mapper from performing well.

If an interface, a class or a shaped array is matched by the input, it will take precedence over arrays or scalars.

(new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(
        signature: 'array<int>|' . Color::class,
        source: [
            'red' => 255,
            'green' => 128,
            'blue' => 64,
        ],
    ); // Returns an instance of `Color`

When superfluous keys are allowed, if the input matches several interfaces, classes or shaped array, the one with the most children node will be prioritized, as it is considered the most specific type:

(new \CuyZ\Valinor\MapperBuilder())
    ->allowSuperfluousKeys()
    ->mapper()
    ->map(
        // Even if the first shaped array matches the input, the second one is
        // used because it's more specific.
        signature: 'array{foo: int}|array{foo: int, bar: int}',
        source: [
            'foo' => 42,
            'bar' => 1337,
        ],
    );

If the input matches several types within the union, a collision will occur and cause the mapper to fail:

(new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(
        // Even if the first shaped array matches the input, the second one is
        // used because it's more specific.
        signature: 'array{red: int, green: int, blue: int}|' . Color::class,
        source: [
            'red' => 255,
            'green' => 128,
            'blue' => 64,
        ],
    );

// ⚠️ Invalid value array{red: 255, green: 128, blue: 64}, it matches at
//    least two types from union.

Fixes #395
Fixes #356
Fixes #385
Closes #487
Closes #495

Ocramius

LGTM, but please consider rebasing on top of #487 to preserve research/history

docs/pages/usage/type-reference.md

Ocramius · 2024-03-13T18:21:11Z

src/Mapper/Object/FilteredObjectBuilder.php

- $this->arguments = $this->delegate->describeArguments();
+ }
+
+ public static function from(mixed $source, ObjectBuilder ...$builders): ObjectBuilder


s/from/mix, perhaps? You seem to aggregate multiple builders?

It actually filters builders depending on the input, so that only one remains at the end. I added this static constructor to check if there is only one builder, in which case it can be returned as is and does not need the whole filtering process.

Ocramius · 2024-03-13T18:22:01Z

src/Mapper/Tree/Builder/CasterProxyNodeBuilder.php

+ return ! $type instanceof CompositeTraversableType
+ && ! $type instanceof ShapedArrayType


Excluded on purpose? 🤔

From a reader's perspective, it's not 100% clear why these checks are in place: shouldn't CompositeTraversableType and ShapedArrayType also have accepts()?

This is a tricky one, and I'm still not convinced this class is at the correct place.

The goal was to save resources by checking if the input is accepted by the current node's type, before trying casting. It led to issues during development of this PR, so I adapted it, but I think I need to find a better way to check whether the input is valid or not. I wrote it in my internal notes, I'll try to improve it in the future.

Ocramius · 2024-03-13T18:24:05Z

src/Mapper/Tree/Exception/TooManyResolvedTypesFromUnion.php

+ ];
+
+ $this->body = TypeHelper::containsObject($unionType)
+ ? 'Invalid value {source_value}, it matches at least two types from union.'


Nice! We should probably add "couldn't take a decision" or such?

What sentence would you have in mind?

FYI the TypeHelper::containsObject is there because right now the system cannot display a meaningful representation of an object constructor. This is because an object might have several constructors, for instance when taking the Color class from the docs, its representation should be non-empty-string|array{red: int<0, 255>, green: int<0, 255>, blue: int<0, 255>}. I have plans to improve it, but this will be in a separate PR.

-'Invalid value {source_value}, it matches at least two types from union.' -'Invalid value {source_value}, it matches two or more types in an union: cannot take a decision.'

This would already be a good improvement :)

src/Mapper/Tree/Exception/TooManyResolvedTypesFromUnion.php

TimWolla · 2024-03-13T22:55:44Z

Possibly related issues of mine you might want to look into: #356, #358.

The algorithm used by the mapper to narrow a union type has been greatly improved, and should cover more edge-cases that would previously prevent the mapper from performing well. If an interface, a class or a shaped array is matched by the input, it will take precedence over arrays or scalars. ```php (new \CuyZ\Valinor\MapperBuilder()) ->mapper() ->map( signature: 'array<int>|' . Color::class, source: [ 'red' => 255, 'green' => 128, 'blue' => 64, ], ); // Returns an instance of `Color` ``` When superfluous keys are allowed, if the input matches several interfaces, classes or shaped array, the one with the most children node will be prioritized, as it is considered the most specific type: ```php (new \CuyZ\Valinor\MapperBuilder()) ->allowSuperfluousKeys() ->mapper() ->map( // Even if the first shaped array matches the input, the second one is // used because it's more specific. signature: 'array{foo: int}|array{foo: int, bar: int}', source: [ 'foo' => 42, 'bar' => 1337, ], ); ``` If the input matches several types within the union, a collision will occur and cause the mapper to fail: ```php (new \CuyZ\Valinor\MapperBuilder()) ->mapper() ->map( // Even if the first shaped array matches the input, the second one is // used because it's more specific. signature: 'array{red: int, green: int, blue: int}|' . Color::class, source: [ 'red' => 255, 'green' => 128, 'blue' => 64, ], ); // ⚠️ Invalid value array{red: 255, green: 128, blue: 64}, it matches at // least two types from union. ```

See #356

TimWolla · 2024-03-17T15:24:56Z

tests/Integration/Mapping/UnionMappingTest.php

@@ -94,6 +94,18 @@ public static function union_mapping_works_properly_data_provider(): iterable
 'assertion' => fn (mixed $result) => self::assertSame(['key' => [42, 1337]], $result),
 ];

+ yield 'shaped array representing http response with status 200' => [
+ 'type' => "array{status: 200, data: array{text: string}} | array{status: 400, error: string}",
+ 'source' => ['status' => 200, 'data' => ['text' => 'foo']],


Note: The test case in my issue was specifically about allowing superfluous fields, because I don't care about extra fields in the response, i.e. this:

'source' => ['status' => 200, 'data' => ['text' => 'foo', 'extra' => 'bar']],

Yeah, I edited my message already. Just pushed a new test case that should match!

Awesome, thank you!

See #356

romm · 2024-03-17T15:39:57Z

There we go! 🎉

romm mentioned this pull request Mar 12, 2024

Failing test case: Valinor cannot understand array{k: T|list<T>} #487

Closed

romm force-pushed the feat/better-union-narrowing branch from 9331f1a to 210b33e Compare March 12, 2024 22:32

Ocramius approved these changes Mar 13, 2024

View reviewed changes

This was referenced Mar 15, 2024

allowSuperfluousKeys and union do not work together #356

Closed

Failing test case: ''|array{foo: int} if superfluous key exists #495

Closed

romm added 3 commits March 17, 2024 16:13

misc: chance union error message

c6f0ab6

misc: enhance union error message

59520c1

romm force-pushed the feat/better-union-narrowing branch from efd6267 to 59520c1 Compare March 17, 2024 15:13

test: add union mapping test case for http response

fb7463f

See #356

TimWolla reviewed Mar 17, 2024

View reviewed changes

test: add union mapping test case with superfluous key for http response

79fc823

See #356

romm merged commit f731586 into master Mar 17, 2024
21 checks passed

romm deleted the feat/better-union-narrowing branch March 17, 2024 15:39

romm mentioned this pull request Mar 17, 2024

Top-level union with allowSuperfluousKeys and overlapping types behaves unpredictably #358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve union type narrowing during mapping #493

feat: improve union type narrowing during mapping #493

romm commented Mar 12, 2024 •

edited

Loading

Ocramius left a comment

Ocramius Mar 13, 2024

romm Mar 15, 2024

Ocramius Mar 13, 2024

Ocramius Mar 13, 2024

romm Mar 15, 2024

Ocramius Mar 13, 2024

romm Mar 15, 2024

Ocramius Mar 16, 2024 •

edited

Loading

TimWolla commented Mar 13, 2024

TimWolla Mar 17, 2024

romm Mar 17, 2024

TimWolla Mar 17, 2024

romm commented Mar 17, 2024

		return ! $type instanceof CompositeTraversableType
		&& ! $type instanceof ShapedArrayType

feat: improve union type narrowing during mapping #493

feat: improve union type narrowing during mapping #493

Conversation

romm commented Mar 12, 2024 • edited Loading

Ocramius 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

Ocramius Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

TimWolla commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romm commented Mar 17, 2024

romm commented Mar 12, 2024 •

edited

Loading

Ocramius Mar 16, 2024 •

edited

Loading