Uh oh!
There was an error while loading. Please reload this page.
Allow non-generic return types to be read from single generic call signatures - #54477
Conversation
TypeScript Bot (typescript-bot)
commented
May 31, 2023
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| @@ -0,0 +1,53 @@ | |||
| // @strict: true | |||
There was a problem hiding this comment.
test case reduced from https://github.com/Thinkmill/graphql-ts
interestingly, this one also depends on a quite recent change from #53246
| declare function fn2<T>(): (cb: () => any) => (a: T) => void; | ||
| const res2 = fn2()(() => res2); | ||
| declare function fn3<T>(): <T2>(cb: (arg: T2) => any) => (a: T) => void; | ||
| const res3 = fn3()(() => res3); |
There was a problem hiding this comment.
The only diff between those 2 is the T2 type parameter. The fn2 case works today but the fn3 doesn't - even though its return type doesn't actually depend on T2.
There was a problem hiding this comment.
How does fn3 fail? It looks like inference still fails, given that res3: (a: unknown) => void -- but I'm not sure, and I don't see how to observe what's inferred for T2. Maybe the return type of fn3 should be (a: T) => T2 in the test?
There was a problem hiding this comment.
How does fn3 fail?
We end up with: "'res3' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.(7022)"
It looks like inference still fails, given that res3: (a: unknown) => void
It doesn't exactly fail, it's just that there are no inference candidates here so the constraint is used and that's unknown. This isn't meant to be used as a compelling use case for this PR, just a super minimal repro. The better motivating example is showcased in circularReferenceInReturnType2.ts.
I don't see how to observe what's inferred for T2.
It's just unknown. I re-checked this with console.log(typeToString(inference.typeParameter), typeToString(inference.inferredType)) before exiting getInferredType.
Maybe the return type of fn3 should be (a: T) => T2 in the test?
That's not good here because that reintroduces the circularity because the return type starts to depend on a type parameter again.
…pes-avoid-circularity
TypeScript Bot (@typescript-bot) test this |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the extended test suite on this PR at 3b17f59. You can monitor the build here. |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the diff-based user code test suite (tsserver) on this PR at 3b17f59. You can monitor the build here. Update: The results are in! |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the perf test suite on this PR at 3b17f59. You can monitor the build here. Update: The results are in! |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the diff-based user code test suite on this PR at 3b17f59. You can monitor the build here. Update: The results are in! |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the diff-based top-repos suite (tsserver) on this PR at 3b17f59. You can monitor the build here. Update: The results are in! |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the diff-based top-repos suite on this PR at 3b17f59. You can monitor the build here. Update: The results are in! |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the parallelized Definitely Typed test suite on this PR at 3b17f59. You can monitor the build here. Update: The results are in! |
Heya Nathan Shively-Sanders (@sandersn), I've started to run the tarball bundle task on this PR at 3b17f59. You can monitor the build here. |
Hey Nathan Shively-Sanders (@sandersn), 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 and an npm module you can use via |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| declare function fn2<T>(): (cb: () => any) => (a: T) => void; | ||
| const res2 = fn2()(() => res2); | ||
| declare function fn3<T>(): <T2>(cb: (arg: T2) => any) => (a: T) => void; | ||
| const res3 = fn3()(() => res3); |
There was a problem hiding this comment.
How does fn3 fail? It looks like inference still fails, given that res3: (a: unknown) => void -- but I'm not sure, and I don't see how to observe what's inferred for T2. Maybe the return type of fn3 should be (a: T) => T2 in the test?
TypeScript Bot (typescript-bot)
commented
Jun 20, 2023
Nathan Shively-Sanders (@sandersn) Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
TypeScript Bot (typescript-bot)
commented
Jun 20, 2023
Nathan Shively-Sanders (@sandersn) Here are the results of running the user test suite comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Jun 20, 2023
Nathan Shively-Sanders (@sandersn) Here they are:CompilerComparison Report - main..54477
System
Hosts
Scenarios
TSServerComparison Report - main..54477
System
Hosts
Scenarios
StartupComparison Report - main..54477
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Jun 20, 2023
Hey Nathan Shively-Sanders (@sandersn), the results of running the DT tests are ready. |
TypeScript Bot (typescript-bot)
commented
Jun 20, 2023
Nathan Shively-Sanders (@sandersn) Here are the results of running the top-repos suite comparing Everything looks good! |
1 similar comment
TypeScript Bot (typescript-bot)
commented
Jun 20, 2023
Nathan Shively-Sanders (@sandersn) Here are the results of running the top-repos suite comparing Everything looks good! |
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
Since this isn't a bug fix, let's hold this till right at the beginning of 5.3 to give us plenty of time to watch for problems.
Discovered in microsoft#57117 The implementation should not `couldContainTypeVariables`--it's not intended a fast path, and should not be used in places where its unreliability can be observed. The tests stay, but with a note added that they should pass but do not.
This PR permits circular references in some return types of generic call signatures - something that is already allowed in non-generic call signatures.
If the return type doesn't depend on type variables then the same thing can be allowed for it (including cases with instantiated return types not depending on type variables).