-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 ReadonlyArray overload to Array.isArray #28916
Conversation
@sandersn a lot to unpack here; we should discuss before merging |
@ulrichb @RyanCavanaugh function test(arg: string | ReadonlyArray<string>): string {
if (Array.isArray(arg)) {
return arg.join('\n');
}
// Type 'string | readonly string[]' is not assignable to type 'string'.
// Type 'readonly string[]' is not assignable to type 'string'.ts(2322)
return arg;
} It's fine with |
interface MyArray<T> extends Array<T> { manifest: any; }
interface MyReadOnlyArray<T> extends ReadonlyArray<T> { manifest: any; }
namespace Testbench {
declare const Array: {
isArray<T>(arg: T): arg is T extends ReadonlyArray<any> ? T : Array<any> extends T ? (T & any[]) : never;
};
function fn1(arg: string | string[]) {
if (Array.isArray(arg)) arg.push(""); // Should OK
}
function fn2(arg: unknown) {
if (Array.isArray(arg)) arg.push(""); // Should OK
}
function fn3(arg: object) {
if (Array.isArray(arg)) arg.push(""); // Should OK
}
function fn4(arg: {}) {
if (Array.isArray(arg)) arg.push(""); // Should OK
}
function fn5(arg: string | ReadonlyArray<string>) {
if (Array.isArray(arg)) arg.push(10); // Should FAIL
if (Array.isArray(arg)) arg.push(""); // Should FAIL
}
function fn6(arg: string | string[]) {
if (Array.isArray(arg)) arg.push(10); // Should FAIL
}
function fn7(arg: boolean | number[] | string[]) {
if (Array.isArray(arg)) arg.push(null as any as string & number); // Should OK
}
function fn8(arg: string | number[] | readonly string[]) {
if (Array.isArray(arg)) arg.push(10); // Should FAIL
}
function fn9(arg: string | number[] | readonly string[]) {
if (Array.isArray(arg)) arg.push(10); // Should FAIL
}
function fn10(arg: string | MyArray<string>) {
if (Array.isArray(arg)) arg.push(10); // Should FAIL
if (Array.isArray(arg)) arg.push(""); // Should OK
if (Array.isArray(arg)) arg.manifest; // Should OK
}
function fn11(arg: string | MyReadOnlyArray<string>) {
if (Array.isArray(arg)) arg.push(""); // Should FAIL
if (Array.isArray(arg)) arg.indexOf(10); // Should FAIL
if (Array.isArray(arg)) arg.indexOf(""); // Should OK
if (Array.isArray(arg)) arg.manifest; // Should OK
}
function fn12<T>(arg: T | T[]) {
if (Array.isArray(arg)) arg.push(null as any as T); // Should OK
}
function fn13<T>(arg: T | ReadonlyArray<T>) {
if (Array.isArray(arg)) arg.push(null as any as T); // Should fail
if (Array.isArray(arg)) arg.indexOf(null as any as T); // Should fail
}
function fn14<T>(arg: T | [T]) {
if (Array.isArray(arg)) arg.push(null as any as T); // Should OK
}
function fn15<T>(arg: T | readonly [T]) {
if (Array.isArray(arg)) arg.push(null as any as T); // Should fail
if (Array.isArray(arg)) arg.indexOf(null as any as T); // Should OK
}
function fn16<T extends string | string[]>(arg: T) {
if (Array.isArray(arg)) arg.push("10"); // Should OK
}
function fn17() {
const s: Array<string | string[]> = [];
const arrs = s.filter(Array.isArray);
arrs.push(["one"]);
}
} |
@ulrichb we hammered out the definition above (see the top of the file); do you want to try updating the PR to use it and clean up the merge conflicts? |
…yOverload # Conflicts: # tests/baselines/reference/destructuringParameterDeclaration4.errors.txt # tests/baselines/reference/promisePermutations.errors.txt # tests/baselines/reference/promisePermutations2.errors.txt # tests/baselines/reference/promisePermutations3.errors.txt # tests/baselines/reference/promiseTypeInference.errors.txt
…load for 'isArray' This has the advantage that it upcasts non-array inputs to Array<any> instead of ReadonlyArray<any> (=> no breaking change in this case).
Hi @RyanCavanaugh, updated the PR and merged master. I like the approach with the conditional type (avoids the breaking change mentioned in the description). One question: Why not I made small changes in the test cases:
|
/ping @RyanCavanaugh |
I was looking for a better type this as well. I'm using isArray<T>(arg: T): T is Extract<NonNullable<T>, Array | ReadonlyArray>; I had to wrap EDIT: This doesn't seem to work right in more complex cases. I need to use the conditional extends approach. |
Sorry, I lost track of this. I don't want to merge it during the 3.8 beta, but I added it to my list of work when the 3.9 release starts. |
Edit: tests @RyanCavanaugh @ulrichb So there's an important and obvious test case that both of your versions fail: function fn0(arg: any) {
if (Array.isArray(arg)) {
arg.push(""); // Should OK
arg.anything(); // Should FAIL, but both your versions pass,
// because `arg` is being typed as `any`
} else {
arg.anything(); // Should OK, but both your versions fail,
// because `arg` is being narrowed to `never`
// => This is a pretty big problem!!! <=
}
} Note that this is different from the However, Of course, I didn't figure all that out until after much, much, much trial-and-error down endless dead-ends, but with all that in mind, the basic idea behind my fix is to special-case unknown extends T ? Extract<any[], T> : (T extends readonly any[] ? T : T & any[])
// ^^^^^^^same as @ulrichb's version^^^^^^^ Notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use GitHub's inline suggest-changes feature to apply the fix I proposed in this comment: #28916 (comment)
|
||
function fn13<T>(arg: T | ReadonlyArray<T>, t: T) { | ||
if (Array.isArray(arg)) arg.push(t); // Should fail | ||
if (Array.isArray(arg)) arg.indexOf(t); // OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency; also add another test:
if (Array.isArray(arg)) arg.indexOf(t); // OK | |
if (Array.isArray(arg)) arg.indexOf(t); // Should OK | |
if (Array.isArray(arg)) arg.indexOf(0); // Should FAIL |
} | ||
|
||
function fn13<T>(arg: T | ReadonlyArray<T>, t: T) { | ||
if (Array.isArray(arg)) arg.push(t); // Should fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, let's make this (and all the other places):
if (Array.isArray(arg)) arg.push(t); // Should fail | |
if (Array.isArray(arg)) arg.push(t); // Should FAIL |
|
||
if (Array.isArray(maybeArray)) { | ||
maybeArray.length; // OK | ||
function fn1(arg: string | string[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function fn1(arg: string | string[]) { | |
function fn0(arg: any) { | |
if (Array.isArray(arg)) { | |
arg.push(""); // Should OK | |
arg.anything(); // Should FAIL | |
} else { | |
arg.anything(); // Should OK | |
} | |
} | |
function fn1(arg: string | string[]) { |
@@ -1341,7 +1341,7 @@ interface ArrayConstructor { | |||
(arrayLength?: number): any[]; | |||
<T>(arrayLength: number): T[]; | |||
<T>(...items: T[]): T[]; | |||
isArray(arg: any): arg is Array<any>; | |||
isArray<T>(arg: T): arg is T extends ReadonlyArray<any> ? T : Array<any> extends T ? (T & any[]) : never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isArray<T>(arg: T): arg is T extends ReadonlyArray<any> ? T : Array<any> extends T ? (T & any[]) : never; | |
isArray<T>(arg: T): arg is Extract<any[], T> | Extract<[any], T> | (unknown extends T ? never : Extract<T, readonly any[]>) { | |
// the first two clauses, `Extract<any[], T>` and `Extract<[any], T>`, | |
// ensure that the type predicate will extract `B[]` out of `A | B[]` | |
// and `[B]` out of `A | [B]`, just like a the naive predicate `arg is any[]` | |
// would. The final clause is a special case if T is known to include | |
// a readonly array, to extract `readonly B[]` out of `A | readonly B[]`, | |
// and `readonly [B]` out of `A | readonly [B]`, but as a special exception | |
// it needs to ignore the case of T = any, because `any` is an ill-behaved | |
// type. See https://github.com/microsoft/TypeScript/pull/28916#issuecomment-573217751 |
OK so, my fix fails tests Note, however, that this is a potentially breaking change from the old |
if (Array.isArray(arg)) arg.push(10); // Should FAIL | ||
} | ||
|
||
function fn7(arg: boolean | number[] | string[], stringAndNumber: string & number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string & number = never
, I don't think this tests what we want, how about:
function fn7(arg: boolean | number[] | string[], stringAndNumber: string & number) { | |
function fn7(arg: boolean | Number[] | String[], stringAndNumber: String & Number) { |
@laughinghan the problem you point about narrowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem that @laughinghan brings up with narrowing any
means we can't merge this as-is. However, their solution breaks backward compatibility. I'm not sure how to fix this PR at the moment.
@DanielRosenwasser we might consider bringing this back up at a design meeting or we could just drop it unless a community member is able to come up with a solution.
@sandersn I see. I will take a stab at passing (Unless it would make more sense to wait until you've brought it up at a design meeting?) |
@laughinghan if you can come up with a solution that passes all the tests, that would help our discussion a lot. Otherwise that's what we'll spend our time doing first. |
This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here. |
@sandersn Oh—did it come up at the design meeting at all whether we want the |
@sandersn Actually, I fixed it! See for yourself. It's actually a little simpler and easier to explain, so I also included a comment with an overview of how it works. I'll modify my suggested-changes comments. (Can you accept them, or can only @ulrichb accept them?) |
if (Array.isArray(arg)) arg.push(10); // Should FAIL | ||
} | ||
|
||
function fn9(arg: string | number[] | readonly string[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn9()
is identical to fn8()
, how about instead:
function fn9(arg: string | number[] | readonly string[]) { | |
function fn9(arg: string | readonly number[] | string[]) { |
function fn12<T>(arg: T | T[], t: T) { | ||
if (Array.isArray(arg)) arg.push(t); // Should OK | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function fn12<T>(arg: T | T[], t: T) { | |
if (Array.isArray(arg)) arg.push(t); // Should OK | |
} | |
function fn12a<T>(arg: T | T[], t: T) { | |
if (Array.isArray(arg)) arg.push(t); // Should OK | |
if (Array.isArray(arg)) arg.indexOf(t); // Should OK | |
if (Array.isArray(arg)) arg.indexOf(0); // Should FAIL | |
} | |
function fn12b<T>(arg: T | T[] | (T & Number)[], t: T & Number) { | |
if (isArrayTest(arg)) arg.push(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} | |
function fn12c<T, U>(arg: T | T[] | U[], t: T, u: U, tu: T & U) { | |
if (isArrayTest(arg)) arg.push(tu); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(tu); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} | |
function fn12d<T>(arg: T | readonly T[], t: T) { | |
if (isArrayTest(arg)) arg.push(t); // Should FAIL | |
if (isArrayTest(arg)) arg.indexOf(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} |
function fn14<T>(arg: T | [T], t: T) { | ||
if (Array.isArray(arg)) arg.push(t); // Should OK | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function fn14<T>(arg: T | [T], t: T) { | |
if (Array.isArray(arg)) arg.push(t); // Should OK | |
} | |
function fn14a<T>(arg: T | [T], t: T) { | |
if (isArrayTest(arg)) arg.push(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} | |
function fn14b<T>(arg: T | [T] | [T & Number], t: T & Number) { | |
if (isArrayTest(arg)) arg.push(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} | |
function fn14c<T, U>(arg: T | [T] | [U], t: T, u: U, tu: T & U) { | |
if (isArrayTest(arg)) arg.push(tu); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(tu); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} | |
function fn14d<T>(arg: T | readonly [T], t: T) { | |
if (isArrayTest(arg)) arg.push(t); // Should FAIL | |
if (isArrayTest(arg)) arg.indexOf(t); // Should OK | |
if (isArrayTest(arg)) arg.indexOf(0); // Should FAIL | |
} |
Fixes #17002
This is an alternative to #22942 which doesn't introduce a breaking change for
any
inputs by using an overload instead of changing theArray.isArray()
signature.Therefore it still works for
any
/unknown
arguments to cast them toArray<any>
(instead ofReadonlyArray<any>
like in #22942). See the test cases inisArray.ts
.In the following case (the argument is some non-array + non-
any
/unknown
) this PR still introduces a breaking change.IMO it's okay to break this situation (where the static type isn't an array and should be changed anyways) and on the other hand we win type safe
ReadonlyArray<T>
narrowing (#17002, which by the way is a breaking change anyways).