Skip to content

Remove redundant 'reduce' overloads#25455

Closed
ghost wants to merge 4 commits intomasterfrom
arrayReduce
Closed

Remove redundant 'reduce' overloads#25455
ghost wants to merge 4 commits intomasterfrom
arrayReduce

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jul 5, 2018

Fixes #25454

@ghost ghost requested review from DanielRosenwasser and rbuckton July 5, 2018 15:59
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jul 5, 2018

Well.. with the removed overload there is no contextual type for the initial value parameter...

@rbuckton, @DanielRosenwasser and @weswigham thoughts?

@sandersn
Copy link
Copy Markdown
Member

sandersn commented Jul 5, 2018

The contextual type would be incorrect anyway in the case that U !== T, wouldn't it? And I don't think initialValue will commonly be context sensitive. Except in the case of tuples, maybe.

Edit: Shouldn't inference be able to pick up U from the return type of the reducer function and use it as the contextual type? I don't think we're losing much here. Maybe running on the user tests is a good idea though.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 5, 2018

I did come up with a test case that's broken by this, see the bottom of arrayReduce.ts. Probably not a common situation though... I don't think there's a way we can support initialValue having an arbitrary type, and also give it a contextual type based on the assumption that it's the same type as the array elements (which may be a supertype of what you wanted initialValue to be, which caused the original issue).

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 10, 2018

@mhegazy @sandersn @rbuckton @DanielRosenwasser @weswigham Could you review?

1 similar comment
@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 27, 2018

@mhegazy @sandersn @rbuckton @DanielRosenwasser @weswigham Could you review?

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jul 28, 2018

I am still concerned about the valid patterns that will be errors with this change.

@RyanCavanaugh
Copy link
Copy Markdown
Member

@Andy-MS let's get this up to date and then see what the RWC run turns up

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 17, 2018
@ghost ghost force-pushed the arrayReduce branch from 5447cbe to 09229f4 Compare November 9, 2018 18:05
@RyanCavanaugh
Copy link
Copy Markdown
Member

@typescript-bot test this

@typescript-bot
Copy link
Copy Markdown
Collaborator

typescript-bot commented Apr 25, 2019

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at ab17070. You can monitor the build here. It should now contribute to this PR's status checks.

@RyanCavanaugh
Copy link
Copy Markdown
Member

Nontrivial RWC breaks occurred

@RyanCavanaugh RyanCavanaugh deleted the arrayReduce branch April 25, 2019 22:46
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
@typescript-bot
Copy link
Copy Markdown
Collaborator

The TypeScript team hasn't accepted the linked issue #25454. If you can get it accepted, this PR will have a better chance of being reviewed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Poor type inference for reduce

6 participants