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

Allow unknown type annotation on catch clause variable #36775

Closed
5 tasks done
yokomotod opened this issue Feb 13, 2020 · 38 comments · Fixed by #39015
Closed
5 tasks done

Allow unknown type annotation on catch clause variable #36775

yokomotod opened this issue Feb 13, 2020 · 38 comments · Fixed by #39015
Assignees
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript

Comments

@yokomotod
Copy link

yokomotod commented Feb 13, 2020

Search Terms

catch clause unknown exception

Suggestion

Now any kind of type annotation on catch clauses is not allowed.
In my understanding, it's because that it's unsafe to annotate like catch (err: SpecialError) since any value will be captured actually.

However, due to the type of err is any, it's also not type safe.

So I suggest to allow annotating unknown , as most safe typing.

(And annotation with any other type won't be allowed as is)

Examples

try {
  throw 42;
} catch (err) {
  console.error(err.specialFunction()); 
}

can be written safer as

try {
  throw 42;
} catch (err: unknown) {
  if (err instanceof SpecialError) {
    console.error(err.specialFunction()); 
  } else {
    ...
  }
}

Related Issue

#20024

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@phiresky
Copy link

This would be great (together with a lint that forces doing this always).

Right now, catch (e) {} is basically a situation that should be forbidden by noImplicitAny, except it can't be because you can't annotate it with any type (except for unknown) since that would be misleading.

@bradzacher
Copy link
Contributor

I would really love this as a compiler option.
Being able to set a flag that makes all caught exceptions unknown instead of manually annotating them would be much better, but I guess it would be harder to migrate to.

@hediet
Copy link
Member

hediet commented Mar 29, 2020

I think I would also prefer a new strict* compiler option over new syntax (that would need to be enforced with a lint rule).

In my understanding, it's because that it's unsafe to annotate like catch (err: SpecialError) since any value will be captured actually.

I think type safety is not the only issue here. Unexperienced TypeScript developers would think that this catch statement might only handle errors of type SpecialError!

@Raynos
Copy link

Raynos commented May 5, 2020

👍 this would be amazing. To allow a type annotation on catch but only if its err: unknown

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels May 5, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone May 5, 2020
@MicahZoltu
Copy link
Contributor

Assuming this is the appropriate place to discuss this, I would like to advocate that the change in 4.0 be either (in order of preference):

  1. catch (error) results in error having a type of unknown
  2. Add a new strict option that makes error be of type unknown
  3. Add a new strict option that requires the user to do catch (error: unknown)

I understand that error is any for legacy reasons (it was before unknown existed) and therefore can't be trivially changed (at least without a major version bump). However, I would like to see a path that allows us to get away from that eventually, either with a breaking change (error is unknown) or with a new compiler setting that is flipped on with the strict flag.

@Raynos
Copy link

Raynos commented May 9, 2020

I am excited typescript 4.0 will address this issue.

We had to add a specific --ignore-catch flag to the type-coverage command ( https://github.com/plantain-00/type-coverage#ignore-catch ) to handle the fact that catch {} is the only place an implicit any is mandatory and unavoidable.

@ExE-Boss
Copy link
Contributor

It should probably also be possible to do:

try {
	// code
} catch (err: any) {
	// error handling
}

In case unknown ever becomes the default inferred type (e.g.: #27265).

@Akxe
Copy link

Akxe commented Jun 5, 2020

Why not to try infer the possible types of error? They are very well known to the compiler. Isn't it right that the error is union of all thrown & Error?

function doOrThrow<T>(error: T): true, throws T {
  if (Math.random() > .5) {
    return true;
  } else {
    throw error;
  }
}
try {
  doOrThrow('err1');
  doOrThrow('err2');
  doOrThrow('err3');
} catch (e) { // Type of e = Error | 'err1' | 'err2' | 'err3'.
}

You may ask, why to handle all errors at the same place. For me, the reason was that Express. For every response you can only send headers once (, logically but annoying to handle an error with it when trying to parallel all async tasks).

router.get('path', async (req, res) => {
  try {
    const [part1, part2] = await Promise.all([
       query('SELECT * FROM ...', 'part1 failed'), 
       query('SELECT * FROM ...', 'part2 failed'), 
    ]);
    res.sent({ ...part1, ...part2 });
  } catch (e) { // I would love this err to be infered
    switch (err) {
      case 'part1 failed':
        return res.send(500).send('The first part of data fetching failed');

      case 'part2 failed':
        return res.send(500).send('The second part of data fetching failed');

      default:
        const error: Error = err;
        console.error(err);
        return res.send(500).send('Unknown error');
    }
  }
});

@Jack-Works
Copy link
Contributor

Yeah, I'd like to have check exception in typescript but I think it might be easily misused.

@Akxe
Copy link

Akxe commented Jun 5, 2020

Yeah, I'd like to have check exception in typescript but I think it might be easily misused.

Misused? How so?

@Jack-Works
Copy link
Contributor

Yeah, I'd like to have check exception in typescript but I think it might be easily misused.

Misused? How so?

Not handling the exception properly, popup them into the upper function instead. So you will get a tons of unioned exception types.
You can see this in Java (if the programmer is lazy).

Or someone writing "throws any" , then the whole work become useless.

Or someone writing "throws Error", then all subclass of Error get merged into the widen type.

@MicahZoltu
Copy link
Contributor

Don't forget about stack overflow, divide by zero, and out of memory errors! Every function MUST throw those (unless you can prove one or more is impossible, which I think is only possible at compile time for divide by 0 in theory). Java made the mistake of not having every function throw those which really put a hamper on the utility of checked exceptions IMO.

@Jack-Works
Copy link
Contributor

How do Java, rust or other language handle those three kind of error in their checked exception system?

@MicahZoltu
Copy link
Contributor

@Jack-Works I have only worked with Java's checked exceptions and you'll get a runtime exception after the compiler asserted no exception was possible.

@Akxe
Copy link

Akxe commented Jun 5, 2020

Not handling the exception properly, popup them into the upper function instead. So you will get a tons of unioned exception types.
You can see this in Java (if the programmer is lazy).

Enabeling types for errors does not change how user will use this.

Or someone writing "throws any" , then the whole work become useless.

Or someone writing "throws Error", then all subclass of Error get merged into the widen type.

Well now you are forced to have err of type any (, or unknown, if this passes).

@Akxe
Copy link

Akxe commented Jun 5, 2020

Don't forget about stack overflow, divide by zero, and out of memory errors! Every function MUST throw those (unless you can prove one or more is impossible, which I think is only possible at compile time for divide by 0 in theory). Java made the mistake of not having every function throw those which really put a hamper on the utility of checked exceptions IMO.

For JavaScript/TypeScript this would be solved by extending all thrown parameters with error...

We are sure that a try catch block might throw Error, but if there is some additional throw statements we can infer them.

Thus the end type might be something like Error | MyError | 'myReason' and so on...

@phiresky
Copy link

phiresky commented Jun 5, 2020

I doubt adding checked exception is ever going to work in Typescript, because anytime you call an API you don't fully control you are adding any to your error type, which makes the whole type any.

I've previously tried doing this:

Never use throw. Instead create this simple API:

type CheckedReturn<R, E> = {ok: true, value: R} | {ok: false, value: E}

function Err<E>(e: E): CheckedReturn<any, E> { return {ok: false, value: E} }
function Ok<R>(r: R): CheckedReturn<R, any> { return {ok: true, value: R} }

(optionally add some monadic functions like and_then etc akin to Rust Result)

. Then you always do this:

function foo(...): CheckedReturn<string, SomeError> {
    // ...
    return Ok(actualValue);
    // or
    return Err(errorValue);
}

This sounds great in theory and gives you fully checked exceptions always... Except when you use literally any external or other API, and then you're back to where you started, except it's worse: you now have lots of additional effort, two layers of kinds of exceptions and still can get untyped / unchecked exceptions all the time.


The best that can be done is making a union type of all your own possible checked errors (or inherited error classes if you're okay with not having exhaustive checking / open domain errors):

type OwnErrorType = 
    | { kind: "FetchError", status: 404 | 500, message: string }
    | { kind: "DatabaseConnectionError", reason: pg.ErrorCode }
     // ...

// OR, if you don't care about exhaustiveness, 
// and don't pass your errors through serialization so instanceof does not break

abstract class OwnError extends Error {}
class FetchError extends OwnError {
     constructor(public status: 404 | 500, public message: string) {}
}
// ...

and then on top of each catch block, add a assertion / condition to always instantly narrow your type and rethrow others:

function assertOwnError(err: unknown): asserts err is OwnErrorType {
    /// some check, using instanceof or brands or whatever
   if (!err || !err.__isOwnErrorType) throw err;
   // or: if (err instanceof OwnError)
}

// ...

try {
   // ...
} catch (e: unknown) {
   assertOwnError(e);
   // error is correctly typed here
}

This is then basically the same as catch blocks you expect from other languages:

try {
} catch (OwnError e) {
    // ...
}

I'm already doing this in my own code bases, but to make it ergonomic these things are missing:

  1. Solving this issue: allowing declaring errors as unknown
  2. Eslint-typescript lint that forces always declaring errors as unknown (or strict flag)
  3. Force throwing only of errors of specific types. Possible to do with eslint, needs a rule similar to no-throw-literal

@Akxe
Copy link

Akxe commented Jun 5, 2020

This sounds great in theory and gives you fully checked exceptions always... Except when you use literally any external or other API, and then you're back to where you started, except it's worse: you now have lots of additional effort, two layers of kinds of exceptions and still can get untyped / unchecked exceptions all the time.

Since we know that every function in JS might throw Error (TypeError, ReferenceError or others), we can union them with the user made errors.

try {
  externalApi()
} catch(e) {
  if (e instanceof Error) {
    // Probably send this error to developer of API
  } else if (e instanceof ApiError) {
    // We messed up, log it and fix it in next patch
  }
}

If the *.d.ts files get updated, you will once again get full error handling, and even better one, than before! Now you often have to go to comment of function to know what errors it throws.

The best that can be done is making a union type of all your own possible checked errors (or inherited error classes if you're okay with not having exhaustive checking / open domain errors):

type OwnErrorType = 
    | { kind: "FetchError", status: 404 | 500, message: string }
    | { kind: "DatabaseConnectionError", reason: pg.ErrorCode }
     // ...

// OR, if you don't care about exhaustiveness, 
// and don't pass your errors through serialization so instanceof does not break

abstract class OwnError extends Error {}
class FetchError extends OwnError {
     constructor(public status: 404 | 500, public message: string) {}
}
// ...

Sadly you can't reliably extend error.

What is wrong with giving the option? This won't prevent, but nor support error sinking... Error sinking just might be useful sometimes...

I'm already doing this in my own code bases, but to make it ergonomic these things are missing:

  1. Solving this issue: allowing declaring errors as unknown
  2. Eslint-typescript lint that forces always declaring errors as unknown (or strict flag)

I would say that we should look at how much people assign the type to the parameter of the catch block. If it is high enough then, it might be right to allow the user to infer that information.

catch\((\s*.+\s*)\)(?:.|\n)+?\1:\s*(.*?)\s*= regexr

Force throwing only of errors of specific types. Possible to do with eslint, needs a rule similar to no-throw-literal

Sure, that's nice but it does not solve the issue at all...

@phiresky
Copy link

phiresky commented Jun 5, 2020

Since we know that every function in JS might throw Error (TypeError, ReferenceError or others), we can union them with the user made errors.

That's not actually true, since you can throw anything. throw "hi" works, as does throw null or throw {a: 1} and libraries may use different error types not extended from Error because extending Error is not actually that simple as you described. Which means the correct union type is unknown | 'myReason' | ... which is just unknown, so trying to type it any different does not work.
Trying to make the error some union type by inferring also doesn't solve anything about errors thrown in nested functions, which are in my opinion most of them - usually I don't even want to throw an error if I know I'm going catch it right afterwards anyways.

@Akxe
Copy link

Akxe commented Jun 5, 2020

@phiresky only if the type annotation (for throws) are missing... Then they get updated, they will gen narrowed down to a union that will not contain unknown.

And since TS can parse type of all throw statement it could simply generate type definition files including all errors too!


Including non-typed external programs sure will lead to problems but users often create types for what they are missing anyway. (At least I do)


EDIT:

The only time the compiler could not infer type for throw, is when the function includes eval. In all other cases, the compiler can simply look up the type of thrown value.

Please correct me, if I am wrong, but to me, the only reason we cannot have typed catch blocks is the amount of work that would go into it & unresolved syntax...

@Jack-Works
Copy link
Contributor

We're using ts-results (https://github.com/vultix/ts-results) to use Rust style checked exception in our program. But it is not good enough cause TypeScript doesn't and won't have the Rust ? try operator to automatically spread the type to the containing function.

And another concern is that we cant use the checked exception with confidence, because the rest of the world, including the JS builtin library, the npm libraries... doesn't obey the checked exception way, they throw.

but if there is some additional throw statements we can infer them.

@Akxe: It won't work because the calling function might come from the npm library with a dts declaration. The source code even might not be the JavaScript / TypeScript (e.g. other languages that compile to JavaScript) so that it is impossible to analyze the source code to infer the throws type.

optionally add some monadic functions like and_then etc akin to Rust Result

@phiresky: Hi, please try ts-results, it's awesome! (I have made a PR to implement into_ok and and_then and not merged yet)

@Akxe
Copy link

Akxe commented Jun 5, 2020

@Jack-Works sure not all libraries have type anotation, but with time (and this feature in place) they would come... Meantime and for some occasion, forcing throw type would be necessary...


It is basically the same as saying TS will never be used because npm packages are not always written with types in mind... I do think this argument is invalid as it would be solved soon after releasing typed catch blocks

elibarzilay added a commit that referenced this issue Jun 10, 2020
In addition, allow an explicit `any`; anything else throws an error.

Also adjust and reorganize existing tests.

Fixes #36775.
@elibarzilay
Copy link
Contributor

Note that the above adds the ability to add an any/unknown type, but no new flag for its default any not changing existing flags.

@phiresky
Copy link

Awesome! I've made a PR to force adding a unknown or explicit any type to all catch clauses to typescript-eslint: typescript-eslint/typescript-eslint#2202

@hazae41
Copy link

hazae41 commented Jun 11, 2020

We're using ts-results (https://github.com/vultix/ts-results) to use Rust style checked exception in our program. But it is not good enough cause TypeScript doesn't and won't have the Rust ? try operator to automatically spread the type to the containing function.

And another concern is that we cant use the checked exception with confidence, because the rest of the world, including the JS builtin library, the npm libraries... doesn't obey the checked exception way, they throw.

but if there is some additional throw statements we can infer them.

@Akxe: It won't work because the calling function might come from the npm library with a dts declaration. The source code even might not be the JavaScript / TypeScript (e.g. other languages that compile to JavaScript) so that it is impossible to analyze the source code to infer the throws type.

optionally add some monadic functions like and_then etc akin to Rust Result

@phiresky: Hi, please try ts-results, it's awesome! (I have made a PR to implement into_ok and and_then and not merged yet)

Just use array destructuring

const [result, error] = f()
if(error) // catch error
// use result
// ...
function f(): [number, Error]

If you want to ignore the error

const [result] = f()

@Akxe
Copy link

Akxe commented Jun 11, 2020

@hazae41 Sure, but throw error is standard other things are not.

@Jack-Works
Copy link
Contributor

@hazae41 the go style doesn't work well in typescript cause the type system doesn't understand if error is none, the result must be valid. But the rust style is different, typescript do recognize this tagged union pattern therefore you can't miss the error handing

phiresky added a commit to phiresky/typescript-eslint that referenced this issue Jun 22, 2020
@WORMSS
Copy link

WORMSS commented Jun 30, 2020

Is this meant to be 'fixed' in 4.0.0-beta?
Still shows 'any' type on Playground and on vscode

@samhh
Copy link

samhh commented Jun 30, 2020

@WORMSS This issue is to allow you to manually type like this:

catch (err: unknown)

@WORMSS
Copy link

WORMSS commented Jun 30, 2020

Any way to force it to unknown? We have "noImplictAny" checked, which says "Warn on expressions and declarations with an implied 'any' type."

But we get no warning that err is an 'any' type.

@phiresky
Copy link

Any way to force it to unknown? We have "noImplictAny" checked, which says "Warn on expressions and declarations with an implied 'any' type."

you can use a future eslint version, hopefully

maybe another issue should be opened here to add a strictCatchClauseTypes flag?

@Jack-Works
Copy link
Contributor

Agree, definitely a new strict flag for it to treat it as unknown or enforce to write unknown

@Akxe
Copy link

Akxe commented Jun 30, 2020

They already said that you can write your own linter rule. Don't expect a flag for this...

@WORMSS
Copy link

WORMSS commented Jun 30, 2020

They already said that you can write your own linter rule. Don't expect a flag for this...

Guess someone will have to rewrite the description of noImplyAny to be Warn on expressions and declarations with an implied 'any' type, except for catch argument

@MicahZoltu
Copy link
Contributor

For a 4.0 change (major version update) this certainly feels like it should be included in noImplicitAny. At the moment, I believe this is the last place where implicit any is allowed when noImplicitAny is on, and getting rid of that last place sure would be nice.

@bradzacher
Copy link
Contributor

bradzacher commented Jun 30, 2020

For a 4.0 change (major version update)

TS doesn't follow semver. The version is just an arbitrary number that's monotonically increasing.

This release is 4.0 purely because 3.9 + 0.1 === 4.0

See: #14116

@ExE-Boss
Copy link
Contributor

This doesn’t work in JavaScript with JSDoc type annotations:

try {
	// something
} catch (/** @type {unknown} */ err) {
	// `err` is still typed as `any`:
	err; // $ExpectType unknown
}

Playground link: 🔗

mslosarek added a commit to mslosarek/lambda-backend-challenge that referenced this issue Feb 28, 2021
FossPrime added a commit to FossPrime/feathers-saml that referenced this issue Sep 27, 2021
The catch blocks have some "new" syntax we need to adhere to: microsoft/TypeScript#36775

As of Feathers 4, the request and response body are actually of type any, but we have to specify that explicitly.

We can probably get rid of this `// @ts-ignore` now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript
Projects
None yet