Option to have no-empty rule flag empty 'catch' blocks

[bedney]

Many times when just prototyping for the first time, folks will use this construct because they're not interested in the error:

try {
... code that code could throw
} catch (e) {
}

But later, for production-ready code, you want to make sure that there is indeed error handling code in those 'catch' blocks.

The current rule doesn't flag 'catch' blocks. I'd like to propose a config option for the 'no-empty' rule that would flag them.

Maybe something like: "no-empty": [2, {"includeCatchBlocks": true}]or some such.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[doberkofler]

Sounds helpful to me

[nzakas]

The one question I have is if this should be a separate rule or not. My concern is that you might want to allow empty catch block in one spot but not in another within the same for. Since you can only disable rules inline, not reconfigure them, having this as an option in no-empty would mean less configurability. Also, could there be other exceptions?

[btmills]

Could requiring a comment like // Intentionally empty in empty blocks suffice for exceptions?

[nzakas]

Ooh, that's a good idea. Maybe nice and short // empty would do it?

[doberkofler]

Will submit a PR on this by today

[doberkofler]

I created a separate rule (no-empty-catch) that allows to flag empty catch blocks with an option if a block only containing a comment is regarded as empty that you rejected because it is not what was agreed upon.
Could you please elaborate on why you dislike the idea of separating this rule from the existing one ?
Although I originally misunderstood your intention, I now feel that separating the rules does make sense or at least has no disadvantages.

[nzakas]

The issue is really about opting-in to no-empty for blocks that are currently ignored by default. @btmills correctly points out that it would make more sense for people to opt-out on a case-by-case basis rather than explicitly ignoring certain blocks and asking people to opt-in.

While I originally thought a separate rule might be in order, @btmills's suggestion made me realize that didn't make sense because we would have two rules doing essentially the same thing.

So it makes the most sense to introduce a breaking change to no-empty where no blocks are ignored by default but you can explicitly opt-out for a specific block by using a comment.

[ljharb]

The documentation for this still claims it allows empty catch blocks. I want empty catch blocks in a number of places, and this change appears to prevent me from keeping this rule turned on, unless I change the contents to just "// empty" - when in fact I want a different comment, or none at all.

Can there please be an option instead of just a breaking change??

[bedney]

@ljharb - I had originally thought the same thing, as per my comment when I opened the issue. On further reflection, though, not doing error handling is a pretty bad code smell. For the few places where I specifically don't want to handle an error, putting in the // empty isn't an onerous burden (if this also precludes having other comments in that empty block, then I would consider that a bug - I haven't tried it).
OTOH, if you have so many empty catch blocks that the burden for putting in // empty is an onerous burden, then I would argue that you need to reconsider your approach.

[bedney]

As a followup, it looks to me like the documentation has been updated for this newer behavior. Where are you seeing the claim that it still allows empty catch blocks? (NB: I'm looking at the online docs for 0.18)

[nzakas]

@ljharb the docs look correct to me, but if you find a problem, please open another issue.

Also, we made this change because we always favor explicit over implicit. We really shouldn't have allowed empty blocks anywhere by default, and using a comment is an established pattern we have in other rules.

If you find the comment check is too specific, please open an issue and describe how you'd like to enhance it.

[ljharb]

Re the docs, I think I just didn't understand that "// empty" was required, but I realized that when reading this thread. They do say "A block will not be considered a warning if it contains a comment line with only the word empty." which does suggest that any other comments wouldn't work - but in fact as long as // empty appears as a line in the block, other comment lines appear to be ignored. Which means I have to do something like this:

try { … }
catch (e) {
  // try the shim if the real one doesn't work
  // empty
}

Having my style linter force code changes on me is very jslint-like, rather than allowing me to configure them (I understand we're in the context of the opt-in rule, but rule configurations exist precisely so that someone isn't forced into not using a rule at all because of small vagaries of them)

I do find a proscribed comment an onerous burden in even one approach - in my opinion any comments in a codebase describe "what" are something to avoid and a code smell. Also, while others may have run into this problem, I've never run into a problem from having empty catch blocks - in shim code especially, it's very common to use them.

[ljharb]

Sorry @nzakas, didn't see your reply before I posted mine. I've filed #2187 and #2188

[nzakas]

Just to address our concerns: I'm sorry you feel like the linter is forcing you to make changes you don't want. We do our best to assure that our rules are rational and not onerous to work with. However, we do mistakes, and we reserve the right to fix those mistakes when they are made.

In this case, it was a mistake to implicitly ignore empty blocks in certain positions but not others. For instance, there was no way to flag empty catch blocks as an error previously. While this is one circumstance where an empty block isn't necessarily a bad thing, having no way to distinguish between wanted and unwanted empty catch blocks was a mistake.

While ESLint has no agenda as it relates to code style, we do believe in having consistent rules that minimize false negatives. For this rule, it was necessary to make a breaking change to do that, and while that may cause you to make changes to your code, it makes the rule completely consistent across the board. In the long term, that has a lot more value than keeping it inconsistent for the sake of backwards compatibility.

Once we reach 1.0.0, we'll be a lot stricter about making such changes, but up until then, we want to keep getting things into a better and more consistent shape.

[bedney]

@nzakas - Excellent explanation. The reason that eslint is, IMHO, representative of the best stewardship of an open source JavaScript project is the time and consideration you continue to put into the entire development model and cycle. Thank you.

[ljharb]

@nzakas I totally agree with all of your policies here, and your position. I think that absolutely the default behavior the rule now evinces is the sanest.

However, I think that in every case, regardless of pre-1.0 or post-1.0 (since 1.0 is not a magic milestone, it's just a number), whenever it makes sense, a breaking change should be done such that the behavior can be re-opted-into via a configuration flag.

[bedney]

@ljharb - I would have to disagree with your statement that 1.0 is not a 'magic number'. To this project (and @nzakas has been very clear on this point) it is: it defines when a breaking change will have to be opted in to rather than opted out of.
Otherwise, what version is 'breaking' relative to? 0.15? 0.10? First public release? First commit @nzakas ever did?
@nzakas has decided that 1.0 is "that" release. In the end, eslint is his project and he is spending a huge amount of time on it, so he (like Guido Van Rossum is for Python, Larry Wall is for Perl) gets to be the "BDFL" (that's Benevolent Dictator For Life :-) ).

[ljharb]

@bedney Some versions of semver say that a minor version bump pre-1.0 is a breaking change, and a major bump post-1.0 is a breaking change - which means that 0.15.0 → 0.16.0 is the same as 1.0.0 → 2.0.0. There's a reason npm init now defaults projects to start at 1.0.0 :-)

That said, of course the owner of a project is always entitled to do whatever they like with it. My opinion remains, however, that when the breaking change is a change of defaults, it keeps the upgrade path clear to allow old behavior to remain simply by changing configuration options, rather than forcing me to change code or abandon a rule in order to continue upgrading.

[nzakas]

Okay guys, good discussion. We have a different of opinion here so it's time to close the discussion.