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 type annotation on catch clause variable #20024

Open
jaredru opened this issue Nov 15, 2017 · 77 comments
Open

Allow type annotation on catch clause variable #20024

jaredru opened this issue Nov 15, 2017 · 77 comments
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@jaredru
Copy link

jaredru commented Nov 15, 2017

TypeScript Version: 2.6.1

const rejected = Promise.reject(new Error());

async function tryCatch() {
  try {
    await rejected;
  } catch (err: Error) { // TS1196: Catch clause variable cannot have a type annotation
    // Typo, but `err` is `any`, so it results in runtime error
    console.log(err.mesage.length);
  }
}

function promiseCatch() {
  rejected.catch((err: Error) => { // OK
    // Compiler error; Yay!
    console.log(err.mesage.length);
  });
}

This was discussed in #8677 and #10000. It was closed as "fixed" in #9999, but as far as I can tell neither of the issues was actually resolved. In either event, I'd like to make a case for allowing type annotations in catch clauses.

Especially with the introduction of downlevel async functions, I'd suggest that disallowing catch clause type annotations leads to less safe code. In the example, the two methods of handling the promise are functionally equivalent, but one allows you to type the error, and the other doesn't. Without writing extra code for the try/catch version (if (err instanceof Error) { or const e: Error = err or something), you'll get a runtime error that you wouldn't get with the pure Promise version.

The primary rationale for not allowing this is that any object can be thrown, so it's not guaranteed to be correct. However, most of the benefit of TypeScript comes from making assertions about your and other people's code that can't be strictly guaranteed (especially when importing JavaScript). And unless one would argue that the Promise catch function also shouldn't allow a type annotation on the error parameter, this argument seems to make very little practical sense.

I believe one of the other arguments against is that it might be confusing, as it looks like the typed exception handling you might see in other languages (e.g., Java), and folks may think the catch will only catch errors of the annotated type. I don't personally believe that's a legitimate issue, but if it really is I'd propose at least allowing a catch (err as Error) { syntax or similar as a way of emphasizing that it's a type assertion.

If nothing else at all, it seems that there should be a way to trigger a warning (similar to an implicit any warning) when using an untyped err directly within a catch block.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 15, 2017

I do like the idea of catch (err as Error) because, damn it, if you were going to cast it anyway, maybe we should make your life a little easier while keeping it explicit.

@DanielRosenwasser DanielRosenwasser added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Nov 15, 2017
@yortus
Copy link
Contributor

yortus commented Nov 15, 2017

I agree, it is possible to safely annotate the error variable in many situations. Older discussion in #8677 (comment).

It also true that it's easy to misunderstand/abuse, but then so are type annotations in general, and people just do the cast in the first line of the catch block anyway.

@aluanhaddad
Copy link
Contributor

@jaredru code like

function promiseCatch() {
  rejected.catch((err: Error) => { // OK
    // Compiler error; Yay!
    console.log(err.mesage.length);
  });
}

is dangerous and misleading. It only passes the type checker because the parameter of the catch method's callback is declared to be any. At best this is a disguised type assertion.

Promise.reject(NaN).catch((e: Error) => e.message.length);

@jaredru
Copy link
Author

jaredru commented Nov 21, 2017

I disagree. It is no more dangerous or misleading than const foo: Foo = JSON.parse(input);, which is a very reasonable pattern. It represents our expectations and assertions about the code we're dealing with.

The irony of your example is that it throws a TypeError, which gets at my underlying point. Aside from contrived examples, the expected currency for errors is Error. A simple search through the TypeScript repo itself shows that catch (e) virtually always assumes the e to be an Error. That doesn't make it "right", of course, but it highlights the realistic expectation of a large, real-world project that (presumably) reflects an understanding of the balance between correctness and pragmatism, particularly as it relates to TypeScript.

@aluanhaddad
Copy link
Contributor

I disagree. It is no more dangerous or misleading than const foo: Foo = JSON.parse(input);, which is a very reasonable pattern. It represents our expectations and assertions about the code we're dealing with.

I don't think that is a good pattern either.

const foo = <Foo>JSON.parse(input);

Is much clearer as to intent.

As for the example, contrived as it may be, it doesn't matter what gets thrown (TypeError or not) because it is thrown from the catch causing an unexpected failure.

@jaredru
Copy link
Author

jaredru commented Nov 21, 2017

Sorry, consider it a typo. <Foo>JSON.parse(s) is a common pattern but is as unsafe as .catch((e: Error) => {. Both treat an any as another type with no guarantee of correctness beyond the developer's word.

it doesn't matter what gets thrown (TypeError or not) because it is thrown from the catch causing an unexpected failure.

Of course it matters. That the runtime throws an Error for all runtime errors strengthens the position that it's reasonable to expect the argument to catch to be an Error. This expectation is common in real world codebases--including, but certainly not limited to, TypeScript's.

@felixfbecker
Copy link
Contributor

felixfbecker commented Nov 28, 2017

My 2ct: If there is any party that should be able to type the exception it should be the called function, not the caller. I've seen codebases that throw strings everywhere or nothing at all (undefined). Just declaring that the type is Error doesn't make it more type safe, because you never know what other functions the function you are calling may call into and what things those nested functions throw - Errors are not strongly typed and not part of the interface contract. Maybe you know it now but in the next patch version of some transient dependency it could change. So the only correct way to mirror the runtime JS is to start with anyunknown and narrow it down manually

if (err instanceof MyError) ... else throw err

switch (err && err.code) {
  case 'ENOENT': ...
  case 'EPERM': ...
  default: throw err
}

const isExpectedHTTPError = (err: any): err is HTTPError => err && typeof err.status === 'number'

if (isExpectedHTTPError(err)) {
  res.status(err.status).set(err.headers || {}).send(err.body || '')
} else {
  res.status(500)
}

And if the check doesn't pass, rethrow the error. If you don't and your usage of the error doesn't directly produce some kind of TypeError then you might swallow unexpected programming errors and create a debugging nightmare.

Is this more code to write? Yes. But does that justify a new feature to make the unsafe version easier to write?

@jaredru
Copy link
Author

jaredru commented Nov 28, 2017

My argument is for pragmatism. One of the stated non-goals of TypeScript's design is to "Apply a sound or 'provably correct' type system. Instead, strike a balance between correctness and productivity." Yes, I could get something besides an Error, but in reality--in the projects I'm working in--I won't.

As in the TypeScript codebase itself (1 2 3 4 5), it is very often safe and reasonable to assume that the thrown value is not, in fact, undefined (or null, or a string, ...) and is with almost certainty an Error. I would simply like the compiler to help me out in this case without additional code or runtime overhead.

@lordazzi
Copy link

lordazzi commented Jun 1, 2018

TypeScript language is already the best I've ever worked.
If you type the catch clause this will make much better.

@ghost
Copy link

ghost commented Aug 31, 2018

I'd like to at least be able to do

  try {
    await rejected;
  } catch (err: unknown) { 
    // ...
  }

@felixfbecker
Copy link
Contributor

I would certainly love a exceptionTypeUnknown flag that makes all exception types (and Promise rejections) type unknown instead of any. That would definitely make the error handling safer as opposed to allowing any annotation. Although this could also be solved by writing a TSLint rule that forces them to always be declared unknown, if we had this annotation.

@RyanCavanaugh RyanCavanaugh added the Add a Flag Any problem can be solved by flags, except for the problem of having too many flags label Aug 31, 2018
@bluelovers
Copy link
Contributor

need this

@woodcockjosh
Copy link

It's not productive for me to guess which properties are on the Error and have to look it up. Please implement.

@VincentLanglet
Copy link

@DanielRosenwasser

I do like the idea of catch (err as Error) because, damn it, if you were going to cast it anyway, maybe we should make your life a little easier while keeping it explicit.

I think it would be awesome !

@Shinigami92
Copy link

I like @Andrew5569 's Idea, but want to have the ability to define it like I want:

try {
  // ...
} catch (err: unknown) {
  // ...
}

try {
  // ...
} catch (err: Error) {
  // ...
}

try {
  // ...
} catch (err: AWS.AWSError) {
  // ...
}

@GabrielDelepine
Copy link

@Shinigami92 It will never be possible. TS is being transcoded to Javascript for execution. And Javascript does not implement the type.

What would be possible is

try {
  // ...
} catch (err: unknown | Error | AWS.AWSError) {
  if (err instanceof Error) {
    // ...
  } else if (err instanceof AWS.AWSError) {
    // ...
  } else {
    // ...
  }
}

@Shinigami92
Copy link

@GabrielDelepine sorry you get me wrong. I only want to assume the type, not to define it.

@thomas-darling
Copy link

thomas-darling commented Feb 11, 2019

Yeah, please just fix this :-)


The fact that this is not allowed is both inconsistent and inconvenient:

try { ... } catch(error: Error) { ... }

Especially considering that this is allowed:

promise.catch((error: Error) => ... )

You can't argue that allowing it would be dangerous, just because people might incorrectly assume the type is Error, when it can in fact be anything. If someone were to assumes that, they will just cast to Error anyway, which would be just as much of an error. And this could be said for any type used anywhere, so there's really no valid reason to treat errors as a special case.

Not to mention, that in the vast majority of cases, assuming that the error will in fact be an Error, is actually perfectly reasonable. Please be a bit more pragmatic about those things, especially when it is something that gets in our way on a daily basis.


While you are at it, please also add an optional reject type to promises:

interface Promise<TResolve, TReject = any>

Because yes, we can, and often do, guarantee that an async function will always reject with a certain type - and arguing against that is kinda ridiculous. We can just wrap everything in the function in a try block, and throw a custom error if we end up in the catch.

For example, we have an ApiClient, which guarantees that if it rejects, it will always be with an ApiError. It's extremely annoying that we can't declare that fact in our types, but instead have to manually cast the errors everywhere we use it. That's neither practical, nor type safe.


These would not be breaking changes, they would provide a lot of value, and people have been asking for it for years - so please just implement it :-)

@jednano
Copy link
Contributor

jednano commented Mar 13, 2019

I would love it if it were implemented with a new Throws<TReason = Error> type and Promise<TResolve, TReason> for Promises.

Ability to define and infer expected error types

function foo(): Throws<Error> { // should be inferred
    throw new Error()
}
try {
    foo()
} catch (error) { // <-- Error
    // noop
}

Throw something other than an Error (e.g., a number)

function bar(): Throws<number> { // should be inferred
    throw 42;
}
try {
    bar()
} catch (code) { // <-- number
    // noop
}

Multiple expressions within a try block

try {
    foo();
    bar();
} catch (reason) { // <-- Error | number
    // noop
}

Promise

async function baz(): Promise<never, TReason = Error> { // should be inferred
    return Promise.reject(new Error())
}
async function qux() {
    try {
        await baz()
    } catch (err) { // <-- Error
        // noop
    }
}

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code

As pointed out by @Shinigami92, it would be a breaking change unless implemented as Throws<TReason = any>, but that's not really a sensible default, as discussed above.

  • 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.

@Shinigami92
Copy link

Shinigami92 commented Mar 14, 2019

@jedmao

Why do you think your approach does not accept this?

function foo(): Throws<Error> { // should be inferred
    throw new Error()
}
try {
    const err: Throws<Error> = foo()
} catch (error) { // <-- Error
    // noop
}

I also think that Throws<TReason = Error> introduces a breaking change when it is inferred (and it should).
Throws<TReason = any> would not do this.

@jednano
Copy link
Contributor

jednano commented Mar 14, 2019

@Shinigami92 good point on the breaking change.

I don't think your example is any different than the never type.

function foo(): never {
    throw new Error();
}
try {
    const err: never = foo()
} catch (error) { // <-- any (existing implementation)
    // noop
}

@bpasero
Copy link
Member

bpasero commented Apr 24, 2019

+1 for having type-check flow through try/throw/catch code. This is driving me nuts currently for adopting async/await with try/catch where now I have no way of typing the error (with promises I could at least add a type).

@jpike88
Copy link

jpike88 commented Jul 3, 2019

At the very least, you should be allowed to use a type that extends the Error type, as is done with @types/mysql.

                try {
			// perform database fetch
		} catch (error: MysqlError) {
			if (error.code === 'ER_DUP_ENTRY') {
				return;
			} else {
				throw error;
			}
		}

@chase2981
Copy link

In addition, you guys should allow multiple catch clauses that catch different types of exceptions--just like every other programming language supports, like so:

  try {
      ...
  } catch (err: RdError<Server>) {
     // do stuff
  } catch (err: RdError<Client>) {
     // do other stuff
  } catch (err) {
     // catch all
  }

That would be awesome! Just sayin.

I'm sure we'll get there one of these days but who knows how long it will take to get there in JavaScript/TypeScript...

@Shinigami92
Copy link

In addition, you guys should allow multiple catch clauses that catch different types of exceptions--just like every other programming language supports, like so:

  try {
      ...
  } catch (err: RdError<Server>) {
     // do stuff
  } catch (err: RdError<Client>) {
     // do other stuff
  } catch (err) {
     // catch all
  }

That would be awesome! Just sayin.

I'm sure we'll get there one of these days but who knows how long it will take to get there in JavaScript/TypeScript...

We already discussed this earlier
This will never be made from TS side
The typedefs are stripped away by using tsc and that would leaf only

try {
    ...
} catch (err) {
   // do stuff
} catch (err) {
   // do other stuff
} catch (err) {
   // catch all
}

How will you distinguish between the three different catch err? You see, it's impossible... 🤷
So please go to tc39 and propose something, but it's not TS related

@bodograumann
Copy link

This is now a real pain in typescript 4.4 with --strict, where exceptions are typed as unknown instead of any in the catch block.
I‘m not really happy with any solution I have seen, but will probably go with what @kabaluyot wrote.

  • catch (error: any) is not allowed by @typescript-eslint/no-explicit-any
  • catch (error: MyExceptionType) is forbidden by “TS1196: Catch clause variable type annotation must be ‘any’ or ‘unknown’ if specified.”

I would prefer to do the latter. Multiple catch blocks should be out-of-scope.

@bcla22
Copy link

bcla22 commented Sep 13, 2021

Still see no reason exceptions cannot be typed like most other languages, especially when running TS in strict mode.

catch (e) -> catch (e: MyError) { ... } would make typechecking rejected promises using try/catch infinitely easier than adding another line to cast an untyped e like const error = <MyError>e; or const error = e as MyError.

Additionally, adding a bunch of if statements to check whether e instanceof SomeError or e instanceof AnotherError seems tedious and introduces unnecessary code complexity, but provides safer transpilation than multiple catch blocks.

Multiple catch blocks would not solve this and should not be considered.

@Bessonov
Copy link

Sorry to say, but you guys are wrong. Some years ago I was on the same track like you. And it does nothing to do with TypeScript, but with JavaScript and expectations. You can't just say catch (error: MyExceptionType) because, well, this wouldn't be true. It's like wrong typing. Wrong typing is worse than no typing at all, because you expect typings to be true. What should TypeScript do, if you expect MyExceptionType, but got IDontCareAboutDownstreamCodeExcpetion? Should TypeScript ignore it? How should TypeScript do that for types or interfaces? Does it make click? Therefore I think that any and unknown as default are best values, which TypeScript can offer atm. And yes, this is'nt convenient, but it reflects TyoeScript's type safety and limits.

@DanielRosenwasser has described the challenge short and precise in his comment.

I think it would be better to lock this discussion, because there are no new conceptual proposals, but +1 only. It can be unlock again, if there are relevant news from tc39.

@whenmoon
Copy link

Nothing yet?

@vegerot
Copy link

vegerot commented Sep 23, 2021

Therefore I think that any and unknown as default are best values

@Bessonov I agreed with everything you said up to this point. By the points you've given, then any shouldn't be allowed either! any is even worse than MyExceptionType!

My real issue with it is its inconsistency.

If I were the TypeScript dictator, I would allow no type annotations in catch clauses. However, since any is worse than any other annotation, if you allow any I think it makes sense to allow other types too

@tonycoco
Copy link

You could just cast to Error inside the catch:

try {
  // do something
} catch (error) {
  console.error(error as Error);
}

In TypeScript 4.4+, those errors are now unknown in the catch. Or, you just type check like they do in the example here: https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#use-unknown-catch-variables

@mckravchyk
Copy link

mckravchyk commented Oct 25, 2021

This could be really useful for test suites after 4.4 defaults to unknown. In test suites, defaulting to unknown is not appreciated as generally you know which error you expect and even if for some reason the code threw anything else, it would pop up on the test anyway.

@Dador
Copy link

Dador commented Nov 3, 2021

@Shinigami92

How will you distinguish between the three different catch err? You see, it's impossible... 🤷
So please go to tc39 and propose something, but it's not TS related

It's possible. You can check error type inside catch and re-throw error if there was no intend to catch it. Just as suggested in other comments from @zzzzBov and from @cyraid.

So with this solution catch will work just as in most typed languages.

Actually it looks like a good solution. Or is there something wrong with it?

@octogonz
Copy link

This is now a real pain in typescript 4.4 with --strict, where exceptions are typed as unknown instead of any in the catch block.
I‘m not really happy with any solution I have seen

@bodograumann As of TypeScript 4.4 you can set useUnknownInCatchVariables=false which restores the old any behavior. It's not ideal, but nobody was complaining about this in TypeScript 3.x. (In fact the TypeScript compiler itself is currently building this way -- if you set useUnknownInCatchVariables=true in src/tsconfig-base.json, the build fails.)

@spephton
Copy link

A nice way to safely unwrap your unknown error is to use instanceof (as mentioned upthread)

catch (e) {
    if (e instanceof Error) {
       // type-safe, compiler can help you in this block
        console.log(e.message);
    }
    else {
        // optionally
        throw e;
    }
} 

You're explicitly handling all possibilities, so it's type-safe.

I think catch variable as unknown is a good default as it encourages you to think about the values that variable can take.

It may make sense to have a tsconfig flag to allow catch (e: Error) to ease the pain for some users however.

@dimikot
Copy link

dimikot commented Jan 30, 2024

Another (type safe) option is to allow to safely cast to something like:

} catch (e: MaybeError) {

where MaybeError is some custom type defined similar to:

type MaybeError = null | undefined | { message?: unknown; stack?: unknown; };

I.e. allow e: MaybeError if MaybeError is null, undefined, or is an object with all props being optional and of types unknown. In this case, it still allows to do lots of things (checking for props being present, checking for equality like e?.code === "some" etc.), but at the same time, does not compromise type safety.

This would work, because, when MaybeError is defined the above way, there is no JS thing which would not match it. Even null or a plain string or a number thrown will still match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.