Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test: constructors with input array{key: T} and array{key: list<T>} considered colliding #488

Merged

Conversation

Ocramius
Copy link
Contributor

@Ocramius Ocramius commented Mar 5, 2024

In this test, Valinor considers two disjoint input types as colliding.

Similar to #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:

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

Ocramius added a commit to Ocramius/Valinor that referenced this pull request Mar 5, 2024
@Ocramius Ocramius force-pushed the issue/collision-between-non-intersecting-hashmaps branch from a7f3d58 to 87bd298 Compare March 5, 2024 19:03
… 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 romm force-pushed the issue/collision-between-non-intersecting-hashmaps branch from 87bd298 to 5ba87a6 Compare September 2, 2024 12:38
@romm
Copy link
Member

romm commented Sep 2, 2024

Just rebased on master, after merging #552, which was not an easy task!

Enjoy this new "feature" in the upcoming release. 😊

@romm romm merged commit e1142fe into CuyZ:master Sep 2, 2024
11 checks passed
@Ocramius Ocramius deleted the issue/collision-between-non-intersecting-hashmaps branch September 2, 2024 12:52
@Ocramius
Copy link
Contributor Author

Ocramius commented Sep 2, 2024

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple object builders with same argument names but different types
2 participants