Failing test case: Valinor cannot understand array{k: T|list<T>}
#487
Conversation
In this test, Valinor is being challenged to understand de-serialization of
a given hashmap when given a type `array{a-key: T|list<T>}`.
Specifically, at the time of this writing, Valinor only picks `T` during de-serialization,
and seems to completely ignore the `list<T>` portion of the data.
This kind of de-serialization is relevant when dealing with XML structures,
where an element could be singular or plural depending on context:
```xml
<Invoices>
<Invoice>
<!-- in this example, `Item` is singular, and needs to be handled as `Item` -->
<Item><Price>123</Price></Item>
</Invoice>
<Invoice>
<!-- in this example, `Item` is plural, and needs to be handled as `list<Item>` -->
<Item><Price>456</Price></Item>
<Item><Price>789</Price></Item>
</Invoice>
</Invoices>
```
Initially discovered by @Tigerman55
… list<T>}` considered colliding In this test, Valinor considers two disjoint input types as colliding. Similar to CuyZ#487 ( ffe0f0f ), this scenario was detected while trying to map multiple constructors for XML structures that may present different data depending on singular/plural entries found: ```xml <Invoices> <Invoice> <!-- in this example, `Item` is singular, and needs to be handled as `Item` --> <Item><Price>123</Price></Item> </Invoice> <Invoice> <!-- in this example, `Item` is plural, and needs to be handled as `list<Item>` --> <Item><Price>456</Price></Item> <Item><Price>789</Price></Item> </Invoice> </Invoices> ``` In CuyZ#487, we attempted to map a single constructor using `array{foo: T|list<T>}`, while in this patch, we found the issue because we attempted to attack the problem by declaring separate constructors that would work on `array{foo: T}` and `array{foo: list<T>}` disjointly, but failed to do so due to aggressive collision detection logic. Initially discovered by @Tigerman55
|
@romm btw, if you have better suggestions on how to map those XML structures meanwhile, I'm all ears :D The "this XML element could be one or many" seems to be very common. |
… list<T>}` considered colliding In this test, Valinor considers two disjoint input types as colliding. Similar to CuyZ#487 ( ffe0f0f ), this scenario was detected while trying to map multiple constructors for XML structures that may present different data depending on singular/plural entries found: ```xml <Invoices> <Invoice> <!-- in this example, `Item` is singular, and needs to be handled as `Item` --> <Item><Price>123</Price></Item> </Invoice> <Invoice> <!-- in this example, `Item` is plural, and needs to be handled as `list<Item>` --> <Item><Price>456</Price></Item> <Item><Price>789</Price></Item> </Invoice> </Invoices> ``` In CuyZ#487, we attempted to map a single constructor using `array{foo: T|list<T>}`, while in this patch, we found the issue because we attempted to attack the problem by declaring separate constructors that would work on `array{foo: T}` and `array{foo: list<T>}` disjointly, but failed to do so due to aggressive collision detection logic. Initially discovered by @Tigerman55
|
Hey there, thanks for the report and failing test case, it helped me identifying the real-world needs. :) I opened a separate PR for this, could you take a look at #493 and tell me if it solves your issue? |
|
Thanks @romm: probably checking in the evening. A suggestion (in general, not just for Valinor), since it's hard to say if your patch "works": keep commits understandable. For example:
This keeps things traceable / makes a meaningful patch history, also for posterity (squash intentionally to be avoided) :) |
|
Procedure followed: git checkout issue/union-type-mapping-ambiguity
git fetch upstream
git cherry-pick 210b33ed379f66299ae4f21968b35a82d27eb0d6 # picked my changes when resolving conflicts
./vendor/bin/phpunitGot: I guess it's fine, besides the commit squashing hiccup.It's your project, but please don't squash: commit history is VERY valuable, when commits are meaningful, and I put effort clarifying exactly what's going on in the commit messages too. I'd say 🚢 whenever you feel like it 👍 |
|
Thanks for checking it!
I usually follow this procedure, in this case it was different because #493 is a big rework of how union types are handled by the mapper, it is not just a fix of what is tested in this PR. I could have merged my own branch here, though, sorry you had to do it. 😅
And I thank you for it, it helps a lot when starting to work on a fix! When merging I usually squash though, because I prefer to have consistent and conventional commit messages in the history. When I need more information about why a commit was made, GitHub allows me to easily find which PR led to it. For instance I can see that a53ef93 is related to PR 446: 
…which has the history of what happened: 
I know opinions may differ and workflow can be improved, but while I'm the main contributor I prefer to stick to this “convention”, mainly for consistency. |
|
Released with 1.11.0, enjoy! |
|
Awesome! |
… list<T>}` considered colliding In this test, Valinor considers two disjoint input types as colliding. Similar to CuyZ#487 ( ffe0f0f ), this scenario was detected while trying to map multiple constructors for XML structures that may present different data depending on singular/plural entries found: ```xml <Invoices> <Invoice> <!-- in this example, `Item` is singular, and needs to be handled as `Item` --> <Item><Price>123</Price></Item> </Invoice> <Invoice> <!-- in this example, `Item` is plural, and needs to be handled as `list<Item>` --> <Item><Price>456</Price></Item> <Item><Price>789</Price></Item> </Invoice> </Invoices> ``` In CuyZ#487, we attempted to map a single constructor using `array{foo: T|list<T>}`, while in this patch, we found the issue because we attempted to attack the problem by declaring separate constructors that would work on `array{foo: T}` and `array{foo: list<T>}` disjointly, but failed to do so due to aggressive collision detection logic. Initially discovered by @Tigerman55
In this test, Valinor considers two disjoint input types as colliding. Similar to #487, this scenario was detected while trying to map multiple constructors for XML structures that may present different data depending on singular/plural entries found: ```xml <Invoices> <Invoice> <!-- `Item` is singular, and needs to be handled as `Item` --> <Item><Price>123</Price></Item> </Invoice> <Invoice> <!-- `Item` is plural, and needs to be handled as `list<Item>` --> <Item><Price>456</Price></Item> <Item><Price>789</Price></Item> </Invoice> </Invoices> ``` In #487, we attempted to map a single constructor using `array{foo: T|list<T>}`, while in this patch, we found the issue because we attempted to attack the problem by declaring separate constructors that would work on `array{foo: T}` and `array{foo: list<T>}` disjointly, but failed to do so due to aggressive collision detection logic. Initially discovered by @Tigerman55
In this test, Valinor is being challenged to understand de-serialization of a given hashmap when given a type
array{a-key: T|list<T>}.Specifically, at the time of this writing, Valinor only picks
Tduring de-serialization, and seems to completely ignore thelist<T>portion of the data.This kind of de-serialization is relevant when dealing with XML structures, where an element could be singular or plural depending on context:
Initially discovered by @Tigerman55