Skip to content

5.6 regression: Array.prototype.reduce type inference regression #59763

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

Closed
trevorade opened this issue Aug 26, 2024 · 8 comments
Closed

5.6 regression: Array.prototype.reduce type inference regression #59763

trevorade opened this issue Aug 26, 2024 · 8 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@trevorade
Copy link

πŸ”Ž Search Terms

Inference, Reduce

πŸ•— Version & Regression Information

  • This changed between versions 5.5.4 and 5.6.0-beta (still happening in 5.6.1-rc)

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.6.1-rc#code/JYOwLgpgTgZghgYwgAgMoHsC2EAqBPABxQG8AoZC5CEAVwC40sIBRWzAblIF9TTqbMjbKwHIylZAEFkAXmQBGADTlKAIVnIATMokBhDQGYdlACIaALMp6kAJhAQAbOFBQJ0IAM5hkANzgOPBgxsfCIAbQBdTlI3T28PTH8HCC8RDT8AgDoXGxokAAoVCnzMUBFFX38ASlkAPmQAWTgwAAtM0pASstoKjMz+KuMKYJY2TJNlKs4EpJSwEU4AekXkAD0AfiA

πŸ’» Code

interface SomeType {
    enu: SomeEnum;
}

enum SomeEnum {
    A = 1,
    B = 2,
    C = 3,
    D = 4,
}

declare const vals: SomeType[];

const smallestEnu = vals.reduce(
    (minEnu, val) => Math.min(minEnu, val.enu),
    SomeEnum.D,
);
smallestEnu;
// ^?

πŸ™ Actual behavior

smallestEnu has type number

πŸ™‚ Expected behavior

smallestEnu has type SomeEnum as it did in previous releases

Additional information about the issue

Interestingly, this does not repro if you remove the container type:
https://www.typescriptlang.org/play/?ts=5.6.1-rc#code/KYOwrgtgBAyg9hYBRc0DeAoK2oEEoC8UAjADRY4BChUATOTlAMI0DMDOAIjQCzkC+GDABNgAYwA2AQwBOwKGLggAzgBcooSMoBcsBMlQBtALoBuIYpXrlEKRInA1KMDU0RlAOjnCwY4AAoKbH8IAEsQZ1INcABKQgA+KABZKVUACw8wkBDwyOiwGI5seERnCA9OchjzGzsHJ3BzAHomqAA9AH4MIA

(Google note: relevant source location http://shortn/_JgMRGaMa4e)

@Andarist
Copy link
Contributor

Thanks for the repro! I'll investigate this one soon. Based on my quick check this one is not related to #57909

@RyanCavanaugh
Copy link
Member

πŸ€”

Building TypeScript...
TypeScript built successfully!
git bisect good
a71841c is the first bad commit
commit a71841c
Author: Mateusz BurzyΕ„ski [email protected]
Date: Tue Jun 18 00:25:06 2024 +0200

@RyanCavanaugh
Copy link
Member

I would say this is a correct result. From the compiler's POV, Math.min is no different from Math.pow, and it's inarguable that

const smallestEnu = vals.reduce(
    (minEnu, val) => Math.pow(minEnu, val.enu),
    SomeEnum.D,
);

would more correctly be number than SomeEnum

@trevorade
Copy link
Author

trevorade commented Aug 26, 2024

I can see this being an improvement as well in this specific case.

It's a bit harder though to more abstractly decide what is appropriate behavior for code like:

function func<T>(a: T, b: T): T;

When called with a and b that are slightly different types.

I would argue that past behavior should be the guiding principle.

I have another regression that is similar to this: #59764

@trevorade
Copy link
Author

Confirmed that #59709 does not resolve this issue.

@Andarist
Copy link
Contributor

Andarist commented Aug 27, 2024

Eh, for some reason I was looking at the other playground - the one that doesn't reproduce the issue.

I agree with @RyanCavanaugh that this is more correct (although number enums are at times more permissive/unsound, see bitmasks using them). I don't like how the outcome depends on whether the type is wrapped in a container type or not though. I'll look into this bit

It's a bit harder though to more abstractly decide what is appropriate behavior for code like:
function func(a: T, b: T): T;
When called with a and b that are slightly different types.

Yes, that's true. It's decided today based on heuristics that have been found to produce good results. In a case like this, it tries to find a common supertype among the candidates, and if none is found then the leftmost candidate gets preferred (so whatever was passed in as a). It's a little bit of an oversimplification since getCovariantInference has some more logic. I just want to say that the algorithm for this is not "perfect" - it uses local heuristics and those are subject to change. That doesn't mean that regressions are taken lightly but this is not a set of strict unchangeable rules. Hyrum's law applies here just like anywhere else. IMHO, past behavior can't be the deciding factor in situations like this because it would effectively strangle progress altogether.

@trevorade trevorade changed the title Array.prototype.reduce type inference regression 5.6 regression: Array.prototype.reduce type inference regression Aug 27, 2024
@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Aug 27, 2024
@Andarist
Copy link
Contributor

The difference between those 2 cases is quite surprising here - especially given how this was accidentally affected by the change in inferred type selection. It boils down to how reduce overloads are structured:

    reduce(callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: T[]) => T): T;
    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;

callbackfn is given those 2 types respectively:

(minEnu: SomeEnum, val: SomeType) => number
(minEnu: SomeEnum, enu: SomeEnum) => number

Notice that the first one uses different (unrelated) types for its parameters while the second one has parameters of the same type. So what happens here is that the second case never gets to the inference-based overload*, it stops at the second one since this candidate is perfectly applicable**.

*It gets to it in the first subtype-based pass but not in the second assignable-based pass of chooseOverload
**Perfectly applicable under current set of rules according to which number always is assignable to numeric enums

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

5 participants
@RyanCavanaugh @Andarist @trevorade @typescript-bot and others