-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Add fallback logic for generating signatures for unions of array members #53489
Conversation
@typescript-bot test this |
Heya @weswigham, I've started to run the tarball bundle task on this PR at 975ba5d. You can monitor the build here. |
Heya @weswigham, I've started to run the perf test suite on this PR at 975ba5d. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 975ba5d. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 975ba5d. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the extended test suite on this PR at 975ba5d. You can monitor the build here. |
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@weswigham Here they are:
CompilerComparison Report - main..53489
System
Hosts
Scenarios
TSServerComparison Report - main..53489
System
Hosts
Scenarios
StartupComparison Report - main..53489
System
Hosts
Scenarios
Developer Information: |
Hey @weswigham, the results of running the DT tests are ready. |
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
This is kind of a big caveat, don’t you think? Filter, for example, doesn’t change the type of the array, so for it to convert |
I'm not sure what the proposed alternative is. If you're writing filter, you're probably writing |
For // Assuming the array is `A[] | B[]`
(predicate: (item: A | B, index: number, array: A[] | B[]) => boolean) => A[] | B[] The type of the item in the predicate could be either Granted, this current implemented is definitely a step in the right direction, much better than not working at all. |
@typescript-bot pack this |
Heya @weswigham, I've started to run the tarball bundle task on this PR at 975ba5d. You can monitor the build here. |
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@andrewbranch @ahejlsberg I'm pinging more people for a second review :) |
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 975ba5d. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 975ba5d. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 975ba5d. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 975ba5d. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 975ba5d. You can monitor the build here. Update: The results are in! |
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@DanielRosenwasser Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
@DanielRosenwasser Here they are:
CompilerComparison Report - main..53489
System
Hosts
Scenarios
TSServerComparison Report - main..53489
System
Hosts
Scenarios
StartupComparison Report - main..53489
System
Hosts
Scenarios
Developer Information: |
Hey @DanielRosenwasser, the results of running the DT tests are ready. |
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
Hurray! Thank you so much @weswigham 🎉 🙇🏻 |
Seems to fix #42646 |
That functional definition of |
The way I see it, that value predicate should fail. There is no guarantee from a type system perspective that item is an In the linked issue, however, you have don’t just use a predicate of that single type ( BTW, usually it’s not ideal to start competing conversations in different issues, so let’s carry this on in the linked issue completely? |
Fixes #44373
With this PR, when we see a type like
(A[] | B[])["member"]
and look for signatures on it, and find none, we reconstruct(A | B)[]["member"]
and instead look up signatures on that. This only applies to array and tuple types because, well, arrays are special. We treat them as covariant when they're not, and we know their collection-like behavior is consistent. This logic does not apply to any custom subtypes of arrays or array-similar things, likeUInt8Array
- those may have less or more type parameters that both make the reconstruction difficult and potentially break our expectations on how collection-element-like the type parameters are.This allows things like this:
to all work mostly like you'd expect (with maybe the caveat that output arrays are unions of elements within a single array rather than unions of array types).
I'd say that it's possible to broaden this logic to other types using variance measurement logic and some hearty assumptions about how types behave based on their variance, but I feel much better about it making this an array-only change, at least right now.