Bug in arrays map return type

[n1kk]

The return type for the map method in Array and ReadonlyArray seems to be causing some trouble.

  map<U, This = undefined>(
    callbackfn: (this: This, value: T, index: number, array: this) => U,
    thisArg?: This
  ): { [K in keyof this]: U };

In case when it is an instance of the class that extends the Array it will change all the keys of the class to the mapped value type, including methods and public fields.

class ArrX extends Array<number> {
  constructor(nums: number[]) {
    super(...nums);
  }

  sum(): number {
    return this.reduce((s, n) => s + n, 0);
  }
}

let arrX = new ArrX([2, 3, 4]);

let mapped = arrX.map((v) => v + 1);

let mapped: {
    [x: number]: number;
    sum: number;
    length: number;
    toString: number;
    toLocaleString: number;
    pop: number;
    push: number;
    concat: number;
    join: number;
    reverse: number;
    shift: number;
    slice: number;
    sort: number;
    ... 19 more ...;
    readonly [Symbol.unscopables]: number;
}

CodeSandbox playground link

[aaditmshah]

Wow. Researching this issue opened up a big can of worms. And after studying what the ECMAScript specification says about Array.prototype.map, I've come to the conclusion that the best solution to this problem would be to change the return type of Array.prototype.map from { [K in keyof this]: U } to U[] or readonly U[].

My investigation began with the current type definition of Array.prototype.map in better-typescript-lib.

  map<U, This = undefined>(
    callbackfn: (this: This, value: T, index: number, array: this) => U,
    thisArg?: This
  ): { [K in keyof this]: U };

The culprit in this type definition is the return type { [K in keyof this]: U }. Everything else is all right. Mapped types in TypeScript work differently for arrays and for non-array objects. A mapped type for arrays produces another array, and a mapped type for non-array objects produces another non-array object. The problem is that TypeScript considers sub classes of Array to be non-array objects. This is what's causing the issue.

But, what piqued my interest was why did the return type depend upon this? Turns out that when you create a sub class of Array, the inherited .map method will return an instance of the sub class instead of a plain old array.

class StringArray extends Array<string> {
  public constructor(strings: string[]) {
    const { length } = strings;
    super(length);
    let index = 0;
    while (index < length) {
      this[index] = strings[index];
      index = index + 1;
    }
  }
}

const strings = new StringArray(["hello", "world"]);
const numbers = strings.map((string) => string.length);
console.assert(numbers instanceof StringArray); // assertion passes

This was surprising for me because I expected the result of .map to be a plain old array. I didn't expect the result to be an instance of StringArray. Turns out, this is part of the ECMAScript specification. The .map method creates a result array of the same species as the input array. So, if you map over a StringArray then the result will also be a StringArray. This is terrible software design, but unfortunately it's a part of the language.

Why is this a terrible design? First, consider that in our above example numbers is an instance of StringArray. However, numbers is an array of numbers whereas a StringArray must always be an array of strings. Second, the way .map creates a new instance of StringArray is by internally calling new StringArray(length) where length is the length of the input array. However, the StringArray constructor expects an array of strings as input. It doesn't expect to be provided a number as input.

This can lead to subtle bugs, like when you map over an empty StringArray.

const noStrings = new StringArray([]);
const noNumbers = noStrings.map((string) => string.length);
console.log(noNumbers); // Expected to see `[]` but we see `[undefined]` instead

So, what's the takeaway? Don't create sub classes of Array. It can lead to unexpected bugs in your code.

Coming back, what should the return type of Array.prototype.map be in TypeScript? I think the only "good" return types are U[] and readonly U[]. For example, the return type of .map for StringArray can't be StringArray because what if the mapping function returns numbers instead of strings? Thus, it would make more sense to return number[] instead of StringArray.

Of course, this is also unsafe because the result is actually a StringArray (according to ECMAScript) and not number[]. I don't think it's possible to solve this problem completely. However, changing the return type to the lowest common denominator, i.e. U[] and readonly U[], seems to be the best that we can do.

In conclusion, just avoid all these problems by never creating a sub class of Array. It's not worth the effort. Use composition over inheritance instead.

[n1kk]

Yes, extending an array is a known security vulnerability that allows arbitrary code execution. Although, if you're aware of it, it can be fixed by overriding the Symbol.species of the class, or removing it. But the feature itself has officially been recognized as a security issue and the new array methods ignore special species property. See the warning in the MDN Docs, and there is an ongoing investigation to remove this feature: https://github.com/tc39/proposal-rm-builtin-subclassing.

To be clear I am not extending any arrays, this was just to show the example of the resulting type. I probably could've been more explicit about it, sorry, didn't have much time.

As shown in my code sandbox playground I've encountered this issue bu implementing a custom string template tag function. Those functions receive a TemplateStringsArray which is a readonly string array but has an extra field .raw. That is where this issue occurred, this type is defined as an interface extending ReadonlyArray<string>, so the return type of the .map from this library made the resulting object incompatible with ReadonlyArray<string>.

interface TemplateStringsArray extends ReadonlyArray<string> {
    readonly raw: readonly string[];
}

I see now what was your goal with overriding the return of the .map method, but yes, it probably was not the best way to achieve it. I think what you were going for is just an intersection: this & U[]. This way it will keep all the custom properties of the custom class but also will be backwards compatible with a regular array

    map<U, This = undefined>(
        callbackfn: (this: This, value: T, index: number, array: this) => U,
        thisArg?: This,
    ): this & U[];

Here's a Playground link with your type where it causes the conflict.

And here is how this type fixes that conflict: Playground link

However! if you run the code in the latest example you will see that there is another problem. That the mapped array does not have a .raw field. This means that the TemplateStringsArray is not actually a custom class that extends an Array, it's just an array object with a .raw property added to it, it's constructor is an Array class, so all the resulting outputs of its methods will be ordinary arrays. So this approach will affect also every array that will have just an extra field added to it, all its resulting maps, and their maps, will have that field, which will not actually be there.

Sadly there is no easy way to distinguish that, and although this & U[] does achieve what you were aiming for better, it still isn't a bug-proof way to go.

So, in the end, I would agree with you @aaditmshah and put the return type of those Array methods to be just U[].

[graphemecluster]

It's my fault that I made this change in #12:
16e8eb7#diff-4543c62dcf560451d352906704dcf2c1d53af354414ade9d50b92f25f74e637eR481

The reason behind this change is to deal with mapping of TypeScript tuples, which is discussed in microsoft/TypeScript#29841:
_{(not the one from the R&T proposal)}

let point: [number, number] = [4, 2];
const distance = Math.hypot(...point);
point = point.map(coord => coord / distance);

In order to solve this, I played around with the TypeScript repo and tried numerous possibilities in my old days, but none of the solutions seem feasible. The PR still exists there anyway:
microsoft/TypeScript#50046

[uhyo]

Thank you all for the discussion!

I'm going to change the return type to just U[] as suggested. Althoguh losing the tuple type support is a bit of pity. 🥲

[graphemecluster]

The reason the solution proposed in my old PR wasn't feasible was merely due to the difficulty of introducing breaking changes in TypeScript.
I wonder if Cast<{ [P in keyof this]: U }, U[]> is a suitable solution for this library.

[n1kk]

If your goal was to preserve the size of the array when mapping a tuple then it's achievable. I have a few internal types in my project that make it quite simple. In fact, this is all it took for me:

Let me see if I can pack it in a minimal reproducible type

[n1kk]

Here's the minimal repro that I managed to extract:

type IsNumConst<T> = [T] extends [number] ? ([number] extends [T] ? false : true) : false;

type LEN<T extends readonly any[]> = T extends { length: infer L } ? L : never;

type TupleOfSize<T, N extends number, _REST extends any[] = []> =
    IsNumConst<N> extends true
        ? _REST extends { length: N }
            ? _REST
            : TupleOfSize<T, N, [..._REST, T]>
        : T[];

type MappedArray<T extends readonly any[], U> = TupleOfSize<U, LEN<T>>;

declare global {
    interface ReadonlyArray<T> {
        map<U>(cb: (item: T) => U): MappedArray<this, U>;
    }
}

let three_numbers = [1, 2, 3] as const
let three_strings = three_numbers.map(v => String(v));

Not very minimal, but works. It uses recursion so it will obviously break at some point, but a tuple has to be very huge, I think about 1000 elements.

TS Playground

[graphemecluster]

@n1kk FYI here is an implementation that builds a tuple in O(log(n)): microsoft/TypeScript#26223 (comment)

[n1kk]

@graphemecluster Awesome, thanks!

[uhyo]

I wonder if Cast<{ [P in keyof this]: U }, U[]> is a suitable solution for this library.

Hi, I checked if this works, and it indeed seemed to fix the issue without degrading the tuple type support. I'm going this way. Thank you 🙂