Merge branch 'add-delegate-predicate-method-cop' into 'master'

Add cop to disallow delegating predicate methods

See merge request gitlab-org/gitlab!54658

.rubocop_manual_todo.yml

@@ -3577,3 +3577,19 @@ Performance/OpenStruct:
     - 'lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb'
     - 'lib/gitlab/testing/request_inspector_middleware.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'

rubocop/cop/gitlab/delegate_predicate_methods.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

spec/rubocop/cop/gitlab/delegate_predicate_methods_spec.rb

+# 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