Skip to content

Add cop to disallow delegating predicate methods

Tan Le requested to merge add-delegate-predicate-method-cop into master

Description of the proposal

This MR adds a new 👮 Gitlab/DelegatePredicateMethods to discourage delegation of predicate methods with allow_nil: true. When delegating with this option enabled, we end up with true, false and nil and this does not preserve the boolean nature of predicate methods.

I do not hold a strong opinion on this cop. My motivation is to reduce the future effort in correcting these coding styles given that we have done/accepted a few of those fixes already.

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

Offense

Example

app/models/clusters/cluster.rb:96:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: on_creation?.
    delegate :on_creation?, to: :provider, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:97:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: knative_pre_installed?.
    delegate :knative_pre_installed?, to: :provider, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
22134 files inspected, 29 offenses detected 
app/models/clusters/cluster.rb:96:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: on_creation?.
    delegate :on_creation?, to: :provider, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:97:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: knative_pre_installed?.
    delegate :knative_pre_installed?, to: :provider, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:99:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: active?.
    delegate :active?, to: :platform_kubernetes, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:100:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: rbac?.
    delegate :rbac?, to: :platform_kubernetes, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:101:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: available?.
    delegate :available?, to: :application_helm, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:102:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: available?.
    delegate :available?, to: :application_ingress, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:103:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: available?.
    delegate :available?, to: :application_prometheus, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:104:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: available?.
    delegate :available?, to: :application_knative, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/cluster.rb:105:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: available?.
    delegate :available?, to: :application_elastic_stack, prefix: true, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/platforms/kubernetes.rb:53:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: enabled?.
      delegate :enabled?, to: :cluster, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/platforms/kubernetes.rb:54:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: provided_by_user?.
      delegate :provided_by_user?, to: :cluster, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/clusters/platforms/kubernetes.rb:55:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: allow_user_defined_namespace?.
      delegate :allow_user_defined_namespace?, to: :cluster, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/concerns/ci/metadatable.rb:21:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: has_exposed_artifacts?.
      delegate :has_exposed_artifacts?, to: :metadata, prefix: false, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/concerns/diff_positionable_note.rb:6:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: on_text?, on_image?.
    delegate :on_text?, :on_image?, to: :position, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/concerns/resolvable_discussion.rb:31:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: resolved_by_push?.
    delegate  :resolved_at, ...
    ^^^^^^^^^^^^^^^^^^^^^^^
app/models/concerns/services/data_fields.rb:10:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: activated?.
      delegate :activated?, to: :service, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/project.rb:389:3: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: feature_available?, builds_enabled?, wiki_enabled?, merge_requests_enabled?, forking_enabled?, issues_enabled?, pages_enabled?, analytics_enabled?, snippets_enabled?, public_pages?, private_pages?, operations_enabled?.
  delegate :feature_available?, :builds_enabled?, :wiki_enabled?, ...
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/project.rb:398:3: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: show_default_award_emojis?.
  delegate :show_default_award_emojis, :show_default_award_emojis=, ...
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/project.rb:401:3: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: scheduled?, started?, in_progress?, failed?, finished?.
  delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?, ...
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/project.rb:404:3: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: no_import?.
  delegate :no_import?, to: :import_state, allow_nil: true
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/project.rb:423:3: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: active?.
  delegate :active?, to: :prometheus_service, allow_nil: true, prefix: true
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/concerns/ee/ci/metadatable.rb:9:9: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: secrets?.
        delegate :secrets?, :secrets, to: :metadata, prefix: false, allow_nil: true
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/ee/group.rb:53:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: enforced_group_managed_accounts?, enforced_sso?.
      delegate :enforced_group_managed_accounts?, :enforced_sso?, to: :saml_provider, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/ee/group.rb:54:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: repository_read_only?.
      delegate :repository_read_only, :repository_read_only?, to: :namespace_settings, allow_nil: true
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/ee/namespace.rb:79:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: temporary_storage_increase_enabled?, eligible_for_temporary_storage_increase?.
      delegate :additional_purchased_storage_size, :additional_purchased_storage_size=, ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/ee/namespace.rb:96:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: trial?, upgradable?.
      delegate :trial?, :trial_ends_on, :trial_starts_on, :trial_days_remaining, ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/ee/project.rb:200:7: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: last_update_succeeded?, last_update_failed?, ever_updated_successfully?, hard_failed?.
      delegate :last_update_succeeded?, :last_update_failed?, ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ee/app/models/license.rb:287:5: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: block_changes?, feature_available?.
    delegate :block_changes?, :feature_available?, to: :current, allow_nil: true
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/gitlab/ci/trace/stream.rb:15:9: C: Gitlab/DelegatePredicateMethods: Using delegate with allow_nil on the following predicate methods is discouraged: valid?.
        delegate :valid?, to: :stream, allow_nil: true
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

22134 files inspected, 29 offenses detected

Related to #322100

Check-list

  • Make sure this MR enables a static analysis check rule for new usage but ignores current offenses
  • Mention this proposal in the relevant Slack channels (e.g. #development, #backend, #frontend)
  • If there is a choice to make between two potential styles, set up an emoji vote in the MR:
    • CHOICE_A: 🅰
    • CHOICE_B: 🅱
    • Vote yourself for both choices so that people know these are the choices
  • The MR doesn't have significant objections, and is getting a majority of 👍 vs 👎 (remember that we don't need to reach a consensus)
  • (If applicable) One style is getting a majority of vote (compared to the other choice)
  • (If applicable) Update the MR with the chosen style
  • Create a follow-up issue to fix the current offenses as a separate iteration: #324629 (closed)
  • Follow the review process as usual
  • Once approved and merged by a maintainer, mention it again:
    • In the relevant Slack channels (e.g. #development, #backend, #frontend)
    • (Optional depending on the impact of the change) In the Engineering Week in Review

/cc @gitlab-org/maintainers/rails-backend

Edited by Tan Le

Merge request reports