Skip to content

Stop looking at binding patterns for type argument inference - #45719

Merged
andrewbranch merged 2 commits into
microsoft:mainfrom
andrewbranch:bug/43605
Sep 8, 2021
Merged

Stop looking at binding patterns for type argument inference#45719
andrewbranch merged 2 commits into
microsoft:mainfrom
andrewbranch:bug/43605

Conversation

@andrewbranch

@andrewbranchandrewbranch commented Sep 3, 2021

Copy link
Copy Markdown
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-bottypescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 3, 2021
@andrewbranch

andrewbranch commented Sep 3, 2021

Copy link
Copy Markdown
MemberAuthor

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

@typescript-bot

typescript-bot commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

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

typescript-bot commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

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

typescript-bot commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

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.

@andrewbranchandrewbranch added the Breaking Change Would introduce errors in existing code label Sep 3, 2021
@andrewbranchandrewbranch 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
Comment threadtests/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[]

@andrewbranchandrewbranchSep 3, 2021

Copy link
Copy Markdown
MemberAuthor

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

andrewbranch commented Sep 7, 2021

Copy link
Copy Markdown
MemberAuthor

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

@typescript-bot

typescript-bot commented Sep 7, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

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

typescript-bot commented Sep 7, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@typescript-bot

typescript-bot commented Sep 7, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

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

typescript-bot commented Sep 7, 2021

Copy link
Copy Markdown
Contributor

@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

@sandersnsandersn left a comment

Copy link
Copy Markdown
Member

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

@weswighamweswigham left a comment

Copy link
Copy Markdown
Member

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:mainSep 8, 2021
@andrewbranch
andrewbranch deleted the bug/43605 branch September 8, 2021 00:14
@andrewbranchandrewbranch mentioned this pull request Sep 13, 2021
Closed
@rbucktonrbuckton mentioned this pull request Sep 16, 2021
Closed
@andrewbranchandrewbranch 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.
@andrewbranchandrewbranch 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
@andrewbranchandrewbranch mentioned this pull request May 12, 2022
Merged
@microsoftmicrosoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersnsandersnsandersn approved these changes
@weswighamweswighamweswigham approved these changes
@ahejlsbergahejlsbergAwaiting requested review from ahejlsberg

Assignees

@andrewbranchandrewbranch

Labels

Author: TeamBreaking ChangeWould introduce errors in existing codeExperimentA fork with an experimental idea which might not make it into masterFor Milestone BugPRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

4 participants

@andrewbranch@typescript-bot@sandersn@weswigham