Navigation Menu

Skip to content
This repository has been archived by the owner on Dec 31, 2019. It is now read-only.

Why ? #2

Closed
SiegfriedEhret opened this issue Jul 28, 2017 · 58 comments
Closed

Why ? #2

SiegfriedEhret opened this issue Jul 28, 2017 · 58 comments

Comments

@SiegfriedEhret
Copy link

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
Copy link
Member

ljharb commented Jul 28, 2017

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
Copy link
Author

SiegfriedEhret commented Jul 28, 2017

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
Copy link
Member

ljharb commented Jul 28, 2017

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

@bakkot
Copy link

bakkot commented Jul 28, 2017

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

@michaelficarra
Copy link
Member

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
Copy link
Author

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
Copy link
Member

ljharb commented Jul 31, 2017

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
Copy link

sonhanguyen commented Jul 31, 2017

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
Copy link
Author

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.

@ljharb ljharb mentioned this issue Aug 6, 2017
5 tasks
@danny-andrews
Copy link

danny-andrews commented Aug 22, 2017

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
Copy link
Member

ljharb commented Aug 22, 2017

@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
Copy link
Author

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
Copy link

sonhanguyen commented Aug 23, 2017

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.

  • For example, for point 1 I think someone already commented in the esdiscuss thread that the devtools should do the logging for you if it detects that the error object is not used. It is a good point, but I wonder if people would see that annoying and eventually add if guards to block it anyway.
  • Here is when it comes to point 2, whether you log or not or do anything/nothing at all, should at least have a guard to ensure this is the exception you think it is, the guard can in many cases eliminate the need for comments like trust me this is a trivial exception

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
Copy link
Member

ljharb commented Aug 23, 2017

@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
Copy link

sonhanguyen commented Aug 23, 2017

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

@bterlson
Copy link
Member

bterlson commented Aug 23, 2017

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
Copy link

sonhanguyen commented Aug 25, 2017

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
Copy link

jcrben commented Sep 23, 2017

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
Copy link
Member

ljharb commented Sep 23, 2017

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

@danny-andrews
Copy link

danny-andrews commented Sep 23, 2017

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
Copy link

danny-andrews commented Sep 23, 2017

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
Copy link

danny-andrews commented Sep 23, 2017

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
Copy link
Member

ljharb commented Sep 23, 2017

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
Copy link

bakkot commented Sep 23, 2017

@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
Copy link

danny-andrews commented Sep 24, 2017

@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
Copy link

@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
Copy link

bakkot commented Sep 24, 2017

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

@sonhanguyen
Copy link

sonhanguyen commented Sep 24, 2017

@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
Copy link

sonhanguyen commented Sep 24, 2017

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
Copy link

jcrben commented Sep 24, 2017

@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
Copy link
Member

ljharb commented Sep 24, 2017

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

@danny-andrews
Copy link

danny-andrews commented Sep 24, 2017

@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
Copy link

danny-andrews commented Sep 24, 2017

@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
Copy link
Member

ljharb commented Sep 25, 2017

@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
Copy link

jcrben commented Sep 25, 2017

@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
Copy link

bakkot commented Sep 25, 2017

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

@jcrben
Copy link

jcrben commented Sep 25, 2017

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
Copy link

sonhanguyen commented Sep 26, 2017

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
Copy link
Member

ljharb commented Sep 26, 2017

@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
Copy link

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
Copy link
Member

ljharb commented Sep 26, 2017

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
Copy link

danny-andrews commented Sep 26, 2017

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
Copy link
Member

ljharb commented Sep 26, 2017

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
Copy link
Member

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

@bakkot
Copy link

bakkot commented Sep 26, 2017

@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
Copy link

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

Better than the one which checks the error type?

@ljharb
Copy link
Member

ljharb commented Sep 26, 2017

@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
Copy link

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?

@bakkot
Copy link

bakkot commented Sep 26, 2017

How can a test catch an error being swallowed?

I can test that if I pass valid JSON I get some expected result instead of undefined, which would fail if I had written prase instead of parse.

I also don't know what "other tools" you are referring to except for maybe Flow

Flow and TypeScript would both suffice, yes.

But what if, for instance, you did some other possibly-throwing operation in the same try block, like write to a file?

But I am not doing some other possibly-throwing operation in the same try block.

I'm not trying to claim we should always discard the exception. Heaven forfend! I'm just claiming that there are some cases when a catch block might reasonably not use the error object, and that in those cases the language ought not force you to create a binding for it anyway.

@danny-andrews
Copy link

danny-andrews commented Sep 26, 2017

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

Okay, I think we're arguing past each other. I think we're talking about two different 'better's:

  1. catch {} is better than catch (e) {} because it makes it more clear that you don't want to use the error object.
  2. catch (e) {} is better than catch {} because it adds friction in the form of lint errors/looking "weird" to have an unused variable which can urge someone to use it if for no other reason than to selectively ignore errors based on its type.

It's simply a matter of determining which "better" is more important. I'm arguing the latter, you, the former.

@ljharb
Copy link
Member

ljharb commented Sep 26, 2017

Lint errors can apply equally well to either form; as for "looking weird", that's pretty subjective.

@danny-andrews
Copy link

What if I used the word "unclear." You said before that catch {} is more clear, which implies catch (e) {} is less. (Which I agree with.)

@ljharb
Copy link
Member

ljharb commented Sep 26, 2017

Sure, I think that "with the binding" is less clear as to the intention of the dev than "without the binding".

@danny-andrews
Copy link

danny-andrews commented Sep 26, 2017

I think I'm not as opposed to this proposal as I initially thought. I think the empty-block examples threw me off. Like, if we were comparing:

catch {
  console.log('Something bad happened'); // I'm not using the error object, but at least I'm not ignoring the error entirely
}

to:

catch (e) {
  console.log('Something bad happened');
}

I'd be more likely to accept it. I still like to see that e there to entice you to do something with it (if nothing else, then conditionally rethrow) but catch {} looks simply pernicious.

@danny-andrews
Copy link

danny-andrews commented Sep 26, 2017

In closing: I don't like this proposal because it makes an anti-pattern feel more natural. But it is a pretty small change, so w/e. 🤷‍♂️

@danny-andrews
Copy link

Also, sorry for all the noise, and thanks for the discussion. I hope I haven't been annoying.
Too much free time, unmedicated OCD, etc. :D

@tc39 tc39 locked and limited conversation to collaborators Sep 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants