Explicit undefined return type checked similar to explicit void return type - #53607
Conversation
TypeScript Bot (typescript-bot)
added
the
For Milestone Bug
|
TypeScript Bot (@typescript-bot) test this |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at 39ed3b8. You can monitor the build here. |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based user code test suite on this PR at 39ed3b8. You can monitor the build here. Update: The results are in! |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the parallelized Definitely Typed test suite on this PR at 39ed3b8. You can monitor the build here. Update: The results are in! |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the abridged perf test suite on this PR at 39ed3b8. You can monitor the build here. Update: The results are in! |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based top-repos suite on this PR at 39ed3b8. You can monitor the build here. Update: The results are in! |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing Unfortunately, something went wrong, but it probably wasn't caused by your change. |
|
Anders Hejlsberg (@ahejlsberg) Here they are:Comparison Report - main..53607
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. |
|
TypeScript Bot (@typescript-bot) pack this |
|
Heya Gabriela Araujo Britto (@gabritto), I've started to run the tarball bundle task on this PR at 39ed3b8. You can monitor the build here. |
|
Hey Gabriela Araujo Britto (@gabritto), 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 |
|
TypeScript Bot (@typescript-bot) perf test faster |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the abridged perf test suite on this PR at 39ed3b8. You can monitor the build here. Update: The results are in! |
|
Anders Hejlsberg (@ahejlsberg) Here they are:Comparison Report - main..53607
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I reviewed, but let's just answer outstanding comments from Gabriela Araujo Britto (@gabritto) and others first. |
|
TypeScript Bot (@typescript-bot) user test this inline |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based user code test suite on this PR at 39ed3b8. You can monitor the build here. Update: The results are in! |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing Unfortunately, something went wrong, but it probably wasn't caused by your change. |
|
I'm looking into what's going on with those tests. Not sure what's going wrong; it looks green but then decides to say that something failed. |
|
It looks like one of the user tests no longer npm installs correctly; not totally sure why we fail the whole thing due to that, though. Otherwise, reading the logs, this PR shows no change in any of those user tests. |
|
Gabriela Araujo Britto (@gabritto) Yeah, it would be more consistent for all of them to return the error. I will change the logic to have |
| // Ok, contextual type for implicit return is undefined | ||
| } | ||
|
|
||
| const f21: () => undefined | number = () => { |
There was a problem hiding this comment.
For these new cases, I would appreciate keeping the convention from the other tests of indicating if this will error :)
There was a problem hiding this comment.
Sure, I can fix the comments.
|
In addition to addressing Gabriela Araujo Britto (@gabritto)'s feedback, latest commits align the rules for |
|
TypeScript Bot (@typescript-bot) test this |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the parallelized Definitely Typed test suite on this PR at 40f9512. You can monitor the build here. Update: The results are in! |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based user code test suite on this PR at 40f9512. You can monitor the build here. Update: The results are in! |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at 40f9512. You can monitor the build here. |
|
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based top-repos suite on this PR at 40f9512. You can monitor the build here. Update: The results are in! |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing Unfortunately, something went wrong, but it probably wasn't caused by your change. |
|
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
A few new errors in |
|
Otherwise things look good, so I'm going to merge this. |
|
Sorry for replying to an old thread, but can I ask a question about this? Why is this not allowed for unions with function addListener(listener: (ev: Event) => undefined | boolean) {/*...*/}
addListener(ev => {/*... (no return)*/});
// Argument of type '(ev: Event) => void' is not assignable to parameter of type '(ev: Event) => boolean | undefined'.
// Type 'void' is not assignable to type 'boolean | undefined'.I think this could be a nice use case. |
7 participants
This PR (hopefully) completes the work started in #53092 and subsequently modified in #53490. With this and the other PRs, checking of functions changes as follows compared to 5.0:
undefined, expression-lessreturnstatements and the implicit return and the end of the function are considered to have typeundefined(as opposed tovoid). Note that this does not apply to functions with union return types that includeundefined.undefinedaren't required to have return statements (similar to functions with an explicitly specified return type ofvoidorany). Note that this does not apply to functions with union return types that includeundefined.--noImplicitReturnsoption doesn't apply to functions with return types that includeundefined(similar to functions with return types that includevoid,any, orunknown).Fixes #36288.
Fixes #53473.