From 35b9cc0f70c6b34d64e8fc662af37de85ac493d9 Mon Sep 17 00:00:00 2001 From: Tomas Hromada Date: Tue, 1 Oct 2019 11:19:40 +0200 Subject: [PATCH] Add Rails/SafeNavigationWithBlank cop --- CHANGELOG.md | 2 + config/default.yml | 12 +++++ .../cop/rails/safe_nagivation_with_blank.rb | 48 +++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + manual/cops.md | 1 + manual/cops_rails.md | 25 ++++++++++ .../rails/safe_navigation_with_blank_spec.rb | 43 +++++++++++++++++ 7 files changed, 132 insertions(+) create mode 100644 lib/rubocop/cop/rails/safe_nagivation_with_blank.rb create mode 100644 spec/rubocop/cop/rails/safe_navigation_with_blank_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 23967a031b..ed9beefbd4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/config/default.yml b/config/default.yml index 2c37d8beea..fc4734bcf3 100644 --- a/config/default.yml +++ b/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' diff --git a/lib/rubocop/cop/rails/safe_nagivation_with_blank.rb b/lib/rubocop/cop/rails/safe_nagivation_with_blank.rb new file mode 100644 index 0000000000..71cef258e9 --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 8a2a548135..1f6acb4537 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/manual/cops.md b/manual/cops.md index a152ffc659..00cf65ca00 100644 --- a/manual/cops.md +++ b/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) diff --git a/manual/cops_rails.md b/manual/cops_rails.md index e1c1b09491..97b8e39e7a 100644 --- a/manual/cops_rails.md +++ b/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 diff --git a/spec/rubocop/cop/rails/safe_navigation_with_blank_spec.rb b/spec/rubocop/cop/rails/safe_navigation_with_blank_spec.rb new file mode 100644 index 0000000000..7b90f6b7d8 --- /dev/null +++ b/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