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

Remove redundant 'reduce' overloads #25455

Closed
wants to merge 4 commits into from
Closed

Remove redundant 'reduce' overloads #25455

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 5, 2018

Fixes #25454

@ghost ghost requested review from DanielRosenwasser and rbuckton July 5, 2018 15:59
@mhegazy
Copy link
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
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
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
Author

ghost commented Jul 10, 2018

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

1 similar comment
@ghost
Copy link
Author

ghost commented Jul 27, 2018

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

@mhegazy
Copy link
Contributor

mhegazy commented Jul 28, 2018

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

@RyanCavanaugh
Copy link
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
@RyanCavanaugh
Copy link
Member

@typescript-bot test this

@typescript-bot
Copy link
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
Member

Nontrivial RWC breaks occurred

@RyanCavanaugh RyanCavanaugh deleted the arrayReduce branch April 25, 2019 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor type inference for reduce
6 participants