Skip to content

Stop looking at binding patterns for type argument inference #45719

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 8, 2021
Merged

Stop looking at binding patterns for type argument inference #45719

merged 2 commits into from
Sep 8, 2021

Conversation

andrewbranch
Copy link
Member

Binding patterns are already sketchy enough as contextual types where type argument inference is not involved, but I see no good reason to let them play a part in actual generic inference.

Fixes #45663
Fixes #43605

Related: #39081

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 3, 2021
@andrewbranch
Copy link
Member Author

@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot test this

@typescript-bot
Copy link
Collaborator

Heya @andrewbranch, I'm starting to run the extended test suite on this PR at c8f6392. Hold tight - I'll update this comment with the log link once the build has been queued.

@typescript-bot
Copy link
Collaborator

Heya @andrewbranch, I'm starting to run the parallelized Definitely Typed test suite on this PR at c8f6392. Hold tight - I'll update this comment with the log link once the build has been queued.

@typescript-bot
Copy link
Collaborator

Heya @andrewbranch, I'm starting to run the inline community code test suite on this PR at c8f6392. Hold tight - I'll update this comment with the log link once the build has been queued.

@andrewbranch andrewbranch added the Breaking Change Would introduce errors in existing code label Sep 3, 2021
@andrewbranch andrewbranch added the Experiment A fork with an experimental idea which might not make it into master label Sep 3, 2021
andrewbranch
andrewbranch commented Sep 3, 2021
View reviewed changes
tests/baselines/reference/destructuringTuple.types
>oops1 : any
>[1, 2, 3].reduce((accu, el) => accu.concat(el), []) : number
>oops1 : never
>[1, 2, 3].reduce((accu, el) => accu.concat(el), []) : never[]
Copy link
Member Author

Choose a reason for hiding this comment

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

We used to think [1, 2, 3].reduce((accu, el) => accu.concat(el), []) is a number. You probably need the (unfortunate) overload resolution error in the function body to get that far off track in the first place, but there’s really no reason why destructuring [oops1] from the result should have any effect on what we think this type is.

@andrewbranch
Copy link
Member Author

@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 7, 2021 •
edited
Loading

Heya @andrewbranch, I've started to run the inline community code test suite on this PR at c8f6392. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 7, 2021 •
edited
Loading

Heya @andrewbranch, I've started to run the extended test suite on this PR at c8f6392. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 7, 2021 •
edited
Loading

Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at c8f6392. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@andrewbranch
Great news! no new errors were found between main..refs/pull/45719/merge

sandersn
sandersn approved these changes Sep 7, 2021
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This sounds reasonable to me, although I'd like a second opinion from @weswigham in case there are any unforeseen consequences.

(Although shipping early in 4.5 is probably enough to turn up those consequences as well.)

weswigham
weswigham approved these changes Sep 8, 2021
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Yeah, I mean - I made the original change to elide them when type parameters had defaults; binding patterns are pretty bad inference sources - probably the worst ones we have.

@andrewbranch andrewbranch merged commit be618b1 into microsoft:main Sep 8, 2021
@andrewbranch andrewbranch deleted the bug/43605 branch September 8, 2021 00:14
@andrewbranch andrewbranch mentioned this pull request Sep 13, 2021
Closed
@rbuckton rbuckton mentioned this pull request Sep 16, 2021
Closed
@andrewbranch andrewbranch mentioned this pull request Sep 22, 2021
Closed
andrewbranch added a commit that referenced this pull request Sep 22, 2021
@andrewbranch
Revert "Stop looking at binding patterns for type argument inference (#…
c0e5bad 
…45719)"

This reverts commit be618b1.
@andrewbranch andrewbranch mentioned this pull request Sep 22, 2021
Merged
andrewbranch added a commit that referenced this pull request Sep 23, 2021
@andrewbranch
Revert "Stop looking at binding patterns for type argument inference" (…
0396723 
…#46013)

* Revert "Stop looking at binding patterns for type argument inference (#45719)"

This reverts commit be618b1.

* Update error baseline for moved lib file declaration
@andrewbranch andrewbranch mentioned this pull request May 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

@weswigham weswigham weswigham approved these changes

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

Assignees

@andrewbranch andrewbranch

Labels
Author: Team Breaking Change Would introduce errors in existing code Experiment A fork with an experimental idea which might not make it into master For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
4 participants
@andrewbranch @typescript-bot @sandersn @weswigham