Commit 150e877f authored by Tan Le's avatar Tan Le

Add cop to disallow delegating predicate methods

This cop looks for delegations to predicate methods with `allow_nil`
option. When delegating with this option enabled, we ends up with true,
false and nil and this does not preserve the boolean nature of predicate
methods.

The preferred implementation is to define a method to handle `nil` and
returns the correct Boolean value instead.

delegate :is_foo?, to: :bar, allow_nil: true

def is_foo?
  return false unless bar
  bar.is_foo?
end

def is_foo?
  !!bar&.is_foo?
end
parent fc25343d
...@@ -2590,3 +2590,19 @@ Performance/OpenStruct: ...@@ -2590,3 +2590,19 @@ Performance/OpenStruct:
- 'lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb' - 'lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb'
- 'lib/gitlab/testing/request_inspector_middleware.rb' - 'lib/gitlab/testing/request_inspector_middleware.rb'
- 'lib/mattermost/session.rb' - 'lib/mattermost/session.rb'
# WIP: https://gitlab.com/gitlab-org/gitlab/-/issues/324629
Gitlab/DelegatePredicateMethods:
Exclude:
- 'app/models/clusters/cluster.rb'
- 'app/models/clusters/platforms/kubernetes.rb'
- 'app/models/concerns/ci/metadatable.rb'
- 'app/models/concerns/diff_positionable_note.rb'
- 'app/models/concerns/resolvable_discussion.rb'
- 'app/models/concerns/services/data_fields.rb'
- 'app/models/project.rb'
- 'ee/app/models/concerns/ee/ci/metadatable.rb'
- 'ee/app/models/ee/group.rb'
- 'ee/app/models/ee/namespace.rb'
- 'ee/app/models/license.rb'
- 'lib/gitlab/ci/trace/stream.rb'
# frozen_string_literal: true
module RuboCop
module Cop
module Gitlab
# This cop looks for delegations to predicate methods with `allow_nil: true` option.
# This construct results in three possible results: true, false and nil.
# In other words, it does not preserve the strict Boolean nature of predicate method return value.
# This cop suggests creating a method to handle `nil` delegator and ensure only Boolean type is returned.
#
# @example
# # bad
# delegate :is_foo?, to: :bar, allow_nil: true
#
# # good
# def is_foo?
# return false unless bar
# bar.is_foo?
# end
#
# def is_foo?
# !!bar&.is_foo?
# end
class DelegatePredicateMethods < RuboCop::Cop::Cop
MSG = "Using `delegate` with `allow_nil` on the following predicate methods is discouraged: %s."
RESTRICT_ON_SEND = %i[delegate].freeze
def_node_matcher :predicate_allow_nil_option, <<~PATTERN
(send nil? :delegate
(sym $_)*
(hash <$(pair (sym :allow_nil) true) ...>)
)
PATTERN
def on_send(node)
predicate_allow_nil_option(node) do |delegated_methods, _options|
offensive_methods = delegated_methods.select { |method| method.end_with?('?') }
next if offensive_methods.empty?
add_offense(node, message: format(MSG, offensive_methods.join(', ')))
end
end
end
end
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require_relative '../../../../rubocop/cop/gitlab/delegate_predicate_methods'
RSpec.describe RuboCop::Cop::Gitlab::DelegatePredicateMethods do
subject(:cop) { described_class.new }
it 'registers offense for single predicate method with allow_nil:true' do
expect_offense(<<~SOURCE)
delegate :is_foo?, :do_foo, to: :bar, allow_nil: true
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Using `delegate` with `allow_nil` on the following predicate methods is discouraged: is_foo?.
SOURCE
end
it 'registers offense for multiple predicate methods with allow_nil:true' do
expect_offense(<<~SOURCE)
delegate :is_foo?, :is_bar?, to: :bar, allow_nil: true
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Using `delegate` with `allow_nil` on the following predicate methods is discouraged: is_foo?, is_bar?.
SOURCE
end
it 'registers no offense for non-predicate method with allow_nil:true' do
expect_no_offenses(<<~SOURCE)
delegate :do_foo, to: :bar, allow_nil: true
SOURCE
end
it 'registers no offense with predicate method with allow_nil:false' do
expect_no_offenses(<<~SOURCE)
delegate :is_foo?, to: :bar, allow_nil: false
SOURCE
end
it 'registers no offense with predicate method without allow_nil' do
expect_no_offenses(<<~SOURCE)
delegate :is_foo?, to: :bar
SOURCE
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment