Merge pull request #133 from gyfis/th/safe-nagivation-with-blank

Add Rails/SafeNavigationWithBlank cop

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 * [#123](https://github.com/rubocop-hq/rubocop-rails/pull/123): Add new `Rails/ApplicationController` and `Rails/ApplicationMailer` cops. ([@eugeneius][])
 * [#130](https://github.com/rubocop-hq/rubocop-rails/pull/130): Add new `Rails/RakeEnvironment` cop. ([@pocke][])
+* [#132](https://github.com/rubocop-hq/rubocop-rails/pull/132): Add new `Rails/SafeNavigationWithBlank` cop. ([@gyfis][])
 
 ### Bug fixes
 
@@ -98,4 +99,5 @@
 [@forresty]: https://github.com/forresty
 [@sinsoku]: https://github.com/sinsoku
 [@pocke]: https://github.com/pocke
+[@gyfis]: https:/github.com/gyfis
 [@DNA]: https://github.com/DNA

config/default.yml

@@ -414,6 +414,18 @@ Rails/SafeNavigation:
   # implement the intended method. `try` will not raise an exception for this.
   ConvertTry: false
 
+Rails/SafeNavigationWithBlank:
+  Description: 'Avoid `foo&.blank?` in conditionals.'
+  Enabled: true
+  VersionAdded: '2.4'
+  # While the safe navigation operator is generally a good idea, when
+  # checking `foo&.blank?` in a conditional, `foo` being `nil` will actually
+  # do the opposite of what the author intends.
+  #
+  # foo&.blank? #=> nil
+  # foo.blank? #=> true
+  SafeAutoCorrect: false
+
 Rails/SaveBang:
   Description: 'Identifies possible cases where Active Record save! or related should be used.'
   StyleGuide: 'https://rails.rubystyle.guide#save-bang'

lib/rubocop/cop/rails/safe_nagivation_with_blank.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks to make sure safe navigation isn't used with `blank?` in
+      # a conditional.
+      #
+      # While the safe navigation operator is generally a good idea, when
+      # checking `foo&.blank?` in a conditional, `foo` being `nil` will actually
+      # do the opposite of what the author intends.
+      #
+      # @example
+      #   # bad
+      #   do_something if foo&.blank?
+      #   do_something unless foo&.blank?
+      #
+      #   # good
+      #   do_something if foo.blank?
+      #   do_something unless foo.blank?
+      #
+      class SafeNavigationWithBlank < Cop
+        MSG =
+          'Avoid calling `blank?` with the safe navigation operator ' \
+          'in conditionals.'
+
+        def_node_matcher :safe_navigation_blank_in_conditional?, <<-PATTERN
+          (if $(csend ... :blank?) ...)
+        PATTERN
+
+        def on_if(node)
+          return unless safe_navigation_blank_in_conditional?(node)
+
+          add_offense(node)
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(
+              safe_navigation_blank_in_conditional?(node).location.dot,
+              '.'
+            )
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -51,6 +51,7 @@
 require_relative 'rails/request_referer'
 require_relative 'rails/reversible_migration'
 require_relative 'rails/safe_navigation'
+require_relative 'rails/safe_nagivation_with_blank'
 require_relative 'rails/save_bang'
 require_relative 'rails/scope_args'
 require_relative 'rails/skips_model_validations'

manual/cops.md

@@ -50,6 +50,7 @@
 * [Rails/RequestReferer](cops_rails.md#railsrequestreferer)
 * [Rails/ReversibleMigration](cops_rails.md#railsreversiblemigration)
 * [Rails/SafeNavigation](cops_rails.md#railssafenavigation)
+* [Rails/SafeNavigationWithBlank](cops_rails.md#railssafenavigationwithblank)
 * [Rails/SaveBang](cops_rails.md#railssavebang)
 * [Rails/ScopeArgs](cops_rails.md#railsscopeargs)
 * [Rails/SkipsModelValidations](cops_rails.md#railsskipsmodelvalidations)

manual/cops_rails.md

@@ -2144,6 +2144,31 @@ Name | Default value | Configurable values
 --- | --- | ---
 ConvertTry | `false` | Boolean
 
+## Rails/SafeNavigationWithBlank
+
+Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+--- | --- | --- | --- | ---
+Enabled | Yes | Yes (Unsafe) | 2.4 | -
+
+This cop checks to make sure safe navigation isn't used with `blank?` in
+a conditional.
+
+While the safe navigation operator is generally a good idea, when
+checking `foo&.blank?` in a conditional, `foo` being `nil` will actually
+do the opposite of what the author intends.
+
+### Examples
+
+```ruby
+# bad
+do_something if foo&.blank?
+do_something unless foo&.blank?
+
+# good
+do_something if foo.blank?
+do_something unless foo.blank?
+```
+
 ## Rails/SaveBang
 
 Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

spec/rubocop/cop/rails/safe_navigation_with_blank_spec.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::SafeNavigationWithBlank do
+  subject(:cop) { described_class.new }
+
+  context 'in a conditional' do
+    it 'registers an offense on a single conditional' do
+      expect_offense(<<~RUBY)
+        do_something unless foo&.blank?
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid calling `blank?` with the safe navigation operator in conditionals.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        do_something unless foo.blank?
+      RUBY
+    end
+
+    it 'registers an offense on chained conditionals' do
+      expect_offense(<<~RUBY)
+        do_something unless foo&.bar&.blank?
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid calling `blank?` with the safe navigation operator in conditionals.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        do_something unless foo&.bar.blank?
+      RUBY
+    end
+
+    it 'does not register an offense on `.blank?`' do
+      expect_no_offenses(<<~RUBY)
+        return if foo.blank?
+      RUBY
+    end
+  end
+
+  context 'outside a conditional' do
+    it 'registers no offense' do
+      expect_no_offenses(<<~RUBY)
+        bar = foo&.blank?
+      RUBY
+    end
+  end
+end