Merge pull request #607 from natematykiewicz/duplicate_scope_cop

[Fix #427] Add Rails/DuplicateScope cop

Author: koic

changelog/new_add_railsduplicatescope_cop.md

@@ -0,0 +1 @@
+* [#427](https://github.com/rubocop/rubocop-rails/issues/427): Add `Rails/DuplicateScope` cop. ([@natematykiewicz][])

config/default.yml

@@ -266,6 +266,11 @@ Rails/DuplicateAssociation:
   Enabled: pending
   VersionAdded: '<<next>>'
 
+Rails/DuplicateScope:
+  Description: 'Multiple scopes share this same where clause.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Rails/DurationArithmetic:
   Description: 'Do not use duration as arithmetic operand with `Time.current`.'
   StyleGuide: 'https://rails.rubystyle.guide#duration-arithmetic'

lib/rubocop/cop/mixin/class_send_node_helper.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # A mixin to return all of the class send nodes.
+    module ClassSendNodeHelper
+      def class_send_nodes(class_node)
+        class_def = class_node.body
+
+        return [] unless class_def
+
+        if class_def.send_type?
+          [class_def]
+        else
+          class_def.each_child_node(:send)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails/after_commit_override.rb

@@ -32,6 +32,8 @@ module Rails
       #   after_update_commit :log_update_action
       #
       class AfterCommitOverride < Base
+        include ClassSendNodeHelper
+
         MSG = 'There can only be one `after_*_commit :%<name>s` hook defined for a model.'
 
         AFTER_COMMIT_CALLBACKS = %i[
@@ -63,18 +65,6 @@ def each_after_commit_callback(class_node)
           end
         end
 
-        def class_send_nodes(class_node)
-          class_def = class_node.body
-
-          return [] unless class_def
-
-          if class_def.send_type?
-            [class_def]
-          else
-            class_def.each_child_node(:send).to_a
-          end
-        end
-
         def after_commit_callback?(node)
           AFTER_COMMIT_CALLBACKS.include?(node.method_name)
         end

lib/rubocop/cop/rails/duplicate_association.rb

@@ -23,6 +23,7 @@ module Rails
       class DuplicateAssociation < Base
         include RangeHelp
         extend AutoCorrector
+        include ClassSendNodeHelper
 
         MSG = "Association `%<name>s` is defined multiple times. Don't repeat associations."
 
@@ -49,18 +50,6 @@ def offenses(class_node)
                                       .group_by { |node| association(node).to_sym }
                                       .select { |_, nodes| nodes.length > 1 }
         end
-
-        def class_send_nodes(class_node)
-          class_def = class_node.body
-
-          return [] unless class_def
-
-          if class_def.send_type?
-            [class_def]
-          else
-            class_def.each_child_node(:send)
-          end
-        end
       end
     end
   end

lib/rubocop/cop/rails/duplicate_scope.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks for multiple scopes in a model that have the same `where` clause. This
+      # often means you copy/pasted a scope, updated the name, and forgot to change the condition.
+      #
+      # @example
+      #
+      #   # bad
+      #   scope :visible, -> { where(visible: true) }
+      #   scope :hidden, -> { where(visible: true) }
+      #
+      #   # good
+      #   scope :visible, -> { where(visible: true) }
+      #   scope :hidden, -> { where(visible: false) }
+      #
+      class DuplicateScope < Base
+        include ClassSendNodeHelper
+
+        MSG = 'Multiple scopes share this same where clause.'
+
+        def_node_matcher :scope, <<~PATTERN
+          (send nil? :scope _ $...)
+        PATTERN
+
+        def on_class(class_node)
+          offenses(class_node).each do |node|
+            add_offense(node)
+          end
+        end
+
+        private
+
+        def offenses(class_node)
+          class_send_nodes(class_node).select { |node| scope(node) }
+                                      .group_by { |node| scope(node) }
+                                      .select { |_, nodes| nodes.length > 1 }
+                                      .values
+                                      .flatten
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -2,6 +2,7 @@
 
 require_relative 'mixin/active_record_helper'
 require_relative 'mixin/active_record_migrations_helper'
+require_relative 'mixin/class_send_node_helper'
 require_relative 'mixin/enforce_superclass'
 require_relative 'mixin/index_method'
 require_relative 'mixin/migrations_helper'
@@ -33,6 +34,7 @@
 require_relative 'rails/delegate'
 require_relative 'rails/delegate_allow_blank'
 require_relative 'rails/duplicate_association'
+require_relative 'rails/duplicate_scope'
 require_relative 'rails/duration_arithmetic'
 require_relative 'rails/dynamic_find_by'
 require_relative 'rails/eager_evaluation_log_message'

spec/rubocop/cop/rails/duplicate_scope_spec.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::DuplicateScope, :config do
+  it 'registers an offense when a duplicate scope is detected' do
+    expect_offense(<<~RUBY)
+      class Post < ApplicationRecord
+        scope :visible, -> { where(visible: true) }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Multiple scopes share this same where clause.
+        scope :hidden, -> { where(visible: true) }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Multiple scopes share this same where clause.
+        scope :new, -> { where(created_at: 1.week.ago..Date.current) }
+        scope :popular, -> { where(comments_count: 1000..Float::INFINITY) }
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when there are no duplicates' do
+    expect_no_offenses(<<~RUBY)
+      class Post < ApplicationRecord
+        scope :visible, -> { where(visible: true) }
+        scope :hidden, -> { where(visible: false) }
+        scope :new, -> { where(created_at: 1.week.ago..Date.current) }
+        scope :popular, -> { where(comments_count: 1000..Float::INFINITY) }
+      end
+    RUBY
+  end
+end