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

Turning errors into values is dangerous and will lead to bugs. Exceptions should be either caught or propagated. #44

Open
justinfagnani opened this issue Sep 13, 2024 · 23 comments

Comments

@justinfagnani
Copy link

I think that the very idea of automatically turning errors into values is dangerous and should not be added to the language. It would make it far too easy to ignore exceptions and cause silent failures.

It's still useful to improve try/catch ergonomics, but I think that any proposal should only allow for propagating an exception or handling the exception, not accidentally ignoring it.

The problem is the potential for this:

const [value, error] = try foo();

doSomething(value);

It's nice that I could use const on the value variable instead of let, but that's a small benefit.

What I really want is a way to make the success case much more ergonomic, while still giving me a block to handle exceptions in. Maybe something like:

// We still get a catch block to handle errors in
const value = try foo() catch (e) {
  throw new MyError('Error trying to foo');
};

Ideally there would be a way to return a new value from the catch() block, similar to how Promise.catch() works.

I'm not sure if some of the previous try-expressions proposals have covered this ground.

@arthurfiorette
Copy link
Owner

Its the reverse.

const [error, value] = try foo();

You also won't be able to forget to handle error because value will be undefined (or null) and a Uncaught TypeError: Cannot read properties of undefined will be thrown

@ljharb
Copy link

ljharb commented Sep 13, 2024

why would an exception be thrown? const [a, b] = [1]; doesn't throw.

@justinfagnani
Copy link
Author

@arthurfiorette is talking about the success value.

But undefined might be a valid return value.

If the correct usage is to always check the error, then why not enforce that syntactically? I think the most annoying boiler plate that could be reduced with new syntax is the separate variable declaration and assignment inside the try block.

The catch block and error conditional would be roughly equivalent, so I'm not sure there's much to gain there.

Its the reverse.

Oops, though that might be a reason to not use a tuple.

@arthurfiorette
Copy link
Owner

arthurfiorette commented Sep 13, 2024

why would an exception be thrown?

It would throw inside doSomething where it expects a defined value.

@justinfagnani
Copy link
Author

why would an exception be thrown?

It would throw inside doSomething where it expects a defined value.

But of course doSomethingmay not require a defined value.

@ljharb
Copy link

ljharb commented Sep 14, 2024

Indeed, every possible value is legitimately throwable, including null and undefined.

@DScheglov
Copy link

DScheglov commented Sep 17, 2024

@arthurfiorette

You also won't be able to forget to handle error because value will be undefined (or null) and a Uncaught TypeError: Cannot read properties of undefined will be thrown

It depends on how we handle the value after the function call.

For instance:

const [, filename] = try config.read('outputFile');

fs.writeFileSync(
  `${new Date().toISOString().slice(0, 10)}/${filename}`,
  someData,
);

No error is thrown, but we might end up with a very unexpected file being written. Even TS doesn't help in this case. Consider the snippet

This brings us back to the well-known concern: the "safe assignment operator" (or "try-expression") can lead to implicitly ignored errors.

This tradeoff must be acknowledged, and the proposal should include coverage of this issue, such as a linting rule or even rules:

  • check that error is read (or assigned)
  • check the error is correctly checked (!= null or something like that)

@arthurfiorette
Copy link
Owner

For me, [, filename] or [_, filename] is very well explicit. No developer tools will end up suggesting this pattern and the user should have to write it manually.

I think your point is the same way as saying that using || instead of ?? will lead to implicit bugs considering how JS works with falsy values.

@DScheglov
Copy link

@justinfagnani

In general, turning errors into values doesn't lead to bugs, at least not in TypeScript.
The error-values must be explicitly declared as part of the return value.

Go and Rust use two types of error handling in parallel:

  • The Result type in Rust and the (value, error) tuple in Go indicate that a specific function can fail. While there is an option to specify the exact type of error, it is not a widely adopted pattern in either of these languages.
  • panic and recovery are used to throw and catch unexpected and potentially unrecoverable errors.

In JavaScript, and especially in TypeScript, we have analogs for both of these features:

  • We can return errors (either wrapped or pure), which is analogous to Result and (value, error).
  • We can throw and catch exceptions, which is similar to a panic and recover.

The main issue is that neither the JS nor TS codebase maintainers currently have explicit rules about when to use which approach.

Some time ago, the TypeScript team conducted an analysis that showed even mature and widely used libraries don't document their error-handling strategies well enough.

This proposal, not a turning errors to values, leads to bugs ;)

@arthurfiorette
Copy link
Owner

arthurfiorette commented Sep 17, 2024

Either way, I'm leaning in favor towards a const [ok, result] = try with type ([true, T] | [false, unknown]) expression, where its harder to ignore the error and solves the throw undefined problem.

Wdyt?

@DScheglov
Copy link

@arthurfiorette

For me, [, filename] or [_, filename] is very well explicit. No developer tools will end up suggesting this pattern and the user should have to write it manually.

Common!!! You work alone and touch the same file single time during entire project life???

Perhaps we should define what does "implicit" or "explicit" means.

Implicit error ignoring means that developer is not warned about potential incorrect error handling and doesn't suppress this warning explicitly to express the intent to ignore the error.

The expression bellow is also not suggested by any developer tool to be used, but currently eslint alarms here with no-empty rule

try { doSomething(); } catch {}

So, the developer is forced to ignore this linting rule here.

I think your point is the same way as saying that using || instead of ?? will lead to implicit bugs considering how JS works with falsy values.

Yes, using || instead of ?? can lead to bugs! Actually ?? was introduced specifically to help resolve the bugs caused by using || inappropriately (mostly for 0, false and empty arrays).

And yes, you know, using == instead of === leads to bugs. We have a hundreds of linting rules to cover simillar buggy cases

@DScheglov
Copy link

@arthurfiorette

Either way, I'm leaning in favor towards a const [ok, result] = try with type ([true, T] | [false, unknown]) expression, where its harder to ignore the error and solves the throw undefined problem.

It brings a naming issue. The term result is too abstract, as instance which names to use for the fetch:

const [fetchOk, fetchResult] = try fetch(...);

it doesn't seem to be a good naming approach )

@arthurfiorette
Copy link
Owner

Common!!! You work alone and touch the same file single time during entire project life???

...

So, the developer is forced to ignore this linting rule here.

Then the same can be done for this proposal.

Yes, using || instead of ?? can lead to bugs!

Yes it will, what I was trying to explain is the following:

Miss use of || only happens because there were a time where ?? wasn't available, so we had to use ||. This "partial" implementation doesn't happen in a proposal.

It brings a naming issue. The term result is too abstract, as instance which names to use for the fetch:

That's why its not { ok, result }, a tuple can be destructured to any name.

@DScheglov
Copy link

DScheglov commented Sep 17, 2024

@arthurfiorette

Common!!! You work alone and touch the same file single time during entire project life???

Sorry for the familiarity. I just want to say that sometimes things just go wrong. We spend a lot of time, effort, and money ensuring code quality, yet we still encounter many bugs and 'non-clean' code.

So, we SHOULD presume such misuse.

So, the developer is forced to ignore this linting rule here.

Then the same can be done for this proposal.

The problem is that the proposal is used incorrectly and that leads to bugs.

Miss use of || only happens because there were a time where ?? wasn't available, so we had to use ||.

No! We had not. We use value != null ? value : defaultValue.

That's why its not { ok, result }, a tuple can be destructured to any name.

When we are talking about [error, value] -- yes, it is better, because we have two different names for success and error values.
With [ok, result] we have a confusion

And the object here is better:

const fetchResult = try fetch('....');

if (!fetchResult.ok) {
  throw fetchResult.error;
}

const response = fetchResult.value;

is much more readable. We don't use the same name for different entities.

@arthurfiorette
Copy link
Owner

Again, I don't see a world where "forgetting to handle the error" is an issue.

try is a opt-in syntax used by the Caller when intended to handle possible errors and rejections.

  • If you want to handle errors in the callee side, this proposal isn't of any help
  • If you do not to handle errors in the caller side, just don't use the try syntax.

try, explicitly, will only be used in error handling scenarios.

If someone is explicitly writing something that has the only purpose to improve error handling, to then ignore it, he is clear with his behavior.


In a different world we can say the same about Promise.withResolvers() where it can also allow the user to shoot himself in the foot in a scenario like this one:

const { promise, reject } = Promise.withResolvers()

if (getError()) {
  reject('error')
}

await promise

Did you found the error? No calls to resolve() were ever written, and its even worse than our scenario because resolve does not even shows up in the screen.

But why this isn't an issue? Because we would only want to use Promise.withResolvers when we will manually call resolve or reject, otherwise we wouldn't even use Promise.withResolvers in the first place.

@arthurfiorette
Copy link
Owner

The problem is that the proposal is used incorrectly and that leads to bugs.

The proposal is not being used incorrectly.

This is programming and any dev can do anything it wants and there's nothing we can do to avoid it.

The only time something can be done "wrongly" in your terms is when someone explicitly writes something to handle errors and then explicitly ignores the error. But I can argue that if 2 things were explicitly written, its not an implicit behavior.

@arthurfiorette
Copy link
Owner

[ok, error, data], [ok, result] [error, result] are all alternatives that we should raise a poll because it will fall back to our personal opinion.

[true, T, T] | [false, E]

Is also a pattern that allows both of these use cases.

const [ok, error, data] = try ...

if(ok) {
  data.something
} else {
  error.something
}

const [ok, result] = try ...

if(ok) {
  result.something
} else {
  result.something
}

But as I said above, a poll should be raised to figure a better solution.

@arthurfiorette
Copy link
Owner

And the object here is better:

That's your preference, and its ok.

We could also make the try operator return a object that implements iterable, which would allow for both tuple and object usage.

@DScheglov
Copy link

@arthurfiorette

Please compare with a regular try ... catch

We cannot omit catch if we have written the try. And thanking to eslint we cannot leave the catch-block empty.
For the try-expression we need just the same linting rule.

@arthurfiorette
Copy link
Owner

I don't see a reason to why not propose a linting rule after this proposal gets accepted.

I just don't why turning errors into values is dangerous.

@justinfagnani
Copy link
Author

Again, I don't see a world where "forgetting to handle the error" is an issue.

try is a opt-in syntax used by the Caller when intended to handle possible errors and rejections.

I don't think this is an accurate view of what a lot the desire is for simpler try syntax.

In many, many cases the friction with try/catch isn't the error handling code - which you can see by many of the examples in this proposal having very similar error handling structure as the analogous catch block - the friction that we want to avoid is with the success case for simple assignments, while still handling errors. Optimizing the syntax for that is great.

To argue my point that errors should not be trivially turned into values, I'll start from some points that I hope are relatively uncontroversial:

  1. Exceptions should either be handled or propagated. Rarely just ignored.
  2. Evaluating expressions that possibly throw outside of a try/catch block propagate errors by default.
  3. Evaluating expressions that possibly throw inside of a try/catch block force the caller to at least write a catch block, where it's pretty obvious if you're trivially ignoring the error.
  4. Returning an error as a value makes it easier to ignore errors than with a try/catch block.
  5. try/catch makes it cumbersome to assign a possibly throwing expression to a variable.

With these points I don't think a solution for the syntax overhead of try/catch has to lead to a syntax that's more dangerous.

Instead, we can solve the actual pain point and make safe assignments easier while still syntactically encouraging handling of the error.

A shape of that type of solution might be to allow for the try part to be an expression and require the catch block to still be a block. That's why I'm suggesting something like:

// Try is an expression we can use in assignments, but
// we still get a catch block to handle errors in

const value = try foo() catch (e) {
  // handle errors here.
  console.error('foo() threw', e);
};

Ignoring errors is still as obvious as with try/catch blocks:

// You must include the catch block
const value = try foo() catch (e) {
  // ignore error
};

I don't see a reason to why not propose a linting rule after this proposal gets accepted.

If such a syntax were added to the standard, many people would add such a linting rule banning this syntax as unsafe. That's a sign that the syntax itself could be a lot better.

@DScheglov
Copy link

DScheglov commented Sep 17, 2024

@arthurfiorette

I don't see a reason to why not propose a linting rule after this proposal gets accepted.

This proposal must prepare the criteria for this rule. And it must be designed with minimizing this criteria and rules required.
So, it is better to develop this proposal considering the need of the rule

We could also make the try operator return a object that implements iterable

it will work only for the JS. For TS it is impossible to specify the Iterator type with different type, only with a single type, so the value will always be T | undefined and actually the problem with nulish errors comes back.

@arthurfiorette
Copy link
Owner

arthurfiorette commented Sep 17, 2024

@DScheglov the same way tuples are literables that have their typings ordered, this could be a case for it as well.

We propose and improve JS and then TS aligns itself with the language, not the opposite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants