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

Array.isArray type narrows to any[] for ReadonlyArray<T> #17002

Open
jinder opened this issue Jul 7, 2017 · 55 comments · Fixed by #39258 · May be fixed by #48228 or #49855
Open

Array.isArray type narrows to any[] for ReadonlyArray<T> #17002

jinder opened this issue Jul 7, 2017 · 55 comments · Fixed by #39258 · May be fixed by #48228 or #49855
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@jinder
Copy link

jinder commented Jul 7, 2017

TypeScript Version: 2.4.1

Code

declare interface T {
  foo: string;
}

const immutable: T | ReadonlyArray<T> = [];

if (Array.isArray(immutable)) {
  const x = immutable; // Any[]  - Should be ReadonlyArray<T>
} 

const mutable: T | Array<T> = [];

if (Array.isArray(mutable)) {
  const x = mutable; // T[]
} 

Expected behavior: Should type narrow to ReadonlyArray<T>, or at the very least T[].

Actual behavior: Narrows to any[]. Doesn't trigger warnings in noImplicitAny mode either.

@jcalz
Copy link
Contributor

jcalz commented Jul 7, 2017

If you add the following declaration to overload the declaration of isArray():

interface ArrayConstructor {
    isArray(arg: ReadonlyArray<any> | any): arg is ReadonlyArray<any>    
}

the post-checked mutable is still narrowed to T[] and the post-checked immutable is narrowed to ReadonlyArray<T> as desired.

I'm not sure if this or similar would be an acceptable addition to the standard typing libraries or not, but you can at least use it yourself.

@jinder
Copy link
Author

jinder commented Jul 7, 2017

@jcalz yes, I think these overloads should be in the standard library. There are a few other scenarios where ReadonlyArray isn't accepted where it should be (for example Array.concat).

@vidartf
Copy link

vidartf commented Jul 10, 2017

The same narrowing problem also exists for this check:

if (immutable instanceof Array) {
  const x = immutable; // Any[]  - Should be ReadonlyArray<T>
} 

I'm not sure if there exists a similar workaround for this.

@jcalz
Copy link
Contributor

jcalz commented Jul 10, 2017

I would probably do this if I wanted a workaround:

interface ReadonlyArrayConstructor  {
    new(arrayLength?: number): ReadonlyArray<any>;
    new <T>(arrayLength: number): ReadonlyArray<T>;
    new <T>(...items: T[]): ReadonlyArray<T>;
    (arrayLength?: number): ReadonlyArray<any>;
    <T>(arrayLength: number): ReadonlyArray<T>;
    <T>(...items: T[]): ReadonlyArray<T>;
    isArray(arg: any): arg is ReadonlyArray<any>;
    readonly prototype: ReadonlyArray<any>;
}
const ReadonlyArray = Array as ReadonlyArrayConstructor;

And then later

if (ReadonlyArray.isArray(immutable)) {
  const x = immutable; // ReadonlyArray<T>
} 
if (immutable instanceof ReadonlyArray) {
  const x = immutable; //  ReadonlyArray<T>
} 

but of course, since at runtime there's no way to tell the difference between ReadonlyArray and Array, you would have to be careful to use the right one in the right places in your code. ¯\_(ツ)_/¯

@jinder
Copy link
Author

jinder commented Jul 10, 2017

@vidartf As instanceof works at runtime, I'm not sure that should narrow to ReadonlyArray. You're asking if the runtime representation of that is an array, which is true. So I'd expect it to narrow to Array<T>, not ReadonlyArray<T>.

@vidartf
Copy link

vidartf commented Jul 10, 2017

@jinder I didn't state it explicitly, but my code was meant to be based on yours (same variables and types), so it should already know that it was T | ReadonlyArray<T>. As such, instanceof should narrow it to ReadonlyArray<T>.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Aug 22, 2017
@NN---
Copy link

NN--- commented Sep 20, 2017

What is the best workaround here ?
I just ended up with ugly type assertion:

function g(x: number) {}

function f(x: number | ReadonlyArray<number>) {
    if (!Array.isArray(x)) { 
        g(x as number); // :(
    }
}

@adrianheine
Copy link

I think this is fixed in 3.0.3.

@jinder
Copy link
Author

jinder commented Sep 18, 2018

@adrianheine doesn't seem to be for me.

@adrianheine
Copy link

Oh, yeah, I was expecting the code in the original issue to not compile, bu that's not even the issue.

@bluelovers
Copy link
Contributor

let command: readonly string[] | string;

let cached_command: Record<string, any>;

if (Array.isArray(command))
{

}
else
{
	cached_command[command] = 1;
	// => Error: TS2538: Type 'readonly string[]' cannot be used as an index type.
}

@mamiu
Copy link

mamiu commented May 22, 2019

Temporary solution from @aleksey-l (on stackoverflow) until the bug is fixed:

declare global {
    interface ArrayConstructor {
        isArray(arg: ReadonlyArray<any> | any): arg is ReadonlyArray<any>
    }
}

@lll000111
Copy link

lll000111 commented Jul 20, 2019

It is not only ReadonlyArray: #33700

@jwalton
Copy link

jwalton commented Jul 29, 2019

Here's a concrete example of ReadonlyArray<string> behaving differently than Array. The !Array.isArray() case fails to eliminate the ReadonlyArray<string> when narrowing.

@kambing86
Copy link

kambing86 commented Oct 1, 2019

it should be like this

interface ArrayConstructor {
  isArray(arg: unknown): arg is unknown[] | readonly unknown[];
}

and test it in typescript

const a = ['a', 'b', 'c'];
if (Array.isArray(a)) {
  console.log(a); // a is string[]
} else {
  console.log(a); // a is never
}

const b: readonly string[] = ['1', '2', '3']

if (Array.isArray(b)) {
  console.log(b); // b is readonly string[]
} else {
  console.log(b); // b is never
}

function c(val: string | string[]) {
  if (Array.isArray(val)) {
    console.log(val); // val is string[]
  }
  else {
    console.log(val); // val is string
  }
}

function d(val: string | readonly string[]) {
  if (Array.isArray(val)) {
    console.log(val); // val is readonly string[]
  }
  else {
    console.log(val); // val is string
  }
}

function e(val: string | string[] | readonly string[]) {
  if (Array.isArray(val)) {
    console.log(val); // val is string[] | readonly string[]
  }
  else {
    console.log(val); // val is string
  }
}

@MicahZoltu
Copy link
Contributor

Would a PR that adds the appropriate overload to Array.isArray be reasonable at this point? This issue is marked as In Discussion, but I'm not sure what discussion is necessary. This just feels like a mistake in the definition files, which should be an easy fix that any contributor can submit.

Proposed addition to built-in TS libraries:

interface ArrayConstructor {
    isArray(arg: ReadonlyArray<any> | any): arg is ReadonlyArray<any>    
}

@kg-currenxie
Copy link

Any news here? :|

@P-Daddy
Copy link

P-Daddy commented Oct 20, 2022

I've found an oversight in my ArrayType<T> above (either version of it). If T is object, then the resolved type is never, which is obviously not right.

const object: object = {};
if (Array.isArray(object))
    typeOf(object).is<never>("🟢 true");  // 😞

So I tried a simplified version of @graphemecluster's implementation:

type ArrayType<T> = Extract<
    true extends false & T ?
        any[] :
    T extends readonly any[] ?
        T :
    unknown[],
    T
>;

This one works correctly for all my previously tested cases, plus ones involving object.

Note that, like my first version, it converts unknown to unknown[], which is incompatible with the current production Array.isArray, which would return any in this case. I still think this is the better behavior, but it's a breaking change.

I also made a more compact, less explicit version of these tests.

@graphemecluster
Copy link
Contributor

@P-Daddy I would like to see unknown being converted to unknown[] too – but unfortunately in my trial there are too many compatibility errors related to unknown[] – see #50454 (comment). Please also see my test case files and see if I'm missing something, and do try out a workable solution yourself with a PR. Remember to deal with ArrayLike and Iterable too. My solution seems to be too complicated that is likely unacceptable by the Team.

@graphemecluster
Copy link
Contributor

@RyanCavanaugh @sandersn Suppose my solution is still not feasible and convincing enough (at least from TypeScript 4.8), has the Team ever considered using an intrinsic type? I guess it must be one of the most probable solutions for this long-standing issue.

@RyanCavanaugh
Copy link
Member

I'll bring it to the design meeting next week. This is incredibly subtle, unfortunately.

@sindresorhus
Copy link

@RyanCavanaugh Just wondering whether this was brought up on the design meeting and whether any decision was made?

@bradzacher
Copy link
Contributor

image

This is one of the more frustrating bugs to keep running into when dealing with readonly arrays.

cc @DanielRosenwasser is this something we can bring back to the table? I understand that it's not going to be a simple problem to solve broadly - but I think there's a lot of value in making this work broadly - there's a lot of usecases where people opt for a mutable array instead of a readonly array purely because you can't narrow the type.

It looks like internally TS itself is bitten by this problem, leading you guys to define your own internal signature to work around it:

/**
* Tests whether a value is an array.
*
* @internal
*/
export function isArray(value: any): value is readonly unknown[] {
// See: https://github.com/microsoft/TypeScript/issues/17002
return Array.isArray(value);
}

@P-Daddy
Copy link

P-Daddy commented Mar 21, 2023

Thanks, @bradzacher, for pinging this. I'd also love to see this addressed.

Just to underscore how nuanced it is, and sympathize with the TypeScript team for not yet providing a solution, here's a situation I ran into just yesterday, in which none of the signatures I tried for Array.isArray worked properly:

type Args<A extends readonly any[]> = A | {args: A};

function foo<A extends readonly any[]>(args: Args<A>): readonly any[] {
    if (Array.isArray(args))
        return args;

    return args.args;
}

Playground link with one example signature that fails.

For the curious, I worked around it with the signature below, which handles this particular case, but fails on a number of other ones. Because of that, I just created a local override of isArray instead of defining it this way globally:

const isArray = Array.isArray as <T extends readonly any[]>(obj: unknown) => obj is T;

@dupasj
Copy link

dupasj commented Apr 30, 2023

Hi everyone,

I was having some trouble overriding the Array.isArray type in my Typescript project. For the story, I refactored all functions/methods on one of my projects to return readonly array and not "classic" array. I noticed this morning that this broke all my existing guarding.

Thanks to @P-Daddy and @graphemecluster comments, here's the file I came up with:

type ArrayType<T> = Extract<
    true extends T & false ?
        any[] :
        T extends readonly any[] ?
            T :
            unknown[],
    T
>;

interface ArrayConstructor {
    isArray<T>(arg: T): arg is ArrayType<T>;
}

I save it in a declaration file and this resolves all my guards without breaking other functions.

Hope this can help someone !

@parischap
Copy link

How about:

type ArrayType<T> = T extends unknown ? T extends readonly unknown[] ? T : never: never;

interface ArrayConstructor {
    isArray<T>(arg: T): arg is ArrayType<T>;
}

Simpler and works for me.

@jcalz
Copy link
Contributor

jcalz commented Jan 10, 2024

That would mean Array.isArray(x) narrows x to never when x is of a wide type like unknown.

@parischap
Copy link

Correct!

@robertherber
Copy link

It seems like myArray instanceof Array works well with Readonly arrays now

@bradzacher
Copy link
Contributor

@robertherber I believe that doesn't work across windows and the like (eg iframes) (which is the reason that Array.isArray exists).

@mmckenziedev
Copy link

@robertherber I believe that doesn't work across windows and the like (eg iframes) (which is the reason that Array.isArray exists).

That is correct:

This makes it safe to use with cross-realm objects, where the identity of the Array constructor is different and would therefore cause instanceof Array to fail.

also:

Array.isArray() also rejects objects with Array.prototype in its prototype chain but aren't actual arrays, which instanceof Array would accept.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray#description

There are good reasons why isArray() exists, things a person might not consider until they happen to you at runtime.

@denis-migdal
Copy link

denis-migdal commented Sep 21, 2024

Question : would the following code help to solve this issue (I passed all test I could think of)?

    type X<T> = Exclude<unknown,T> extends never ? T[] : T extends readonly unknown[] ? T : never;

    /*
    A type predicate's type must be assignable to its parameter's type.
    Type 'X<T>' is not assignable to type 'T'.
    'T' could be instantiated with an arbitrary type which could be unrelated to 'X<T>'.
    
        => @ts-ignore ?
    */

    function isArray<T>(a: T): a is X<T> {
        return Array.isArray(a);
    }

Playground Link

let h = 4;
if( isArray(h) )
    h; // never
else
    h; // number

let a = {}
if( isArray(a) )
    a; // never
else
    a; // {}

let b = new Array<number>();
if( isArray(b) )
    b // number[]
else
    b // never

let d = Array<number>() as readonly number[];
if( isArray(d) )
    d // readonly number[]
else
    d // never

let e = Array<number>() as number[]|readonly number[];
if( isArray(e) )
    e // number[] | readonly number[]
else
    e // never

function foo(): number|number[] {
    throw 'e';
}
let c = foo();
if( isArray(c) )
    c // number[]
else
    c // number

let t = foo() as unknown;
if( isArray(t) )
    t // unknown[]
else
    t // unknown

// doesn't work
let g = null as any;
if( isArray(g) )
    g; // any[]
else
    g; // any

let f: never = null as unknown as never;
if( isArray(f) )
    f; // never
else
    f; // never


let j = [] as never[];
if( isArray(j) )
    j; // never[]
else
    j; // never

let i = [] as unknown[];
if( isArray(i) )
    i; // unknown[]
else
    i; // never

let k = [] as any[];
if( isArray(k) )
    k; // any[]
else
    k; // never

EDIT1: fix issue when T is any.
EDIT2: add more tests.
EDIT3: found a way to remove the TS warning :

type X<T> = Exclude<unknown,T> extends never ? T[] : T extends readonly unknown[] ? T : never;
type X2<T> = Exclude<unknown,T> extends never ? unknown : T;

function isArray<T>(a: T|X2<T>): a is X<T> {
    return Array.isArray(a);
}

@denis-migdal
Copy link

denis-migdal commented Sep 22, 2024

New version :

  • objects can be an array => unknown [] (can be changed to any[] if it makes more sense).
  • Iterable<T> => readonly T[] (can be changed to T[] if it makes more sense).
  • Support of Iterable<T> generate a TS warning, dunno how to remove it in this case. (EDIT: fixed)

Do I forget some cases ?

type X<T> = Exclude<unknown,T> extends never      ? T[]                   // any/unknown => any[]/unknown
          : T extends readonly unknown[]          ? T                     // unknown[] - obvious case
          : T extends Iterable<infer U>           ?       readonly U[]    // Iterable<U> might be an Array<U>
          :          unknown[] extends T          ?          unknown[]    // something that could be an array - no ways to get the real type ?
          : readonly unknown[] extends T          ? readonly unknown[]    // something that could be an array - no ways to get the real type ?
          :              any[] extends T          ?              any[]    // something that could be an array - no ways to get the real type ?
          : readonly     any[] extends T          ? readonly     any[]    // something that could be an array - no ways to get the real type ?
                                                  : never;                // else never
type X2<T> = Exclude<unknown,T> extends never ? unknown
                                              : unknown; // required for any/unknown + Iterable<U>

function isArray<T>(a: T|X2<T>): a is X<T>
function isArray   (a: unknown): boolean {
    return Array.isArray(a);
}

Playground Link

@ziloen
Copy link

ziloen commented Oct 10, 2024

// @ts-expect-error error but works
export const isArray = Array.isArray as <T>(arg: T) => arg is unknown extends T // T is `unknown` or `any`
  ? unknown[]
  : T extends readonly unknown[]
    ? T
    : never

@ajvincent
Copy link

ajvincent commented Oct 10, 2024

export const isArray = Array.isArray as (arg: T) => arg is unknown extends T // T is unknown or any
? unknown[]
: T extends readonly unknown[]
? T
: never

Try this:

export const isArray = Array.isArray as <T>(arg: T) => typeof arg extends (infer K extends unknown)[] // T is `unknown` or `any`
  ? K[]
  : T extends readonly (infer K extends unknown)[]
    ? T
    : never

This is probably closer, but not guaranteed.

Obligatory playground link

@denis-migdal
Copy link

denis-migdal commented Oct 10, 2024

// @ts-expect-error error but works
export const isArray = Array.isArray as (arg: T) => arg is unknown extends T // T is unknown or any
? unknown[]
: T extends readonly unknown[]
? T
: never

I have a kind of test suite in my playground.
Your solution doesn't pass :

  • when we give it an Iterable or an object : they can also be an array.
  • any gives unknown[] when any[] is expected.

@denis-migdal
Copy link

export const isArray = Array.isArray as (arg: T) => typeof arg extends (infer K extends unknown)[] // T is unknown or any
? K[]
: T extends readonly (infer K extends unknown)[]
? T
: never

I think you made a mistake somewhere :

  • when giving number, your deduced type is number.

Currently, TS deduce it as number & unknown[], while I assume it to be never.

On one side, in JS we can do :

class B extends Array {  valueOf(){ return 4; } } // B can also be viewed as a number.
Array.isArray(new B()); // true

Maybe the last never in my solution should be replaced by unknown[].

@denis-migdal
Copy link

denis-migdal commented Oct 12, 2024

Update: maybe a cleaner version :

    type X<T> = Exclude<unknown,T> extends never      ? T[]                   // any/unknown => any[]/unknown[]
              : T extends readonly unknown[]          ? T                     // unknown[] - obvious case
              : T extends Iterable<infer U>           ?       readonly U[]    // Iterable<U> might be an Array<U>
                                                      : unknown[];                // by default
    type X2<T> = Exclude<unknown,T> extends never ? unknown
                                                  : unknown; // required for any/unknown + Iterable<U>

    function isArray<T>(a: T|X2<T>): a is X<T>
    function isArray   (a: unknown): boolean {
        return Array.isArray(a);
    }

Update2: handling partial types:

type PickMatching<V, T> = { [K in keyof T]: (T&V)[K] };

type X<T> = Exclude<unknown,T> extends never      ? T[]                   // any/unknown => any[]/unknown
          : T extends readonly unknown[]          ? T                     // unknown[] - obvious case
          : T extends PickMatching<Array<infer U>, T> ? readonly U[]      // try to deduce the generic argument
          // or : T extends PickMatching<Array<infer U>, T> ? U[]      // try to deduce the generic argument
          : T extends Iterable<infer U>           ?       readonly U[]    // Iterable<U> might be an Array<U>
                                                  : unknown[];                // else never
type X2<T> = Exclude<unknown,T> extends never ? unknown
                                              : unknown; // required for any/unknown + Iterable<U>

function isArray<T>(a: T|X2<T>): a is X<T>
function isArray   (a: unknown): boolean {
    return Array.isArray(a);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment