Merge pull request #1461 from vlad-pisanov/vp_presence_1

Add support for chained method calls in `Rails/Presence`

Author: koic

changelog/change_presence_to_support_chained_calls.md

@@ -0,0 +1 @@
+* [#932](https://github.com/rubocop/rubocop-rails/issues/932): Add support for chained method calls in `Rails/Presence`. ([@vlad-pisanov][])

config/default.yml

@@ -844,6 +844,7 @@ Rails/Presence:
   Description: 'Checks code that can be written more easily using `Object#presence` defined by Active Support.'
   Enabled: true
   VersionAdded: '0.52'
+  VersionChanged: '<<next>>'
 
 Rails/Present:
   Description: 'Enforces use of `present?`.'

lib/rubocop/cop/rails/presence.rb

@@ -37,6 +37,22 @@ module Rails
       #
       #   # good
       #   a.presence || b
+      #
+      # @example
+      #   # bad
+      #   a.present? ? a.foo : nil
+      #
+      #   # bad
+      #   !a.present? ? nil : a.foo
+      #
+      #   # bad
+      #   a.blank? ? nil : a.foo
+      #
+      #   # bad
+      #   !a.blank? ? a.foo : nil
+      #
+      #   # good
+      #   a.presence&.foo
       class Presence < Base
         include RangeHelp
         extend AutoCorrector
@@ -46,29 +62,29 @@ class Presence < Base
         def_node_matcher :redundant_receiver_and_other, <<~PATTERN
           {
             (if
-              (send $_recv :present?)
-              _recv
+              {(send $_recv :blank?) (send (send $_recv :present?) :!)}
               $!begin
+              _recv
             )
             (if
-              (send $_recv :blank?)
-              $!begin
+              {(send $_recv :present?) (send (send $_recv :blank?) :!)}
               _recv
+              $!begin
             )
           }
         PATTERN
 
-        def_node_matcher :redundant_negative_receiver_and_other, <<~PATTERN
+        def_node_matcher :redundant_receiver_and_chain, <<~PATTERN
           {
             (if
-              (send (send $_recv :present?) :!)
-              $!begin
-              _recv
+              {(send $_recv :blank?) (send (send $_recv :present?) :!)}
+              {nil? nil_type?}
+              $(send _recv ...)
             )
             (if
-              (send (send $_recv :blank?) :!)
-              _recv
-              $!begin
+              {(send $_recv :present?) (send (send $_recv :blank?) :!)}
+              $(send _recv ...)
+              {nil? nil_type?}
             )
           }
         PATTERN
@@ -82,18 +98,26 @@ def on_if(node)
             register_offense(node, receiver, other)
           end
 
-          redundant_negative_receiver_and_other(node) do |receiver, other|
-            return if ignore_other_node?(other) || receiver.nil?
+          redundant_receiver_and_chain(node) do |receiver, chain|
+            return if ignore_chain_node?(chain) || receiver.nil?
 
-            register_offense(node, receiver, other)
+            register_chain_offense(node, receiver, chain)
           end
         end
 
         private
 
         def register_offense(node, receiver, other)
-          add_offense(node, message: message(node, receiver, other)) do |corrector|
-            corrector.replace(node, replacement(receiver, other, node.left_sibling))
+          replacement = replacement(receiver, other, node.left_sibling)
+          add_offense(node, message: message(node, replacement)) do |corrector|
+            corrector.replace(node, replacement)
+          end
+        end
+
+        def register_chain_offense(node, receiver, chain)
+          replacement = chain_replacement(receiver, chain, node.left_sibling)
+          add_offense(node, message: message(node, replacement)) do |corrector|
+            corrector.replace(node, replacement)
           end
         end
 
@@ -105,8 +129,12 @@ def ignore_other_node?(node)
           node&.type?(:if, :rescue, :while)
         end
 
-        def message(node, receiver, other)
-          prefer = replacement(receiver, other, node.left_sibling).gsub(/^\s*|\n/, '')
+        def ignore_chain_node?(node)
+          node.method?('[]') || node.arithmetic_operation?
+        end
+
+        def message(node, replacement)
+          prefer  = replacement.gsub(/^\s*|\n/, '')
           current = current(node).gsub(/^\s*|\n/, '')
           format(MSG, prefer: prefer, current: current)
         end
@@ -146,6 +174,12 @@ def build_source_for_or_method(other)
         def method_range(node)
           range_between(node.source_range.begin_pos, node.first_argument.source_range.begin_pos - 1)
         end
+
+        def chain_replacement(receiver, chain, left_sibling)
+          replaced = "#{receiver.source}.presence&.#{chain.method_name}"
+          replaced += "(#{chain.arguments.map(&:source).join(', ')})" if chain.arguments?
+          left_sibling ? "(#{replaced})" : replaced
+        end
       end
     end
   end

spec/rubocop/cop/rails/presence_spec.rb

@@ -233,6 +233,81 @@
     RUBY
   end
 
+  context 'when a method is called on the receiver' do
+    it 'registers an offense and corrects when `a.present? ? a.foo : nil' do
+      expect_offense(<<~RUBY)
+        a.present? ? a.foo : nil
+        ^^^^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.present? ? a.foo : nil`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        a.presence&.foo
+      RUBY
+    end
+
+    it 'registers an offense and corrects when `a.blank? ? nil : a.foo' do
+      expect_offense(<<~RUBY)
+        a.blank? ? nil : a.foo
+        ^^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.blank? ? nil : a.foo`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        a.presence&.foo
+      RUBY
+    end
+
+    it 'registers an offense and corrects when `a.foo if a.present?`' do
+      expect_offense(<<~RUBY)
+        a.foo if a.present?
+        ^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.foo if a.present?`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        a.presence&.foo
+      RUBY
+    end
+
+    it 'registers an offense and corrects when `a.foo unless a.blank?`' do
+      expect_offense(<<~RUBY)
+        a.foo unless a.blank?
+        ^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.foo unless a.blank?`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        a.presence&.foo
+      RUBY
+    end
+
+    it 'registers an offense and corrects when chained method takes parameters' do
+      expect_offense(<<~RUBY)
+        a.present? ? a.foo(42, key: :value) : nil
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo(42, key: :value)` instead of `a.present? ? a.foo(42, key: :value) : nil`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        a.presence&.foo(42, key: :value)
+      RUBY
+    end
+
+    it 'does not register an offense when chained method is `[]`' do
+      expect_no_offenses(<<~RUBY)
+        a.present? ? a[1] : nil
+      RUBY
+    end
+
+    it 'does not register an offense when chained method is an arithmetic operation' do
+      expect_no_offenses(<<~RUBY)
+        a.present? ? a + 42 : nil
+      RUBY
+    end
+
+    it 'does not register an offense when multiple methods are chained' do
+      expect_no_offenses(<<~RUBY)
+        a.present? ? a.foo.bar : nil
+      RUBY
+    end
+  end
+
   context 'when multiline ternary can be replaced' do
     it 'registers an offense and corrects' do
       expect_offense(<<~RUBY)