-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Suggestion: throws
clause and typed catch clause
#13219
Comments
Just to clarify - one the ideas here is not to force users to catch the exception, but rather, to better infer the type of a catch clause variable? |
@DanielRosenwasser
But it will give developers a way to express which exceptions can be thrown (would be awesome to have that when using other libraries |
how is a checked throw different from type Tried<Result, Error> = Success<Result> | Failure<Error>;
interface Success<Result> { kind: 'result', result: Result }
interface Failure<Error> { kind: 'failure', error: Error }
function isSuccess(tried: Tried<Result, Error>): tried is Success<Result> {
return tried.kind === 'result';
}
function mightFail(): Tried<number, string> {
}
const tried = mightFail();
if (isSuccess(tried)) {
console.log(tried.success);
} else {
console.error(tried.error);
} instead of try {
const result: Result = mightFail();
console.log(success);
} catch (error: Error) {
console.error(error);
} |
You're suggesting not to use
Adding
And this is the good case when the error is documented. |
is there a reliable way in javascript to tell apart SyntaxError from Error?
other than that encoding an exception as a special result case is a very common practice in FP world whereas splitting a possible outcome into 2 parts:
looks a made up difficulty in my opinion, throw is good for failing fast and loud when nothing you can do about it, explicitly coded results are good for anything that implies a bad yet expected situation which you can recover from |
consider: // throw/catch
declare function doThis(): number throws string;
declare function doThat(): number throws string;
function doSomething(): number throws string {
let oneResult: number | undefined = undefined;
try {
oneResult = doThis();
} catch (e) {
throw e;
}
let anotherResult: number | undefined = undefined;
try {
anotherResult = doThat();
} catch (e) {
throw e;
}
return oneResult + anotherResult;
}
// explicit results
declare function doThis(): Tried<number, string>;
declare function doThat(): Tried<number, string>;
function withBothTried<T, E, R>(one: Tried<T, E>, another: Tried<T, E>, haveBoth: (one: T, another: T) => R): Tried<T, R> {
return isSuccess(one)
? isSuccess(another)
? successFrom(haveBoth(one.result, another.result))
: another
: one;
}
function add(one: number, another: number) { return one + another; }
function doSomething(): Tried<number, string> {
return withBothTried(
doThis(),
doThat(),
add
);
} |
My point with You can represent the same bad situation with throwing an error. Sometimes you have a long chain of function invocations and you might want to deal with some of the errors in different levels of the chain. I disagree with you, in a lot of cases you can recover from thrown errors, and if the language lets you express it better than it will be easier to do so. Like with a lot of things in typescript, the lack of support of the feature in javascript isn't an issue.
Will work as expected in javascript, just without the type annotation. Using |
if we talking about browsers var child = window.open('about:blank');
console.log(child.Error === window.Error); so when you do: try { child.doSomething(); } catch (e) { if (e instanceof SyntaxError) { } } you won't catch it another problem with exceptions that they might slip into your code from far beyond of where you expect them to happen try {
doSomething(); // <-- uses 3rd party library that by coincidence throws SyntaxError too, but you don' t know it
} catch (e) {} |
besides class StandardError {}
class CustomError extends StandardError {
}
function doSomething() { throw new CustomError(); }
function oldCode() {
try {
doSomething();
} catch (e) {
if (e instanceof StandardError) {
// problem
}
}
} |
@Aleksey-Bykov Explicitly threading errors as you suggest in monadic structures is quite hard and daunting task. It takes a lot of effort, makes the code hard to understand and requires language support / type-driven emit to be on the edge of being bearable. This is a comment comming from somebody who puts a lot of effort into popularising Haskell and FP as a whole. It is a working alternative, especially for enthusiasts (myself included), however I don't think it's a viable option for the larger audience. |
Actually, my main concern here is that people will start subclassing Error. I think this is a terrible pattern. More generally, anything that promotes the use of the instanceof operator is just going to create additional confusion around classes. |
i really think this should be pushed harder to the audience, not until it's digested and asked for more can we have better FP support in the language and it's not as daunting as you think, provided all combinators are written already, just use them to build a data flow, like we do in our project, but i agree that TS could have supported it better: #2319 |
Monad transformers are a real PITA. You need lifting, hoisting and selective running fairly often. The end result is hardly comprehendible code and much higher than needed barrier of entry. All the combinators and lifting functions (which provide the obligatory boxing/unboxing) are just noise distracting you from the problem at hand. I do believe that being explicit about state, effects, etc is a good thing, but I don't think we have found a convenient wrapping / abstraction yet. Until we find it, supporting traditional programming patterns seems like the way to go without stopping to experiment and explore in the mean time. PS: I think we need more than custom operators. Higher Kinded Types and some sort of type classes are also essential for a practical monadic library. Among them I'd rate HKT first and type classes a close second. With all that said, I believe TypeScript is not the language for practicing such concepts. Toying around - yes, but its philosophy and roots are fundamentally distant for a proper seamless integration. |
Back to the OP question - |
Developers should be aware of the different js issues you described, after all adding The fact that 3rd party libraries ca throw errors is exactly my point. @aluanhaddad @gcnew |
@nitzantomer Subclassing native classes ( |
@gcnew In anycase this suggestion doesn't assume that the user is subclassing the Error class, it was just an example. |
@nitzantomer I'm not arguing that the suggestion is limited to
For the cases where you want to distinguish among known alternatives TypeScript has Tagged Unions (also called discriminated unions or algebraic data types). The compiler makes sure that all cases are handled which gives you nice guarantees. The downside is that if you want to add a new entry to the type, you'll have to go through all the code discriminating on it and handle the newly added option. The upside is that such code would have most-likely been broken, but would have failed at runtime. |
I just gave this proposal a second thought and became against it. The reason is that if |
@gcnew Also, this suggestion takes error inferring into account, something that won't happen if errors are coming from documentation comments. With inferred checked exceptions the developer won't even need to specify the I agree that enforcing error handling isn't a good thing, but having this feature will just add more information which can be then used by those who wish to.
Is that there's no standard way of doing it. |
I would love to have information in the tooltip in VS if a function (or called function) can throw. For |
@HolgerJeromin |
here is a simple question, what signature should be inferred for function mightThrow(): void throws string {
if (Math.random() > 0.5) {
throw 'hey!';
}
}
function dontCare() {
return mightThrow();
} according to what you said in your proposal it should be function dontCare(): void throws string { i say it should be a type error since a checked exception wasn't properly handled function dontCare() { // <-- Checked exception wasn't handled.
^^^^^^^^^^ why is that? because otherwise there is a very good chance of getting the state of the immediate caller corrupt: class MyClass {
private values: number[] = [];
keepAllValues(values: number[]) {
for (let index = 0; index < values.length; index ++) {
this.values.push(values[index]);
mightThrow();
}
}
} if you let an exception to slip through you can not infer it as checked, because the behavior contract of the only safe way to is catch them immediately and rethrow them explicitly keepAllValues(values: number[]) {
for (let index = 0; index < values.length; index ++) {
this.values.push(values[index]);
try {
mightThrow();
} catch (e) {
// the state of MyClass is going to be corrupt anyway
// but unlike the other example this is a deliberate choice
throw e;
}
}
} otherwise despite the callers know what can be trown you can't give them guarantees that it's safe to proceed using code that just threw so there is no such thing as automatic checked exception contract propagation and correct me if i am wrong, this is exactly what Java does, which you mentioned as an example earlier |
@Aleksey-Bykov
Means that both
Won't have a
Will have As for your
|
I have a few insights so thought I would write some here. The first is what benefits having a type defined for the
Inference of thrown valuesThe big problem as noted is that this requires adding / changing pretty much all function type descriptors. Ones that aren't changed would have to assume some value. IMO to do this properly and today TS needs to able to recognise thrown types from functions. To do this TS would need to add an 'events system'. When synthesizing a function, it would need to record all throw events. Handing
This would be a big change to TS's direction, moving away from type annotations as the source of truth instead to code as truth... Another part that I haven't seen mentioned, is not just registering The checked exceptions is also interesting task. For some contexts like the top level, uncaught errors would be useful to be reported. Functions on the other hand there are uses cases for errors to bubble up. For the callback for Finally, while I do like Rust's |
@kaleidawave in my view a function's Example: class A {
public foo?: string;
public get bar() {
return this.foo ?? "was undefined";
}
public baz() {
return this.bar;
}
}
const a = new A().baz(); // type detected as `string` Therefore, I do not see how this would be much different: class A {
public foo?: Error;
public get bar() {
throw this.foo ?? new TypeError("was undefined");
}
public baz() {
return this.bar;
}
}
try {
new A().baz();
}
catch (e) {
// e: Error | TypeError
} Could be further simplified by adding optional |
This is a huge problem in JS/TS. Some libraries throws exceptions as a normal part of the flow – as a sentinel value if you wish. The problem? You are not even informed of their existence. A concrete example: You would expect the following code (copying source into destination) to fail silently if the file already exists in destination – as the linux command await vscode.workspace.fs.copy(source, destination, { overwrite: false }); But instead, in this case, this function will throw an exception to inform you that the file already exists at destination! Yes. Indeed. It will throw an exception to let you know that everything is fine! And you are not even warned of this possible behavior by the type system! This is hell 🤪 JS devs do not deserve to get PTSD-like symptoms each time they call a function – worrying that it may explode to their face at any time, without warning. |
|
why is this issue closed? Is there any definitive conclusion? |
Just scroll up. #13219 (comment) |
I love the idea of checked exceptions. Personally I see value in this in regards to our team projects. If I implement function X that throws errors, and a junior developer wants to use it, they should handle the required errors. If TS caught them, I wouldn't have to rely on having to flag it down in code review (or if someone else was conducting the review, hoping the reviewer knew about the errors!) As noted, annotating function signatures sounds like it could be pretty nutty. I wonder if this information could be placed alongside async function doSomething(arg1: string, arg2: number) {
if (arg1.length > 10) {
throw safe new StringTooLongError();
}
if (arg2 < 0) {
throw safe new NumberLessThanZeroError();
}
// `safe` meaning "You did it wrong" error, don't enforce try/catch
// However if you think the developer absolutely should be required to try/catch...
try {
return await fetch('http://some-api.com/');
} catch (err) {
// fetch can throw "TypeError" (i.e. 'Failed to fetch')
// If the fetch types were updated, it could even force the developer to handle it.
// Regardless, if the developer knows of an undocumented, unsafe error, they could make it unsafe by proxy:
throw unsafe new RequestFailedError();
}
// Non-annotated errors behave as-is:
throw new Error('Whoops!')
} In this case, the resulting type signature would be something like: function doSomething(arg1: string, arg2: number): Promise<Response> throws unsafe RequestFailedError | safe StringTooLongError | safe NumberLessThanZeroError; Example usage: await doSomething("hello", 5);
// TS error: unsafe RequestFailedError exception must be caught
try {
await doSomething("hello", 5);
} catch (err) { // TS error: unsafe RequestFailedError exception not narrowed in catch block
// err: RequestFailedError | StringTooLongError | NumberLessThanZeroError
// Problem: after handling all of the above, does err become never, or unknown?
// Ideally it would become unknown, but current union behavior would reflect never.
} In the case of an un-documented error, to avoid unintentional inference, they should be ignored completely. I think this results in a much clearer experience. Developers opt into annotation, explicitly choosing which exceptions need to be handled and which ones don't. Or avoid it entirely if undesirable, however I imagine library consumers would argue otherwise. A counter argument to this may be "If we don't enforce annotation, we're stuck with the same problem that exists now with the lack of documentation." My response to this is, by providing rails (i.e. the "right way") then maintainers have to think less. I'm more likely to add one-word "unsafe" to my code, as opposed to maintaining the alternative in some sort of API documentation (that may/may not exist in the first place.) This leads into a point regarding one of the reconsideration points:
In my opinion, TypeScript is best positioned to champion this effort with this feature. I don't believe, without some sort of innovation, that this could be achieved. The idea here is incremental adoption, to encourage developers (and library maintainers) to handle errors more elegantly. Just because the culture isn't prevalent doesn't mean it can't be developed over time. One of the biggest complaints about JS is it's the "wild-west" where anything could go wrong, and this would be a good step in the right direction. To address the point of:
I think that's one of the main reasons developers want this feature. If this were to be implemented, I'm sure we'd find many consumers asking maintainers to "annotate exceptions in X function". This working out-of-box for all existing TS projects is not feasible, however given time for adoption, the ecosystem could vastly improve.
I wonder if there could be some sort of way to bypass this. // RegExp constructor is unsafe by default
const regex = new RegExp("[A-Z]") as safe; This sounds scary, but various linter rules could be implemented to push developers away from using Alternatively, a function could work around this if implementing function safe<T>(fn: () => T throws unsafe unknown): T {
try {
return fn()
} catch (error) {
throw safe error;
}
}
const regex = safe(() => new RegExp("[a-z]")); Which less ideal, but more readable than having to do Forgive me if any of these suggestions are naive or lack understanding! This feature seems very ideal to me and I'd rather the conversation continue than otherwise. In closing, my two-cents would be:
|
+1 I think consensus seems to be we are not requesting a perfect solution, a good one would be a great first step. This is a key place TS can help to reduce bugs and improve developers' efficiency to al least warn them against some known possible errors. It shouldn't be that hard. I liked @KashubaK Here's the swift official doc about throwing functions and a good example class VendingMachine {
var inventory = [
"Candy Bar": Item(price: 12, count: 7),
"Chips": Item(price: 10, count: 4),
"Pretzels": Item(price: 7, count: 11)
]
var coinsDeposited = 0
func vend(itemNamed name: String) throws {
guard let item = inventory[name] else {
throw VendingMachineError.invalidSelection
}
guard item.count > 0 else {
throw VendingMachineError.outOfStock
}
guard item.price <= coinsDeposited else {
throw VendingMachineError.insufficientFunds(coinsNeeded: item.price - coinsDeposited)
}
coinsDeposited -= item.price
var newItem = item
newItem.count -= 1
inventory[name] = newItem
print("Dispensing \(name)")
}
} |
On the point of the TS team's desire for an existing adoption of some sort of exception handling standard, I wonder what that could look like. Perhaps some other CLI tool that looks at If JSDoc and new TS syntax is out of the question for now, then the alternative is some sort of descriptor file that documents a file's exception handling. (To be clear, I'm not suggesting TS support something like this. I'm spitballing an idea for a different tool.) For example:
export class VendingMachine {
inventory = {
"Candy Bar": new Item({ price: 12, count: 7 }),
"Chips": new Item({ price: 10, count: 4 }),
"Pretzels": new Item({ price: 7, count: 11 })
}
coinsDeposited = 0;
vend(name: string) {
const item = inventory[name];
if (!item) {
throw new InvalidSelectionError();
}
if (item.count === 0) {
throw new OutOfStockError();
}
if (item.price > this.coinsDeposited) {
throw new InsufficientFundsError({ coinsNeeded: item.price - this.coinsDeposited })
}
this.coinsDeposited -= item.price;
item.count -= 1;
console.log(`Dispensing ${name}`)
}
} Then an adjacent file that documents this:
export default {
VendingMachine: {
vend: {
safe: { // Could accept an object containing more documentation
error: InvalidSelectionError,
description: "Lorem ipsum sit dolor amet",
},
unsafe: [ // If there are multiple errors, use an array
OutOfStockError, // For ease of use, accept the error class without extra documentation
{ error: InsufficientFundsError, description: "coins deposited is less than the item price" }
]
}
}
} Then have a CLI tool that introspects this information and inlines errors/warnings where applicable. I don't know how feasible this would be, or if there are improvements that could be made. However I think if we're passionate about this feature, we should try to address the TS team's concerns in one way or another. If some sort of solution could be built and there is sufficient developer demand for adoption then a TS implementation could be warranted in the future. |
@KashubaK, riffing off that same idea, I thought about creating an ESLint rule since that's already a widely adopted standard. It has some serious limitations, but maybe ones that someone who has built a rule before could weigh in on. The benefits would be that, for anyone using ESLint, it would be just one line they'd need to add to their config (or two, if we made multiple rules). I took a pass at a convo with ChatGPT which I'll share here in case it has some merits: https://chat.openai.com/share/8354ae88-a968-41c8-ab9a-eec8a23fa5ef The requirement I gave it: /**
* @throws {@link TypeError} if the notification is not found
*/
function calleeFunction1() {
}
/**
* This doc has text on top
* @throws {@link TypeError} if the notification is not found
*/
function calleeFunction2() {
}
/**
* @throws {@link TypeError} if the notification is not found
* This doc has text on bottom
*/
function calleeFunction3() {
}
function callerFunction() {
// This line should have a lint warning because it isn't wrapped in try / catch
calleeFunction1()
// So should this one
calleeFunction2()
// So should this one
calleeFunction3()
} The relevant es-lint rule it createdmodule.exports = {
rules: {
"enforce-exception-handling": {
create: function(context) {
const sourceCode = context.getSourceCode();
let functionsThatThrow = new Set();
let scopeChain = [];
function enterScope(node) {
scopeChain.push(node);
}
function exitScope() {
scopeChain.pop();
}
function currentScope() {
return scopeChain.map(node => sourceCode.getText(node)).join(' ');
}
return {
Program: enterScope,
FunctionDeclaration: enterScope,
FunctionExpression: enterScope,
ArrowFunctionExpression: enterScope,
"Program:exit": exitScope,
"FunctionDeclaration:exit": exitScope,
"FunctionExpression:exit": exitScope,
"ArrowFunctionExpression:exit": exitScope,
FunctionDeclaration(node) {
const throwsComment = node.leadingComments &&
node.leadingComments.find(comment => comment.value.includes("@throws"));
if (throwsComment) {
functionsThatThrow.add(currentScope() + ' ' + node.id.name);
}
},
CallExpression(node) {
const functionId = currentScope() + ' ' + node.callee.name;
if (functionsThatThrow.has(functionId)) {
let parent = node;
while (parent) {
if (parent.type === "TryStatement") return;
parent = parent.parent;
}
context.report({
node,
message: "The function " + node.callee.name + " can throw an exception and needs to be called inside a try-catch block."
});
}
}
};
}
}
}
}; As a side note, I agree with everything you've said. This would be a MASSIVE level-up to TypeScript and I'm sure we could find a way to make this feature opt-in for those who value it. And I bet we'd all be surprised how quickly it's adopted (by large library authors). Function calls into libraries that throw undocumented errors (even though they explicitly call throw) has been a real struggle with choosing node as my server solution. |
@kvenn I wonder if that could also be implemented in a Language Service Plugin? As for actually emitting errors, we could adopt a strategy akin to how tsc-strict has a CLI that will actually throw them (for CI or other purposes) One reason I'm leaning away from relying on |
I'm kind of liking the idea of a JSDoc linter rule solution, actually. Ideally it could:
In order to make this impactful a significant effort will be required.
Perhaps even go over to DefinitelyTyped and elsewhere to add documentation for libraries that we personally use. I also wonder how the TS team would respond to incrementally updating the Considering the 1,400+ upvotes on this issue, if we had a solid plan we could make this work! |
@RyanCavanaugh would it be possible to reopen this please? Thank you |
I don't think there's any actionable items in regards to this specific issue at this time. I think the next step should revolve around implementing a solution that encourages error checking in hopes to create widespread adoption of error documentation. After that point I imagine this issue could be brought back up, though I am definitely curious about their thoughts on our recent comments. I'm going to try my hand at a linter rule over the weekend. If it goes well, I'll link the repo back here and we can move that conversation there. |
I think it’s important enough to not be closed and encourage more discussion |
Agreed @KashubaK, I think the TypeScript team has made their opinion known. While I personally don't agree with the decision to deprioritize exception handling, I understand it. They don't think people will adopt it, and so it's on the community to prove otherwise. (That being said, I'd love if they reconsidered :)) Their reconsideration points made that clear:
@KashubaK, your steps sound right to me. And your idea of integrating with DefinitelyTyped would be a massive step towards adoption (and maybe the only step needed to satisfy reconsideration point 1). If you go with the ESLint approach, it looks like eslint-plugin-jsdoc already has the first bullet point (via As far as the spec of how exactly it should work
Java seems closest to the hybrid of supporting both (safe and unsafe). But I'd personally advocate for matching swift (checked exceptions), but with the ability to opt into
|
Thanks for your input @kvenn! The reason I wanted to introduce "safe" exceptions is to address an important note mentioned earlier:
But perhaps it would be more dangerous and confusing than helpful. Some configuration to whitelist certain functions might be appropriate instead. Regarding an RFC, for something like this I wouldn't hesitate, but I have some ideas that I need to make sure are even feasible before I potentially waste others' time. Regardless I would love a community effort, as it will be required to move the needle in any meaningful direction. |
Personally I agree. That being said, as @kvenn stated, the TS team has already made their stance clear. Robust error handling needs to be normalized in the community before such an invasive new concept is added to countless others' workflows. If our ideas aren't able to proliferate, well, perhaps they were doomed to begin with. |
It would be simpler to just use |
How would you specify the error type? |
|
Yeah, but then you lose all other type-checking for that line. |
I made an extension similar to this! Feel free to check it out: https://github.com/michaelangeloio/does-it-throw |
Very cool! Any plans to build for IntelliJ? |
@michaelangeloio Super cool, thanks for sharing! I would also love to see a JetBrains plugin implemented. Any plans/ideas about a CLI? Could it report the class of error? i.e. |
Locking for clarity. This issue is concluded and will not be reopened; see #13219 (comment) . For general discussion on the topic, continue if needed at #56365. |
The typescript type system is helpful in most cases, but it can’t be utilized when handling exceptions.
For example:
The problem here is two fold (without looking through the code):
In many scenarios these aren't really a problem, but knowing whether a function/method might throw an exception can be very useful in different scenarios, especially when using different libraries.
By introducing (optional) checked exception the type system can be utilized for exception handling.
I know that checked exceptions isn't agreed upon (for example Anders Hejlsberg), but by making it optional (and maybe inferred? more later) then it just adds the opportunity to add more information about the code which can help developers, tools and documentation.
It will also allow a better usage of meaningful custom errors for large big projects.
As all javascript runtime errors are of type Error (or extending types such as
TypeError
) the actual type for a function will always betype | Error
.The grammar is straightforward, a function definition can end with a throws clause followed by a type:
When catching the exceptions the syntax is the same with the ability to declare the type(s) of the error:
catch(e: string | Error) { ... }
Examples:
Here it’s clear that the function can throw an error and that the error will be a string, and so when calling this method the developer (and the compiler/IDE) is aware of it and can handle it better.
So:
Compiles with no errors, but:
Fails to compile because
number
isn'tstring
.Control flow and error type inference
Throws
string
.Throws
MyError | string
.However:
Throws only
MyError
.The text was updated successfully, but these errors were encountered: