tc39 / proposal-optional-catch-binding

proposal for ECMAScript to allow omission of the catch binding
https://tc39.github.io/proposal-optional-catch-binding/
86 stars 6 forks source link

Why ? #2

Closed SiegfriedEhret closed 7 years ago

SiegfriedEhret commented 7 years ago

Hello @michaelficarra !

I saw earlier today that this spec is now at stage 3. I don't understand the motivation and use-cases for it.

Actually I am pretty scared that allowing this may make devs forget about error handling and making production code untraceable etc. I think errors must be logged, traced and handled instead of being silent.

Would you mind detailing the why ?

Thanks !

ljharb commented 7 years ago

They can already, and very frequently, do this with try { … } catch (unused) { }, so this simply allows them to avoid the unused binding for these cases.

One common use case is JSON.parse, where you don't care what the error is, just "if it's not json, do nothing; if it is, parse it".

SiegfriedEhret commented 7 years ago

Hi @ljharb, thanks for the input.

I understand your example and I am convinced that we should log the error, maybe the data, to actually know that something unexpected happened.

Unhandled errors are a bad practice, pushing the possibility to hide them as a language feature is dangerous.

Side note: devs can have some input about unused vars by using the rule no-unused-vars from eslint.

ljharb commented 7 years ago

This proposal means that caughtErrorsIgnorePattern is no longer needed at all, and a useless variable binding need not be created.

bakkot commented 7 years ago

Also, omitting the binding makes it more explicit that you are intending to discard the error.

michaelficarra commented 7 years ago

As an example of its usefulness, just today I wrote code like

function hasCompatibleWebAPI() {
  try {
    // try to exercise the edge cases of the API
    return true;
  } catch(unused) {
    return false;
  } finally {
    // clean up any effects that our usage of the API may have had
  }
}

which would have made use of this feature.

SiegfriedEhret commented 7 years ago

Thanks @michaelficarra.

I think the real problem is not «how to help developers hide unused bindings» but «how to help developers handle errors correctly».

In your example, you should actually have a trace of what happened, and that is why error handling is important, and must not be hidden.

If you prefer to escape from the error handling tyranny, you should make a babel or whatever plugin, but not push a language feature that will allow devs to forget the real nature of try/catch statements.

(I am really sorry about being stubborn about this)

ljharb commented 7 years ago

Nothing's being hidden. Using this proposal is simply a cleaner form of including but ignoring the catch binding, which devs have always both been able to do and done.

sonhanguyen commented 7 years ago

I can imagine it makes sense with some sort of try catch expression (anyone knows such proposal please link here): const val = try 1/x catch 0 but not very useful to me in imperative try catch

@michaelficarra @ljharb with

try {
    // do stuff
}
catch (presumablyXError) {

you have a chance to assert your assumption and may be log something which is always a good practice. It's one thing if you don't, which is normal, it's another thing to encourage such habit by language support.

@bakkot What needs to be explicit is not that you omit, but why you do so. So often there should be a comment explaining anyway. IMO if you have to type a comment, why not do a better thing which is logging that message with the Error object.

once we start adding stuff like this there will be all sorts of proposal whose sole purpose is to save keystrokes. Conciseness is good, but Occam's razor can be applied here.

SiegfriedEhret commented 7 years ago

An interesting thread happened in the es-discuss mailing list: https://esdiscuss.org/topic/an-operator-for-ignoring-any-exceptions

I join Kai Zhu and Frederick Stark here, and in my opinion, their comments also applies to the optional catch binding proposal.

danny-andrews commented 7 years ago

Cool, looks like we're making Javascript more unsafe than it already is.

Wrote this very late last night. Sorry for the sarcasm. It's uncalled for.

ljharb commented 7 years ago

@danny-andrews try { something(); } catch (e) {} is actually less "safe" than try { something(); } catch {} - the difference is that in the former example, you can't be certain if the original developer forgot to handle the exception, or if they intended it to be swallowed - whereas in the latter example, the developer explicitly indicated that they intended the error to be swallowed.

This proposal makes JS more safe, because it allows the developer's intentions to be more explicitly communicated.

SiegfriedEhret commented 7 years ago

I just found this article about some use cases for this proposal from Dr. Axel Rauschmayer (this issue is actually linked at the bottom of the article).

I am with him in this part:

My recommendation is to avoid doing that:

  • Instead of completely ignoring an error, at least log it to the console.
  • Instead of assuming you know what the error will be, check for unexpected types of exceptions and re-throw them.
sonhanguyen commented 7 years ago

the thing that many people seem to oppose is swallowing exceptions. So I think @ljharb you should elaborate that, specifically response to the practices in @SiegfriedEhret's comment, before going further so say how this PR helps swallow exceptions.

This is where any planned idea for multiple catches/ pattern matching or similar should be considered while assessing this, and I'd like to see anyone who's involved in such proposal to share their view here.

My other point in addition to those is that people/ourselves are just gonna abuse it, we simply can't deny nor prevent that. The cost this can potentially impose on code quality across the ecosystem far outweigh a few cases when it is useful.

ljharb commented 7 years ago

@sonhanguyen You can already intentionally swallow exceptions, so it's simply not necessary to justify that use case - it exists, it's used, it's valuable to some.

Anyone who uses this proposed feature will be using it instead of catch (e) {} (ie, an empty catch block with an unused binding). There's no cost, and only benefits.

sonhanguyen commented 7 years ago

@ljharb you said that already but imho "it already happens" is not the same as "it's justifiable".

bterlson commented 7 years ago

The cost of this proposal on the language itself is miniscule. It's a tiny syntax change with no new semantics. So the root question seems to be whether this proposal will encourage developers to handle fewer errors when it makes sense to do so. This seems like a hard question to answer definitively without data.

My gut feeling is that the sum of bad catch { }es and bad catch (e) { }'s will be about the same before and after this change. In other words, developers may switch from one bad form to the other, but the total number of improperly swallowed errors won't change. Tools like eslint will rapidly learn new rules for this syntax as well.

Meanwhile, developers gain expressiveness and a segment of developers who like this pattern for certain uses get a nice syntax sugar. On the whole, seems like a positive to me.

sonhanguyen commented 7 years ago

occasionally I need to write callbacks like (ignored, concerning) => doSmt(concerning). I wish that the language allowed me to write (, concerning) => doSmt(concerning) too and in this case there doesn't seem to be anything bad coming out of it. Except for everytime such thing is added the language specs become a little longer, it takes a little more for teaching materials to cover.

Imagine what a javascript book will be 10 years from now when lots edge cases like this have been added to the language. Every page will have a footnote "by the way you can omit this when such and such". I don't think that makes a good learning experience.

jcrben commented 6 years ago

Another thought along the lines of what @sonhanguyen is saying above: allow for catch () instead since it looks more similar to the typical usage.

ljharb commented 6 years ago

The similarity would make it harder to know if the omission of a catch binding was intentional or a mistake.

danny-andrews commented 6 years ago

You can already intentionally swallow exceptions

@ljharb Just because you can already do something unadvisable, doesn't mean we should make it less painful to do so by adding a language feature to support it. It's a good thing that linters complain about these unused variables. It forces us to give a reason for not dealing with them via a comment or something else. This spec puts convenience over best practices which never turns out well.

The core issue here and the reason programmers find themselves ignoring errors in Javascript so often (as you've already alluded to) is because core language features (e.g. JSON.parse) throw Errors in non-exceptional circumstances. Getting invalid JSON is not exceptional. JSON.parse should return an Error instead of throwing an Error. Then you could write code like this.

const result = JSON.parse(fileContents);
if (result instanceof SyntaxError) {
  console.log('hey bro, your JSON file is not JSON');
} else {
  doTheThing(result);
}

There's an easy fix for this, however. Wrap JSON.parse in your own function which fixes this design flaw:

const parseJSON = (...args) => {
  try {
    return fn(...args);
  } catch(e) {
    return e;
  }
};
danny-andrews commented 6 years ago

In other words, developers may switch from one bad form to the other, but the total number of improperly swallowed errors won't change.

@bterlson I have to disagree, especially for junior programmers. If I write:

let result;
try {
  result = JSON.parse(fileContents);
} catch(e) {}

and get a linter error, I will be forced to think twice about what I'm doing. Sure, I might just add a comment 90% of the time (which is still better than doing nothing). But the other 10% I might be prodded to log the error or wrap the error in my own.

danny-andrews commented 6 years ago

Since engineering is really about evaluating PROs and CONs, I say we lay them out here and see which wins out. Here they are, as I see it:

PROs:

  1. When you want to ignore an Error for some reason, you can do so more explicitly and tersely.

CONs:

  1. Implicitly encourages programmers to ignore Errors by adding a language construct which makes it easy to do so.
  2. Adds more complexity to an already complex language (small but not non-existent).

I really can't comprehend how the PROs outweigh the CONs here. It really seems like we're just adding a feature to save character strokes, which is almost never the right approach since we spend ~10x more time reading code than writing it.

p.s. I'm not trying to create a straw-man here, so please let me know if there are any PRO's I'm missing, or if I have mischaracterized the one I listed.

ljharb commented 6 years ago

Errors that should be ignored are fine to ignore, and errors that are already ignored will continue to be ignored with or without this proposal; considering this a "con" doesn't make any sense.

As for "adds more complexity", every proposal does that, and that's not a sufficient reason on its face to combat any proposal - and the weight of that is already thus considered with every proposal.

bakkot commented 6 years ago

@danny-andrews

I'm not trying to create a straw-man here

Your "PRO" isn't really a strawman, though I'd put emphasis on the "explicitly" more than "tersely". But your later summary is: you summarize as "we're just adding a feature to save character strokes", but that has little to do with it. It's about expressing intent clearly.

And making it easier to clearly express intent, at a language level, is indeed mostly for the benefit of readers of the code, not writers.

danny-andrews commented 6 years ago

@ljharb

Errors that should be ignored are fine to ignore, and errors that are already ignored will continue to be ignored with or without this proposal; considering this a "con" doesn't make any sense.

You missed my point. This spec adds syntax to Javascript specifically for the purpose of ignoring errors. The "CON" is that it removes the friction involved in taking this action.

As for "adds more complexity", every proposal does that, and that's not a sufficient reason on its face to combat any proposal

Actually, that is a sufficient reason on its face to combat a proposal. The burden is on the proposal author to demonstrate that the improvements contained therein legitimize adding said complexity. My argument is that they don't.

danny-andrews commented 6 years ago

@bakkot

making it easier to clearly express intent, at a language level, is indeed mostly for the benefit of readers of the code, not writers.

You're correct in saying that this spec makes it easier to clearly express your intent to ignore an error, but it absolves you of giving a reason why, which is what is really important when taking an inadvisable action.

bakkot commented 6 years ago

The spec has never required you to give a reason why you are ignoring an error.

danny-andrews commented 6 years ago

Of course not, but linting rules will require you to add a comment to suppress them. Although, I suppose this will still apply to optional catch bindings... 🤔

I think I was getting no-unused-vars' caughtErrors option confused with no-empty's allowEmptyCatch option.

At any rate, at the very least, you should be using the error object to determine whether you should rethrow or not. E.g.:

let jsonData;
try {
  jsonData = JSON.parse(str);
} catch (err) {
  if (!(err instanceof SyntaxError)) {
    throw err;
  } 
}

http://2ality.com/2017/08/optional-catch-binding.html#use-case-jsonparse

"But oh, this is so verbose! I don't like all that boilerplate!" Me neither! That's why I use a wrapper which does this for me (see https://github.com/tc39/proposal-optional-catch-binding/issues/2#issuecomment-331673553).

danny-andrews commented 6 years ago

Last thing: try/catch is a bad construct and using it for control flow is bad practice. Let's preserve the pain involved in using try/catch so people seek out a better alternative.

preserve the pain

Probably the most emo thing I've said since middle school. 🔥🤕🔥

sonhanguyen commented 6 years ago

@ljharb if you encounter the following

catch {
   // it was a stupid thing, don't worry about it
}

you'd be confident whoever wrote that explicitly ignored the exception, right? Does that mean you could factor out 100% the chance of this having something to do with the bizarre issue you are trying to debug? If not then what's the benefit of being "explicit" here?

sonhanguyen commented 6 years ago

about catch {} being supposedly more explicit than catch (ignored) {}. I'm not so sure that's the case for all humans (they both look pretty cryptic without further comment). To the non-humans aka static analysers if you don't use the exception they're gonna know anyway, I don't see how not typing it helps?

And I wonder how is catch {} less likely a mistake than catch() {} or catch(error) { //... do nothing, as well as who actually ignores exception by mistake?

jcrben commented 6 years ago

@ljharb huh, I also think catch() {} is just as explicit as catch {} - the main difference is that with the latter I'm going to wonder if it's a syntax error, then I'll have to go through and read up on the recent spec change

ljharb commented 6 years ago

That won't be true for new JS devs tho, which will outnumber existing ones in the long run :-)

danny-andrews commented 6 years ago

@ljharb

That won't be true for new JS devs tho

You know what will be true for new JS devs? Having an extra special case to learn when trying to master what should be a straight-forward construct!

danny-andrews commented 6 years ago

@michaelficarra We have a pretty standard cross-language convention for marking an argument as unused/unimportant. It's _. In your example:

function hasCompatibleWebAPI() {
  try {
    // try to exercise the edge cases of the API
    return true;
  } catch(_) {
    return false;
  } finally {
    // clean up any effects that our usage of the API may have had
  }
}

I've made it explicit that I don't care about the error object. No language change necessary! SHUT IT DOWN BOYS! 🔥 🔫 💥 (Seriously, though, let's not add this superfluous feature to an already complex language. Please.)

p.s. You should have the following rule in your .eslintrc.yaml:

no-unused-vars:
  - error
  - argsIgnorePattern: ^_
    caughtErrors: all
ljharb commented 6 years ago

@danny-andrews this makes the construct more straightforward; it's nonsensical to be forced to create a binding when you don't intend to use it.

jcrben commented 6 years ago

@ljharb thanks for fielding the comments, really appreciate your calm tone and kind feedback

@danny-andrews I suppose that if we really want to make our concerns heard, it might be best to direct them to folks on the TC39 - especially community-oriented reps - such as the JS Foundation rep @maggiepint

At the same time, not sure how they might be expected to weight the opinion of a few passers by.

The PHP world has an interesting approach in that their new features are actually voted upon thru democratic RFCs. Not saying that's necessarily a better approach, but it would be interesting to get a statistical sample to sense of what devs outside of the elite world of the TC39 generally think about new developments.

On the bright side, this is consistent with Python's except: which allows for omitting a param. On the other hand, Java and PHP don't seem to have the same feature. I also don't quite agree with http://2ality.com/2017/08/optional-catch-binding.html about always at least logging an error - no need to spew spurious data to the console. With that said, while I've ran into plenty of harmless errors, every time I've entirely silenced an error I've regarded it as a temporary hack to be ultimately prevented (sometimes by fixing upstream code) but maybe I haven't coded enough.

Anyhow, worried about the complexity in JavaScript's future, but on the bright side, hey, job security...

bakkot commented 6 years ago

@jcrben, all of michealficarra, bterlson, ljharb, and myself are on TC39.

jcrben commented 6 years ago

Ah, good to hear. Off-topic, but wish I could find a maintained list along with affiliations... http://tc39wiki.calculist.org/about/people/ is clearly ancient

By the way, this really does open the question from @sonhanguyen about (, concerning) => doSmt(concerning) instead of (unneeded, concerning) => doSmt(concerning) - I have to do that often enough. Tho maybe that takes removing unneeded constructs a bit too far? I dunno.

sonhanguyen commented 6 years ago

it's nonsensical to be forced to create a binding when you don't intend to use it - @ljharb again that's not the point, the point is to avoid doing it and making the use of it ugly. It might come down to the philosophy of the language: do we want to be opinionated and (implicitly or explicitly) promote certain practices (like "use the Error object") or do we want not to and just accommodate for every minor use case?

ljharb commented 6 years ago

@sonhanguyen a mix of the two. it's not TC39's place to tell people how to program; similarly, it's part of our job to ensure that the language encourages good practices. Using an unused variable to swallow errors is not a good practice; but it will be fine to use catch { to swallow errors, since it's intentional. This is fine.

danny-andrews commented 6 years ago

I looked into how other functional languages handle this case where you don't want to use the exception: F#:

try
    failwith "fail"
with
    | Failure msg -> "caught: " + msg
    | MyFSharpError1 msg -> " MyFSharpError1: " + msg
    | :? System.InvalidOperationException as ex -> "unexpected" // ex is unused here. nbd.

OCaml:

exception NotFound of string
let getItem theList = raise (NotFound "oh")
let result =
  try getItem [1; 2; 3]
  with
  | NotFound e -> print_endline "Item not found!" (* e is unused. Whatever, yo. *)

Elm:

import Html exposing (text)

view userInputAge =
  case String.toInt userInputAge of
    Err msg -> text "Not OK!" -- msg is unused

    Ok age ->
      text "OK!"

main = view "34"

I realize these languages are statically typed, so listing the exception variable explicitly comes with the territory, but the fact that you are ignoring it is actually pretty apparent.

ljharb commented 6 years ago

Ruby:

def raise
  raise Error.new
end

foo = raise rescue nil

Which is dynamically typed, and thus perhaps a better exemplar for JS.

danny-andrews commented 6 years ago

Yep, and that indiscriminately swallows errors up, regardless of their type or origin. Wouldn't you agree that this:

let jsonData;
try {
  jsonData = JSON.prase(str);
} catch (err) {
  if (!(err instanceof SyntaxError)) {
    throw err;
  } 
}

is better than this:

let jsonData;
try {
  jsonData = JSON.prase(str);
} catch(e) {}

Note: I left a little surprise in my examples. 😄 In the first example, the TypeError thrown by our typo of JSON.prase, in the second, it will just silently fail.

I realize that requiring a variable binding doesn't force you to actually use it, but it should at least raise your eyebrows when you see a linter error about it, and that counts for something I think.

I also realize it's verbose, but I'd take verbosity over swallowing errors any day! Maybe pattern matching or conditional catch clauses can clean this up in the future, but until then, I think it's better to not add a construct which gives you an excuse to be lazy.

p.s. I think we should be aspiring for higher standard than Ruby (I realize we're talking about JavaScript, but hey let's not make it any worse, right?). It's a fine scripting languages, but doesn't scale worth shit because of it's numerous convenient, seductive escape hatches.

ljharb commented 6 years ago

That's a choice you're welcome to make, but the comparison is actually to:

let jsonData;
try {
  jsonData = JSON.prase(str);
} catch (e) {}

In which case, I think the one that omits the binding is objectively better.

bterlson commented 6 years ago

@danny-andrews I promise there will be a lint rule to flag identifier-less catch, fwiw.

bakkot commented 6 years ago

@danny-andrews

Wouldn't you agree that this [...] is better than this [...]

No.

If I wouldn't put if (x > 1) throw new Error; after every x = Math.random() - as indeed I would not - then I shouldn't check that the error thrown by JSON.parse is a SyntaxError, either. In either case, it's an invariant of the language, and I'm comfortable relying on it. If it's violated, I have more serious problems.

(The thing with the typo really feels like a red herring to me. Plenty of other tools and the most trivial of tests will catch that. I'm not going to ship code whose only purpose is to check that I don't have a typo in a method name; I'm just going to confirm that I don't have a typo.)

danny-andrews commented 6 years ago

I think the one that omits the binding is objectively better.

Better than the one which checks the error type?

ljharb commented 6 years ago

@danny-andrews personally i'd prefer the one that checks the type - but that's not the alternative here. Checking the type is a personal subjective choice, and the language shouldn't (and doesn't currently) force you to do that.

This is a comparison strictly between catch (e) {} and catch {}, nothing more, and the latter is much better there.

danny-andrews commented 6 years ago

The thing with the typo really feels like a red herring to me. Plenty of other tools and the most trivial of tests will catch that

How can a test catch an error being swallowed? You wouldn't assert that a syntax error is thrown when you make a typo. That's silly. I also don't know what "other tools" you are referring to except for maybe Flow, but even then, if you calling a library function which doesn't have Flow definitions, you would have no way to catch the typo at compile-time.

But I'll give you that it was a bad example. But what if, for instance, you did some other possibly-throwing operation in the same try block, like write to a file?