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

Consider re-ordering Array#reduce overloads in lib.d.ts #7014

Open
yortus opened this issue Feb 11, 2016 · 14 comments
Open

Consider re-ordering Array#reduce overloads in lib.d.ts #7014

yortus opened this issue Feb 11, 2016 · 14 comments
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@yortus
Copy link
Contributor

yortus commented Feb 11, 2016

Example code:

type UnaryFunction = (arg1) => any;
type BinaryFunction = (arg1, arg2) => any;

let binaryFuncs: BinaryFunction[] = [];
let unaryFunc = arg1 => {};
let reduced = binaryFuncs.reduce((prev, next) => prev, unaryFunc);

// ACTUAL:
let f: UnaryFunction = reduced;     // ERROR binary not assignable to unary

// EXPECTED:
let f: UnaryFunction = reduced;     // OK - both lhs and rhs really are unary

The call to Array#reduce in the above example definitely returns a unary function, but the type system erroneously infers the return type as a binary function.

This seems to be caused by the declaration order of the two overloads of Array#reduce in lib.d.ts. If the declaration order is reversed, the problem is solved.

The two overloaded declarations in lib.d.ts are as follows:

reduce(callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: T[]) => T, initialValue?: T): T;
reduce<U>(callbackfn: (previousValue: U, currentValue: T, currentIndex: number, array: T[]) => U, initialValue: U): U;

The first overload matches the example with T=BinaryFunction, since it satisfies the compiler's assignability checks. So the second overload is never considered, even though it is a strictly better match, with T=BinaryFunction and U=UnaryFunction.

Would it be possible to swap the overload order for Array#reduce (and Array#reduceRight) in lib.d.ts to resolve this issue?

@yortus
Copy link
Contributor Author

yortus commented Feb 11, 2016

Simpler example:

let arrayOfObjs: {}[] = [/*...*/];
let countOfProps = arrayOfObjs.reduce((sum, obj) => sum + Object.keys(obj).length, 0);

This doesn't compile, tsc reports "Operator '+' cannot be applied to types '{}' and 'number'".

In fact the code is fine and runs fine.

tsc took the first overload since it works with T={}, so the second (correct) overload is not considered.

In general, the second overload will always be skipped if U is assignable to T (in this example, number is assignable to {}).

Swapping the order of the two Array#reduce overloads in lib.d.ts fixes this.

@yortus yortus changed the title Type inference problem with Array.reduce (lib.d.ts) Consider re-ordering Array#reduce overloads in lib.d.ts Feb 11, 2016
@RyanCavanaugh
Copy link
Member

I believe it's safe to do this now -- the order we had was dependent on behavior that is no longer the case (ancient history, see http://typescript.codeplex.com/workitem/2352).

@yortus
Copy link
Contributor Author

yortus commented Feb 12, 2016

Wow, nice archaeological find 😄 Those were the days...

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Feb 16, 2016
@aluanhaddad
Copy link
Contributor

Glad this can now be resolved, and I really appreciate the effort the team takes in polishing these corner cases.

As a side not, I really dislike the argument ordering that TC39 chose for Array.prototype.reduce and Array.prototype.reduceRight. The seed should come first, not just because it makes type inference easier, but because it makes things more readable. Anyway, that decision is ancient history, but I wanted to rant about it.

@johnnyreilly
Copy link

Just stumbled on this - look forward to this being resolved! For anyone else wondering what to do in the interim just supply the type parameter. So to take @yortus example, this:

let arrayOfObjs: {}[] = [/*...*/];
let countOfProps = arrayOfObjs.reduce((sum, obj) => sum + Object.keys(obj).length, 0);

Becomes:

let arrayOfObjs: {}[] = [/*...*/];
let countOfProps = arrayOfObjs.reduce<number>((sum, obj) => sum + Object.keys(obj).length, 0);

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript and removed In Discussion Not yet reached consensus labels Jun 9, 2016
@RyanCavanaugh
Copy link
Member

Taking PRs on this with the caveat we're still not 100% sure this is going to be an overall improvement, so reserving the right to reject it if it turns out to be bad on net.

@RyanCavanaugh RyanCavanaugh added the Good First Issue Well scoped, documented and has the green light label Jun 9, 2016
@mhegazy mhegazy added this to the Community milestone Jun 9, 2016
@vilicvane
Copy link

vilicvane commented Sep 25, 2016

I am on this PR.

@bdurrani
Copy link

bdurrani commented Oct 2, 2018

Looks like @vilic took care of this issue. maybe this issue should be closed? It's been around for awhile. @RyanCavanaugh

@vilicvane
Copy link

@bdurrani Just noticed that I closed the PR myself. Might had encountered some problem but forgot to update the comment here.

@chriskrycho
Copy link

This appears to still be an issue. Anyone know why #11356 ended up closed? @vilic?

@starpit
Copy link

starpit commented Dec 20, 2019

agreed, we encounter this with typescript 3.7.3

const A = [1, '2', 3]
const str: string = A.reduce((str, a) => `${str} ${a.toString()}`, '')

which results in this compilation error:

Type 'string | number' is not assignable to type 'string'.
  Type 'number' is not assignable to type 'string'.

2     const str: string = A.reduce((str, a) => `${str} ${a.toString}`, '')
            ~~~

there is an easy enough workaround for now, i.e. A.map(_ => _.toString()).reduce(...), but this seems worth fixing?

@Meowu
Copy link

Meowu commented Feb 21, 2020

agreed, we encounter this with typescript 3.7.3

const A = [1, '2', 3]
const str: string = A.reduce((str, a) => `${str} ${a.toString()}`, '')

which results in this compilation error:

Type 'string | number' is not assignable to type 'string'.
  Type 'number' is not assignable to type 'string'.

2     const str: string = A.reduce((str, a) => `${str} ${a.toString}`, '')
            ~~~

there is an easy enough workaround for now, i.e. A.map(_ => _.toString()).reduce(...), but this seems worth fixing?

An easier workaround.We provide the generic paramater here, so it matches the second overload.

const A = [1, '2', 3]
const str: string = A.reduce<string>((str, a) => `${str} ${a.toString()}`, '')

@okmttdhr

This comment has been minimized.

@SCWR
Copy link

SCWR commented Sep 2, 2021

may be fix this with typescript 4.2.2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.