microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.28k stars 12.52k forks source link

Talk about Exceptions Here #56365

Open RyanCavanaugh opened 1 year ago

RyanCavanaugh commented 1 year ago

Acknowledgement

Comment

13219 is locked so that the conclusion doesn't get lost in the discussion, so talk about exceptions here instead

fatcerberus commented 1 year ago

I acknowledge that issues using this template may be closed without further explanation at the maintainer's discretion.

I love the irony that a maintainer was forced to check this box 😄

michaelangeloio commented 1 year ago

https://github.com/microsoft/TypeScript/issues/13219#issuecomment-1806332624

@KashubaK @kvenn I'm going to start looking at intellij (and also see if I can make an eslint plugin for it!). If you want to help, let me know!

kvenn commented 1 year ago

Would be happy to be involved. I did like the idea of ESLint plugin because it's piggybacking off of an already established static analysis solution, but I think IDE plugins also check that box.

This comment has some code for the starting point of the ESLint plugin (in a collapsed text block): https://github.com/microsoft/TypeScript/issues/13219#issuecomment-1693733605

michaelangeloio commented 10 months ago

Would be happy to be involved. I did like the idea of ESLint plugin because it's piggybacking off of an already established static analysis solution, but I think IDE plugins also check that box.

This comment has some code for the starting point of the ESLint plugin (in a collapsed text block): #13219 (comment)

@kvenn see my comment here about eslint https://github.com/michaelangeloio/does-it-throw/issues/70#issuecomment-1879726088

I've got jetbrains-intellij working now, but waiting on jetbrains to approve it! You can check the code for that here if you'd like: https://github.com/michaelangeloio/does-it-throw/tree/main/jetbrains

kvenn commented 10 months ago

Heck yes! I'll happily use the IntelliJ plugin. I'll check back in a bit and install when it's approved.

Shame that eslint doesn't support async and that it's a ways away. But there's even more you can do with a plugin.

Nicely done!

arivera-xealth commented 10 months ago

@kvenn jetbrains is now available! https://plugins.jetbrains.com/plugin/23434-does-it-throw-

Feel free to share with others!

kvenn commented 10 months ago

I've since gotten the opportunity to try out the JetBrains plugin for does-it-throw and after looking into it a bit more, I don't think that really solves the problem I'm having with exceptions.

That plugin seems to mostly be about alerting of where throw statements are used. Which appears to be for enforcing that you don't use throw statements. I think throw statements are here to stay, even if I agree first class support for errors has advantages. And that if you're already in a codebase which relies on throws, this adds a lot of noise.

I had proposed an ESLint exception to warn when invoking a function that can throw. Encouraging you to either mark(document) this function as one that re-throws or to catch it. With the intention being to prevent you from accidentally having a function that throws bubble up all the way to the top of your program. But allowing that to be the case if it makes sense (like in a GraphQL resolver, where the only way to notify Apollo of the error is via throwing, or a cloud function / queue where throwing is used to retry).

If it can be found implicitly (without documentation), that's better. And it seems like a plugin could actually achieve that (and even offer quick fixes, which would be SO COOL). I'd advocate for using an already used standard TSDoc annotation (@throws) as the acknowledgement that this throw statement is there on purpose (as opposed to introducing a new one - @it-throws or @does-it-throw-ignore).

does-it-throw has some great bones. And it seems like it's solving a problem for others, it just might not be the right fit for me.

Kashuab commented 10 months ago

@kvenn I've tried my hand at an eslint plugin: https://github.com/Kashuab/eslint-plugin-checked-exceptions/tree/main

It introduces two rules:

Checks to see if a function you're calling has a @throws JSDoc annotation. If you don't wrap the function call in a try/catch it will output an error.

Warns you if a function has a throw statement without a corresponding @throws annotation. Matches based on what you're throwing, in case you have custom error classes.

Check out the tests for examples and what it covers. It's been a while since I looked at this, but I remember it being a bit buggy (i.e. nested branches, complicated logic) so there's a ton of room for improvement. I might take another look to improve it.

Side note - the README suggests you can install it from NPM, this is not the case haha.

(This is my work GH account, dunno why I have a separate one but oh well. I previously contributed here as @KashubaK)

wiredmatt commented 9 months ago

I'm definitely a complete noob when it comes to how Javascript/Typescript works, but would it be possible to add the modifier throws to a Typescript function signature?

https://docs.oracle.com/javase/tutorial/essential/exceptions/declaring.html

As both a Typescript user and a library maintainer, I hate not being able to consume / deliver proper error declarations. I know I can use JSDoc's @throws, but having that keyword as part of the function signature would be so great...

KashubaK commented 9 months ago

@wiredmatt That suggestion was discussed in detail in the linked issue: #13219

The TL;DR is essentially, it's not worth doing because there isn't sufficient existing practice/documentation/runtime behavior to facilitate such a feature. TypeScript is designed to fit within the scope of JS' behavior, and since JavaScript doesn't give us reliable, native tools for things like checked exceptions it's challenging to fit it within scope.

What we're pondering now is, what's the next best thing? How can we encourage better error handling practices enough that the community has some common ground to operate on?

wiredmatt commented 9 months ago

@KashubaK thank you for your response.

I was thinking about the newly introduced type guards / type predicates, in my mind it seemed totally possible, especially knowing we have conditional types as well.

I'll keep an eye on the eslint solution, that makes sense to me knowing what you just explained. thanks!

mharj commented 9 months ago

I was thinking about actual "throws" keyword as optional in return type, so TS would automatically add defaults to current functions/methods .. something like throws<any> or throws<unknown> at least for starting point. so when we do write modules we can actually more strictly expose what type of things we are throwing out like example.

function doAuth(): AuthPayload throws<TypeError | AuthError> {}

and maybe have some utility type similar as typeof to extract errors types from function so we can more easily utilize other modules throw types without directly importing those.

function someStuff(): AuthPayload throws<throwof doAuth | FatalError> {}

Also maybe this have later impact on catch argument type to actually know throw types, but just actual documentation of throw types is way more important atm for interoperability between modules as currently we are just quessing and reading module source code to undestand what might actually get thrown.

Edit: this would also work for indication that function will never throw .. throws<never>

RyanCavanaugh commented 9 months ago

Also maybe this have later impact on catch argument type to actually know throw types

This doesn't really work unless you have a level of information that doesn't exist in the real world, and requires the ability to express many patterns that are basically arbitrarily complicated. See https://github.com/microsoft/TypeScript/issues/13219#issuecomment-1515037604

thw0rted commented 9 months ago

but just actual documentation of throw types is way more important atm for interoperability between modules

If documentation is the issue, you don't need a TS keyword -- https://jsdoc.app/tags-throws has existed for ages. I don't know about you, but I really don't see it used very often. This is the heart of the problem Ryan described in the comment linked above (summarizing the original issue): JS developers don't, broadly speaking, document expected exception behavior, so there's a chicken and egg problem where trying to implement checked-exception types would go against the grain of the current ecosystem.

Use of the @throws JSDoc tag can be treated as a sort of demand signal for better exception handling. Poor @throws adoption indicates that the community doesn't want it. And without good @throws coverage, a throws keyword in TS wouldn't be very useful in the best scenario, and would be actively misleading at worst, giving devs the impression that they've handled the "expected" throw scenarios when they haven't.

All that said, I still think there could be a place for some limited ability to perform static analysis of exception/rejection handling. I originally found the previous issue when I enabled a linter rule that looks for unhandled Promise rejections, which overlaps with try/catch once await enters the picture. I was looking for a way to decorate some async function calls as being unable to throw or reject (think return Promise.resolve('static value')), which would let me build out exception-safety from the bottom up, slowly. Maybe this could work if we split the feature into declaring or asserting throws-type (basically the @throws JSDoc tag), with a separate keyword or directive for enabling exception checking:

/** unsafe-assertion-that-this-throws-strings */
function throwsSometimes(): number {
  if (Math.random() < 0.5) { throw 'nope!'; }
  return (Math.random() < 0.5) ? 0 : 1;
}

/** unsafe-assertion-that-this-throws-never */
function throwsNever(): number { return JSON.parse('2'); }

/** checked-assertion-that-this-throws-never */
function maybeSafe(): number {
  return throwsSometimes() || throwsNever(); // error, unhandled throws-strings does not match declared throws-never
}

Note that this is a different scope from what was discussed in the checked-exceptions section of https://github.com/microsoft/TypeScript/issues/13219#issuecomment-1515037604. I'm trying to statically analyze that explicit/declared throws propagate correctly up the chain, and importantly, to limit where those checks are performed. I want to be able to decorate one function as not calling functions with decorated/expected exceptions outside of a try block -- to annotate one function as "exception-prone" and another as "bad at exception handling". I think there's value in that even if most library functions I call don't (currently) have their expected exceptions documented. (In Ryan's terminology, this requires "option one", unannotated functions are assumed not to throw anything.)

kvenn commented 9 months ago

Poor @throws adoption indicates that the community doesn't want it.

I don't know if this is true. Annotating with @throws doesn't actually enforce anything. So its value is only to document and therefore solves a different problem than checked exceptions (prevent unhandled exceptions). For those that document their code, I've found it very common to use @throws. But people aren't going out of their way to annotate.

A static analysis solution does seem to be the best. And leveraging @throws (for those who do use it) feels like a natural solution. And I agree if it's omitted, it's assumed it doesn't throw. But it would also be easy to have a linter tell you you're missing the annotation of a function that has a "throw" in its body (or a function it calls) - for those that do care.

KashubaK commented 9 months ago

How about instead of all this, in your code you just return an Error, instead of throwing altogether. This is more reliable, easily supported by runtime behavior, requires less syntax to handle, already works in TypeScript, and wouldn't require massive refactoring if adopting a Result return type paradigm.

function wow(value: unknown) {
  if (typeof value === 'string') return new StringNotSupportedError();

  return { value: 1234 };
}

const result = wow('haha');

// @ts-expect-error
result.value; // TS error, you have to narrow the type

if (result instanceof Error) {
  // Handle the error
  return;
}

console.log(result.value); // Good!

It forces you to handle errors. Seems pretty similar to what people are asking for. I know that actually throwing behaves differently, but I wonder actually how much this would suffice.

I find that the more I think about this, the more I care about it only in my application source code. I'm not all that worried about third party libraries. I don't think there's been a single time where I wished a library had an error documented. Usually good type definitions avoid runtime errors that are worth catching. I also wonder if errors are even suitable for the things I have in mind. Things like validation contain useful state that don't really make sense to wrap in an error, and should instead just be included in a return value.

My questions are, what are the real world use-cases here? How do you guys actually see yourselves using a feature like this in practice? What errors do you have to explicitly handle with a try/catch, and do these instances occur frequently? How would the added type information help you?

phaux commented 9 months ago

chicken and egg problem

I don't see it that way.

First step should be to implement typechecking of throw types the same way as return types. Then add throw types to standard library definitions which are part of TypeScript.

Then the library authors could just regenerate their definitions like always and have the throw types inferred the same way return types are inferred when you don't specify them. For backward compatibility, functions without a throw type would be treated as throw any or throw unknown, so if a library depends on another library which haven't been updated yet, it just gets it's own throw types inferred as unknown.

RyanCavanaugh commented 9 months ago

"What if TS had typed/checked exceptions" is off-topic here; this is not a place to re-enact #13219

thw0rted commented 9 months ago

"What if TS had typed/checked exceptions" is off-topic here

"Talk about exceptions here" 🤔

ETA: any chance the TS team would consider enabling the GitHub "Discussions" feature for posts like these? Issues are terrible at capturing long-running discussions because once there are too many comments, context gets lost behind the "Load more..." link and search breaks down.

bensaufley commented 9 months ago

I agree that the topic of this thread is unclear about what's already been litigated, but it has already been extensively litigated (even if I'm bummed about the result). I think this thread was intended to be more of "other options, now that that decision has been made"

phaux commented 9 months ago

I only found this issue after the previous one was closed.

My usecase was:

I wanted to enforce that a handler function will throw only HttpErrors:

type Handler<T, E extends HttpError<number>> = (req: Request) => T throw E

and I wanted to infer possible responses and their status codes based on what the function actually throws:

type handlerResponse<H extends Function> = 
  H extends (...args: any[]) => infer T throw HttpError<infer N>
    ? TypedResponse<200, T> | TypedResponse<N, string>
    : never
Kashuab commented 8 months ago

I wonder if a simple util function could suffice.

function attempt<E extends Error,  T>(cb: () => T, ...errors: E[]): [T | null, E | null] {
  let error: E | null = null;
  let value: T | null = null;

  try {
    value = cb();
  } catch (err) {
    const matches = errors.find(errorClass => err instanceof errorClass);

    if (matches) {
      error = err;
    } else {
      throw err;
    }
  }

  return [value, error];
}

class StringEmptyError extends Error {}

function getStringLength(arg: string) {
  if (!arg.trim()) throw new StringEmptyError();

  return arg.length;
}

// Usage:

const [length, error] = attempt(() => getStringLength(" "), StringEmptyError);
// if error is not a StringEmptyError, it is thrown

if (error) {
  // error is a StringEmptyError
}

This is just an idea. It would need to be improved to handle async functions. It could also probably be changed to compose new functions to avoid repeating callbacks and errors, for example:

class StringEmptyError extends Error {}
class SomeOtherError extends Error {}

function getStringLength(arg: string) {
  if (!arg.trim()) throw new StringEmptyError();

  return arg.length;
}

// ... Assuming `throws` is defined
const getStringLength = throws(
  (arg: string) => {
    if (!arg.trim()) throw new StringEmptyError();

    return arg.length;
  },
  StringEmptyError,
  SomeOtherError
);

// Same usage, but a bit simpler

const [length, error] = getStringLength(" ");
// if error is not a StringEmptyError or SomeOtherError, it is thrown

if (error) {
  // error is a StringEmptyError or SomeOtherError
}
thw0rted commented 8 months ago

That's a handy wrapper for, uh, turning TS into Go I guess? (There are worse ideas out there!) But I can't figure out how this helps with static analysis to enforce error checking. In particular, it looks like attempt(...) can only ever return [T,null] | [null,E] but TS isn't able to take advantage of that with flow-control based narrowing.

Kashuab commented 8 months ago

My example wasn't meant to be perfect. It was just an idea on how to accomplish some way of better error handling. I've since improved the approach and implemented a way to enforce that errors are caught.

Example:

class StringEmptyError extends Error {}
class SomeOtherError extends Error {}

const getStringLength = throws(
  (arg: string) => {
    if (!arg.trim()) throw new StringEmptyError();

    return arg.length;
  },
  StringEmptyError,
  SomeOtherError
);

const length = getStringLength(' ')
  .catch(SomeOtherError, err => console.error(err))
  .catch(StringEmptyError, err => console.error(err));

console.log(length); // would be undefined in this case, it hits StringEmptyError

See CodeSandbox for a working throws implementation. src/throws.ts

If you don't add .catch(Error, callback) for each required error, you cannot access the original function's return value, and the function won't even be called. All errors are typed as expected. There are probably bugs and ways to improve it, I didn't take too much time here. This is definitely not compatible with async functions. Just wanted to prove that something like this is feasible.

Update: I also took the liberty of publishing this in a ts-throws NPM package. If anyone is interested in this feel free to try it out and add suggestions/issues on the repo: https://github.com/Kashuab/ts-throws

After some further development on this there are some obvious problems. But I think they can be addressed.

UPDATE 2: I've added more improvements to ts-throws to handle async functions and fixed quite a few bugs. It's in a pretty solid spot and I imagine it would work for a lot of developers. Check out the README for latest usage examples! Would love to hear some feedback.

raythurnvoid commented 8 months ago

@kvenn I've tried my hand at an eslint plugin: https://github.com/Kashuab/eslint-plugin-checked-exceptions/tree/main

It introduces two rules:

  • uncaught-errors

Checks to see if a function you're calling has a @throws JSDoc annotation. If you don't wrap the function call in a try/catch it will output an error.

  • undocumented-errors

Warns you if a function has a throw statement without a corresponding @throws annotation. Matches based on what you're throwing, in case you have custom error classes.

Check out the tests for examples and what it covers. It's been a while since I looked at this, but I remember it being a bit buggy (i.e. nested branches, complicated logic) so there's a ton of room for improvement. I might take another look to improve it.

Side note - the README suggests you can install it from NPM, this is not the case haha.

(This is my work GH account, dunno why I have a separate one but oh well. I previously contributed here as @KashubaK)

This is really neat, btw I would suggest to not enforce try catch, because it's legit to ignore the error and let it propagate without putting eslint comments to disable the rule everywhere. Instead I would propose to force the user to annotate a function that is not catching an error with a @throws as well, this way the user can choose to ignore errors but at least the function openly declares that it may @throws.

mharj commented 8 months ago

We can always use and wrap something like Rest style Result to handle error types, but long as actual throw error types are not part of TS this is just extra layer hack (same as trying to handle this on JSDoc) Easy things are propably just utilize Promise generics for Error Promise<string, TypeError>. Also adding throws keyword return type would also make sense

function hello(arg: unknown): string throws<TypeError> {}

... and have defaults like throws<any> or throws<unknown> based on TS settings. or maybe more compatible return type setup would be actually string & throws<TypeError> ?

Kashuab commented 8 months ago

I'd like to re-plug a library I put together, since it's more refined than the examples I posted before. It lets you wrap a given function with enforced error catching, using syntax with similar verbosity when compared to a function using throws proposal and try/catch

import { throws } from 'ts-throws';

class StringEmptyError extends Error {}
class NoAsdfError extends Error {}

const getStringLength = throws(
  (str: string) => {
    if (!str.trim()) throw new StringEmptyError();
    if (str === 'asdf') throw new NoAsdfError();

    return str.length;
  },
  { StringEmptyError, NoAsdfError }
);

/*
  `throws` will force you to catch the provided errors.
  It dynamically generates catch* methods based on the object of errors
  you provide. The error names will be automatically capitalized.
*/

let length = getStringLength(' ')
  .catchStringEmptyError(err => console.error('String is empty'))
  .catchNoAsdfError(err => console.error('String cannot be asdf'));

// length is undefined, logged 'String is empty'

length = getStringLength('asdf')
  .catchStringEmptyError(err => console.error('String is empty'))
  .catchNoAsdfError(err => console.error('String cannot be asdf'));

// length is undefined, logged 'String cannot be asdf'

length = getStringLength(' ')
  .catchStringEmptyError(err => console.error('String is empty'))

// Only one error caught, `length` is:
// { catchNoAsdfError: (callback: (err: NoAsdfError) => void) => number | undefined }
// Function logic not invoked until last error is handled with `.catch`

length = getStringLength('hello world')
  .catchStringEmptyError(err => console.error('String is empty'))
  .catchNoAsdfError(err => console.error('String cannot be asdf'));

// length is 11

One improvement might be error pattern matching for things like throw new Error('Some custom message'), this would help with wrapping third-party functions where their exception classes aren't public/exported

I think the only advantage that a native throws keyword would have over something like this would be conditionals (i.e. extends in a throws definition, function overrides, etc.) This solution doesn't seem like a hack to me, since it accomplishes the critical goal of forcing consumers of a given function to catch specific errors. I also prefer this catch-callback approach, it's cleaner than having to narrow error types manually within a catch block in most scenarios.

raythurnvoid commented 8 months ago

I'd like to re-plug a library I put together, since it's more refined than the examples I posted before. It lets you wrap a given function with enforced error catching, using syntax with similar verbosity when compared to a function using throws proposal and try/catch

  • Handle each error case with a separate callback, improved flow control vs. try/catch
  • Consumers don't need to import error classes
  • Everything is typed properly, auto-complete of catch* methods is available and they do get narrowed down so you don't have duplicates
  • No Result, but changes the return type to T | undefined if a checked error is thrown
  • No known bugs as of this comment
import { throws } from 'ts-throws';

class StringEmptyError extends Error {}
class NoAsdfError extends Error {}

const getStringLength = throws(
  (str: string) => {
    if (!str.trim()) throw new StringEmptyError();
    if (str === 'asdf') throw new NoAsdfError();

    return str.length;
  },
  { StringEmptyError, NoAsdfError }
);

/*
  `throws` will force you to catch the provided errors.
  It dynamically generates catch* methods based on the object of errors
  you provide. The error names will be automatically capitalized.
*/

let length = getStringLength(' ')
  .catchStringEmptyError(err => console.error('String is empty'))
  .catchNoAsdfError(err => console.error('String cannot be asdf'));

// length is undefined, logged 'String is empty'

length = getStringLength('asdf')
  .catchStringEmptyError(err => console.error('String is empty'))
  .catchNoAsdfError(err => console.error('String cannot be asdf'));

// length is undefined, logged 'String cannot be asdf'

length = getStringLength(' ')
  .catchStringEmptyError(err => console.error('String is empty'))

// Only one error caught, `length` is:
// { catchNoAsdfError: (callback: (err: NoAsdfError) => void) => number | undefined }
// Function logic not invoked until last error is handled with `.catch`

length = getStringLength('hello world')
  .catchStringEmptyError(err => console.error('String is empty'))
  .catchNoAsdfError(err => console.error('String cannot be asdf'));

// length is 11

One improvement might be error pattern matching for things like throw new Error('Some custom message'), this would help with wrapping third-party functions where their exception classes aren't public/exported

I think the only advantage that a native throws keyword would have over something like this would be conditionals (i.e. extends in a throws definition, function overrides, etc.) This solution doesn't seem like a hack to me, since it accomplishes the critical goal of forcing consumers of a given function to catch specific errors. I also prefer this catch-callback approach, it's cleaner than having to narrow error types manually within a catch block in most scenarios.

i would highly discourage leveraging on throw to try mimic Go/Rust way of handling errors because each throw has a massive performance impact and this can easily get out of hand if overused + this forces you to wrap each function in a "throws" function that also adds a very small overhead.

I would suggest returning the errors instead of throwing them.

Kashuab commented 8 months ago

i would highly discourage leveraging on throw to try mimic Go/Rust way of handling errors because each throw has a massive performance impact and this can easily get out of hand if overused + this forces you to wrap each function in a "throws" function that also adds a very small overhead.

I would suggest returning the errors instead of throwing them.

One of the benefits here is that it's plug-and-play with functions that already throw. You don't need to modify the function at all, just wrap it (this still introduces a breaking change for consumers, as would returning an error.)

Your suggestion would require refactors whose cost outweighs the value of performance in most scenarios. If you are running first-party functions in a loop where error handling performance becomes significant, those functions shouldn't throw to begin with as you suggested. My solution does not aim to address those use-cases.

My goal was to augment native error checking capabilities, making it more convenient/type-safe without needing to change the "throwing" code. Any overhead introduced would be negligible in 90% of use-cases, and in the latter 10% the library would not be applicable. That being said, I'll benchmark the library and see where optimizations can be made.


Update: I'm actually glad this got brought up. I put together some benchmarks and discovered my implementation was a bit over-complicated. Originally, throws wrapped functions were ~63% slower than a normal try/catch, but I've since refactored my approach to get it down to ~25% slower. It's still a considerable difference, but much better and easier to weigh.

In this benchmark, these were the results:

Thrower runs per second: 423095
Returner runs per second: 480135
ts-throws wrapped runs per second: 313880
raythurnvoid commented 8 months ago

i would highly discourage leveraging on throw to try mimic Go/Rust way of handling errors because each throw has a massive performance impact and this can easily get out of hand if overused + this forces you to wrap each function in a "throws" function that also adds a very small overhead. I would suggest returning the errors instead of throwing them.

One of the benefits here is that it's plug-and-play with functions that already throw. You don't need to modify the function at all, just wrap it (this still introduces a breaking change for consumers, as would returning an error.)

Your suggestion would require refactors whose cost outweighs the value of performance in most scenarios. If you are running first-party functions in a loop where error handling performance becomes significant, those functions shouldn't throw to begin with as you suggested. My solution does not aim to address those use-cases.

My goal was to augment native error checking capabilities, making it more convenient/type-safe without needing to change the "throwing" code. Any overhead introduced would be negligible in 90% of use-cases, and in the latter 10% the library would not be applicable. That being said, I'll benchmark the library and see where optimizations can be made.

Update: I'm actually glad this got brought up. I put together some benchmarks and discovered my implementation was a bit over-complicated. Originally, throws wrapped functions were ~63% slower than a normal try/catch, but I've since refactored my approach to get it down to ~25% slower. It's still a considerable difference, but much better and easier to weigh.

In this benchmark, these were the results:

Thrower runs per second: 423095
Returner runs per second: 480135
ts-throws wrapped runs per second: 313880

The additional overhead is coming from generating the call stack when new Error() is called. That should be avoided as much as possible in frequently invoked code. The way I see it throw new Error() equals panic in rust/go, if your goal is to return recoverable errors you should use some custom class perhaps. However this means lot refactoring is necessary and not being plug n play.

So as a rule a would avoid to use the error class for expected situations, eg: the string in input is empty and it shouldn't be.

Kashuab commented 8 months ago

The additional is overhead is coming from generating the call stack when new Error() is called. That should be avoided as much as possible in frequently invoked code. The way I see it throw new Error() equals panic in rust/go, if your goal is to return recoverable errors you should use some custom class perhaps. However this means lot refactoring is necessary and not being plug n play.

So as a rule a would avoid to use the error class for expected situations, eg: the string in input is empty and it shouldn't be.

This is interesting. By not extending Error, performance is improved by ~5.5x. Thanks for your insight! I've published a change that removes the extends Error constraint in throws, and updated the README to discourage this unless necessary.

reececomo commented 8 months ago

+1 to throwing untyped exceptions.

Knowing which function calls are supposed to throw anything is the 90-10 solution we're after.

function foo(bar: string): boolean throws {
  if (someConditionNotMet()) {
    throw 'an error';
  }
}

Example error

   foo('bar');
// ^💥TSError: Throwable not caught or rethrown.

Example handled cases

Catch
try {
  foo('bar');
} catch (err) {
  // ...
}
Rethrow
function bar() throws {
  foo('bar');
}
Swift equivalent (for posterity) Or Swift style (if a keyword would be easier on the AST): ```ts function bar() throws { try foo('bar'); } ```

We get a little derailed when we start thinking too hard into the future about typed exceptions. A simpled, opt-in, mark and unmark system would be a perfect place to start.

thw0rted commented 8 months ago

I don't know about you but when I have problems with an unexpected exception, it's never the kind of problem where just swallowing it and blundering forward would have solved anything. Do other people really need a shorthand for that?

reececomo commented 8 months ago

I won't go into the merits or flaws of a shorthand, I was just pulling an illustrative example from our Swift friends.

+1 for "blundering forward", excellent phrase.

edit: updated above example to remove the shorthand, so as not to distract/derail.

felds commented 7 months ago

How about instead of all this, in your code you just return an Error, instead of throwing altogether.

@KashubaK I don't like this solution because it forces the developer to mix error handling and logic flow.

With throws, it's possible to write the whole happy path of the function separate from the error handling, which I think it's much more easy to read.

Compare these two pseudocodes and tell me which one is easier to follow:

fn getIntValueFromFile(path) {
  fileHandler = open(path)
  contents = read(fileHandler)
  parsedValue = parseInt(contents)
  return parsedValue
} catch (UnableToOpenFile err) {
  // handle open error
  return NaN
} catch (NotAnInt err) {
  // handle parseInt error
  return NaN
}

against

fn getIntValueFromFile(path) {
  fileHandler = open(path)
  if (fileHandler instanceof UnableToOpenFile) {
    // handle open error
    return NaN
  }
  contents = read(fileHandler)
  parsedValue = parseInt(contents)
  if (parsedValue instanceof NotAnInt) {
    // handle parseInt error
    return NaN
  }
  return parsedValue
}

The first one is my preferred way to do error handling in other languages (I've thrown in Exlixir's implicit try just because I thinks it's a great idea), and having the language itself help me with what errors to handle would be a great thing to have.

KashubaK commented 7 months ago

How about instead of all this, in your code you just return an Error, instead of throwing altogether.

@KashubaK I don't like this solution because it forces the developer to mix error handling and logic flow.

Do you mean to tag me in this? I'm not proposing people handle returned errors as you compared against. However ts-throws can capture returned errors so you can handle them in a more catch-y style.

felds commented 7 months ago

@KashubaK Yes, I did.

I quoted a part of your answer, and this is the code you suggested:

const result = wow('haha');
if (result instanceof Error) {
  // Handle the error
  return;
}

I my second exemple I tried to show how it would look inside a function body and how, to me, it doesn't seem very ergonomic. (but maybe I got something wrong, as it's known to happen)

KashubaK commented 7 months ago

@KashubaK Yes, I did.

I quoted a part of your answer, and this is the code you suggested:

const result = wow('haha');
if (result instanceof Error) {
  // Handle the error
  return;
}

I my second exemple I tried to show how it would look inside a function body and how, to me, it doesn't seem very ergonomic. (but maybe I got something wrong, as it's known to happen)

Ohhhhh gotcha. In retrospect, I'm not a huge fan of that either. I was trying to look for "How can we achieve this in current TS" since the conversation of "How can TS change to support this new idea" isn't productive anymore.

I wrote a library called ts-throws that, perhaps verbosely, can facilitate typed error handling (enforced or otherwise.) I'm very curious as to the community's thoughts on something like this. I would love to add improvements and fix things to address concerns.

callistino commented 5 months ago

I read all comments with my best effort before trying once more to beat down on a simingly dead horse but I may've missed if a similar post was already created so I appologize if that's the case. I appreciate the thorough analysis and feedback regarding the proposal to introduce throws clauses in TypeScript. The points about the JavaScript runtime, ecosystem, and the current state of exception handling in JavaScript are well-taken. However, I believe there is still a compelling case for supporting throws clauses in a way that aligns with the dynamic nature of JavaScript and the static type-checking goals of TypeScript. Here are my counterpoints and suggestions:

Problem

If TypeScript aims to be a statically-typed superset of JavaScript, it should support and enhance all features present in existing JavaScript. Exceptions and error handling, do not have direct static typing counterparts in TypeScript.

Key Points:

Exception Handling: TypeScript's current handling of exceptions using instanceof and type guards is effective but verbose and less intuitive compared to a dedicated throws clause. While the proposed TC39 pattern matching in catch clauses will improve ergonomics, it does not address the core need for static analysis and documentation of possible exceptions thrown by functions.

Ecosystem survey: It is true that many JavaScript libraries do not document exceptions explicitly. However, this should not deter TypeScript from offering a mechanism to improve this situation. By providing throws clauses, TypeScript can lead by example, encouraging better documentation practices in the ecosystem. This is similar to how TypeScript has encouraged the adoption of static types in a traditionally dynamically-typed language community.

Language Capabilities and Cultural Absence: The absence of strongly-typed exceptions in JavaScript is largely due to historical and practical reasons rather than inherent language limitations. JavaScript’s evolution and its use in varied environments (e.g., browser, server) have contributed to this. However, TypeScript’s goal is to bring type safety to JavaScript, and incorporating throws clauses can be seen as a natural extension of this goal, improving error handling and documentation.

Argument for Including throws Clauses

To address the concerns about the impracticality of immediate, widespread adoption and the issues with existing undocumented exceptions, I propose an incremental and flexible approach:

Optional throws Clauses with Default Behavior: Functions can optionally specify throws clauses. If not specified, it defaults to throws unknown. This ensures that existing codebases remain unaffected while new code can opt-in for better error handling and documentation.

function fetchData() throws NetworkError {
  // Function logic
  if (networkFails) {
    throw new NetworkError("Failed to fetch data");
  }
}

Compiler Warnings and Documentation Encouragement: The TypeScript compiler can provide warnings for unhandled exceptions only if a function being called in its body explicitly declares a throws clause. This reduces noise for developers and allows gradual adoption without overwhelming them with warnings from unannotated legacy code.

function processData() {
  fetchData(); // Warning: unhandled NetworkError
}

Enhanced Error Handling and Clarity: By encouraging developers to specify throws clauses, we improve the clarity and maintainability of code. This is especially useful in large codebases and for third-party library integrations, where knowing the types of potential exceptions is crucial.

Aligning with TypeScript’s Goals: Introducing throws clauses aligns with TypeScript’s mission to enhance JavaScript with static types. It provides developers the tools to write safer, more predictable code without changing JavaScript’s runtime behavior.

Reconsideration Points

Adoption and Documentation: As the TypeScript community adopts throws clauses, we can expect better documentation practices to emerge, similar to how TypeScript has improved type documentation in libraries.

thw0rted commented 5 months ago

@callistino I like a lot of your points but honestly your ideas were pretty well covered in #13219. One key point is that the TS compiler does not generate "warnings", only errors -- any type issues are fatal. You wouldn't think this would be the end of the world, but it means that in a codebase with partial throws-clause coverage, the annotations are either useless (checking flag disabled) or deafening (checking flag enabled).

Personally, I think that this is the first case I've seen of a language keyword that I would like to see added not for the benefit of the compiler, but for 3rd party tooling. The eslint folks have already said that marking promises as "never rejects" is too hard to implement without language support. As you point out, being able to mark unhandled, advertised / expected exceptions as a non-fatal linter warning would be great. And even Intellisense could maybe benefit from "hinting" about exception/rejection types. Still, I understand the team's hesitance to add a keyword that wouldn't actually deliver value to the type-checker.

RyanCavanaugh commented 5 months ago

+1 to @thw0rted's comment; the proposed behavior doesn't seem even at all different from what was proposed in #13219. It's just restating the original proposal.

ethanresnick commented 5 months ago

@callistino I'd encourage you to look at #57943, which tries to get the documentation and ergonomics benefits you're describing, without going for checked exceptions (or manually-maintained throws annotations). It maybe runs into the issue @thw0rted mentioned about "the team's hesitance to add a keyword that wouldn't actually deliver value to the type-checker". There are also some open questions about how/if it could be implemented.

But it is trying to solve a similar problem, and in a way that I think makes sense to have in Typescript, rather than re-implementing in a linter, because it:

sebastianvitterso commented 3 months ago

Most of what I'm reading from others about "type safe try/catch", it seems people want (Java's) function throws statement. As a simpler solution, which probably has been proposed before (I have yet to see it), I think the catch statement should allow types.

So as an example:

// typescript
try { 
  myFunction()
} catch (e: Error) {
  console.error(e)
}

could generate into js:

// javascript
try { 
  myFunction()
} catch (e) {
  if (e instanceof Error) {
    console.error(e)
  } else {
    throw e
  }
}

This would theoretically also allow us to catch multiple types with multiple catch statements, just like Java does, with something like

// typescript
try { 
  myFunction()
} catch (e: Error) {
  console.error(e)
} catch (e: OtherError) {
  console.log("Other error occured")
}

which would become

// javascript
try { 
  myFunction()
} catch (e) {
  if (e instanceof Error) {
    console.error(e)
  } else if (e instanceof OtherError) {
    console.log("Other error occured")
  } else {
    throw e
  }
}

This would require changes to what is accepted as "valid" ts, compared to js, given that multiple catch-statements isn't valid js, but we already write invalid JS as TS, so it's not that far of a stretch.

This would allow us to more easily catch specific errors using the existing try/catch system, without having to touch function syntax or anything of the sort, nor is the developer required to use monads for every function call.

thw0rted commented 3 months ago

I think this has been suggested in the past, and shot down as a violation of "non goal" 5:

Add or rely on run-time type information in programs, or emit different code based on the results of the type system. Instead, encourage programming patterns that do not require run-time metadata

They really don't like emitting substantially-different JS, it should be possible to just strip away types and more or less generate a valid JS program.

sebastianvitterso commented 3 months ago

Ah, alright, that makes sense.

And I guess it would be challenging to only be able to catch class-based types here. Something like this might be a challenge:

type A = Error | string
type B = `${'Funny' | 'Boring'}Error`

try {}
catch (e: A) {}
catch (e: B) {}

What would be generated for this, then?

try {}
catch(e) {
  if (e instanceof Error || typeof e === "string") {}
  else if (typeof e === "string" && /^(Funny|Boring)Error$/.test(e)) { /** this test can get super-complex */ }
  else { throw e }
}

Fair fair.

DScheglov commented 3 months ago

I'd like to add my two cents here as well.

When working on the backend, I encounter two different types of errors:

  1. Expected Errors
  2. Unexpected Errors

Expected Errors

Example: When handling the user registration form, we should return something like "ERR_USER_IS_ALREADY_REGISTERED" to the API caller (instead of something vague like "Unique index violation").

Usually, such errors describe domain-specific cases.

Some protocols working over HTTP typically use the 200 HTTP status for expected errors, treating them as expected but not successful results (as in the case of GraphQL).

Unexpected Errors

Example: When handling the same user registration form, we should return a 500 HTTP status with an error message like "Internal Server Error" if our API server loses the connection to its database.

Usually, such errors describe infrastructure failures or (explicit or implicit) assertion mismatches. These errors are often returned with an HTTP status of 500 to API clients.

Approach for Expected Errors

Returning to the user registration form...

type UserRegistrationErrorCode =
   | 'ERR_USER_IS_ALREADY_REGISTERED'
   | // --- snip --
;

class UserRegistrationError extends Error {
  constructor(public code: UserRegistrationErrorCode) {
     super(`${code}`);
  }
}

interface IUserService {
  register(userData: UserRegistrationData): Promise<User | UserRegistrationError>;
}

So, we simply return the error explicitly from the register method, rather than throwing it—just return it.

On the calling side, we should handle it like this:

async function signUp(
  userData: UserSignUpData,
): Promise<{ user: User, session: Session } | UserRegistrationError> {
  const registrationData = prepareUserRegistrationData(userData);
  const user = await userService.register(registrationData);

  if (user instanceof Error) {
    // user type is correctly narrowed to UserRegistrationError
    return user;
  }

  const session = await sessionService.startUserSession(user);

  return { user, session };
}

In general, this approach looks simple and robust, but if (user instanceof Error) can be annoying, especially in deeply nested code.

This method lacks the implicit error propagation that exceptions provide. To achieve automatic propagation, we might turn to FP’s Either monad and generators. However, this approach can negatively impact performance and, even worse, confuse less experienced developers. On the other hand, it does offer some benefits, such as the ability to handle or map both errors and successful results in a single expression.

Approach for Unexpected Errors

Simply throw them. Catch the corresponding exception in the error boundary (depending on the external application interface), sanitize it, and report it.

Sometimes, we need to transform an unexpected error into an expected one. For example, in the case of the user registration form, the database driver might return a UniqueIndexViolationError. We must intercept the exception, ensure that this is the exact error we caught, and handle it by returning a UserRegistrationError with the appropriate code.

Therefore, the error boundary might not be the only place where we catch unexpected errors. In other areas, we should convert the error into an expected one and return it, or we should re-throw the same error (without any modification) to avoid breaking the stack trace and losing other important information.

It Seems Like We Have Everything We Need

Unfortunately, returning errors introduces a lot of noisy if statements, and it requires explicit result assignment if the main (non-error) function result is void.

To address this, we need an operator that allows us to return the received error, enabling us to write code in the following way:

async function signUp(
  userData: UserSignUpData,
): Promise<{ user: User, session: Session } | UserRegistrationError> {
  const registrationData = prepareUserRegistrationData(userData);
  const user = try? await userService.register(registrationData);
  const session = await sessionService.startUserSession(user);

  return { user, session };
}

Additionally, we need an operator that throws any Error-result if we know that no errors are expected.

async function signUp(
  userData: UserSignUpData,
): Promise<{ user: User, session: Session }> {
  const registrationData = prepareUserRegistrationData(userData);
  const user = try! await userService.register(registrationData);
  const session = await sessionService.startUserSession(user);

  return { user, session };
}

I've chosen try? and try!, but it could be something else.

Additionally, instead of checking if the value is an instance of the Error class, it's better to introduce a Symbol, such as Symbol.result.

type ResultValue<T, E> =
  | { isError: false, value: T }
  | { isError: true, value: E }

interface Result<T, E> {
  [Symbol.result](): ResultValue<T, E>;
}

So, the protocol for try? looks like:

  1. Pre-condition: The value (the operand of the operator) must implement the Result interface (for non-TS projects, assume this condition is met).
  2. Retrieve resultValue using value[Symbol.result]().
  3. If resultValue.isError, exit the function with a value: Result<never, E>; otherwise, return resultValue.value: T.

For try!, steps 1 and 2 are the same, and:

  1. If resultValue.isError, throw resultValue.value; otherwise, return resultValue.value.

Summary

I understand the TS Team's position regarding language extensions: prioritizing ECMAScript first. This approach makes sense, but the JS community is currently considering a proposal for a "Safe Assignment Operator". However, they are not particularly interested in strict error typing, so we cannot expect TC39 to address typed errors anytime soon.

So, We need a Result Type in TypeScript to tackle the typed errors.

DScheglov commented 3 months ago

About ts-throws approach to "expected errors".

I've also tried the similar one Function.prototype.throw

The code looks like the following;

const sqrt = (a: number): number =>
  a >= 0 ? Math.sqrt(a) : sqrt.throw(new SqrtError());

sqrt.throws = [SqrtError] as const;

const div = (a: number, b: number): number => 
  b !== 0 ? a / b :
  a !== 0 ? div.throw(new DivisionError("ERR_DIV_BY_ZERO")) :
            div.throw(new DivisionError("ERR_INDETERMINATE_FORM"));

div.throws = [DivisionError] as const;

const quadraticEquation = (a: number, b: number, c: number): [number, number] => {
  const rootOfDiscriminant = sqrt(b * b - 4 * a * c);

  return [
    div(-b - rootOfDiscriminant, 2 * a),
    div(-b + rootOfDiscriminant, 2 * a),
  ];
}

quadraticEquation.throws = [
  ...div.throws,
  ...sqrt.throws,
] as const;

The approach allows to reflect the "expected" errors in function type that could be using in interfaces as well, but the main problem is a need to manually reflect "inhereted" errors from the called function.

dylanpizzo commented 2 days ago

I haven't read literally every comment here (there are just too many), but I'm a bit confused about why there's so much focus on enforcing that certain exceptions get caught. In the original proposal for the throws type annotation, TypeScript would check the assignability of functions based on their throws types. This means that if I want to write a function and guarantee it doesn't throw, I can type it with throws never. Similarly, if I'm accepting a callback and want to ensure it cannot throw, I can type the callback parameter with throws never. This approach allows us to opt individual functions into throwing only certain errors.

Isn't this level of control sufficient for most cases? Why should TypeScript enforce that every time you call a function, you have to wrap it in a try/catch block? Such enforcement seems unnecessary when we can already manage exception types through throws annotations and function assignability.