Skip to content

Rubocop to ban catch/throw in ruby code

Nick Thomas requested to merge (removed):ban-catch-throw into master

Description of the proposal

Ruby has catch/throw semantics in addition to raise/rescue semantics. In general, we should use raise / rescue for exceptional conditions. This leaves throw/catch in a slightly peculiar state - its main use is as flow control, but I've always viewed using throw/catch as flow control to be about equivalent to goto / longjmp, and something to avoid.

This WIP MR came about from reviewing code in !25024 (diffs) , which demonstrates the kind of use we're talking about here. We also see it in this antecedant MR: !21929 (diffs)

The justification for their use in those two MRs is to implement a "parse, don't validate" idiom, but I'm struggling to see why catch/throw is needed for this. We could do it with return or raise too.

The proposal here is to forbid catch/throw statements in our codebase, since they're used for flow control and make understanding flow control much harder than it needs to be. I'm interested in opinions - we can just 👎 or 👍 :)

In our codebase, we have ~65 catches and ~30 throws, but there seems to be a recent desire to add more.

Some gems, such Rack, use throw as an API detail. These can be individually whitelisted where required, but even in these cases, I think we're better off avoiding than using it.

Some resources:

Some notable quotes from the above:

When should you use this language feature? Not very often [...] There are two plausible examples in the Pickaxe book. Fundamentally, throw-catch is a way to escape from the bowels of multiple layers of control flow without abusing raise-rescue for this purpose. But if you have so many layers of flow control that you’re tempted to throw in order to get out of them quickly, consider trying to eliminate the layers first.

and

Languages with SEH should have a compiler option of not recognizing throw so it cannot be used. Barring that, development leads should forbid its use.

Check-list

  • Make sure this MR enables a static analysis check rule for new usage but ignores current offenses
  • Create a follow-up issue to fix the current offenses as a separate iteration: #209088
  • Mention this proposal in the relevant Slack channels (e.g. #development, #backend, #frontend)
  • The MR doesn't have significant objections, and is getting a majority of 👍 vs 👎 (remember that we don't need to reach a consensus)
  • Follow the review process as usual
  • Once approved and merged by a maintainer, mention it again:
    • In the relevant Slack channels (e.g. #development, #backend, #frontend)
    • (Optional depending on the impact of the change) In the Engineering Week in Review

/cc @gitlab-org/maintainers/rails-backend

Edited by Nick Thomas

Merge request reports