Custom RuboCops to support code reviews

February 2020

Running RuboCop in CI is a good way to enforce a style guide the team has actually agreed on. Out of the box, and through its extensions, it removes a lot of the mental overhead of code review.

But some patterns keep reappearing that no extension covers, because they are specific to your codebase. Those are the ones worth automating, since they are the ones a human is otherwise re-explaining every few weeks.

Your first custom cop

Make somewhere for them to live:

mkdir cop

Then write one:

# cop/check_equals.rb

module RuboCop
  module Cop
    module Style
      class CheckEquals < Base
        MSG = "Do not use `==`."

        def on_send(node)
          return unless node.method_name == :==
          add_offense(node, severity: :warning)
        end
      end
    end
  end
end

Add a file that offends it:

# offender.rb
2 == 3

And tell RuboCop the cop exists:

# at the top of .rubocop.yml
require:
  - ./cop/check_equals

Then run it:

$ bundle exec rubocop offender.rb
Inspecting 1 file
W

Offenses:

offender.rb:1:1: W: Do not use ==.
2 == 3
^^^^^^

1 file inspected, 1 offense detected

How RuboCop sees your code

RuboCop uses the parser gem to build an Abstract Syntax Tree from your source. The tree represents the structure of the code, which lets RuboCop walk it and react when it recognises a shape.

The gem ships a command-line tool for inspecting what that tree looks like.

A method call with no arguments:

$ ruby-parse -e 'random_object.nil?'
(send
  (send nil :random_object) :nil?)

Method dispatch is a send, followed by the receiver and then the method name. (send nil :random_object) has a nil receiver because nothing explicit receives that call.

With an argument:

$ ruby-parse -e 'random_object.find(1)'
(send
  (send nil :random_object) :find
  (int 1))

The argument follows the method name.

Referencing a constant and calling a method on it:

$ ruby-parse -e 'This::Thing.call'
(send
  (const
    (const nil :This) :Thing) :call)

Constant lookups are a const, followed by the namespace and then the name.

That is where on_send comes from. It fires whenever the traversal reaches a send node, and equivalents exist for the rest - on_const for a constant lookup, and so on. The matching node arrives as the argument:

# Traversing: random_object.nil?
#
# (send
#   (send nil :random_object) :nil?)

def on_send(node)
  node.send_type?  # true
  node.const_type? # false
  node.receiver    # (send nil :random_object)
  node.arguments   # []
end

Picking the tree apart by hand is fine for simple matches and gets confusing quickly. def_node_matcher lets you describe the shape instead, using the Node Pattern DSL:

module RuboCop
  module Cop
    module Style
      class CheckEquals < Base
        MSG = "Do not use `==`."

        def_node_matcher :equals_call?, <<~PATTERN
          (send (...) :== (...))
        PATTERN

        def on_send(node)
          return unless equals_call?(node)
          add_offense(node, severity: :warning)
        end
      end
    end
  end
end

Discouraging a third-party call

Say you want to steer the team away from Net::HTTP.get_response for talking to an API.

Start where you always start, by looking at the tree:

$ ruby-parse -e 'Net::HTTP.get_response(uri)'
(send
  (const
    (const nil :Net) :HTTP) :get_response
  (send nil :uri))

It is a send again, so on_send is the hook. Narrow it down a step at a time - first the method name, then the fact that the receiver is a constant, then which constant:

# cop/check_resilient_api_clients.rb

module RuboCop
  module Cop
    module ExternalServices
      class CheckResilientApiClients < Base
        MSG = "Use a more resilient API client."

        def on_send(node)
          return unless node.method_name == :get_response
          return unless node.receiver&.const_type?
          return unless node.receiver.const_name == "Net::HTTP"
          add_offense(node, severity: :warning)
        end
      end
    end
  end
end

const nodes implement const_name, which is what makes that last check readable. The &. matters: a bare get_response with no receiver would otherwise raise on nil.

# offender.rb
::Net::HTTP.get_response(uri)
Net::HTTP.get_response(uri)

and run it:

$ bundle exec rubocop offender.rb
Inspecting 1 file
W

Offenses:

offender.rb:1:1: W: Use a more resilient API client.
::Net::HTTP.get_response(uri)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
offender.rb:2:1: W: Use a more resilient API client.
Net::HTTP.get_response(uri)
^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 2 offenses detected

There is plenty left to improve:

assigning Net::HTTP to a variable and calling http.get_response(uri) slips straight past it
the message should point at documentation showing what to do instead
it needs tests and documentation of its own, per the RuboCop development guide

And the same rewrite with the Node Pattern DSL:

module RuboCop
  module Cop
    module ExternalServices
      class CheckResilientApiClients < Base
        MSG = "Use a more resilient API client."

        def_node_matcher :net_http_get_response?, <<~PATTERN
          (send (const (const _ :Net) :HTTP) :get_response ...)
        PATTERN

        def on_send(node)
          return unless net_http_get_response?(node)
          add_offense(node, severity: :warning)
        end
      end
    end
  end
end

Why bother

A custom cop turns a recurring review comment into something nobody has to write again. That takes a burden off reviewers and lets them spend their attention on the parts of a change that actually need judgement - which is the only part of review a machine cannot do.

Further reading: the RuboCop development guide, and Writing RuboCop Linters for Database Migrations by Tim Downey.