Skip to content

Revert PR 61668 #62158

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Revert PR 61668 #62158

wants to merge 3 commits into from

Conversation

jakebailey
Copy link
Member

This reverts #61668

Fixes #62071
Reopens #59937
Reopens #61667

But, it's not clear to me why this sort of thing was not caught in the extended tests, nor do I know if this is a good or bad break, or just differing inference. Posting the revert anyway for discussion.

@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR reverts #61668, a change that appears to have introduced modifications to TypeScript's type inference system. The revert addresses issue #62071 while reopening #59937 and #61667, suggesting the original changes caused unexpected behavior in type inference.

Key changes:

  • Reverts modifications to type inference and signature handling in the TypeScript compiler
  • Removes the outerReturnMapper infrastructure that was added for handling nested inference contexts
  • Restores previous type variable instantiation behavior for single signature types

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

File Description
tests/cases/compiler/inferencePromiseArrays1.ts Adds new test case for Promise array inference scenarios
src/compiler/types.ts Reverts type system changes including SingleSignatureType interface and outerReturnMapper removal
src/compiler/checker.ts Reverts major inference engine changes and signature instantiation logic
tests/baselines/reference/*.types Updates type inference baselines reflecting the reverted behavior
@Andarist
Copy link
Contributor

IMHO this is a just a change in inference. It broke things - sure. But some extra as const or other type hint~ can fix those. It would be great to figure out how to avoid the need for those “workarounds” but at the end of the day, the bugs this PR fixes were far worse.

I looked into one of the breaks related to Promise.all and that was caused by the fact that a contextual type there is now instantiated based on the outer context mapper and that produced an array result. Previously the type would stay uninstantiated and thus it would provide a tuple context hint (because the contextual type stayed to be a homomorphic mapped type).

But anyway, any change in inference can break something - it’s almost an unavoidable risk.

@i-ayushh18
Copy link

🤖 AI Assistant: Task completed: PR #62158: Revert PR 61668

URL: #62158

This reverts #6...

1 similar comment
@i-ayushh18
Copy link

🤖 AI Assistant: Task completed: PR #62158: Revert PR 61668

URL: #62158

This reverts #6...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants