From 4f00a05152c105814f17a5f582d493435de96747 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 10 Jan 2018 01:30:04 +0800
Subject: [PATCH 1/4] 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 ++-
 .../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

From a93ccf07107c1680b19c16c3365ee6a94f450811 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 10 Jan 2018 18:09:00 +0800
Subject: [PATCH 2/4] Properly cleaning up memoized values

---
 app/models/concerns/resolvable_discussion.rb | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 6a2cb80b6d5..7c236369793 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -44,7 +44,9 @@ module ResolvableDiscussion
   end
 
   def first_note
-    notes.first
+    strong_memoize(:first_note) do
+      notes.first
+    end
   end
 
   def first_note_to_resolve
@@ -102,8 +104,8 @@ module ResolvableDiscussion
     # Set the notes array to the updated notes
     @notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
-    self.class.memoized_values.each do |var|
-      instance_variable_set(:"@#{var}", nil)
+    self.class.memoized_values.each do |name|
+      clear_memoization(name)
     end
   end
 end

From 72c489e9e4f2e2adac1da518cd632f1f4143d56e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 12 Jan 2018 18:51:52 +0800
Subject: [PATCH 3/4] Add changelog entry

---
 changelogs/unreleased/41731-predicate-memoization.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/41731-predicate-memoization.yml

diff --git a/changelogs/unreleased/41731-predicate-memoization.yml b/changelogs/unreleased/41731-predicate-memoization.yml
new file mode 100644
index 00000000000..110f78063f4
--- /dev/null
+++ b/changelogs/unreleased/41731-predicate-memoization.yml
@@ -0,0 +1,5 @@
+---
+title: Properly memoize some predicate methods
+merge_request: 16329
+author:
+type: performance

From d6c69373e54b76232f361aa631e7b6283d3c71ef Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 13 Jan 2018 00:17:30 +0800
Subject: [PATCH 4/4] Make sure it's not offending to use local ||= val

---
 .../cop/gitlab/predicate_memoization_spec.rb       | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb b/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb
index 6c731139468..21fc4584654 100644
--- a/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb
+++ b/spec/rubocop/cop/gitlab/predicate_memoization_spec.rb
@@ -70,6 +70,20 @@ describe RuboCop::Cop::Gitlab::PredicateMemoization do
     end
   end
 
+  context 'when source is a predicate method using local with ||=' 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