Don't widen inferred return types when a contextual signature is available #40311
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
@typescript-bot pack this |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
DanielRosenwasser
added
the
Experiment
|
@DanielRosenwasser Here they are:Comparison Report - master..40311
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Early results: We correctly issue excess property errors as expected. In some cases, we switch from missing property errors to excess property errors. e9b467a#diff-0e97ff0d65c086767a3444a59e9b9e28 We issue one new implicit e9b467a#diff-5a40e91166140ce9c4dc158237e985f1 This seems to have reverted a regression introduced in #29478, and explained in #29478 (comment). I don't really know why this fixes things, but my guess is that we are a little bit more lax about finding the best common type in fresh object types: e9b467a#diff-87ebcaa183574f2823b6ee03abb3316b In TSServer itself, we seem to have caught a bug where we weren't correctly including a https://github.com/microsoft/TypeScript/runs/1043841312#step:7:137 It's unclear whether there's actually a speedup in check-time on material-ui because it's so noisy, but I thought that might be worth mentioning. |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 4f30b40. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 4f30b40. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 4f30b40. You can monitor the build here. |
This comment has been minimized.
This comment has been minimized.
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 4f30b40. You can monitor the build here. |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
One deficiency that I've noticed is that this doesn't work well in the presence of overloads. interface Obj {
a: string;
}
declare function foo(obj: Obj): void;
declare function foo(fn: () => Obj): void;
foo(() => ({ a: "hello", b: 123 }))Right now this doesn't error under this PR (which is bad). Remove the first overload, and you'll an error (which is good). |
|
@DanielRosenwasser Here they are:Comparison Report - master..40311
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
changed the title
|
@typescript-bot user test this |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 4f30b40. You can monitor the build here. |
DanielRosenwasser
removed
the
Experiment
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
| @@ -511,6 +511,7 @@ namespace ts.server.protocol { | |||
| endLocation: Location; | |||
| category: string; | |||
| code: number; | |||
| source?: string; | |||
There was a problem hiding this comment.
Yes, I think @sheetalkamat might have a better idea, but this un-breaks an excess property error we had in the server layer.
There was a problem hiding this comment.
#15296 added this so diagnostics origin can be distinguished.
|
|
||
| ==== tests/cases/conformance/types/tuple/wideningTuples2.ts (1 errors) ==== | ||
| var foo: () => [any] = function bar() { | ||
| let intermediate = bar(); |
There was a problem hiding this comment.
Yeah, as stated in the meeting I do think this is not a reasonable new error in a quite old test (and was dealt with in the prior iteration of this change), and this can be somewhat easily avoided by using the old behavior for named function expressions (which are generally only used when you want recursive references like this anyhow). A self-call witnessing the unwidened type is very odd, given our widening rules (no exterior calls can witness the unwidened type). I think this can manifest in even stranger behavior if you do something with the bar reference rather than just call it - like pass it into a function desiring a callback, eg handle(bar) where declare function handle(a: () => [any]): void, or worse yet declare function handle<T>(a: T): T where the unwidened type will simply propagate via inference - where then you'll get this widening error so far removed from anything that it's relevant to that it's quite clear something's gone wrong.
|
Is merging this still a possibility or will this PR ultimately be abandoned? |
|
The TypeScript team hasn't accepted the linked issue #241. If you can get it accepted, this PR will have a better chance of being reviewed. |
|
@DanielRosenwasser is this change still worth taking? If so, can you address @weswigham's idea for named functions? |
|
Hey everyone, is this pull request still on track to land any time soon? |
|
This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. |
|
@DanielRosenwasser @sandersn I just skimmed the history here and it looks like this implements fixes for a large number of issues, including one that's over 7 years old (filed by @RyanCavanaugh , with 58 upvotes). As far as I can tell the only thing that prevented merging was #40311 (comment) , but there was no further discussion about whether this would be easy to fix. Is my assessment accurate? Is this likely to get picked back up at some later point by the current triage process? |
|
Looking at the history, it seems like @DanielRosenwasser and @weswigham disagreed about the outcome of a design meeting. My guess is that we should bring it up there again. It's up to @DanielRosenwasser though, since he wrote the code and puts together the agenda. |
|
Linking to a relevant StackOverflow question that would've been solved by this PR - I've yet to find any alternative solutions to this. Related test in the typescript playground. |
|
@DanielRosenwasser @sandersn it is now a year later, was this brought up for discussion internally? What will it take to get this resolved? |
|
We've encountered this issue many times before and would love this fix to be included in the next TypeScript release. |
Background
This PR has two high-level objectives:
anys in type argument inference outside ofstrictNullChecksThis PR achieves it by avoiding widening return types for function expressions that have been inferred from the body in cases where the current function has a contextual signature.
Widening Poisoning Inference With
anyHere, we have two function expressions passed into
pickRandom. As it currently behaves, TypeScript will try to figure out the type of each function from its body, and in the function() => null, it will find the typenull. Outside ofstrictNullChecks, the type system then widens thatnulltoany. When it has to infer a type forT, it finds the candidatesnumberandany, which just becomesany. This is a result of widening too early, and it's undesirable to letanyabsorb thenumbertype instead of letting thenumbertype absorb thenulltype.Note that that function's return type isn't an implicit
anybecause the function has a contextual signature (i.e. it's being contextually typed). We make an exception in this case. The rationale for that behavior is that it makes very little sense in many cases to issue an implicitanyerror when there's a contextual type and we know that theanyisn't going to escape; however, in the presence of generics, thatanydoes escape.This behavior is bad, but it's not as bad as it used to be because we live in a world with
strictNullChecks.No Excess Property Checking
Today, if you have the following
Here, TypeScript gives an excess property error on the assignment to
opts, but not within the callback toconfig.The reason is that when it has to infer the type for
() => ({ foo: "hallo", ba: "oh no", }), it picks up a fresh object type for{ foo: "hallo", ba: "oh no", }as an initial candidate, and then widens that to its non-fresh version; however, freshness determines whether excess property checks are performed when determining compatibility againstOptions. This is another result of widening too early.This seems to be the behavior most users we've heard are concerned with, which makes sense because users concerned with stricter checks might be the type to opt themselves into
strictNullChecks.Proposal
The proposal here is to only widen return types for function expressions when there is no contextual signature. The same is proposed for yielded and next'd types for generator expressions.
The key idea is that if a contextual type is found, we can be fairly sure that a return expression is going to need to be validated against that contextual type to some extent. In those cases, you really don't want to widen prematurely. In fact, this same logic is the reason we don't report a
noImplicitAny.Future Work
There are cases beyond checking whether there is no contextual signature (e.g. any argument position in a function call expression); however, those cases are no worse off than they are today.
There are also cases where we'll still widen too early at type argument inference.
We can do better in both these cases in the future.
Relevant Issues
Fixes #241
Subsumes #20976
Duplicates
Fixes #7547
Fixes #20859
Fixes #27237
Fixes #28634
Fixes #30096
Fixes #30198
Fixes #31235
Fixes #12632
Fixes #11895
Fixes #33908
Fixes #37017
Fixes #39635
Fixes #40270
Fixes #40307
Probable Duplicates
I don't feel like installing React/React Native to test these.
Fixes #29390
Fixes #29499
Related But Not Fixed
Does not fix the following:
#7220
#10245 (comment)
#20008
#26999
#31425
#36053
#36354
#31254