Custom RuboCops to support code-reviews

Running RuboCop in your CI is a great way to enforce a commonly agreed upon style-guide and best-practices.

Out of the box and via its many available extensions, it can greatly reduce the mental overhead of code-reviews.

But sometimes certain patterns or worst-practices keep re-appearing which are not part of an existing extension or highly specific to your codebase.

Let’s fix that by creating our own custom RuboCop.

Your first custom cop

Let’s add a folder for our custom cops in your project-root.

> mkdir cop

Next, we’ll create our first custom cop:

# cop/check_equals.rb

module RuboCop
  module Cop
    module Style
      class CheckEquals < Cop
        MSG = 'Do not use `==`.'.freeze

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

Now let’s add a file to our project that is offending our cop:

# offender.rb
2 == 3

And set up the project’s RuboCop-config to use our new cop.

# Add to the beginning of your .rubocop.yml
require:
  - ./cop/check_nil
# ... content of your .rubocop.yml

Now execute RuboCop against our offending file

bundle exec rubocop offender.rb

and see the error message that is defined in the cop.

Inspecting 1 file
W

Offenses:

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

1 file inspected, 1 offense detected

How RuboCop works

RuboCop uses the parser gem to create an Abstract Syntax Tree (AST) from a piece of ruby code.

The AST is a data structure that represents the structure of the code it is created from and allows RuboCop to traverse that tree and trigger behavior when coming across a pattern that it recognizes.

The parser gem comes with a command-line tool that allows us to inspect the AST that is built from different pieces of code.

Method invocation without any arguments:

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

As you can see the method dispatch in the AST is represented by the send-symbol, followed by the receiver of the method call and then the name of the method.

(send nil :random_object) has the receiver nil, since there is no explicit receiver for the call to random_object

Method invocation with an argument:

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

The argument just follows the name of the method to be invoked.

Referencing a constant and invoking a method on that constant:

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

Constant lookups are represented by the const-symbol, followed by the parent namespace/constant to be looked up and then the name of the constant.

Inside our custom Cop, we can see an on_send callback.

This callback is invoked during the traversal of the AST when RuboCop comes across a send-symbol.

These callbacks exist for all kinds of symbols (i.e. on_const for a constant-lookup)

The matching piece of the AST is passed as the first argument into the callback to allow further inspection.

If we look back at our custom Cop, we can inspect the passed in node:

# When traversing the following piece of code:
# random_object.nil?'
#
# AST:
#
# (send
#  (send nil :random_object) :nil?)
#
#
def on_send(node)
  node.symbol_type? # true for our send-node
  node.const_type?  # false for our send-node
  node.receiver     # returns (send nil :random_object)
  node.arguments    # [] since there are none
end

In our custom RuboCop we are checking for the == method being called. If that is not the case we are adding a warning to the output of RuboCop.

Manually traversing and disecting the AST this way is fine for simple matches, but gets very confusing for more involving patterns.

The def_node_matcher macro allows the definition of a custom matcher function using the Node Pattern-DSL.

We can rewrite our simple custom Cop from above like this:

module RuboCop
  module Cop
    module Style
      class CheckEquals < Cop
        MSG = 'Do not use `==`.'.freeze

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

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

Discourage use of third-party library features

Let’s say we wish to discourage our team from using Net::HTTP.get_response to reach out to an API.

Create the file for your new cop and add it to our .rubocop.yml:

> touch cop/check_resilient_api_clients.rb

# Add to the beginning of your .rubocop.yml
require:
  - ./cop/check_resilient_api_clients
# ... content of your .rubocop.yml

We would start by looking at the AST of the code in question

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

Since our AST starts off with a send-symbol again, we should use the on_send-hook.

Let’s create our cop:

# cop/check_resilient_api_clients.rb

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

        def on_send(node)
          add_offense(node, severity: :warning)
        end
      end
    end
  end
end

Now next we can narrow it down by method invocations for get_response:

# cop/check_resilient_api_clients.rb
# ...
        def on_send(node)
          return unless node.method_name == :get_response
          add_offense(node, severity: :warning)
        end
# ...

In the AST we can see that our method receiver is a const-type of course, so let’s narrow that down even further:

# cop/check_resilient_api_clients.rb
# ...
        def on_send(node)
          return unless node.method_name == :get_response
          return unless node.receiver.const_type?
          add_offense(node, severity: :warning)
        end
# ...

Finally, we want to make sure the receiver is exactly the constant Net::HTTP. Fortunately, const-symbols in the AST implement the const_name-method to give us the name of the constant we are dealing with:

# cop/check_resilient_api_clients.rb
# ...
        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
# ...

Let’s setup an offending piece of code:

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

And run RuboCop against it:

> bundle exec rubocop offender.rb
Inspecting 1 file
W

Offenses:

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

1 file inspected, 2 offenses detected

Now there is still lots to improve on this Cop:

• Assigning Net::HTTP to a variable http and using it http.get_response(uri) would not be matched.
• The error message should point us to some documentation on how to improve this piece of code
• We should actually implement tests for it and add documentation inside the cop (Check out the RuboCop Docs)

And finally we could refactor the Cop to use the Node Pattern DSL:

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

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

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

Summary

Custom RuboCops are a great way to automate certain aspects of your code-reviews and protect your codebase from possible bad-practices and your team from developing bad habits. They also take a burden off of reviewers and let them narrow their focus in code-review on the important issues.

Further reading:

RuboCop Documentation on how to create your own extension Writing RuboCop Linters for Database Migrations by Tim Downey