From 4f00a05152c105814f17a5f582d493435de96747 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 10 Jan 2018 01:30:04 +0800 Subject: Introduce PredicateMemoization cop and fix offenses with StrongMemoize --- app/models/concerns/resolvable_discussion.rb | 19 +++-- app/models/project.rb | 11 ++- app/models/repository.rb | 9 ++- .../prepare_untracked_uploads.rb | 10 ++- lib/gitlab/bare_repository_import/repository.rb | 10 ++- lib/gitlab/ci/pipeline/chain/skip.rb | 6 +- lib/gitlab/ci/stage/seed.rb | 6 +- lib/gitlab/github_import/client.rb | 6 +- lib/gitlab/user_access.rb | 10 ++- rubocop/cop/gitlab/predicate_memoization.rb | 39 ++++++++++ rubocop/rubocop.rb | 1 + .../cop/gitlab/predicate_memoization_spec.rb | 86 ++++++++++++++++++++++ 12 files changed, 192 insertions(+), 21 deletions(-) create mode 100644 rubocop/cop/gitlab/predicate_memoization.rb create mode 100644 spec/rubocop/cop/gitlab/predicate_memoization_spec.rb diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index b6c7b6735b9..6a2cb80b6d5 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -1,5 +1,6 @@ module ResolvableDiscussion extend ActiveSupport::Concern + include ::Gitlab::Utils::StrongMemoize included do # A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized. @@ -31,27 +32,35 @@ module ResolvableDiscussion end def resolvable? - @resolvable ||= potentially_resolvable? && notes.any?(&:resolvable?) + strong_memoize(:resolvable) do + potentially_resolvable? && notes.any?(&:resolvable?) + end end def resolved? - @resolved ||= resolvable? && notes.none?(&:to_be_resolved?) + strong_memoize(:resolved) do + resolvable? && notes.none?(&:to_be_resolved?) + end end def first_note - @first_note ||= notes.first + notes.first end def first_note_to_resolve return unless resolvable? - @first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Gitlab/ModuleWithInstanceVariables + strong_memoize(:first_note_to_resolve) do + notes.find(&:to_be_resolved?) + end end def last_resolved_note return unless resolved? - @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Gitlab/ModuleWithInstanceVariables + strong_memoize(:last_resolved_note) do + resolved_notes.sort_by(&:resolved_at).last + end end def resolved_notes diff --git a/app/models/project.rb b/app/models/project.rb index 029f2da2e4e..7ab7df4fdcd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -20,6 +20,7 @@ class Project < ActiveRecord::Base include GroupDescendant include Gitlab::SQL::Pattern include DeploymentPlatform + include ::Gitlab::Utils::StrongMemoize extend Gitlab::ConfigHelper extend Gitlab::CurrentSettings @@ -993,9 +994,13 @@ class Project < ActiveRecord::Base end def repo_exists? - @repo_exists ||= repository.exists? - rescue - @repo_exists = false + strong_memoize(:repo_exists) do + begin + repository.exists? + rescue + false + end + end end def root_ref?(branch) diff --git a/app/models/repository.rb b/app/models/repository.rb index d27212b2058..8e9f33c174c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -895,15 +895,18 @@ class Repository branch = Gitlab::Git::Branch.find(self, branch_or_name) if branch - @root_ref_sha ||= commit(root_ref).sha - same_head = branch.target == @root_ref_sha - merged = ancestor?(branch.target, @root_ref_sha) + same_head = branch.target == root_ref_sha + merged = ancestor?(branch.target, root_ref_sha) !same_head && merged else nil end end + def root_ref_sha + @root_ref_sha ||= commit(root_ref).sha + end + delegate :merged_branch_names, to: :raw_repository def merge_base(first_commit_id, second_commit_id) diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb index 476c46341ae..4e0121ca34d 100644 --- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb +++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb @@ -7,6 +7,7 @@ module Gitlab class PrepareUntrackedUploads # rubocop:disable Metrics/ClassLength # For bulk_queue_background_migration_jobs_by_range include Database::MigrationHelpers + include ::Gitlab::Utils::StrongMemoize FIND_BATCH_SIZE = 500 RELATIVE_UPLOAD_DIR = "uploads".freeze @@ -142,7 +143,9 @@ module Gitlab end def postgresql? - @postgresql ||= Gitlab::Database.postgresql? + strong_memoize(:postgresql) do + Gitlab::Database.postgresql? + end end def can_bulk_insert_and_ignore_duplicates? @@ -150,8 +153,9 @@ module Gitlab end def postgresql_pre_9_5? - @postgresql_pre_9_5 ||= postgresql? && - Gitlab::Database.version.to_f < 9.5 + strong_memoize(:postgresql_pre_9_5) do + postgresql? && Gitlab::Database.version.to_f < 9.5 + end end def schedule_populate_untracked_uploads_jobs diff --git a/lib/gitlab/bare_repository_import/repository.rb b/lib/gitlab/bare_repository_import/repository.rb index 85b79362196..c0c666dfb7b 100644 --- a/lib/gitlab/bare_repository_import/repository.rb +++ b/lib/gitlab/bare_repository_import/repository.rb @@ -1,6 +1,8 @@ module Gitlab module BareRepositoryImport class Repository + include ::Gitlab::Utils::StrongMemoize + attr_reader :group_path, :project_name, :repo_path def initialize(root_path, repo_path) @@ -41,11 +43,15 @@ module Gitlab private def wiki? - @wiki ||= repo_path.end_with?('.wiki.git') + strong_memoize(:wiki) do + repo_path.end_with?('.wiki.git') + end end def hashed? - @hashed ||= repo_relative_path.include?('@hashed') + strong_memoize(:hashed) do + repo_relative_path.include?('@hashed') + end end def repo_relative_path diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb index 9a72de87bab..32cbb7ca6af 100644 --- a/lib/gitlab/ci/pipeline/chain/skip.rb +++ b/lib/gitlab/ci/pipeline/chain/skip.rb @@ -3,6 +3,8 @@ module Gitlab module Pipeline module Chain class Skip < Chain::Base + include ::Gitlab::Utils::StrongMemoize + SKIP_PATTERN = /\[(ci[ _-]skip|skip[ _-]ci)\]/i def perform! @@ -24,7 +26,9 @@ module Gitlab def commit_message_skips_ci? return false unless @pipeline.git_commit_message - @skipped ||= !!(@pipeline.git_commit_message =~ SKIP_PATTERN) + strong_memoize(:commit_message_skips_ci) do + !!(@pipeline.git_commit_message =~ SKIP_PATTERN) + end end end end diff --git a/lib/gitlab/ci/stage/seed.rb b/lib/gitlab/ci/stage/seed.rb index bc97aa63b02..f33c87f554d 100644 --- a/lib/gitlab/ci/stage/seed.rb +++ b/lib/gitlab/ci/stage/seed.rb @@ -2,6 +2,8 @@ module Gitlab module Ci module Stage class Seed + include ::Gitlab::Utils::StrongMemoize + attr_reader :pipeline delegate :project, to: :pipeline @@ -50,7 +52,9 @@ module Gitlab private def protected_ref? - @protected_ref ||= project.protected_for?(pipeline.ref) + strong_memoize(:protected_ref) do + project.protected_for?(pipeline.ref) + end end end end diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index 5da9befa08e..4f160e4a447 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -14,6 +14,8 @@ module Gitlab # puts label.name # end class Client + include ::Gitlab::Utils::StrongMemoize + attr_reader :octokit # A single page of data and the corresponding page number. @@ -173,7 +175,9 @@ module Gitlab end def rate_limiting_enabled? - @rate_limiting_enabled ||= api_endpoint.include?('.github.com') + strong_memoize(:rate_limiting_enabled) do + api_endpoint.include?('.github.com') + end end def api_endpoint diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index d9a5af09f08..f357488ac61 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -16,8 +16,10 @@ module Gitlab def can_do_action?(action) return false unless can_access_git? - @permission_cache ||= {} - @permission_cache[action] ||= user.can?(action, project) + permission_cache[action] = + permission_cache.fetch(action) do + user.can?(action, project) + end end def cannot_do_action?(action) @@ -88,6 +90,10 @@ module Gitlab private + def permission_cache + @permission_cache ||= {} + end + def can_access_git? user && user.can?(:access_git) end diff --git a/rubocop/cop/gitlab/predicate_memoization.rb b/rubocop/cop/gitlab/predicate_memoization.rb new file mode 100644 index 00000000000..3c25d61d087 --- /dev/null +++ b/rubocop/cop/gitlab/predicate_memoization.rb @@ -0,0 +1,39 @@ +module RuboCop + module Cop + module Gitlab + class PredicateMemoization < RuboCop::Cop::Cop + MSG = <<~EOL.freeze + Avoid using `@value ||= query` inside predicate methods in order to + properly memoize `false` or `nil` values. + https://docs.gitlab.com/ee/development/utilities.html#strongmemoize + EOL + + def on_def(node) + return unless predicate_method?(node) + + select_offenses(node).each do |offense| + add_offense(offense, location: :expression) + end + end + + private + + def predicate_method?(node) + node.method_name.to_s.end_with?('?') + end + + def or_ivar_assignment?(or_assignment) + lhs = or_assignment.each_child_node.first + + lhs.ivasgn_type? + end + + def select_offenses(node) + node.each_descendant(:or_asgn).select do |or_assignment| + or_ivar_assignment?(or_assignment) + end + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 57af87f7fb9..9110237c538 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -1,4 +1,5 @@ require_relative 'cop/gitlab/module_with_instance_variables' +require_relative 'cop/gitlab/predicate_memoization' require_relative 'cop/include_sidekiq_worker' require_relative 'cop/line_break_around_conditional_block' require_relative 'cop/migration/add_column' diff --git a/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb b/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb new file mode 100644 index 00000000000..6c731139468 --- /dev/null +++ b/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb @@ -0,0 +1,86 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/gitlab/predicate_memoization' + +describe RuboCop::Cop::Gitlab::PredicateMemoization do + include CopHelper + + subject(:cop) { described_class.new } + + shared_examples('registering offense') do |options| + let(:offending_lines) { options[:offending_lines] } + + it 'registers an offense when a predicate method is memoizing via ivar' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + shared_examples('not registering offense') do + it 'does not register offenses' do + inspect_source(source) + + expect(cop.offenses).to be_empty + end + end + + context 'when source is a predicate method memoizing via ivar' do + it_behaves_like 'registering offense', offending_lines: [3] do + let(:source) do + <<~RUBY + class C + def really? + @really ||= true + end + end + RUBY + end + end + + it_behaves_like 'registering offense', offending_lines: [4] do + let(:source) do + <<~RUBY + class C + def really? + value = true + @really ||= value + end + end + RUBY + end + end + end + + context 'when source is a predicate method using ivar with assignment' do + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + class C + def really? + @really = true + end + end + RUBY + end + end + end + + context 'when source is a regular method memoizing via ivar' do + it_behaves_like 'not registering offense' do + let(:source) do + <<~RUBY + class C + def really + @really ||= true + end + end + RUBY + end + end + end +end -- cgit v1.2.1