Add new Rails/IgnoredSkipActionFilterOption cop

Fixes #35

This cop checks that `if` and `only` (or `except`) are not used together
as options of `skip_*` action filter.

The `if` option will be ignored when `if` and `only` are used together.
Similarly, the `except` option will be ignored when `if` and `except`
are used together.

Author: wata727

config/default.yml

@@ -113,6 +113,13 @@ Rails/HttpStatus:
     - numeric
     - symbolic
 
+Rails/IgnoredSkipActionFilterOption:
+  Description: 'Checks that `if` and `only` (or `except`) are not used together as options of `skip_*` action filter.'
+  Reference: 'https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options'
+  Enabled: true
+  Include:
+    - app/controllers/**/*.rb
+
 Rails/InverseOf:
   Include:
     - app/models/**/*.rb

lib/rubocop/cop/rails/ignored_skip_action_filter_option.rb

@@ -0,0 +1,94 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks that `if` and `only` (or `except`) are not used together
+      # as options of `skip_*` action filter.
+      #
+      # The `if` option will be ignored when `if` and `only` are used together.
+      # Similarly, the `except` option will be ignored when `if` and `except`
+      # are used together.
+      #
+      # @example
+      #   # bad
+      #   class MyPageController < ApplicationController
+      #     skip_before_action :login_required,
+      #       only: :show, if: :trusted_origin?
+      #   end
+      #
+      #   # good
+      #   class MyPageController < ApplicationController
+      #     skip_before_action :login_required,
+      #       if: -> { trusted_origin? && action_name == "show" }
+      #   end
+      #
+      # @example
+      #   # bad
+      #   class MyPageController < ApplicationController
+      #     skip_before_action :login_required,
+      #       except: :admin, if: :trusted_origin?
+      #   end
+      #
+      #   # good
+      #   class MyPageController < ApplicationController
+      #     skip_before_action :login_required,
+      #       if: -> { trusted_origin? && action_name != "admin" }
+      #   end
+      #
+      # @see https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options
+      class IgnoredSkipActionFilterOption < Cop
+        MSG = <<-MSG.strip_indent.chomp.freeze
+          `%<ignore>s` option will be ignored when `%<prefer>s` and `%<ignore>s` are used together.
+        MSG
+
+        FILTERS = %w[
+          :skip_after_action
+          :skip_around_action
+          :skip_before_action
+          :skip_action_callback
+        ].freeze
+
+        def_node_matcher :filter_options, <<-PATTERN
+          (send
+            nil?
+            {#{FILTERS.join(' ')}}
+            _
+            $_)
+        PATTERN
+
+        def on_send(node)
+          options = filter_options(node)
+          return unless options
+          return unless options.hash_type?
+
+          options = options_hash(options)
+
+          if if_and_only?(options)
+            add_offense(options[:if],
+                        message: format(MSG, prefer: :only, ignore: :if))
+          elsif if_and_except?(options)
+            add_offense(options[:except],
+                        message: format(MSG, prefer: :if, ignore: :except))
+          end
+        end
+
+        private
+
+        def options_hash(options)
+          options.pairs
+                 .select { |pair| pair.key.sym_type? }
+                 .map { |pair| [pair.key.value, pair] }.to_h
+        end
+
+        def if_and_only?(options)
+          options.key?(:if) && options.key?(:only)
+        end
+
+        def if_and_except?(options)
+          options.key?(:if) && options.key?(:except)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -32,6 +32,7 @@ module Cop
 require_relative 'rails/helper_instance_variable'
 require_relative 'rails/http_positional_arguments'
 require_relative 'rails/http_status'
+require_relative 'rails/ignored_skip_action_filter_option'
 require_relative 'rails/inverse_of'
 require_relative 'rails/lexically_scoped_action_filter'
 require_relative 'rails/link_to_blank'

manual/cops_rails.md

@@ -916,6 +916,58 @@ Name | Default value | Configurable values
 --- | --- | ---
 EnforcedStyle | `symbolic` | `numeric`, `symbolic`
 
+## Rails/IgnoredSkipActionFilterOption
+
+Enabled by default | Supports autocorrection
+--- | ---
+Enabled | No
+
+This cop checks that `if` and `only` (or `except`) are not used together
+as options of `skip_*` action filter.
+
+The `if` option will be ignored when `if` and `only` are used together.
+Similarly, the `except` option will be ignored when `if` and `except`
+are used together.
+
+### Examples
+
+```ruby
+# bad
+class MyPageController < ApplicationController
+  skip_before_action :login_required,
+    only: :show, if: :trusted_origin?
+end
+
+# good
+class MyPageController < ApplicationController
+  skip_before_action :login_required,
+    if: -> { trusted_origin? && action_name == "show" }
+end
+```
+```ruby
+# bad
+class MyPageController < ApplicationController
+  skip_before_action :login_required,
+    except: :admin, if: :trusted_origin?
+end
+
+# good
+class MyPageController < ApplicationController
+  skip_before_action :login_required,
+    if: -> { trusted_origin? && action_name != "admin" }
+end
+```
+
+### Configurable attributes
+
+Name | Default value | Configurable values
+--- | --- | ---
+Include | `app/controllers/**/*.rb` | Array
+
+### References
+
+* [https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options](https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options)
+
 ## Rails/InverseOf
 
 Enabled by default | Supports autocorrection

spec/rubocop/cop/rails/ignored_skip_action_filter_option_spec.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::IgnoredSkipActionFilterOption do
+  subject(:cop) { described_class.new(config) }
+
+  let(:config) { RuboCop::Config.new }
+
+  it 'registers an offense when `if` and `only` are used together' do
+    expect_offense(<<-RUBY.strip_indent)
+      skip_before_action :login_required, only: :show, if: :trusted_origin?
+                                                       ^^^^^^^^^^^^^^^^^^^^ `if` option will be ignored when `only` and `if` are used together.
+    RUBY
+  end
+
+  it 'registers an offense when `if` and `except` are used together' do
+    expect_offense(<<-RUBY.strip_indent)
+      skip_before_action :login_required, except: :admin, if: :trusted_origin?
+                                          ^^^^^^^^^^^^^^ `except` option will be ignored when `if` and `except` are used together.
+    RUBY
+  end
+
+  it 'does not register an offense when `if` is used only' do
+    expect_no_offenses(<<-RUBY.strip_indent)
+      skip_before_action :login_required, if: -> { trusted_origin? && action_name == "show" }
+    RUBY
+  end
+end