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

Improve catch with automatic instanceof checks #46690

Closed
4 of 5 tasks
pickypg opened this issue Nov 4, 2021 · 9 comments
Closed
4 of 5 tasks

Improve catch with automatic instanceof checks #46690

pickypg opened this issue Nov 4, 2021 · 9 comments
Labels
Duplicate An existing issue was already created

Comments

@pickypg
Copy link

pickypg commented Nov 4, 2021

Suggestion

The recent change that, by default, makes catch variables as unknown (#41016) was an unexpected breaking change.

While writing some code to effectively workaround the change, I found myself writing a lot of duplicative, generic code similar to the example. In addition to err: unknown being the default behavior, I think that Typescript could add some syntax sugar to simplify code blocks for types that support instanceof checks similar to what Java and C# do:

try {
  invokeSomething();
} catch (err: Error | HttpError) {
  console.error(err.message);
} catch (err: OtherType) {
  console.error(err.otherTypeProperty);
} catch (err: unknown) {
  // good luck
  throw err;
}

There are obviously legacy codebases that throw strings and other silly "error" types, but the vast majority could be pushed into using structured Error types with what amounts to syntax sugar. The emitted JS code could be

try {
  invokeSomething();
} catch (err) {
  if (err instanceof Error || err instanceof HttpError) {
    console.error(err.message);
  } else if (err instance OtherType) {
    console.error(err.otherTypeProperty);
  } else {
    // good luck
    throw err;
  }
}

with the err being rethrown automatically if there is no matching catch clause (e.g., the developer never specifies err: unknown).

Bonus points if there was an is functional way to do it for non-instanceof types (for codebases that use canonically typed Error objects and, ideally, a built-in version of that that automatically matches to err: Error). C# effectively supports this notion as an "exception filter".

🔍 Search Terms

catch, unknown, instanceof, Error

Relates to #20024, #42596

✅ Viability 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, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Add Typescript syntax sugar to consistently simplify unknown error type handling.

📃 Motivating Example

No more repetitive code checks for consistently typed codebases for errors! Less errors in your errors!

💻 Use Cases

Stop requiring boilerplate code for every error that will regularly lead to silently ignored errors (including in the example for err:unknown itself).

@MartinJohns
Copy link
Contributor

This is out of scope of TypeScript, and a duplicate of #30830.

@pickypg
Copy link
Author

pickypg commented Nov 4, 2021

Is there a reason it's out of scope, particularly in light of the err: unknown change?

@pickypg
Copy link
Author

pickypg commented Nov 4, 2021

I think I see in the issue I missed: Typescript does not want to require type-related changes. Although the compiler should be able to generate the checks to avoid dynamic runtime changes, it definitely depends on the the types in the catch.

Even so, I'd wholly recommend this be exempted from that. The unknown change's effect on actual code is far worse than implementing this.

@MartinJohns
Copy link
Contributor

Is there a reason it's out of scope, particularly in light of the err: unknown change?

That change doesn't result in different emitted JavaScript.

@MartinJohns
Copy link
Contributor

MartinJohns commented Nov 4, 2021

The unknown change's effect on actual code is far worse than implementing this.

It really isn't. First, it's optional, it's a flag. Secondly, it actually improves type-safety by getting rid of the any. Now you're pushed towards actually checking what error value you're dealing with, whereas with any you can just access and fail.

But again, it's optional. You're free to revert back to the old behaviour.

I'd wholly recommend this be exempted from that.

I also don't see what real benefit your suggestion brings, given that you can just simply write the if check yourself and it's just as readable. It's just additional syntax that may result in incompatibilities with future ECMAScript changes.

@pickypg
Copy link
Author

pickypg commented Nov 4, 2021

Now you're pushed towards actually checking what error value you're dealing with, whereas with any you can just access and fail.

I agree with this at face value, but in practice it's going to cause developers to accidentally, silently ignore unexpected errors because of ECMAScript's past mistakes. The example for how to work with it exemplifies this problem: check for the type you expect, but do nothing otherwise.

I would rather have the runtime failure in the code than that from the developers making the mistake they're inevitably making from copying the example. That's what I mean by being worse; the type safety is good, but the implicitly new else code path they should be adding just doubled the complexity of every catch clause.

I can fix this in my codebase by disabling the setting, but I can't fix it downstream (or up for that matter).

I also don't see what real benefit your suggestion brings, given that you can just simply write the if check yourself and it's just as readable.

One time, of course. But what if you're catching and handling errors in 10s, 100s, or 1000s of places in any reasonably complex codebase?

The vast majority of codebases would likely be

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

With the intent that anything else was thrown up the chain untouched, ideally leading to a crash. That definitely cuts down on the boilerplate and code complexity imposed by unknown, especially if there was a canonical is check for Error.

@andrewbranch andrewbranch added Suggestion An idea for TypeScript Duplicate An existing issue was already created and removed Suggestion An idea for TypeScript labels Nov 4, 2021
@MartinJohns
Copy link
Contributor

You might want to look into railway-oriented programming, or other more functional approaches to error handling, if your code base is littered with catch-blocks. Having catch-blocks is the exception in code bases I work with.

@fatcerberus
Copy link

Indeed, this is starting to sound suspiciously like a request for checked exceptions, just without the compile-time enforcement.

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants