lint: Add linter rule to ban uncommented empty catch blocks

[plibither8]

trafficstars

Closes https://github.com/sourcegraph/sourcegraph/issues/26581

This rule bans catch blocks that have an empty body, unless the block is supported by a helpful comment explaining why this exception can be safely ignore and not processed.

Bad code

try {
  // do something
} catch (error) {}

Good code

try {
  // do something
} catch (error) {
  // This error can be ignored because XYZ.
}

(Bonus) Awesome code

try {
  // do something
} catch (error) {
  Sentry.captureException(error) // ...or something like this
}

[plibither8]

@valerybugakov my bad, completely misinterpreted the issue! Thanks for the clarification :)

Maybe enforcing a structured comment that includes these answers would be a way to go? (thinking out loud)

I like this idea! A structured comment in the following form could be a possible way to go about it, I just hope the approach doesn't feel aggressive.

promise.catch(() => {
  /** Error ignored: [reason] */
})

We can consider modifying the rule I just wrote to have check for the comment body having such a pattern.

We can adapt it to promise.catch() calls because it seems we don't have any machinery for this part of the problem.

Will definitely modify the current rule to adapt it for .catch() methods on promises instead. I think there are a few variations we might need to consider here. Here's a few examples I found in our source:

1. promise.catch(() => {})
2. promise.catch(noop)
3. promise.catch(() => null)
4. promise.catch(() => { /*noop*/ })
5. promise.catch(() => { /*ignore*/ })

There was only one instance of No. 3 I could find, so it might not be worth it to check for that (and that one instance can be manually refactored). For the "noop" and "ignore" comments, we can (smartly) check against the presence of these words in the comment, if at all present.

Thoughts and feedback requested! 😄

[umpox]

Agree with @valerybugakov, a single API to report the error and do anything else we want with it would be pretty useful, and being able to enforce it here would probably catch a lot of issues we didn't even know about before 👍

Would we want to completely ban console.error in this case? I could see scenarios where it might be confusing and we could end up with duplicate logs