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

Improve EPC for unions with multiple discriminants #32711

Closed
wants to merge 1 commit into from

Conversation

jack-williams
Copy link
Collaborator

Fixes #32657

Q: Do I need to explicitly coerce map keys to string like: candidates.set("" + type.id, 1);?

@weswigham
Copy link
Member

A: yes.

}
if (candidates) {
let matchID: string | false | undefined;
candidates.forEach((value, id) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment explaining what criteria this is filtering/selecting on would be useful. I think it's that it returns the first type which matches all discriminant properties (out of multiple which may match), but some text expressing that would be nice.

Copy link
Collaborator Author

@jack-williams jack-williams Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. If there are multiple valid matches it fails rather than picking the first. I did this to avoid situations where the order of the union influenced whether you got an error, sometimes incorrectly. (I think it was a case where some constituents were technically subtypes of another, but the union was unreduced).

Copy link
Collaborator Author

@jack-williams jack-williams Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weswigham Done. In the case where there are multiple candidates we could return a new union of just those candidates, but I'm not sure of the error message and perf consequences---if you want me to experiment with that approach let me know.

@jack-williams
Copy link
Collaborator Author

A: yes.

Thanks.

What I've written feels kind of messy and could be cleaned up, but I hope the algorithm is on the right lines.

@sandersn
Copy link
Member

sandersn commented Aug 8, 2019

Sooooo, I also fixed this bug yesterday, in a different PR, while working on a different bug. The algorithms are almost the same, but I feel like my solution is easier to read:

  1. It has fewer moving parts -- no matchedProperties counter with associated post-loop retrieval.
  2. Instead it uses indices into target.types, so it can use indexOf to retrieve match(es) instead of Array.forEach.

@jack-williams would you mind taking a look?
#32755

I'll merge your commit in order to get your test and make sure you get attribution for this fix as well.

(Thanks @weswigham for pointing this out to me.)

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.

Regression in discriminated unions when a common property may be boolean
3 participants