Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag to change catch variables' default types to unknown #41013

Merged
merged 9 commits into from
Jun 3, 2021

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Oct 9, 2020

This change adds a new flag called useUnknownInCatchVariables which changes the default type of catch clause variables from any (today's existing behavior) to unknown.

More specifically, under this flag, the following code

try {
  // ...
}
catch (err) {
  console.log(err.message);
}

would become equivalent to

try {
  // ...
}
catch (err: unknown) {
  console.log(err.message); // error: Property 'message' does not exist on type 'unknown'.
}

As a result, a user would receive the following error message from TypeScript:

Property 'message' does not exist on type 'unknown'.

To mitigate this, a user could explicitly perform runtime checks

try {
  // ...
}
catch (err) {
  // First check if we have an Error
  if (err instanceof Error) {
    console.log(err.message);
  }
}

or if that is too painful, a user could use a type assertion to any, or provide an explicit annotation on the catch clause variable with the type any to revert to the old default behavior.

try {
  // ...
}
catch (err: any) {
  console.log(err.message);
}

Fixes #41016.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 9, 2020
@DanielRosenwasser DanielRosenwasser changed the title 'unknown' in catch variables --useUnknownInCatchVariables Oct 9, 2020
@DanielRosenwasser DanielRosenwasser changed the title --useUnknownInCatchVariables Add flag to change 'catch' variable default to 'unknown' Oct 9, 2020
@DanielRosenwasser DanielRosenwasser marked this pull request as ready for review October 9, 2020 21:10
@sandersn sandersn added the Experiment A fork with an experimental idea which might not make it into master label Oct 20, 2020
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #41016. If you can get it accepted, this PR will have a better chance of being reviewed.

@johnnyreilly
Copy link

johnnyreilly commented Feb 10, 2021

It would be awesome if strict also enabled this flag; is that likely? (It's possible that this change does that already; but I'm on my phone so it's hard to tell 😅)

@DanielRosenwasser DanielRosenwasser force-pushed the unknownInCatchVariables branch from 7c01840 to f529457 Compare May 5, 2021 21:36
@DanielRosenwasser
Copy link
Member Author

@typescript-bot pack this
@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at a32013b. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at a32013b. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at a32013b. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at a32013b. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at a32013b. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Hey @DanielRosenwasser, 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/102337/artifacts?artifactName=tgz&fileId=CB39C963580F3F2B857D3171C4A6770212DE0222B86B618B209B2F4B2E12A36B02&fileName=/typescript-4.3.0-insiders.20210505.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/[email protected]".;

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..41013

Metric master 41013 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 344,651k (± 0.02%) 344,706k (± 0.03%) +54k (+ 0.02%) 344,444k 344,928k
Parse Time 1.91s (± 0.55%) 1.90s (± 0.43%) -0.01s (- 0.52%) 1.89s 1.92s
Bind Time 0.84s (± 0.57%) 0.84s (± 0.57%) 0.00s ( 0.00%) 0.83s 0.85s
Check Time 5.25s (± 0.47%) 5.25s (± 0.31%) +0.00s (+ 0.06%) 5.23s 5.29s
Emit Time 5.89s (± 0.38%) 5.91s (± 1.20%) +0.02s (+ 0.36%) 5.80s 6.16s
Total Time 13.89s (± 0.29%) 13.91s (± 0.58%) +0.02s (+ 0.12%) 13.78s 14.15s
Compiler-Unions - node (v10.16.3, x64)
Memory used 200,759k (± 0.03%) 200,821k (± 0.03%) +62k (+ 0.03%) 200,671k 200,923k
Parse Time 0.79s (± 0.51%) 0.78s (± 0.97%) -0.01s (- 0.63%) 0.77s 0.80s
Bind Time 0.53s (± 1.27%) 0.52s (± 0.91%) -0.00s (- 0.76%) 0.51s 0.53s
Check Time 7.59s (± 0.57%) 7.59s (± 0.65%) -0.00s (- 0.05%) 7.47s 7.72s
Emit Time 2.51s (± 1.43%) 2.48s (± 0.90%) -0.03s (- 1.12%) 2.43s 2.52s
Total Time 11.42s (± 0.47%) 11.38s (± 0.53%) -0.04s (- 0.36%) 11.24s 11.56s
Monaco - node (v10.16.3, x64)
Memory used 341,715k (± 0.01%) 341,688k (± 0.02%) -27k (- 0.01%) 341,565k 341,920k
Parse Time 1.56s (± 0.50%) 1.57s (± 0.61%) +0.01s (+ 0.38%) 1.54s 1.59s
Bind Time 0.74s (± 0.78%) 0.74s (± 0.98%) -0.00s (- 0.13%) 0.73s 0.76s
Check Time 5.38s (± 0.34%) 5.38s (± 0.52%) -0.00s (- 0.02%) 5.31s 5.45s
Emit Time 3.02s (± 0.86%) 3.01s (± 0.75%) -0.01s (- 0.20%) 2.97s 3.06s
Total Time 10.70s (± 0.39%) 10.70s (± 0.45%) -0.00s (- 0.05%) 10.59s 10.80s
TFS - node (v10.16.3, x64)
Memory used 304,178k (± 0.03%) 304,226k (± 0.02%) +48k (+ 0.02%) 304,099k 304,366k
Parse Time 1.22s (± 0.55%) 1.21s (± 0.48%) -0.00s (- 0.33%) 1.20s 1.23s
Bind Time 0.71s (± 0.78%) 0.71s (± 1.03%) -0.00s (- 0.56%) 0.69s 0.72s
Check Time 4.79s (± 0.42%) 4.80s (± 0.47%) +0.01s (+ 0.23%) 4.73s 4.84s
Emit Time 3.21s (± 1.33%) 3.18s (± 1.60%) -0.03s (- 1.00%) 3.03s 3.27s
Total Time 9.93s (± 0.53%) 9.90s (± 0.66%) -0.02s (- 0.24%) 9.76s 10.03s
material-ui - node (v10.16.3, x64)
Memory used 474,572k (± 0.02%) 474,591k (± 0.01%) +19k (+ 0.00%) 474,488k 474,712k
Parse Time 1.94s (± 0.79%) 1.94s (± 0.41%) -0.00s (- 0.10%) 1.92s 1.96s
Bind Time 0.65s (± 1.23%) 0.65s (± 0.80%) -0.00s (- 0.15%) 0.64s 0.66s
Check Time 14.81s (± 0.66%) 14.76s (± 0.64%) -0.06s (- 0.39%) 14.63s 15.10s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 17.41s (± 0.63%) 17.35s (± 0.54%) -0.06s (- 0.35%) 17.24s 17.70s
Angular - node (v12.1.0, x64)
Memory used 322,324k (± 0.02%) 322,310k (± 0.02%) -14k (- 0.00%) 322,163k 322,427k
Parse Time 1.91s (± 0.63%) 1.91s (± 0.66%) -0.00s (- 0.05%) 1.88s 1.94s
Bind Time 0.82s (± 0.73%) 0.82s (± 0.70%) -0.00s (- 0.61%) 0.81s 0.84s
Check Time 5.17s (± 0.48%) 5.14s (± 0.42%) -0.03s (- 0.52%) 5.11s 5.21s
Emit Time 6.16s (± 0.82%) 6.15s (± 0.87%) -0.02s (- 0.24%) 6.06s 6.27s
Total Time 14.07s (± 0.49%) 14.02s (± 0.46%) -0.05s (- 0.35%) 13.91s 14.21s
Compiler-Unions - node (v12.1.0, x64)
Memory used 187,812k (± 0.14%) 188,033k (± 0.15%) +221k (+ 0.12%) 187,234k 188,396k
Parse Time 0.77s (± 0.62%) 0.77s (± 0.84%) -0.01s (- 0.65%) 0.76s 0.78s
Bind Time 0.53s (± 0.89%) 0.53s (± 0.98%) -0.00s (- 0.19%) 0.52s 0.54s
Check Time 7.04s (± 0.64%) 7.02s (± 0.42%) -0.02s (- 0.30%) 6.98s 7.10s
Emit Time 2.49s (± 1.00%) 2.47s (± 1.28%) -0.02s (- 1.00%) 2.43s 2.57s
Total Time 10.84s (± 0.63%) 10.79s (± 0.32%) -0.05s (- 0.49%) 10.69s 10.85s
Monaco - node (v12.1.0, x64)
Memory used 324,057k (± 0.02%) 324,057k (± 0.01%) +0k (+ 0.00%) 323,992k 324,106k
Parse Time 1.54s (± 0.71%) 1.54s (± 0.87%) +0.00s (+ 0.19%) 1.52s 1.58s
Bind Time 0.72s (± 1.20%) 0.72s (± 0.62%) -0.00s (- 0.55%) 0.71s 0.73s
Check Time 5.20s (± 0.29%) 5.21s (± 0.63%) +0.01s (+ 0.23%) 5.13s 5.28s
Emit Time 3.06s (± 0.70%) 3.06s (± 1.26%) -0.00s (- 0.03%) 3.02s 3.21s
Total Time 10.52s (± 0.33%) 10.54s (± 0.60%) +0.01s (+ 0.12%) 10.41s 10.72s
TFS - node (v12.1.0, x64)
Memory used 288,711k (± 0.01%) 288,704k (± 0.01%) -7k (- 0.00%) 288,622k 288,776k
Parse Time 1.21s (± 0.78%) 1.21s (± 0.43%) -0.00s (- 0.16%) 1.20s 1.22s
Bind Time 0.70s (± 1.16%) 0.69s (± 0.86%) -0.01s (- 1.14%) 0.68s 0.71s
Check Time 4.70s (± 0.61%) 4.70s (± 0.45%) -0.00s (- 0.04%) 4.66s 4.75s
Emit Time 3.18s (± 1.55%) 3.17s (± 0.46%) -0.01s (- 0.35%) 3.14s 3.21s
Total Time 9.80s (± 0.64%) 9.78s (± 0.24%) -0.02s (- 0.22%) 9.73s 9.83s
material-ui - node (v12.1.0, x64)
Memory used 452,433k (± 0.05%) 452,478k (± 0.01%) +45k (+ 0.01%) 452,351k 452,672k
Parse Time 1.94s (± 0.38%) 1.95s (± 0.37%) +0.01s (+ 0.52%) 1.93s 1.96s
Bind Time 0.64s (± 0.63%) 0.64s (± 1.17%) -0.00s (- 0.31%) 0.63s 0.66s
Check Time 13.30s (± 0.52%) 13.26s (± 0.40%) -0.04s (- 0.32%) 13.15s 13.41s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.88s (± 0.43%) 15.84s (± 0.36%) -0.04s (- 0.23%) 15.73s 15.99s
Angular - node (v14.15.1, x64)
Memory used 321,052k (± 0.01%) 321,051k (± 0.00%) -2k (- 0.00%) 321,024k 321,066k
Parse Time 1.90s (± 0.66%) 1.90s (± 0.53%) +0.00s (+ 0.11%) 1.89s 1.93s
Bind Time 0.87s (± 0.66%) 0.87s (± 0.51%) +0.00s (+ 0.23%) 0.86s 0.88s
Check Time 5.15s (± 0.44%) 5.16s (± 0.46%) +0.00s (+ 0.08%) 5.08s 5.20s
Emit Time 6.19s (± 0.64%) 6.22s (± 0.75%) +0.03s (+ 0.52%) 6.10s 6.33s
Total Time 14.11s (± 0.28%) 14.15s (± 0.45%) +0.04s (+ 0.29%) 14.02s 14.31s
Compiler-Unions - node (v14.15.1, x64)
Memory used 188,505k (± 0.64%) 188,815k (± 0.62%) +310k (+ 0.16%) 186,850k 190,136k
Parse Time 0.80s (± 0.59%) 0.80s (± 0.56%) +0.00s (+ 0.25%) 0.79s 0.81s
Bind Time 0.55s (± 0.62%) 0.55s (± 0.62%) 0.00s ( 0.00%) 0.55s 0.56s
Check Time 7.13s (± 0.47%) 7.16s (± 0.72%) +0.03s (+ 0.36%) 7.09s 7.29s
Emit Time 2.49s (± 0.83%) 2.48s (± 0.63%) -0.01s (- 0.44%) 2.45s 2.52s
Total Time 10.98s (± 0.36%) 10.99s (± 0.49%) +0.01s (+ 0.11%) 10.92s 11.14s
Monaco - node (v14.15.1, x64)
Memory used 323,144k (± 0.01%) 323,129k (± 0.01%) -15k (- 0.00%) 323,075k 323,158k
Parse Time 1.57s (± 0.70%) 1.57s (± 0.64%) +0.01s (+ 0.38%) 1.55s 1.60s
Bind Time 0.75s (± 0.77%) 0.75s (± 0.60%) -0.00s (- 0.27%) 0.74s 0.76s
Check Time 5.19s (± 0.30%) 5.17s (± 0.45%) -0.01s (- 0.19%) 5.14s 5.24s
Emit Time 3.13s (± 0.67%) 3.11s (± 0.68%) -0.02s (- 0.61%) 3.08s 3.17s
Total Time 10.63s (± 0.23%) 10.61s (± 0.38%) -0.02s (- 0.23%) 10.54s 10.74s
TFS - node (v14.15.1, x64)
Memory used 287,663k (± 0.01%) 287,670k (± 0.01%) +7k (+ 0.00%) 287,626k 287,720k
Parse Time 1.27s (± 1.48%) 1.27s (± 1.21%) -0.00s (- 0.08%) 1.23s 1.30s
Bind Time 0.72s (± 1.17%) 0.72s (± 0.81%) -0.00s (- 0.55%) 0.71s 0.73s
Check Time 4.70s (± 0.38%) 4.73s (± 0.45%) +0.03s (+ 0.55%) 4.67s 4.77s
Emit Time 3.26s (± 0.40%) 3.29s (± 0.83%) +0.03s (+ 0.80%) 3.23s 3.38s
Total Time 9.95s (± 0.27%) 10.00s (± 0.47%) +0.05s (+ 0.48%) 9.92s 10.11s
material-ui - node (v14.15.1, x64)
Memory used 450,697k (± 0.00%) 450,711k (± 0.01%) +14k (+ 0.00%) 450,646k 450,831k
Parse Time 2.00s (± 0.62%) 1.98s (± 1.10%) -0.02s (- 0.80%) 1.93s 2.03s
Bind Time 0.70s (± 0.74%) 0.70s (± 0.85%) -0.00s (- 0.43%) 0.69s 0.71s
Check Time 13.61s (± 0.91%) 13.41s (± 0.66%) -0.20s (- 1.48%) 13.16s 13.55s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.31s (± 0.76%) 16.09s (± 0.65%) -0.22s (- 1.32%) 15.78s 16.28s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-206-generic
Architecturex64
Available Memory16 GB
Available Memory7 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
Benchmark Name Iterations
Current 41013 10
Baseline master 10

Developer Information:

Download Benchmark

@DanielRosenwasser DanielRosenwasser removed the Experiment A fork with an experimental idea which might not make it into master label May 5, 2021
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 14, 2021
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

commandLineParser entry should make the new flag a strict one, I think.

@@ -0,0 +1,35 @@
// @useUnknownInCatchVariables: true
Copy link
Member

Choose a reason for hiding this comment

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

except for this, I'm not convinced we even need a new test, since we know that unknown narrows, and from some other tests, that strict changes the type of the catch variable to unknown.

I guess there's nothing to change here, except possibly dropping the narrowing tests from this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added that style of code in because most of the purpose of this feature is to ensure that catch variables are narrowed, and that we error on usages if a user doesn't do so.

@gvanrossum
Copy link

Meta: I was confused by the initial comment, because I wasn't familiar with unknown. I'd say that the first snippet, with catch (err) should not have a comment indicating the error (because IIUC without the new flag it doesn't produce an error), and the second snippet, with explicit catch (err: unknown) should have the error comment, because even without the new flag that's an error. (Right?)

@DanielRosenwasser
Copy link
Member Author

@gvanrossum good call, thanks! Must have been an earlier copy/paste error.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

👍🏼 Does what we talked about in the design meeting.

Side note: It's a little worrying that we don't already have more tests with both (1) catch and (2) strict: true.

@DanielRosenwasser DanielRosenwasser merged commit 9906092 into main Jun 3, 2021
@DanielRosenwasser DanielRosenwasser deleted the unknownInCatchVariables branch June 3, 2021 20:13
@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Jun 3, 2021
rekmarks added a commit to MetaMask/eslint-config that referenced this pull request Aug 30, 2021
Fixes the `no-throw-literal` rule configuration for TypeScript by enabling the appropriate rule in the TypeScript config, per [documentation](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-throw-literal.md#how-to-use).

This is timely due to recent [changes to TypeScript](microsoft/TypeScript#41013).
nightah added a commit to authelia/authelia that referenced this pull request Sep 2, 2021
Typescript 4.x changes the default behaviour of try catch and its err type from `any` to [`unknown`](microsoft/TypeScript#41013).

This change ensures that where we rely on said variable it is cast accordingly as an `Error`.
nightah added a commit to authelia/authelia that referenced this pull request Sep 4, 2021
* build(deps): update dependency typescript to v4.4.2

* fix(web): cast try catch err type to error

Typescript 4.x changes the default behaviour of try catch and its err type from `any` to [`unknown`](microsoft/TypeScript#41013).

This change ensures that where we rely on said variable it is cast accordingly as an `Error`.

Co-authored-by: Renovate Bot <[email protected]>
Co-authored-by: Amir Zarrinkafsh <[email protected]>
@pvinis
Copy link

pvinis commented Sep 23, 2021

Is there a way to type a function that it throws specific types? So that catch could know what to expect.

Like a func that can throw MyError, and then catch knows that e is MyError.

@DanielRosenwasser
Copy link
Member Author

Should the flag useUnknownInCatchVariables also affect the return type of Promise.prototype.catch?

@guilhermesimoes #45602 proposes that idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Flag to type 'catch' variables as 'unknown'.
7 participants