Eslint: Proposal: no-optional-catch-binding rule

[cjihrig]

Please describe what the rule should do:

Prevent use of the new optional catch binding syntax.

What category of rule is this? (place an "X" next to just one item)

[X] Enforces code style
[ ] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

try {
  throw new Error('error that should not be ignored');
} catch {
  console.log('something happened');
}

Why should this rule be included in ESLint (instead of a plugin)? It seems like it would be applicable across all environments that support the new syntax. Generally, errors should not be ignored. If an error is ignored, I'd prefer an explicit opt eslint-disable opt out, rather than not knowing if it was ignored accidentally or lazily.

I'm volunteering to do the work. I have a proof of concept already.

[mysticatea]

Thank you for this proposal.

I like this rule.

We cannot accept this proposal for right now because we don't add rules for Stage 3 syntax to core in our policy (and ESTree spec has not exist yet). But I hope to add this rule to core in the future.

[cjihrig]

I was hesitant to open this issue because it's still Stage 3, but figured it might be worth a shot now that the syntax is in Node 10. I'm happy to wait until it hits Stage 4. Thanks for the feedback.

[platinumazure]

With optional catch bindings now in Stage 4, I think we could re-evaluate.

[ljharb]

Why should this be a rule?? It's perfectly fine to ignore errors in many cases, and choosing to omit the catch binding is explicit developer intent to ignore it.

For all the times eslint core members push back on adding reasonable rules with "just use no-restricted-syntax!", it seems like this is a PRIME candidate for "just use no-restricted-syntax".

[mysticatea]

We can develop this after #10392 which adds the support of optional catch binding is merged.

I will champion this.

• Generally, errors should not be ignored in order to investigate bugs fastly. Ignoring errors would help to hide the cause of bugs.
• The no-restricted-syntax rule with CatchClause[param=null] can catch this. However, it's inconvenient if sharable config is using the no-restricted-syntax rule. It overrides and mess the configuration of shareble configs. Independent rule is useful.

[ljharb]

@mysticatea if "it's inconvenient if sharable config is using the no-restricted-syntax rule" is going to apply here, then I'd respectfully request that never again is "use no-restricted-syntax" offered as a reason to reject a rule (which would be great, because tons of useful rules have been shot down because of that strawman).

[mysticatea]

@ljharb I mentioned the reason of inconvenient.

[ljharb]

@mysticatea right, i 1000% agree that it's inconvenient :-) but that reasoning needs to either apply to every rule request, or none of them, otherwise the criteria is inconsistent.

[not-an-aardvark]

I agree with @ljharb, this rule seems like a stylistic preference to me. I think the argument of "errors generally should not be ignored" would have been more compelling if it was made on the TC39 proposal before the proposal reached stage 4. This isn't a case where the language has a legacy feature that needs to remain for compatibility reasons -- it's a new feature which (at least according to TC39) presents a compelling addition to the language. Given that, I'm unconvinced that we should be encouraging people not to use it already.

[ljharb]

It was made, and TC39 dismissed it - because ignoring errors is perfectly legitimate.

[mysticatea]

At least, I think that it's very useful syntax in order to overlook the cause of bugs in my 2 cents.

[mysticatea]

I'm sorry if my comment is offensive. But I think it's enough general matter for core rules.

[ljharb]

let foo; try { foo = JSON.parse(data); } catch {} is 100% legitimate.

[mysticatea]

@ljharb It works well, but it abandons the information how it fails parsing, where the syntax error exists.

[ljharb]

There are many cases where that information is not actionable, and not useful (and to the developer, visible in the Network tab if they choose to debug it in a browser).

There's also no rule that prohibits argumentless .catch callbacks, because that's equally legitimate. Separately, no-unused-vars has special handling for unused catch bindings, for this very purpose.

[not-an-aardvark]

To me, it seems like the central issue isn't whether there exists a use case for ignoring errors -- it's whether the ability to omit the binding for an early error makes it more likely for someone to accidentally ignore the error when they should have handled it. TC39 appears to believe that this is not a concern. If there is consensus among the ESLint team that this is a significant-enough problem that we think it's worth discouraging the use of the new syntax for that reason, then I don't object to adding rule for it, but it seems unfortunate to me that we would effectively be declaring a language feature DOA. (My personal opinion is that the language would be better without this feature, but I know there are a lot of reasonable people that disagree with me.)

[mysticatea]

@ljharb You look to have many assumptions before ignoring errors. I think better if the assumption is written in the code in order to maintain the code. So I'd prefer opt-out to ignore errors. It's mentioned in this first post:

It seems like it would be applicable across all environments that support the new syntax. Generally, errors should not be ignored. If an error is ignored, I'd prefer an explicit opt eslint-disable opt out, rather than not knowing if it was ignored accidentally or lazily.

ESLint is a static analyzer, so it cannot catch Promise#catch instance methods in 100% without type annotations. The no-unused-vars rule reports catch bindings by default.

[mysticatea]

I proposed prefer-optional-catch-binding (#10434) rule which recommends using optional catch binding if the error is ignored.

We can enforce the following style with the combination of those rules:

• Basically, don't ignore errors.
• But use optional catch binding if ignoring errors is reasonable, by explicitly opt-out.

[mysticatea]

One important thing, ESLint will lose the ability to enforce the style "Basically, don't ignore errors" by the new syntax. Now we can enforce to use catch bindings by the no-unused-vars rule. However, the no-unused-vars rule overlooks the optional catch binding syntax because the syntax doesn't define any variable. This rule is needed to keep the ability.

[ljharb]

The point of no-unused-vars isn’t to ensure looking at errors, it’s to ensure no var goes unused.

} catch (e) { e; }

This pattern reports as a used variable (I’m aware it would error on other rules), and it doesn’t ensure you’re actually looking at the error.

In other words, i think the ability you’re worried about losing isn’t actually provided by no-unused-vars.

[not-an-aardvark]

There are a lot of ways to get around linting rules. A rule generally provides value by alerting developers about something they might have forgotten, not by covering every possible way that a dedicated user might try to get around it. In this case, I think no-unused-vars can still be useful for enforcing that errors aren't ignored, even if it doesn't guarantee that every error is handled in the way that one might intend.

[geek]

I'd prefer to never have an error ignored unless it was very explicit with a disable no-option-catch comment. A world that blatantly ignores errors without clear reasoning is really terrifying.

[ljharb]

The use of an optional catch binding is a very explicit indicator that one doesn't care what the error is, only that there was an error - which is, i'll add, not at all "ignoring errors". It's a catch block - it's impossible to ignore the error; only to ignore the specific value that was thrown.

[mysticatea]

@ljharb "ignore" means "abandoning the error information". There are people that want to disallow
"one doesn't care what the error is" basically.

[mysticatea]

Because, the thrown error can be an unintentional error. For example, let foo; try { foo = JSON.parse(e.data); } catch {}, one intended to ignore parsing error, but actually it hided a TypeError if e is null. The try syntax has very wide effect.

[platinumazure]

Maybe it would be best to make a rule that could go either way, so people can use it how they like? I don't think optional catch bindings are so wrong that we should have an opinionated rule which only bans then. I like the idea of preferring either no catch binding (if the binding is unused) or requiring a binding in all cases.

[mysticatea]

@platinumazure Do you mean to merge this and #10434? Actually, I'd like to enable both in my case. I want to report "ignoring errors" basically, but I want to remove catch binding if I explicitly opt-outed the error with //eslint-disable-line no-optional-catch-binding comment for some reason.

[ljharb]

It seems like they should maybe just be the same rule.

[cjihrig]

I think it's a reasonable compromise to support both cases. I can update my code if there is agreement on what it should look like. How is this:

name: prefer-optional-catch-binding
values: 'always', 'never'

When set to 'never', optional catch bindings would be forbidden, as described above. When set to 'always', it would enforce the usage of optional catch bindings and could be autofixed as described in #10434.

[not-an-aardvark]

To me, it still seems like the always behavior is redundant with no-unused-vars. I'm not sure it makes sense to add a new rule for that behavior just to support autofixing.

It's also not clear to me that a user would expect an autofixer to remove their catch binding. Often, when a variable is unused, the problem is that the user forgot to use it, not that they mistakenly included the declaration. (This is why we don't have an autofix for no-unused-vars.)

As a sidenote, if we do add a rule I think we should avoid names like no-optional-catch-binding and instead use names along the lines of no-unused-catch-binding or prefer-catch-binding. I think the term "optional" made sense as a name for the language proposal since contrasted with the previous state of the language (where catch bindings were required). Since catch bindings are always optional now, it seems redundant and potentially confusing to call them "optional catch bindings" in a rule name.

[ljharb]

It’s not redundant because no-unused-vars can’t autofix to optional catch, and because it’s purpose isn’t the same as this one, which deals specifically with catch clauses.

I don’t want to autofix unused vars, but i absolutely want to autofix catch clauses that could omit the binding.

[mysticatea]

As a sidenote, if we do add a rule I think we should avoid names like no-optional-catch-binding and instead use names along the lines of no-unused-catch-binding or prefer-catch-binding. I think the term "optional" made sense as a name for the language proposal since contrasted with the previous state of the language (where catch bindings were required). Since catch bindings are always optional now, it seems redundant and potentially confusing to call them "optional catch bindings" in a rule name.

I agree. The purpose of this rule is to require to use of catch bindings or explicitly opt-out, so I think require-catch-binding is a better name. // eslint-disable-line require-catch-binding will be an understandable note for opt-out. If it's // eslint-disable-line prefer-optional-catch-binding then I feel odd.

---

To support the opposite side, I think better if it's a separated rule. Maybe prefer-no-unused-catch-binding or something like. The rule will overlap with no-unused-vars rule, but I think nice if core rules include the rules which encourage to learn new syntax.

[platinumazure]

@eslint/eslint-team Where are we at on this?

Do we need a new rule here (at least, for right now), when no-restricted-syntax could theoretically be used by people who want to disallow catch blocks with no bindings? Wondering if this is something for which we should wait for feedback before doing anything in core rules.

[mysticatea]

@platinumazure My opinion about no-restricted-syntax is here: https://github.com/eslint/eslint/issues/10264#issuecomment-394037014. Plus, I think valuable if there is the page which explains try-catch syntax catches all exceptions (it can include unintentional exceptions) as a rule.

[michaelficarra]

A new rule will make it seem like this is a reasonable thing to do, and I strongly believe that a rule like this should never be enabled. :-1: Do it with no-restricted-syntax if you must.

[mysticatea]

And I'd like to add one more reason; The purpose of this rule is to disallow disposal of error information basically and allow it with explicitly opt-out. The no-restricted-syntax rule is not proper for the explicitly opt-out.

[ljharb]

is not proper for the explicitly opt-out.

Restating my comment above: I agree with this in general, which is why I think "you can just use no-restricted-syntax" is and should be an invalid reason not to accept a rule.

A new rule will make it seem like this is a reasonable thing to do,

However, I also agree with this, and think this shouldn't be a rule, because the whole point of this language feature is when you want error information disposed.

to disallow disposal of error information

If this is the rule you want, then you'd have to block other patterns too - including a catch with a binding that's unused; any try/finally that throws in the finally (thus disregarding any previously thrown exception); any unreturned/unpassed promise without a .catch; any promise with a .finally that throws or rejects in the finally (thus disregarding any previous rejection)… Otherwise, you're not actually achieving your goal.

[not-an-aardvark]

Re: "If this is the rule you want, then you'd have to block other patterns too", most of these patterns are already possible to block.

a catch with a binding that's unused

This can be done with no-unused-vars.

any try/finally that throws in the finally

This can be done with no-unsafe-finally.

any promise with a .finally that throws or rejects in the finally (thus disregarding any previous rejection)

I don't think there's a built-in rule for this, although it's inherently a bit more difficult to detect statically since it's not a syntax construct.

[ljharb]

Sure. But if "disallow error disposal" needs a standalone rule, to be more explicit and clear (which in general makes sense to me) - then shouldn't all those things be blocked by this new rule?

[cjihrig]

One small note - I had proposed changing this to also incorporate #10434, if that makes any difference.

[mysticatea]

@ljharb Looks reasonable proposition. Yes, "disallow error disposal" rule is nice. However, currently, I think that each core rule is smaller than it. For example, if I want semicolon-less style, I need the following rules:

{
    "semi": ["error", "never", {"beforeStatementContinuationChars": "never"}],
    "semi-spacing": ["error", {"after": true, "before": false}],
    "semi-style": ["error", "first"],
    "no-extra-semi": "error",
    "no-unexpected-multiline": "error"
}

In the try-catch syntax case, I have used the combination of no-unused-vars, no-unused-expression, and some rules in order to disallow error disposal. Then, I want to add the require-catch-binding rule to the rule set.

The combination of small rules lets me get the goal. Each rule might be too small, but I think it's another issue that consideration of the proper rule size.

---

Currently, I'm considering the following rule:

{
    "require-catch-binding": ["error", "always" | "never"]
}

Options:

• "always" ... enforce writing catch binding.
• "never" ... enforce omitting catch binding if it's unused. This supports autofix.

[Mouvedia]

If it's not added to recommended I am all for it.

[binury]

The Optional catch binding proposal has been moved to stage 4

[kaicataldo]

This proposal has been open for quite a long time now. @mysticatea Are you still championing this? There's been a lot of discussion, and it would be great to have a clear proposal that the team can vote on.

[silverwind]

I'm currently "emulating" a requirement for optional catch binding via the caughtErrors option. I guess it could be a viable alternative, but a explicit rule for it might be even better.

[kaicataldo]

Since the creation of this proposal, we have started recommending using no-restricted-syntax for anything that it can reliably catch - especially when it's a simple rule like this. As a result, I'm now of the mind that we should defer to no-restricted-syntax to enforce this.

For anyone wanting to enforce this, you can use the following the configuration:

/* eslint no-restricted-syntax: ["error", {
    "selector": "CatchClause[param=null]",
    "message": "Optional catch bindings are not allowed."
}] */

try {
  throw new Error('error that should not be ignored');
} catch {
  console.log('something happened');
}

You can see it in action here.

[silverwind]

@kaicataldo I guess most users would actually want to require the optional catch binding, is that possible via no-restricted-syntax too?

[kaicataldo]

That's a good point, and yes!

/* eslint no-restricted-syntax: ["error", {
    "selector": "CatchClause[param!=null]",
    "message": "Optional catch bindings are required."
}] */

try {
  throw new Error('error that should be ignored');
} catch (e) {
  console.log('something happened');
}

Demo example here.

[silverwind]

I don't think it's really useful that way when one wants to use the error. That said, I'm happy with the caughtErrors option of no-unused-vars personally. Maybe it should just be made default in a future version of ESLint.

[kaicataldo]

@kaicataldo I guess most users would actually want to require the optional catch binding, is that possible via no-restricted-syntax too?

Did I misunderstand this comment? I thought you were asking if you could do the above.

[silverwind]

Well, you answered the exact question, but I guess I should have been more clear about the "optional" part, otherwise the rule would not really be practical.

I think I'm now currently leaning to not implementing a new rule because:

1. catch() is invalid syntax which makes no-unused-vars usable for requiring optional bindings
2. no-restricted-syntax or this plugin can be used for forbidding optional bindings

[kaicataldo]

Just to be clear, catch() {} is a parser error, and CatchClause[param!=null] represents catch {}.

[nzakas]

As already discussed, and as announced, we are no longer adding new core rules that simply disallow syntax. It's possible to use no-restricted-syntax, or to create your own rule to focus on just blocking this pattern.

Thanks for the discussion, everyone.