Merge pull request #1499 from fatkodima/fix-false-negative-for-env_local

Fix a false negative for `Rails/EnvLocal` when having preceding conditions

Author: koic

changelog/fix_false_negative_for_env_local.md

@@ -0,0 +1 @@
+* [#1478](https://github.com/rubocop/rubocop-rails/issues/1478): Fix a false negative for `Rails/EnvLocal` when having preceding conditions. ([@fatkodima][])

lib/rubocop/cop/rails/env_local.rb

@@ -24,45 +24,69 @@ class EnvLocal < Base
 
         minimum_target_rails_version 7.1
 
-        # @!method rails_env_local_or?(node)
-        def_node_matcher :rails_env_local_or?, <<~PATTERN
-          (or
-            (send (send (const {cbase nil? } :Rails) :env) $%LOCAL_ENVIRONMENTS)
-            (send (send (const {cbase nil? } :Rails) :env) $%LOCAL_ENVIRONMENTS)
-          )
+        # @!method rails_env_local?(node)
+        def_node_matcher :rails_env_local?, <<~PATTERN
+          (send (send (const {cbase nil? } :Rails) :env) $%LOCAL_ENVIRONMENTS)
         PATTERN
 
-        # @!method rails_env_local_and?(node)
-        def_node_matcher :rails_env_local_and?, <<~PATTERN
-          (and
-            (send
-              (send (send (const {cbase nil? } :Rails) :env) $%LOCAL_ENVIRONMENTS)
-              :!)
-            (send
-              (send (send (const {cbase nil? } :Rails) :env) $%LOCAL_ENVIRONMENTS)
-              :!)
-          )
+        # @!method not_rails_env_local?(node)
+        def_node_matcher :not_rails_env_local?, <<~PATTERN
+          (send #rails_env_local? :!)
         PATTERN
 
         def on_or(node)
-          rails_env_local_or?(node) do |*environments|
-            next unless environments.to_set == LOCAL_ENVIRONMENTS
+          lhs, rhs = *node.children
+          return unless rails_env_local?(rhs)
 
-            add_offense(node) do |corrector|
-              corrector.replace(node, 'Rails.env.local?')
-            end
+          nodes = [rhs]
+
+          if rails_env_local?(lhs)
+            nodes << lhs
+          elsif lhs.or_type? && rails_env_local?(lhs.rhs)
+            nodes << lhs.rhs
+          end
+
+          return unless environments(nodes).to_set == LOCAL_ENVIRONMENTS
+
+          range = offense_range(nodes)
+          add_offense(range) do |corrector|
+            corrector.replace(range, 'Rails.env.local?')
           end
         end
 
         def on_and(node)
-          rails_env_local_and?(node) do |*environments|
-            next unless environments.to_set == LOCAL_ENVIRONMENTS
+          lhs, rhs = *node.children
+          return unless not_rails_env_local?(rhs)
+
+          nodes = [rhs]
+
+          if not_rails_env_local?(lhs)
+            nodes << lhs
+          elsif lhs.operator_keyword? && not_rails_env_local?(lhs.rhs)
+            nodes << lhs.rhs
+          end
 
-            add_offense(node, message: MSG_NEGATED) do |corrector|
-              corrector.replace(node, '!Rails.env.local?')
-            end
+          return unless environments(nodes).to_set == LOCAL_ENVIRONMENTS
+
+          range = offense_range(nodes)
+          add_offense(range, message: MSG_NEGATED) do |corrector|
+            corrector.replace(range, '!Rails.env.local?')
           end
         end
+
+        private
+
+        def environments(nodes)
+          if nodes[0].method?(:!)
+            nodes.map { |node| node.receiver.method_name }
+          else
+            nodes.map(&:method_name)
+          end
+        end
+
+        def offense_range(nodes)
+          nodes[1].source_range.begin.join(nodes[0].source_range.end)
+        end
       end
     end
   end

spec/rubocop/cop/rails/env_local_spec.rb

@@ -43,6 +43,20 @@
       RUBY
     end
 
+    it 'registers an offense for `Rails.env.development? || Rails.env.test?` || foo?' do
+      expect_offense(<<~RUBY)
+        Rails.env.development? || Rails.env.test? || foo?
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Rails.env.local?` instead.
+        foo? || Rails.env.test? || Rails.env.development?
+                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Rails.env.local?` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        Rails.env.local? || foo?
+        foo? || Rails.env.local?
+      RUBY
+    end
+
     it 'registers an offense for `!Rails.env.development? && !Rails.env.test?`' do
       expect_offense(<<~RUBY)
         !Rails.env.development? && !Rails.env.test?
@@ -57,12 +71,46 @@
       RUBY
     end
 
+    it 'registers an offense for `!Rails.env.development? && !Rails.env.test?` || foo?' do
+      expect_offense(<<~RUBY)
+        !Rails.env.development? && !Rails.env.test? || foo?
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `!Rails.env.local?` instead.
+        foo? || !Rails.env.test? && !Rails.env.development?
+                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `!Rails.env.local?` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        !Rails.env.local? || foo?
+        foo? || !Rails.env.local?
+      RUBY
+    end
+
+    it 'registers an offense for `!Rails.env.development? && !Rails.env.test?` && foo?' do
+      expect_offense(<<~RUBY)
+        !Rails.env.development? && !Rails.env.test? && foo?
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `!Rails.env.local?` instead.
+        foo? && !Rails.env.test? && !Rails.env.development?
+                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `!Rails.env.local?` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        !Rails.env.local? && foo?
+        foo? && !Rails.env.local?
+      RUBY
+    end
+
     it 'registers no offenses for `Rails.env.local?`' do
       expect_no_offenses(<<~RUBY)
         Rails.env.local?
       RUBY
     end
 
+    it 'registers no offenses for `Rails.env.development? || Rails.env.test? && foo?`' do
+      expect_no_offenses(<<~RUBY)
+        Rails.env.development? || Rails.env.test? && foo?
+      RUBY
+    end
+
     include_examples 'non-local candidates'
   end