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

reconcile two arrays by intersecting them #7470

Merged
merged 1 commit into from Jan 23, 2022
Merged

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Jan 23, 2022

This should fix #7469

This happened because there wasn't a dedicated code to handle an array existing type along with an array assertion

And given the two share the same getKey() value, it was weirdly handled.

This adds some specific code.

Not completely related to this PR: sometimes, we'll have to wonder what does TypeCombiner::combine really does. It's not the result of an union nor a result of an intersection. It kinda looks like the refineTypeWithAnother used somewhere else in the assertion module. I think the name is way better than combine and the way it works seems to be better too

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Jan 23, 2022
@orklah orklah merged commit 5e41e14 into vimeo:master Jan 23, 2022
@AndrolGenhald
Copy link
Collaborator

It's not the result of an union nor a result of an intersection.

If you're referring to combining array{a: int} and array{b: int} = array{a: int, b: int}, that actually is an intersection for unsealed arrays.

I know we haven't decided how to handle sealed arrays yet, but I've recently thought about implementing mapped types for arrays, so an unsealed array in Psalm 4 would be represented as array{a: int}, but in Psalm 5 (or maybe 6 at this point) it would be array{a: int, [array-key]: mixed}. This makes it much more intuitively obvious that array{a: int, [array-key]: mixed} & array{b: int, [array-key]: mixed} = array{a: int, b: int, [array-key]: mixed}, and it also allows for a lot of other cool things.

@orklah
Copy link
Collaborator Author

orklah commented Jan 23, 2022

I think the syntax will need to be discussed with Ondrej, we don't want to create two standard on something this important I think

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Jan 23, 2022

I'll go comment on 5299, do we have somewhere else we're already coordinating with Ondrej or should I ping him?

Edit: Actually I see he already mentioned it here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants