Merge branch 'no-ivar-in-modules' into tmp

* no-ivar-in-modules:
  Cache allowed_ids
  Fix a few layout error
  Make it clear that this is an acceptable use
  Reword Instance variables in views
  Move ModuleWithInstanceVariables to Gitlab namespace
  Explain how to disable it in the doc
  Just define allowed_ids and override it with empty value
  Updates based on feedback
  Remove codes from bad merge
  Allow initialize method and single ivar
  Use StrongMemoize and enable/disable cops properly
  WIP
  Fix grammar: judge -> judgement
  Allow simple ivar ||= form. Update accordingly
  Add cop to make sure we don't use ivar in a module

lib/gitlab/git/popen.rb

@@ -16,8 +16,8 @@ def popen(cmd, path, vars = {}, lazy_block: nil)
         vars['PWD'] = path
         options = { chdir: path }
 
-        @cmd_output = ""
-        @cmd_status = 0
+        cmd_output = ""
+        cmd_status = 0
         Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
           yield(stdin) if block_given?
           stdin.close
@@ -25,14 +25,14 @@ def popen(cmd, path, vars = {}, lazy_block: nil)
           if lazy_block
             return lazy_block.call(stdout.lazy)
           else
-            @cmd_output << stdout.read
+            cmd_output << stdout.read
           end
 
-          @cmd_output << stderr.read
-          @cmd_status = wait_thr.value.exitstatus
+          cmd_output << stderr.read
+          cmd_status = wait_thr.value.exitstatus
         end
 
-        [@cmd_output, @cmd_status]
+        [cmd_output, cmd_status]
       end
 
       def popen_with_timeout(cmd, timeout, path, vars = {})

lib/gitlab/import_export/command_line_util.rb

@@ -32,7 +32,7 @@ def untar_with_options(archive:, dir:, options:)
 
       def execute(cmd)
         output, status = Gitlab::Popen.popen(cmd)
-        @shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero?
+        @shared.error(Gitlab::ImportExport::Error.new(output.to_s)) unless status.zero? # rubocop:disable Gitlab/ModuleWithInstanceVariables
         status.zero?
       end
 

lib/gitlab/metrics/influx_db.rb

@@ -156,6 +156,7 @@ def current_transaction
 
       # When enabled this should be set before being used as the usual pattern
       # "@foo ||= bar" is _not_ thread-safe.
+      # rubocop:disable Gitlab/ModuleWithInstanceVariables
       def pool
         if influx_metrics_enabled?
           if @pool.nil?
@@ -172,6 +173,7 @@ def pool
           @pool
         end
       end
+      # rubocop:enable Gitlab/ModuleWithInstanceVariables
     end
   end
 end

lib/gitlab/metrics/prometheus.rb

@@ -4,6 +4,7 @@ module Gitlab
   module Metrics
     module Prometheus
       include Gitlab::CurrentSettings
+      include Gitlab::Utils::StrongMemoize
 
       REGISTRY_MUTEX = Mutex.new
       PROVIDER_MUTEX = Mutex.new
@@ -17,16 +18,18 @@ def metrics_folder_present?
       end
 
       def prometheus_metrics_enabled?
-        return @prometheus_metrics_enabled if defined?(@prometheus_metrics_enabled)
-
-        @prometheus_metrics_enabled = prometheus_metrics_enabled_unmemoized
+        strong_memoize(:prometheus_metrics_enabled) do
+          prometheus_metrics_enabled_unmemoized
+        end
       end
 
       def registry
-        return @registry if @registry
-
-        REGISTRY_MUTEX.synchronize do
-          @registry ||= ::Prometheus::Client.registry
+        strong_memoize(:registry) do
+          REGISTRY_MUTEX.synchronize do
+            strong_memoize(:registry) do
+              ::Prometheus::Client.registry
+            end
+          end
         end
       end
 

lib/gitlab/slash_commands/presenters/issue_base.rb

@@ -11,28 +11,28 @@ def status_text(issuable)
         end
 
         def project
-          @resource.project
+          resource.project
         end
 
         def author
-          @resource.author
+          resource.author
         end
 
         def fields
           [
             {
               title: "Assignee",
-              value: @resource.assignees.any? ? @resource.assignees.first.name : "_None_",
+              value: resource.assignees.any? ? resource.assignees.first.name : "_None_",
               short: true
             },
             {
               title: "Milestone",
-              value: @resource.milestone ? @resource.milestone.title : "_None_",
+              value: resource.milestone ? resource.milestone.title : "_None_",
               short: true
             },
             {
               title: "Labels",
-              value: @resource.labels.any? ? @resource.label_names.join(', ') : "_None_",
+              value: resource.labels.any? ? resource.label_names.join(', ') : "_None_",
               short: true
             },
             {
@@ -42,6 +42,10 @@ def fields
             }
           ]
         end
+
+        private
+
+        attr_reader :resource
       end
     end
   end

lib/tasks/gettext.rake

@@ -23,6 +23,7 @@ namespace :gettext do
   desc 'Lint all po files in `locale/'
   task lint: :environment do
     require 'simple_po_parser'
+    require 'gitlab/utils'
 
     FastGettext.silence_errors
     files = Dir.glob(Rails.root.join('locale/*/gitlab.po'))

lib/tasks/gitlab/task_helpers.rb

 require 'rainbow/ext/string'
+require 'gitlab/utils/strong_memoize'
 
 module Gitlab
   TaskFailedError = Class.new(StandardError)
   TaskAbortedByUserError = Class.new(StandardError)
 
   module TaskHelpers
+    include Gitlab::Utils::StrongMemoize
+
     extend self
 
     # Ask if the user wants to continue
@@ -105,16 +108,16 @@ def gitlab_user
     end
 
     def gitlab_user?
-      return @is_gitlab_user unless @is_gitlab_user.nil?
-
-      current_user = run_command(%w(whoami)).chomp
-      @is_gitlab_user = current_user == gitlab_user
+      strong_memoize(:is_gitlab_user) do
+        current_user = run_command(%w(whoami)).chomp
+        current_user == gitlab_user
+      end
     end
 
     def warn_user_is_not_gitlab
-      return if @warned_user_not_gitlab
+      return if gitlab_user?
 
-      unless gitlab_user?
+      strong_memoize(:warned_user_not_gitlab) do
         current_user = run_command(%w(whoami)).chomp
 
         puts " Warning ".color(:black).background(:yellow)
@@ -122,8 +125,6 @@ def warn_user_is_not_gitlab
         puts "  Things may work\/fail for the wrong reasons."
         puts "  For correct results you should run this as user #{gitlab_user.color(:magenta)}."
         puts ""
-
-        @warned_user_not_gitlab = true
       end
     end
 

qa/qa/runtime/scenario.rb

@@ -6,13 +6,15 @@ module Runtime
     module Scenario
       extend self
 
-      attr_reader :attributes
+      def attributes
+        @attributes ||= {}
+      end
 
       def define(attribute, value)
-        (@attributes ||= {}).store(attribute.to_sym, value)
+        attributes.store(attribute.to_sym, value)
 
         define_singleton_method(attribute) do
-          @attributes[attribute.to_sym].tap do |value|
+          attributes[attribute.to_sym].tap do |value|
             if value.to_s.empty?
               raise ArgumentError, "Empty `#{attribute}` attribute!"
             end

rubocop/cop/gitlab/module_with_instance_variables.rb

+module RuboCop
+  module Cop
+    module Gitlab
+      class ModuleWithInstanceVariables < RuboCop::Cop::Cop
+        MSG = <<~EOL.freeze
+          Do not use instance variables in a module. Please read this
+          for the rationale behind it:
+
+          https://docs.gitlab.com/ee/development/module_with_instance_variables.html
+        EOL
+
+        def on_module(node)
+          check_method_definition(node)
+
+          # Not sure why some module would have an extra begin wrapping around
+          node.each_child_node(:begin) do |begin_node|
+            check_method_definition(begin_node)
+          end
+        end
+
+        private
+
+        def check_method_definition(node)
+          node.each_child_node(:def) do |definition|
+            # We allow this pattern:
+            #
+            #     def f
+            #       @f ||= true
+            #     end
+            if only_ivar_or_assignment?(definition)
+              # We don't allow if any other ivar is used
+              definition.each_descendant(:ivar) do |offense|
+                add_offense(offense, :expression)
+              end
+            # We allow initialize method and single ivar
+            elsif !initialize_method?(definition) && !single_ivar?(definition)
+              definition.each_descendant(:ivar, :ivasgn) do |offense|
+                add_offense(offense, :expression)
+              end
+            end
+          end
+        end
+
+        def only_ivar_or_assignment?(definition)
+          node = definition.child_nodes.last
+
+          definition.child_nodes.size == 2 &&
+            node.or_asgn_type? && node.child_nodes.first.ivasgn_type?
+        end
+
+        def single_ivar?(definition)
+          node = definition.child_nodes.last
+
+          definition.child_nodes.size == 2 && node.ivar_type?
+        end
+
+        def initialize_method?(definition)
+          definition.children.first == :initialize
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -7,6 +7,7 @@
 require_relative 'cop/polymorphic_associations'
 require_relative 'cop/project_path_helper'
 require_relative 'cop/redirect_with_status'
+require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'

spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb

@@ -23,6 +23,8 @@
     allow_any_instance_of(described_class).to receive(:serialize) do |event|
       event
     end
+    allow_any_instance_of(described_class)
+      .to receive(:allowed_ids).and_return(nil)
 
     stub_const('Gitlab::CycleAnalytics::BaseEventFetcher::MAX_EVENTS', max_events)
 

spec/rubocop/cop/gitlab/module_with_instance_variables_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/module_with_instance_variables'
+
+describe RuboCop::Cop::Gitlab::ModuleWithInstanceVariables 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 instance variable is used in a module' do
+      inspect_source(cop, 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(cop, source)
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'when source is a regular module' do
+    it_behaves_like 'registering offense', offending_lines: [3] do
+      let(:source) do
+        <<~RUBY
+          module M
+            def f
+              @f = true
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a nested module' do
+    it_behaves_like 'registering offense', offending_lines: [4] do
+      let(:source) do
+        <<~RUBY
+          module N
+            module M
+              def f
+                @f = true
+              end
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a nested module with multiple offenses' do
+    it_behaves_like 'registering offense', offending_lines: [4, 12] do
+      let(:source) do
+        <<~RUBY
+          module N
+            module M
+              def f
+                @f = true
+              end
+
+              def g
+                true
+              end
+
+              def h
+                @h = true
+              end
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is using simple or ivar assignment' do
+    it_behaves_like 'not registering offense' do
+      let(:source) do
+        <<~RUBY
+          module M
+            def f
+              @f ||= true
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is using simple ivar' do
+    it_behaves_like 'not registering offense' do
+      let(:source) do
+        <<~RUBY
+          module M
+            def f?
+              @f
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is defining initialize' do
+    it_behaves_like 'not registering offense' do
+      let(:source) do
+        <<~RUBY
+          module M
+            def initialize
+              @a = 1
+              @b = 2
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is using simple or ivar assignment with other ivar' do
+    it_behaves_like 'registering offense', offending_lines: [3] do
+      let(:source) do
+        <<~RUBY
+          module M
+            def f
+              @f ||= g(@g)
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is using or ivar assignment with something else' do
+    it_behaves_like 'registering offense', offending_lines: [3, 4] do
+      let(:source) do
+        <<~RUBY
+          module M
+            def f
+              @f ||= true
+              @f.to_s
+            end
+          end
+        RUBY
+      end
+    end
+  end
+end