Array method definition revamp: Use case collection

[RyanCavanaugh]

We've gotten numerous issue reports and PRs to change the methods of Array, particularly reduce, map, and filter. The built-in test suite doesn't cover these very well, and these methods interact with each other and the surrounding contextual type in fairly subtle ways.

@jablko has done a great job at #33645 collecting a variety of issues into a single PR; we need to augment this PR (or something like this) with a proper test suite so we can be sure about what's being changed.

I'd like to create a clearinghouse issue here to collect self-contained (I CANNOT POSSIBLY STRESS THIS ENOUGH, SELF-CONTAINED, DO NOT IMPORT FROM RXJS OR WHAT HAVE YOU) code samples that make use of the array methods.

Please include with your snippet:

• Compiler settings
• Compiler version you were testing with
• The expected behavior (examples that should and should not produce errors are both useful)
• No imports or exports; snippets need to be self-contained so that we can put them into our test suite without extra complications

Once we've established a critical mass of code snippets, we can start combining the existing PRs into an all-up revamp and assess its impact to real-world code suites to figure out which changes don't result in unacceptable breaking changes.

ｓｅｌｆ－ｃｏｎｔａｉｎｅｄ

[RyanCavanaugh]

From #19535: concat should reflect the flattening of its input argument. Tested on 3.8-beta with target: ESNext, strict on

const foo: [number, string][] = [[1, 'one']];
const a = foo.concat([2, 'two']);
// SHOULD ERROR,
// the actual content of 'a' is [[1, 'one'], 1, 'two']
// so a[1] should be number | string | [number, string]
a[1][0];

[RyanCavanaugh]

From #24579: flat should, well, flatten. Tested on 3.8-beta with target: ESNext, strict on

declare let arr: [[number, boolean], string];
let x0 = arr.flat(0); // Should be [[number, boolean], string] or (number | boolean | string)[]
let x1 = arr.flat(1); // Should be [number, boolean, string] or (number | boolean | string)[] 
let x2 = arr.flat(2); // Should be [number, boolean, string] or (number | boolean | string)[]

[RyanCavanaugh]

From #26976: concat should at least allow an empty array as a target. Arguably it should allow heterogenous operations? Tested on 3.8-beta with target: ESNext, strict on

// Should be OK (currently an error) and produce string[]
let a1 = [].concat(['a']);
// Should be OK (is) and continue to produce string[]
let a2 = ['a'].concat(['b']);

// Should either error (current behavior) or maybe produce (string | number)[]
let a3 = [1].concat(['a']);

[RyanCavanaugh]

From #29604: flat shouldn't produce any[] (?!), Array.prototype.concat should be better if possible. Tested on 3.8-beta with target: ESNext, strict on

// Should be an error
const a: boolean[] = [[17], ["foo"]].flat();
// Should be an error (stretch goal)
const b: boolean[] = Array.prototype.concat([17], [19], [21]);  

[RyanCavanaugh]

From a real-world code suite broken by #33645:

This should error (previously misidentified as "should not error"):

// Should error because add([["a"]], ["b"]) will not produce a string[][]
function add<A>(arr: Array<A>, el: A): Array<A> {
    return arr.concat(el)
}

[RyanCavanaugh]

From a real-world code suite broken by #33645:

// Did not error; probably shouldn't in the future either
class A {
    flattenTree(option: any, changeOnSelect: any, ancestor = []) {
        let flattenOptions: any = [];
        const path = ancestor.concat(option);
        flattenOptions = flattenOptions.concat(this.flattenTree(option, changeOnSelect, path));
   }
}

This one should be simpler to demonstrate exactly what aspect of things got broken

[falsandtru]

([] as any[]).reduce(() => 0, 0); // Expected: number, Actual: any
([] as unknown[]).reduce(() => 0, 0); // Expected: number, Actual: unknown
([] as never[]).reduce(() => 0, 0); // Expected: number, Actual: number

[falsandtru]

@RyanCavanaugh This is caused by a mistake of the order of overloads. I made #36570 and looks like it makes no regression.

[jablko]

From a real-world code suite broken by #33645:

// Should not error
function add<A>(arr: Array<A>, el: A): Array<A> {
    return arr.concat(el)
}

@RyanCavanaugh Shouldn't it? If A is string[], arr is [["a"]] and el is ["b"] then add(arr, el) will return [["a"], "b"], which doesn't match the return type Array<A>?

Should that implementation be updated -> return arr.concat([el])?

[RyanCavanaugh]

@jablko good point. The actual code was in fp-ts and indeed they've removed it; the implementation now uses a loop instead https://github.com/gcanti/fp-ts/blob/master/src/Array.ts#L373

[jablko]

From a real-world code suite broken by #33645:

// Did not error; probably shouldn't in the future either
class A {
    flattenTree(option: any, changeOnSelect: any, ancestor = []) {
        let flattenOptions: any = [];
        const path = ancestor.concat(option);
        flattenOptions = flattenOptions.concat(this.flattenTree(option, changeOnSelect, path));
   }
}

This one should be simpler to demonstrate exactly what aspect of things got broken

@RyanCavanaugh I think this should be an error? The reason it currently doesn't error is because option: any is assignable to ConcatArray<never> (at const path = ancestor.concat(option)). However the only way the return type never[] is accurate is if option is [].

I think the flattenTree() signature needs to be ancestor: any[] = [], to be callable with the const path = ancestor.concat(option), where option can be something other than []? That, or the signature must be option: never[].

[bpasero]

TypeScript Version: 3.8.x

Code

interface Bar {
    property: boolean;
}

let foo: ReadonlyArray<string> | Bar;
if (Date.now() === 0) {
    foo = { property: true};
} else {
    foo = ['Hello'];
}

if (Array.isArray(foo)) {
    console.log(foo[0]);
} else {
    console.log(foo.property); // <-- error
}

Expected behavior:
The else branch correctly understands that foo is of type Bar

Actual behavior:
TypeScript still thinks foo is ReadonlyArray<string> | Bar

Playground Link: link

[HolgerJeromin]

@bpasero This has nothing to do with Array stuff:

Old example

let foo: boolean | string;
if (Date.now() === 0) {
	foo = true;
} else {
	foo = "hello";
}
foo; // still of type: boolean | string

Not possible without defining a specialized type "positive number" (no zero and even no negative value, no decimal points)
I was wrong. if (Array.isArray(foo)) should have saved us. Sorry for the noise :-)

[bpasero]

@HolgerJeromin I just did what @RyanCavanaugh suggested in #37129 (comment) and posted my usecase here.

[benlesh]

Copied example from #46438 (per @andrewbranch's request):

// The type of result is `number`, but should be `string`.
const result = [1, 2, 3].reduce((acc, value) => {
//                              ~~~~~~~~~~~~~~~~~
//                                 ^-- Error Here: "No overload matches this call"
    if (acc === null) {
        return '' + value;
    } else {
        return acc + ', ' + value;
    }
}, null);

console.log(result); // "1, 2, 3"
console.log(typeof result); // "string"

[seansfkelley]

I'd like to be able to pass find and filter type guards that overlap with, but aren't subtypes of, the array element type.

Playground Link (version 4.4.4)

But the gist of it is:

const bar = arrayOfFoosAndBars.find(isBarOrBaz); // does not work

// this behavior already works for if statements
if (isBarOrBaz(fooOrBarValue)) {
  // correctly narrows to just the overlapping type, `Bar`
}

The proposed change is to add:

interface Array<T> {
    find<S>(predicate: (this: void, value: S | T, index: number, obj: T[]) => value is S, thisArg?: any): Extract<T, S> | undefined;
}

(And something similar for filter.)

[lazytype]

I think we could introduce changes to some of these methods in a backwards-compatible way. Here's a definition of concat that I believe satisfies this (for the heterogeneous case, not the flattening case):

declare interface Array<T> {
    concat<U>(this: Array<U>, ...items: ConcatArray<U>[]): Array<U>;
    concat<U>(this: Array<U>, ...items: (U | ConcatArray<U>)[]): Array<U>;
}

[1].concat('x')  // error as expected
[1].concat<number | string>('x')  // No error since type is explicitly widened

Playground: https://tsplay.dev/wXkDJW

I think this approach should work for most of the non-mutating Array methods in question.

[vjau]

I don't know if this is appropriate here, but i have just been bitten by this.

Imho, filter(), some(), every()... should only accept a predicate returning a boolean.

//strict compiler settings

const arr = [1, 2, 3];


const onlyPositives = arr.filter(num=>{
  num > 0 //forgot return, TS doesn't mind
});

console.log("onlyPositives", onlyPositives); // []  empty array


//isPositive has type (num:number)=>void
const isPositive = (num:number)=>{
  num > 0;
}

//filter should not accept a predicate not returning a boolean
const onlyPositives2 = arr.filter(isPositive);//no error that way either

Instead, the predicate type is actually returning unknown in the definitions.

Sorry if this has already been discussed to death, but i find this very unsound and i havn't been able to find a duplicate.

[tjjfvi]

Imho, filter(), some(), every()... should only accept a predicate returning a boolean.

This would be a pretty big breaking change, as many people will use e.g. .filter(x => x) to remove falsy values (e.g. to remove null or empty strings).

Perhaps, instead, the type could be modified in some way to allow unknown but ban void? Perhaps an overload expecting a void return type with @deprecated You probably meant to return something?

[Retsam]

@vjau You can also add the @typescript-eslint/no-unnecessary-condition lint rule - it tries to find dead conditionals and checks these array predicates.

[vjau]

This would be a pretty big breaking change, as many people will use e.g. .filter(x => x) to remove falsy values (e.g. to remove null or empty strings).

I was under the impression that it was idiomatic TS to write .filter(x => !!x) instead but after some research it seems i am probably confusing with another language.

[Brookke]

Hey @RyanCavanaugh, please could I get #41708 added to this so that #44216 can finally get merged/fixed.

[EthanRutherford]

playground link

Array methods such as splice, map, etc. when called on a subclass of Array, return a new instance of the subclass.

This might be related to tuples losing their tuple-ness when calling map, etc., but I'm not sure if that's the same issue. tuples exist only in the type system, while a subclass exists at runtime, so I could see this potentially being a separate issue from the tuple issue.

[ljharb]

@EthanRutherford be aware that browsers are exploring removing that capability, so it should not be relied upon. (also, "Tuple" can refer to the proposed JS language primitive when using a capital T)

[EthanRutherford]

Ah, I was not aware of that.

[EthanRutherford]

After looking further into https://github.com/tc39/proposal-rm-builtin-subclassing, it appears that support for "Type II: subclass instance creation in built-in methods" support is unlikely to be possible to remove without significant webcompat issues. One of the champions has even responded that the removal of type II is highly unlikely: tc39/proposal-rm-builtin-subclassing#21 (comment).

In light of that, and the fact that the proposal is only stage 1, perhaps it would be prudent for Typescript to try to match the current behavior, rather than anticipate what seems to be a rather unlikely future behavior change?

[ackvf]

lib.es2016.array.include.d.ts should not produce error when testing existence of an element that comes from a wider set of values that intersects with Array<T>.

playground link

declare const test1: 1
declare const test2: 1 | 2
declare const test3: 2 | 3
declare const test4: number

declare const array: (0 | 1)[]

array.includes(test1) // OK
array.includes(test2) // Argument of type '1 | 2' is not assignable to parameter of type '0 | 1'. Type '2' is not assignable to type '0 | 1'.(2345)
array.includes(test3) // Argument of type '2 | 3' is not assignable to parameter of type '0 | 1'. Type '2' is not assignable to type '0 | 1'.(2345)
array.includes(test4) // Argument of type 'number' is not assignable to parameter of type '0 | 1'.(2345)

In this example, the result of test2 and test4 is undesired as the test potentially can yield true value, much like the following snippet.

array.findIndex((v) => v === test1) // OK
array.findIndex((v) => v === test2) // OK
array.findIndex((v) => v === test3) // This condition will always return 'false' since the types '0 | 1' and '2 | 3' have no overlap.(2367)
array.findIndex((v) => v === test4) // OK

Disputably, some people may even argue that even test3 should not produce an error. ¯\(ツ)/¯

[whzx5byb]

@ackvf See discussion in #14520.

[nevnein]

I hope this is not out of scope, but when one of the mentioned array methods is called on a tuple of length n, the index parameter could be inferred as 0 | 1 | … | n -1 instead of just number. TS already knows the tuple length, and this could be really useful when accessing other tuples of the same length and noUncheckedIndexedAccess is set to true.

Compiler settings: default, but with noUncheckedIndexedAccess set to true
Compiler version: 5.2.2
Example:

const aTuple = [ "a", "b", "c"] as const
const bTuple = [ "x", "y", "z"] as const

// These are both correctly inferred as exactly 3
const aLength = aTuple.length
const bLength = bTuple.length

type range = 0 | 1 | 2
const possibleIndex = 2 as range // This could be something like getRandomNumber(0,2)
// This is inferred as x | y | z and doesn't include undefined, as expected
const possibleAccess = bTuple[possibleIndex]

// This causes an error, because the return array could contain undefined
const test: string[] = aTuple.map((_, i) => {
  // With noUncheckedIndexedAccess=true access is inferred as possibily undefined, 
  // but TS could/should know that the the index will always be 0 | 1 | 2, as above
  const access = bTuple[i]
  return access
})

Here is the playground

[Clindbergh]

Filter does not work correctly with an array of unions:

Expected

    const ab: number[] | string[] = ([] as string[] | number[]).filter(
      (value) => false,
    );

The error is

TS2322: Type (string | number)[] is not assignable to type string[] | number[]
  Type (string | number)[] is not assignable to type string[]
    Type string | number is not assignable to type string
      Type number is not assignable to type string

Actual

    const ab: (string | number)[] = ([] as string[] | number[]).filter(
      (value) => false,
    );

Typescript Version: 5.2.2

Related: #38380.

[karlismelderis-mckinsey]

I would appreciate if Array.includes would behave like this:

interface Array<T> {
  includes(searchElement: unknown, fromIndex?: number): searchElement is T;
}

interface ReadonlyArray<T> {
  includes(searchElement: unknown, fromIndex?: number): searchElement is T;
}

[icecream17]

it depends, the most you can guarantee in general is searchElement is T ? boolean : false since even if the type matches it's still possible for includes to return false:

const abcs: String[] = "abcs".split('')

abcs.includes("d") // > false, even though "d" matches type String

[Retsam]

@karlismelderis-mckinsey Yeah this has been suggested before e.g. #31018 and there's some issues. One is, like @icecream17 said it can return false even when searchElement actually is T- which would require #15048: a false result on the includes check can't be used to prove that searchElement isn't T.

---

But also, I think widening searchElement from T to unknown is problematic for other use cases. There's two reasons to use includes:

1. You're using the string to check something about the array. e.g. if(colorArray.includes("red"))
2. You're using the array to check something about the string. e.g. if(["red", "blue", "green"].includes(maybeColor))

The type-guard signature you suggest is useful for the second case, but makes the first case worse: if type Color = "red" | "blue" | "green" and colorArray is Color[], it's not great if colorArrray.includes("read") is accepted: that's a typo that's currently caught but wouldn't be if includes took unknown as its argument.

---

Personally, I suggest making a utility function and using it in place of arr.includes(val):

function includes<const S>(haystack: readonly S[], needle: unknown): needle is S {
    const _haystack: readonly unknown[] = haystack;
    return _haystack.includes(needle)
}