Uh oh!
There was an error while loading. Please reload this page.
Do not consider binding patterns in contextual types for return type … - #39081
Merged
weswigham merged 1 commit intoJun 24, 2020
Merged
Conversation
…inference where all the signature type parameters have defaults
weswigham
commented
Jun 15, 2020
MemberAuthor
@typescript-bot user test this |
Contributor
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at a42e1ae. You can monitor the build here. |
Contributor
Heya @weswigham, I've started to run the perf test suite on this PR at a42e1ae. You can monitor the build here. Update: The results are in! |
Contributor
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at a42e1ae. You can monitor the build here. |
Contributor
Heya @weswigham, I've started to run the extended test suite on this PR at a42e1ae. You can monitor the build here. |
typescript-bot
commented
Jun 15, 2020
Contributor
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
weswigham
commented
Jun 15, 2020
MemberAuthor
User baselines are fine, as are rwc baselines. DT has one new failure, in const{ id }=useParams();
exportfunctionuseParams<Paramsextends{[KinkeyofParams]?: string}>(): {[KinkeyofParams]: Params[K]extendsnever ? string : Params[K]}; |
typescript-bot
commented
Jun 15, 2020
Contributor
@weswigham Here they are:Comparison Report - master..39081
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
commented
Jun 15, 2020
MemberAuthor
And perf is fine, not that I expected a diff. |
andrewbranch
approved these changes
Jun 17, 2020
andrewbranch
left a comment
Member
There was a problem hiding this comment.
This seems totally reasonable to me. I’m a bit surprised we ever inferred from binding patterns in the first place.
weswigham
commented
Jun 24, 2020
MemberAuthor
@ahejlsberg wanna take a look at this? I know we've discussed how we handle binding pattern contextual types (and how odd they are) before. |
ahejlsberg
approved these changes
Jun 24, 2020
ahejlsberg
commented
Jun 24, 2020
Member
BTW, looks like this would fix #32003, yes? |
weswigham
commented
Jun 24, 2020
MemberAuthor
easy way to find out: @typescript-bot pack this |
Contributor
Heya @weswigham, I've started to run the tarball bundle task on this PR at a42e1ae. You can monitor the build here. |
weswigham
commented
Jun 24, 2020
MemberAuthor
sigh man that merge commit got deleted the moment I deleted the branch. Anyways, from local testing: Yep, this fixes #32003. So long as the type parameter has a default, we'll prefer it in return type inference over a binding pattern. |
cangSDARM added a commit
to cangSDARM/TypeScript
that referenced
this pull request
Jun 28, 2020
* upstream/master: LEGO: check in for master to temporary branch. Preserve newlines between try/catch/finally, if/else, do/while (microsoft#39280) not narrow static property without type annotation in constructor. (microsoft#39252) Switch to ES Map/Set internally (microsoft#33771) fix(38840): omit completions for a spread like argument in a function definition (microsoft#38897) fix(38785): include in NavigationBar child items from default exported functions (microsoft#38915) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Avoid effect of element access expression (microsoft#39174) Update typescript-eslint to 3.4.1-alpha.1 (microsoft#39260) Handle 'keyof' for generic tuple types (microsoft#39218) Disable unsound T[K] rule in subtype relations (microsoft#39249) LEGO: check in for master to temporary branch. Upgrade typescript-eslint version (microsoft#39242) Handle recursive type references up to a certain level of expansion in inference (microsoft#38011) Do not consider binding patterns in contextual types for return type inference where all the signature type parameters have defaults (microsoft#39081) LEGO: check in for master to temporary branch. # Conflicts: # src/compiler/program.ts # src/compiler/types.ts
sandersn added a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Jun 29, 2020
microsoft/TypeScript#39081, which ships in Typescript 4.0, stops using binding patterns as contextual types for return type inference. react-router's `useParams` relied on this to fill in an object with `any`s in case a binding pattern was used but no type argument was provided. This no longer works. This fix just adds a type argument to the tests. For two other, more complete fixes, see microsoft/TypeScript#39081 (comment) Briefly, the options are (1) go back to returning `any`-filled object types or (2) tries to default to `string`. The second fix is probably the right one, but it may hurt compilation/IDE performance, so I'll leave it to the package owners to decide whether to make that change.
typescript-bot pushed a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Jun 29, 2020
microsoft/TypeScript#39081, which ships in Typescript 4.0, stops using binding patterns as contextual types for return type inference. react-router's `useParams` relied on this to fill in an object with `any`s in case a binding pattern was used but no type argument was provided. This no longer works. This fix just adds a type argument to the tests. For two other, more complete fixes, see microsoft/TypeScript#39081 (comment) Briefly, the options are (1) go back to returning `any`-filled object types or (2) tries to default to `string`. The second fix is probably the right one, but it may hurt compilation/IDE performance, so I'll leave it to the package owners to decide whether to make that change.
ngbrown pushed a commit
to ngbrown-forks/DefinitelyTyped
that referenced
this pull request
Jul 11, 2020
…ersnmicrosoft/TypeScript#39081, which ships in Typescript 4.0, stops using binding patterns as contextual types for return type inference. react-router's `useParams` relied on this to fill in an object with `any`s in case a binding pattern was used but no type argument was provided. This no longer works. This fix just adds a type argument to the tests. For two other, more complete fixes, see microsoft/TypeScript#39081 (comment) Briefly, the options are (1) go back to returning `any`-filled object types or (2) tries to default to `string`. The second fix is probably the right one, but it may hurt compilation/IDE performance, so I'll leave it to the package owners to decide whether to make that change.
danielrearden pushed a commit
to danielrearden/DefinitelyTyped
that referenced
this pull request
Sep 22, 2020
…ersnmicrosoft/TypeScript#39081, which ships in Typescript 4.0, stops using binding patterns as contextual types for return type inference. react-router's `useParams` relied on this to fill in an object with `any`s in case a binding pattern was used but no type argument was provided. This no longer works. This fix just adds a type argument to the tests. For two other, more complete fixes, see microsoft/TypeScript#39081 (comment) Briefly, the options are (1) go back to returning `any`-filled object types or (2) tries to default to `string`. The second fix is probably the right one, but it may hurt compilation/IDE performance, so I'll leave it to the package owners to decide whether to make that change.
This was referenced Sep 2, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…inference where all the signature type parameters have defaults
Fixes#38969
So, when I was looking into #38969 I was tempted to call it working as intended, as it's a logical fallout of current rules for inference and contextual typing; but that felt really unsatisfying, as we know that binding pattern shapes are rather poor inference targets. I went through a few iterations, but here have settled on the simple addition of a new heuristic for inference: If all type parameters the signature we're inferring to have defaults, then we disable the usage of binding patterns in contextual type lookup, which, in turn, allows us to fallback to using those defaults, over those binding pattern types.