Skip to content

Explicit undefined return type checked similar to explicit void return type - #53607

Merged
Anders Hejlsberg (ahejlsberg) merged 9 commits into
mainfrom
fix36288
Apr 1, 2023
Merged

Explicit undefined return type checked similar to explicit void return type#53607
Anders Hejlsberg (ahejlsberg) merged 9 commits into
mainfrom
fix36288

Conversation

@ahejlsberg

Copy link
Copy Markdown
Member

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:

  • In functions with an explicit or contextual return type undefined, expression-less return statements and the implicit return and the end of the function are considered to have type undefined (as opposed to void). Note that this does not apply to functions with union return types that include undefined.
  • Functions with an explicitly specified return type undefined aren't required to have return statements (similar to functions with an explicitly specified return type of void or any). Note that this does not apply to functions with union return types that include undefined.
  • The --noImplicitReturns option doesn't apply to functions with return types that include undefined (similar to functions with return types that include void, any, or unknown).

Fixes#36288.
Fixes#53473.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at 39ed3b8. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing main and refs/pull/53607/merge:

Unfortunately, something went wrong, but it probably wasn't caused by your change.

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..53607

Metricmain53607DeltaBestWorstp-value
Angular - node (v16.17.1, x64)
Memory used361,673k (± 0.00%)361,744k (± 0.01%)+71k (+ 0.02%)361,699k361,785kp=0.005 n=6
Parse Time3.51s (± 0.43%)3.53s (± 0.75%)~3.50s3.56sp=0.331 n=6
Bind Time1.17s (± 0.76%)1.18s (± 0.64%)~1.17s1.19sp=0.053 n=6
Check Time9.49s (± 0.27%)9.52s (± 0.20%)+0.04s (+ 0.39%)9.49s9.54sp=0.019 n=6
Emit Time7.95s (± 0.75%)8.03s (± 1.06%)~7.94s8.17sp=0.128 n=6
Total Time22.12s (± 0.34%)22.27s (± 0.41%)+0.14s (+ 0.64%)22.15s22.40sp=0.013 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used192,523k (± 0.03%)192,581k (± 0.03%)~192,485k192,635kp=0.199 n=6
Parse Time1.59s (± 1.17%)1.60s (± 0.39%)~1.59s1.61sp=1.000 n=6
Bind Time0.83s (± 0.66%)0.83s (± 1.27%)~0.81s0.84sp=1.000 n=6
Check Time10.35s (± 0.67%)10.36s (± 0.56%)~10.29s10.42sp=0.746 n=6
Emit Time2.99s (± 0.70%)3.00s (± 1.10%)~2.97s3.06sp=0.871 n=6
Total Time15.75s (± 0.67%)15.79s (± 0.50%)~15.70s15.90sp=0.872 n=6
Monaco - node (v16.17.1, x64)
Memory used345,757k (± 0.00%)345,677k (± 0.00%)-81k (- 0.02%)345,649k345,691kp=0.005 n=6
Parse Time2.72s (± 0.54%)2.74s (± 0.48%)~2.72s2.75sp=0.050 n=6
Bind Time1.08s (± 0.50%)1.08s (± 0.77%)~1.07s1.09sp=0.855 n=6
Check Time7.77s (± 0.30%)7.77s (± 0.56%)~7.73s7.85sp=0.567 n=6
Emit Time4.46s (± 0.67%)4.50s (± 0.70%)~4.45s4.54sp=0.127 n=6
Total Time16.03s (± 0.24%)16.09s (± 0.43%)~16.04s16.22sp=0.090 n=6
TFS - node (v16.17.1, x64)
Memory used300,032k (± 0.01%)300,061k (± 0.01%)~300,032k300,114kp=0.128 n=6
Parse Time2.16s (± 0.63%)2.17s (± 0.86%)~2.16s2.21sp=0.149 n=6
Bind Time1.23s (± 0.80%)1.24s (± 0.94%)~1.23s1.26sp=0.209 n=6
Check Time7.18s (± 0.41%)7.24s (± 0.68%)+0.06s (+ 0.84%)7.21s7.34sp=0.018 n=6
Emit Time4.35s (± 0.86%)4.34s (± 0.84%)~4.30s4.39sp=0.747 n=6
Total Time14.92s (± 0.43%)15.00s (± 0.63%)~14.92s15.19sp=0.199 n=6
material-ui - node (v16.17.1, x64)
Memory used476,461k (± 0.01%)476,455k (± 0.01%)~476,418k476,488kp=0.810 n=6
Parse Time3.22s (± 0.38%)3.24s (± 0.55%)+0.02s (+ 0.73%)3.21s3.26sp=0.042 n=6
Bind Time0.95s (± 0.00%)0.96s (± 0.43%)+0.01s (+ 0.88%)0.95s0.96sp=0.007 n=6
Check Time18.13s (± 0.56%)18.24s (± 0.43%)~18.14s18.38sp=0.149 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time22.30s (± 0.46%)22.44s (± 0.42%)~22.30s22.59sp=0.066 n=6
xstate - node (v16.17.1, x64)
Memory used550,589k (± 0.02%)550,621k (± 0.02%)~550,485k550,758kp=0.689 n=6
Parse Time3.93s (± 0.21%)3.94s (± 0.25%)~3.93s3.95sp=0.140 n=6
Bind Time1.75s (± 0.86%)1.76s (± 0.51%)~1.75s1.77sp=0.557 n=6
Check Time3.06s (± 0.45%)3.09s (± 0.96%)~3.06s3.13sp=0.084 n=6
Emit Time0.09s (± 0.00%)0.09s (± 0.00%)~0.09s0.09sp=1.000 n=6
Total Time8.83s (± 0.23%)8.88s (± 0.38%)+0.04s (+ 0.47%)8.84s8.93sp=0.029 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
BenchmarkNameIterations
Current536076
Baselinemain6

Developer Information:

Download Benchmark

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing main and refs/pull/53607/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@gabritto

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

Hey Gabriela Araujo Britto (@gabritto), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/151418/artifacts?artifactName=tgz&fileId=B89711228EC2A48ADEDB42C169AA808EADFEBAC64380B790A7B2A3C7A4798BEE02&fileName=/typescript-5.1.0-insiders.20230331.tgz"
}
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-53607-12".;

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) perf test faster

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..53607

Metricmain53607DeltaBestWorstp-value
Angular - node (v16.17.1, x64)
Memory used361,701k (± 0.01%)361,738k (± 0.01%)+37k (+ 0.01%)361,697k361,754kp=0.045 n=6
Parse Time3.51s (± 1.00%)3.51s (± 0.70%)~3.47s3.53sp=0.935 n=6
Bind Time1.18s (± 0.54%)1.18s (± 1.13%)~1.16s1.20sp=0.720 n=6
Check Time9.52s (± 0.15%)9.53s (± 0.52%)~9.48s9.60sp=0.936 n=6
Emit Time7.98s (± 0.54%)7.96s (± 0.67%)~7.88s8.04sp=0.517 n=6
Total Time22.19s (± 0.27%)22.18s (± 0.43%)~22.10s22.33sp=0.470 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used193,105k (± 0.72%)194,643k (± 0.85%)~192,497k195,759kp=0.298 n=6
Parse Time1.59s (± 1.66%)1.59s (± 0.47%)~1.58s1.60sp=0.805 n=6
Bind Time0.83s (± 0.91%)0.82s (± 0.50%)-0.01s (- 1.21%)0.81s0.82sp=0.024 n=6
Check Time10.34s (± 0.74%)10.33s (± 0.42%)~10.25s10.37sp=0.470 n=6
Emit Time3.00s (± 0.46%)2.99s (± 0.95%)~2.96s3.04sp=0.459 n=6
Total Time15.75s (± 0.45%)15.73s (± 0.41%)~15.62s15.82sp=0.373 n=6
Monaco - node (v16.17.1, x64)
Memory used345,738k (± 0.01%)345,680k (± 0.00%)-57k (- 0.02%)345,669k345,695kp=0.005 n=6
Parse Time2.72s (± 0.46%)2.71s (± 0.43%)~2.69s2.72sp=0.210 n=6
Bind Time1.08s (± 0.50%)1.08s (± 1.08%)~1.07s1.10sp=0.498 n=6
Check Time7.81s (± 0.45%)7.76s (± 0.44%)-0.05s (- 0.66%)7.70s7.80sp=0.036 n=6
Emit Time4.46s (± 0.52%)4.43s (± 0.78%)~4.39s4.47sp=0.220 n=6
Total Time16.08s (± 0.33%)15.99s (± 0.40%)~15.92s16.07sp=0.077 n=6
TFS - node (v16.17.1, x64)
Memory used300,030k (± 0.00%)300,051k (± 0.00%)+21k (+ 0.01%)300,034k300,074kp=0.045 n=6
Parse Time2.17s (± 0.50%)2.16s (± 1.03%)~2.14s2.20sp=0.251 n=6
Bind Time1.23s (± 0.80%)1.23s (± 0.42%)~1.22s1.23sp=0.348 n=6
Check Time7.20s (± 0.20%)7.22s (± 0.58%)~7.17s7.29sp=0.329 n=6
Emit Time4.34s (± 0.38%)4.33s (± 0.91%)~4.29s4.38sp=0.573 n=6
Total Time14.96s (± 0.25%)14.94s (± 0.53%)~14.88s15.05sp=0.571 n=6
material-ui - node (v16.17.1, x64)
Memory used476,456k (± 0.00%)476,461k (± 0.01%)~476,426k476,521kp=0.936 n=6
Parse Time3.22s (± 0.46%)3.21s (± 0.23%)~3.20s3.22sp=0.241 n=6
Bind Time0.95s (± 0.43%)0.95s (± 0.79%)~0.94s0.96sp=1.000 n=6
Check Time18.15s (± 0.73%)18.21s (± 0.38%)~18.12s18.30sp=0.297 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time22.32s (± 0.66%)22.37s (± 0.35%)~22.27s22.48sp=0.378 n=6
xstate - node (v16.17.1, x64)
Memory used550,624k (± 0.03%)550,615k (± 0.02%)~550,483k550,782kp=0.810 n=6
Parse Time3.95s (± 0.49%)3.92s (± 0.46%)-0.03s (- 0.80%)3.90s3.95sp=0.029 n=6
Bind Time1.76s (± 0.23%)1.76s (± 0.56%)~1.75s1.77sp=0.930 n=6
Check Time3.08s (± 0.61%)3.06s (± 0.45%)~3.04s3.08sp=0.089 n=6
Emit Time0.09s (± 0.00%)0.09s (± 0.00%)~0.09s0.09sp=1.000 n=6
Total Time8.88s (± 0.40%)8.83s (± 0.30%)-0.05s (- 0.56%)8.79s8.86sp=0.043 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
BenchmarkNameIterations
Current536076
Baselinemain6

Developer Information:

Download Benchmark

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Mar 31, 2023

Copy link
Copy Markdown
Member

I reviewed, but let's just answer outstanding comments from Gabriela Araujo Britto (@gabritto) and others first.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

TypeScript Bot (@typescript-bot) user test this inline

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing main and refs/pull/53607/merge:

Unfortunately, something went wrong, but it probably wasn't caused by your change.

@jakebailey

Copy link
Copy Markdown
Member

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.

@jakebailey

Copy link
Copy Markdown
Member

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.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

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 noImplicitReturns only exclude functions that have exactly undefined as their return type.

Comment threadsrc/services/codefixes/returnValueCorrect.ts
// Ok, contextual type for implicit return is undefined
}

const f21: () => undefined | number = () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these new cases, I would appreciate keeping the convention from the other tests of indicating if this will error :)

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.

Sure, I can fix the comments.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

In addition to addressing Gabriela Araujo Britto (@gabritto)'s feedback, latest commits align the rules for --noImplicitReturns to remove inconsistencies between functions with explicit unknown return types and functions with inferred unknown return types. Previously, only functions with explicit unknown return types would issue errors for implicit returns in --noImplicitReturns mode. Now both do.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 1, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 1, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 1, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at 40f9512. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 1, 2023

Copy link
Copy Markdown
Contributor

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!

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the user test suite comparing main and refs/pull/53607/merge:

Unfortunately, something went wrong, but it probably wasn't caused by your change.

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing main and refs/pull/53607/merge:

Something interesting changed - please have a look.

Details

microsoft/vscode

4 of 53 projects failed to build with the old tsc and were ignored

extensions/references-view/tsconfig.json

src/tsconfig.tsec.json

vuetifyjs/vuetify

4 of 7 projects failed to build with the old tsc and were ignored

packages/vuetify/tsconfig.checks.json

packages/vuetify/tsconfig.dist.json

packages/vuetify/tsconfig.json

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

A few new errors in vscode and vuetify related to the change I mention here. I think these new errors are acceptable--the projects have noImplicitReturns enabled, the functions in question have inferred return types unknown (or Promise<unknown>), and the errors would have been reported if the functions had explicit return type annotations. It looks to me like all of the examples actually intended for the functions to return void but inadvertently returned values in some conditional code paths.

@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 1, 2023

Copy link
Copy Markdown
MemberAuthor

Otherwise things look good, so I'm going to merge this.

@stevenwdv

Copy link
Copy Markdown

Sorry for replying to an old thread, but can I ask a question about this?

Why is this not allowed for unions with undefined? E.g. for event listeners:

functionaddListener(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.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

None yet

7 participants

@ahejlsberg@typescript-bot@gabritto@DanielRosenwasser@jakebailey@stevenwdv@MariaSolOs