Add cop to make sure we don't use ivar in a moduleno-ivar-in-modules

14 files changed, 78 insertions, 32 deletions

diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb
index 782f0be9c4a..2b7f3ba0feb 100644
--- a/app/controllers/concerns/creates_commit.rb
+++ b/app/controllers/concerns/creates_commit.rb
@@ -1,3 +1,4 @@
+# rubocop:disable Cop/ModuleWithInstanceVariables
 module CreatesCommit
   extend ActiveSupport::Concern
 
diff --git a/config/initializers/rugged_use_gitlab_git_attributes.rb b/config/initializers/rugged_use_gitlab_git_attributes.rb
index 7d652799786..b839fd177af 100644
--- a/config/initializers/rugged_use_gitlab_git_attributes.rb
+++ b/config/initializers/rugged_use_gitlab_git_attributes.rb
@@ -11,6 +11,7 @@
 # anyway, and there is no great efficiency gain from just fetching the listed
 # attributes with our implementation, so we ignore the additional arguments.
 #
+# rubocop:disable Cop/ModuleWithInstanceVariables
 module Rugged
   class Repository
     module UseGitlabGitAttributes
diff --git a/features/support/env.rb b/features/support/env.rb
index 608d988755c..d99078d4fa3 100644
--- a/features/support/env.rb
+++ b/features/support/env.rb
@@ -42,11 +42,11 @@ module StdoutReporterWithScenarioLocation
   # Override the standard reporter to show filename and line number next to each
   # scenario for easy, focused re-runs
   def before_scenario_run(scenario, step_definitions = nil)
-    @max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any?
+    max_step_name_length = scenario.steps.map(&:name).map(&:length).max if scenario.steps.any?
     name = scenario.name
 
     # This number has no significance, it's just to line things up
-    max_length = @max_step_name_length + 19
+    max_length = max_step_name_length + 19
     out.puts "\n  #{'Scenario:'.green} #{name.light_green.ljust(max_length)}" \
       " # #{scenario.feature.filename}:#{scenario.line}"
   end
diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb
index 721ed97bb6b..f3e5b1c1109 100644
--- a/lib/extracts_path.rb
+++ b/lib/extracts_path.rb
@@ -1,5 +1,6 @@
 # Module providing methods for dealing with separating a tree-ish string and a
 # file path string when combined in a request parameter
+# rubocop:disable Cop/ModuleWithInstanceVariables
 module ExtractsPath
   # Raised when given an invalid file path
   InvalidPathError = Class.new(StandardError)
diff --git a/lib/gitlab/cycle_analytics/base_event_fetcher.rb b/lib/gitlab/cycle_analytics/base_event_fetcher.rb
index ab115afcaa5..bec201f309c 100644
--- a/lib/gitlab/cycle_analytics/base_event_fetcher.rb
+++ b/lib/gitlab/cycle_analytics/base_event_fetcher.rb
@@ -59,6 +59,12 @@ module Gitlab
         nil
       end
 
+      def load_allowed_ids
+        allowed_ids_finder_class
+          .new(@options[:current_user], project_id: @project.id)
+          .execute.where(id: event_result_ids).pluck(:id)
+      end
+
       def event_result_ids
         event_result.map { |event| event['id'] }
       end
diff --git a/lib/gitlab/cycle_analytics/code_event_fetcher.rb b/lib/gitlab/cycle_analytics/code_event_fetcher.rb
index d5bf6149749..39df80352b6 100644
--- a/lib/gitlab/cycle_analytics/code_event_fetcher.rb
+++ b/lib/gitlab/cycle_analytics/code_event_fetcher.rb
@@ -1,8 +1,6 @@
 module Gitlab
   module CycleAnalytics
     class CodeEventFetcher < BaseEventFetcher
-      include MergeRequestAllowed
-
       def initialize(*args)
         @projections = [mr_table[:title],
                         mr_table[:iid],
@@ -20,6 +18,14 @@ module Gitlab
       def serialize(event)
         AnalyticsMergeRequestSerializer.new(project: @project).represent(event)
       end
+
+      def allowed_ids
+        load_allowed_ids
+      end
+
+      def allowed_ids_finder_class
+        MergeRequestsFinder
+      end
     end
   end
 end
diff --git a/lib/gitlab/cycle_analytics/issue_allowed.rb b/lib/gitlab/cycle_analytics/issue_allowed.rb
deleted file mode 100644
index a7652a70641..00000000000
--- a/lib/gitlab/cycle_analytics/issue_allowed.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-module Gitlab
-  module CycleAnalytics
-    module IssueAllowed
-      def allowed_ids
-        @allowed_ids ||= IssuesFinder.new(@options[:current_user], project_id: @project.id).execute.where(id: event_result_ids).pluck(:id)
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb
index 3df9cbdcfce..cc79e2dfe88 100644
--- a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb
+++ b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb
@@ -1,8 +1,6 @@
 module Gitlab
   module CycleAnalytics
     class IssueEventFetcher < BaseEventFetcher
-      include IssueAllowed
-
       def initialize(*args)
         @projections = [issue_table[:title],
                         issue_table[:iid],
@@ -18,6 +16,14 @@ module Gitlab
       def serialize(event)
         AnalyticsIssueSerializer.new(project: @project).represent(event)
       end
+
+      def allowed_ids
+        load_allowed_ids
+      end
+
+      def allowed_ids_finder_class
+        IssuesFinder
+      end
     end
   end
 end
diff --git a/lib/gitlab/cycle_analytics/merge_request_allowed.rb b/lib/gitlab/cycle_analytics/merge_request_allowed.rb
deleted file mode 100644
index 28f6db44759..00000000000
--- a/lib/gitlab/cycle_analytics/merge_request_allowed.rb
+++ /dev/null
@@ -1,9 +0,0 @@
-module Gitlab
-  module CycleAnalytics
-    module MergeRequestAllowed
-      def allowed_ids
-        @allowed_ids ||= MergeRequestsFinder.new(@options[:current_user], project_id: @project.id).execute.where(id: event_result_ids).pluck(:id)
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/cycle_analytics/review_event_fetcher.rb b/lib/gitlab/cycle_analytics/review_event_fetcher.rb
index 4c7b3f4467f..5a7f1eb00b3 100644
--- a/lib/gitlab/cycle_analytics/review_event_fetcher.rb
+++ b/lib/gitlab/cycle_analytics/review_event_fetcher.rb
@@ -1,8 +1,6 @@
 module Gitlab
   module CycleAnalytics
     class ReviewEventFetcher < BaseEventFetcher
-      include MergeRequestAllowed
-
       def initialize(*args)
         @projections = [mr_table[:title],
                         mr_table[:iid],
@@ -14,9 +12,19 @@ module Gitlab
         super(*args)
       end
 
+      private
+
       def serialize(event)
         AnalyticsMergeRequestSerializer.new(project: @project).represent(event)
       end
+
+      def allowed_ids
+        load_allowed_ids
+      end
+
+      def allowed_ids_finder_class
+        MergeRequestsFinder
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/popen.rb b/lib/gitlab/git/popen.rb
index df9ca3ee5ac..cd86c39ef73 100644
--- a/lib/gitlab/git/popen.rb
+++ b/lib/gitlab/git/popen.rb
@@ -11,15 +11,15 @@ module Gitlab
         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|
-          @cmd_output << stdout.read
-          @cmd_output << stderr.read
-          @cmd_status = wait_thr.value.exitstatus
+          cmd_output << stdout.read
+          cmd_output << stderr.read
+          cmd_status = wait_thr.value.exitstatus
         end
 
-        [@cmd_output, @cmd_status]
+        [cmd_output, cmd_status]
       end
     end
   end
diff --git a/lib/gitlab/performance_bar/peek_performance_bar_with_rack_body.rb b/lib/gitlab/performance_bar/peek_performance_bar_with_rack_body.rb
index d939a6ea18d..464dea44cc2 100644
--- a/lib/gitlab/performance_bar/peek_performance_bar_with_rack_body.rb
+++ b/lib/gitlab/performance_bar/peek_performance_bar_with_rack_body.rb
@@ -1,5 +1,6 @@
 # This solves a bug with a X-Senfile header that wouldn't be set properly, see
 # https://github.com/peek/peek-performance_bar/pull/27
+# rubocop:disable Cop/ModuleWithInstanceVariables
 module Gitlab
   module PerformanceBar
     module PeekPerformanceBarWithRackBody
diff --git a/rubocop/cop/module_with_instance_variables.rb b/rubocop/cop/module_with_instance_variables.rb
new file mode 100644
index 00000000000..13b6d6cfc93
--- /dev/null
+++ b/rubocop/cop/module_with_instance_variables.rb
@@ -0,0 +1,33 @@
+module RuboCop
+  module Cop
+    class ModuleWithInstanceVariables < RuboCop::Cop::Cop
+      MSG = 'Do not use instance variables in a module'.freeze
+
+      def on_module(node)
+        return if rails_helper?(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 rails_helper?(node)
+        node.source_range.source_buffer.name =~
+          %r{app/helpers/\w+_helper.rb\z}
+      end
+
+      def check_method_definition(node)
+        node.each_child_node(:def) do |definition|
+          definition.each_descendant(:ivar, :ivasgn) do |offense|
+            add_offense(offense, :expression)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index f76144275c9..eb6af0b143c 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -6,6 +6,7 @@ require_relative 'cop/polymorphic_associations'
 require_relative 'cop/project_path_helper'
 require_relative 'cop/active_record_dependent'
 require_relative 'cop/in_batches'
+require_relative 'cop/module_with_instance_variables'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_column_with_default_to_large_table'
 require_relative 'cop/migration/add_concurrent_foreign_key'