diff --git a/changelog/fix_false_negative_for_env_local.md b/changelog/fix_false_negative_for_env_local.md new file mode 100644 index 0000000000..1d8046f24b --- /dev/null +++ b/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][]) diff --git a/lib/rubocop/cop/rails/env_local.rb b/lib/rubocop/cop/rails/env_local.rb index 8aea161c71..f6aec62dcb 100644 --- a/lib/rubocop/cop/rails/env_local.rb +++ b/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 diff --git a/spec/rubocop/cop/rails/env_local_spec.rb b/spec/rubocop/cop/rails/env_local_spec.rb index aba7485130..a9a971efa8 100644 --- a/spec/rubocop/cop/rails/env_local_spec.rb +++ b/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