lib Fix Part 6/6 – Array.isArray #50454
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
For Backlog Bug
|
@typescript-bot run dt |
|
Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at f660c25. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the diff-based user code test suite on this PR at f660c25. You can monitor the build here. Update: The results are in! |
|
We tried this a couple of years ago; I don't remember what problems it ran into but I don't think #48228 was the only attempt. |
sandersn
requested review from
sandersn and
DanielRosenwasser
and removed request for
sandersn
|
@sandersn Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
|
@sandersn Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
That's bad; it seems that the checker really hate |
|
It seems that I can't use the TypeScript bot, @sandersn please help me run the test suite again. |
graphemecluster
changed the title
|
@typescript-bot run dt |
|
Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at 2149ae6. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the diff-based user code test suite on this PR at 2149ae6. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the abridged perf test suite on this PR at 2149ae6. You can monitor the build here. Update: The results are in! |
|
@sandersn Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
|
@sandersn Here they are:Comparison Report - main..50454
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@sandersn Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Heck no. It seems that without making it looser it won’t be a solution. |
|
I've just tinkered with it a bit. @sandersn Could you please give it another try? |
|
@typescript-bot run dt |
|
Heya @sandersn, I've started to run the diff-based user code test suite on this PR at dc97890. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the abridged perf test suite on this PR at dc97890. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at dc97890. You can monitor the build here. Update: The results are in! |
|
@sandersn Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
|
@sandersn Here they are:Comparison Report - main..50454
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@sandersn Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Actually most of the cases above just work by adding an |
|
@graphemecluster can you explain what the failures are like that would have been fixed by adding Could you also add a detailed explanation of the new type of |
|
@sandersn type ToArray<T> =
// Distribute it
T extends any ?
(
// if T == any, return any[]
true extends false & T ? Extract<any[], T> :
// if T is an array-like or iterable object but not string or array, return U[]
T extends string | readonly any[] ? never :
T extends ArrayLike<infer U> | Iterable<infer U> ? Extract<U[], T> :
// fallback to the bottom cases
never
) | (
// T == any has been covered above, so skip it
true extends false & T ? never :
// if T is any Record-like object (object with only index signature),
// intersect it with any array (this case should be rare, but a use
// case was found in test suite and being added to the test case file)
{} extends Required<T> ? T & any[] :
// otherwise, for cases not being covered by the above, extract all
// the arrays and readonly arrays out and dispose the remaining
Extract<T, readonly any[]>
)
: never;There are no special reasons to separate the cases into two, but without this TypeScript may not think the result type is an array and fires error on test cases Actually using |
|
I just noticed that replacing |
|
My feeling is that this problem is too complex to have a fix that I'm comfortable shipping in the standard lib. At least one that doesn't break existing code. If you can show that there aren't going to be many breaks across the ecosystem of packages, and that compilation won't slow down, it would still be worthwhile. But I think that's too much effort for this. |
This was referenced Oct 20, 2022
|
I talked with Ryan about this again, and we don't think there's any way that a conditional return type will work. Specifically, it won't be usable in higher-order, generic uses. But it's complex enough that there are likely other corner-case failures I haven't thought of. I think this PR should be closed. However, we want to work on the other lib fix PRs and bring them to the Typescript design meeting after finding out how well they work on existing code. I'll start with the smallest one and start user tests, etc. |
|
Thanks for picking this up. Now that I agree eventually all solutions tinkering with the lib won’t be perfect, I am going to root around in the compiler for a better way out. Let’s close this PR until then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
General Information
PR separated out from #49855, because there might be some members expecting smaller PRs. As mentioned in the comments from the big PR, whether to review a single, large PR or 6 smaller PRs is up to the TypeScript Team to decide. I couldn't have found a better way for this; hopefully this will not bring any trouble to the Team.
This PR partially fixes #49773.
For details and the track list about the changes, please see #49773.
Part 6/6, Array.isArray
Fix #17002
Actually fix #32497
Fix #33700
Fix #41808
Actually fix #43247
This PR supersedes #48228 as it fails to address
readonly number[].I am confident to this solution.Edit: I guess I must retract this. See the results below. 😔