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

Add ReadonlyArray overload to Array.isArray #28916

Closed

Conversation

ulrichb
Copy link

@ulrichb ulrichb commented Dec 8, 2018

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 the Array.isArray() signature.

Therefore it still works for any/unknown arguments to cast them to Array<any> (instead of ReadonlyArray<any> like in #22942). See the test cases in isArray.ts.

In the following case (the argument is some non-array + non-any/unknown) this PR still introduces a breaking change.

const notReallyAnArray = {};

if (Array.isArray(notReallyAnArray)) {
    notReallyAnArray.length; // OK
    const str: string = notReallyAnArray[0]; // OK
    notReallyAnArray.sort(); // Now an Error
}

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).

@msftclas
Copy link

msftclas commented Dec 8, 2018

CLA assistant check
All CLA requirements met.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jan 25, 2019

@sandersn a lot to unpack here; we should discuss before merging

@RyanCavanaugh RyanCavanaugh added this to 𝚜𝚘𝚘𝚗 ™ in Design Meeting Docket Jan 25, 2019
@nstepien
Copy link

@ulrichb @RyanCavanaugh
Would love to see this fixed, currently Array.isArray breaks types in code like this:

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 arg: string | Array<string> of course, but that's not ideal.

@RyanCavanaugh RyanCavanaugh moved this from 𝚜𝚘𝚘𝚗 ™ to This Week in Design Meeting Docket May 31, 2019
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jun 28, 2019

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"]);
    }
}

@RyanCavanaugh
Copy link
Member

@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).
@ulrichb
Copy link
Author

ulrichb commented Jul 17, 2019

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 isArray<T>(arg: T): arg is T extends ReadonlyArray<any> ? T : T & any[]; (no Array<any> extends T case and never)? I ran all tests locally and it behaves equally.


I made small changes in the test cases:

  • fn5: Added positive cases for ReadonlyArray members
  • fn13: Fixed the wrong "should fail" comment
  • fn16/17: Added additional negative test case
  • Changed all the as null as ... casts to parameters.

@ulrichb
Copy link
Author

ulrichb commented Jul 22, 2019

/ping @RyanCavanaugh

@kalbert312
Copy link

kalbert312 commented Dec 29, 2019

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 T in NonNullable otherwise I was running into some issue with undefined.
Seems to work but only tested in one case.
Maybe wouldn't work since there might be some overlap issue in Array/ReadonlyArray? Didn't go that far with it yet though.

EDIT: This doesn't seem to work right in more complex cases. I need to use the conditional extends approach.

@sandersn sandersn added this to In Review in Rolling Work Tracking Jan 9, 2020
@sandersn
Copy link
Member

sandersn commented Jan 9, 2020

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.

@laughinghan
Copy link

laughinghan commented Jan 10, 2020

Edit: tests fn12() and fn14() fail, but I think those are wrong

@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!!! <=
    }
  }

Playground Link

Note that this is different from the fn2() test case where arg is unknown. Consider @ulrichb's proposed type T extends readonly any[] ? T : T & any[]: that works for T = unknown because it is the proper, well-behaved top type, so it isn't assignable to readonly any[], and so the type of arg gets correctly narrowed to unknown & any[] = any[].

However, T = any is magic and acts like both the top and bottom types, so it is assignable to readonly any[], and so the type of arg isn't narrowed at all and remains T = any, instead of being narrowed to any[] like we want.

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 any by checking if unknown extends T:

unknown extends T ? Extract<any[], T> : (T extends readonly any[] ? T : T & any[])
//                                       ^^^^^^^same as @ulrichb's version^^^^^^^

Notes:

  • Extract<any[], T> must be used and not T & any[] because of my fn0() test case: if T = any, then T & any[] = any & any[] = any & AnyTypeExpression = any, instead of any[] like we want.
    • (this is again because any isn't a well-behaved top type and is acting like a bottom type here)
  • Extract<any[], T> must be used and not any[] extends T ? any[] : never, but I don't understand why, is this a TypeScript bug?
  • In fn2(), Array.isArray() narrows unknown to any[], but arguably it should be narrowed to unknown[] instead?

Copy link

@laughinghan laughinghan left a 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
Copy link

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:

Suggested change
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

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):

Suggested change
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[]) {

Choose a reason for hiding this comment

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

Suggested change
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;
Copy link

Choose a reason for hiding this comment

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

Suggested change
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

@laughinghan
Copy link

OK so, my fix fails tests fn12() and fn14(), but I'd argue that those tests are wrong. They test that T | T[] is narrowed to T[] and T | [T] is narrowed to [T], respectively, but that is actually incorrect if T = readonly string[].

Note, however, that this is a potentially breaking change from the old isArray(arg: any): arg is any[] behavior, which did narrow T | T[] to T[] and T | [T] to [T]. Is that an intentionally unsoundness in TypeScript's type narrowing algorithm?

if (Array.isArray(arg)) arg.push(10); // Should FAIL
}

function fn7(arg: boolean | number[] | string[], stringAndNumber: string & number) {
Copy link

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:

Suggested change
function fn7(arg: boolean | number[] | string[], stringAndNumber: string & number) {
function fn7(arg: boolean | Number[] | String[], stringAndNumber: String & Number) {

@sandersn sandersn added this to Curio in Pall Mall Jan 29, 2020
@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020
@sandersn sandersn added this to Needs review in PR Backlog Mar 9, 2020
@sandersn sandersn removed this from Curio in Pall Mall Mar 9, 2020
@sandersn
Copy link
Member

@laughinghan the problem you point about narrowing any is pretty severe. But I also don't think we want to break the narrowing testing in fn12 and fn14, even if they are unsound.

PR Backlog automation moved this from Needs review to Waiting on author Mar 13, 2020
Copy link
Member

@sandersn sandersn left a 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.

@laughinghan
Copy link

laughinghan commented Mar 14, 2020

@sandersn I see. I will take a stab at passing fn12 and fn14 then

(Unless it would make more sense to wait until you've brought it up at a design meeting?)

@sandersn
Copy link
Member

@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.

@sandersn
Copy link
Member

sandersn commented Apr 1, 2020

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 sandersn closed this Apr 1, 2020
PR Backlog automation moved this from Waiting on author to Done Apr 1, 2020
@sandersn sandersn removed this from In Review in Rolling Work Tracking Apr 2, 2020
@laughinghan
Copy link

laughinghan commented May 5, 2020

@sandersn Oh—did it come up at the design meeting at all whether we want the fn12 and fn14 behavior, even though they're unsound?

@laughinghan
Copy link

laughinghan commented May 6, 2020

@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[]) {
Copy link

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:

Suggested change
function fn9(arg: string | number[] | readonly string[]) {
function fn9(arg: string | readonly number[] | string[]) {

Comment on lines +58 to +60
function fn12<T>(arg: T | T[], t: T) {
if (Array.isArray(arg)) arg.push(t); // Should OK
}

Choose a reason for hiding this comment

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

Suggested change
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
}

Comment on lines +67 to +69
function fn14<T>(arg: T | [T], t: T) {
if (Array.isArray(arg)) arg.push(t); // Should OK
}

Choose a reason for hiding this comment

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

Suggested change
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
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Array.isArray type narrows to any[] for ReadonlyArray<T>
7 participants