Merge branch 'ban-catch-throw' into 'master'

Rubocop to ban catch/throw in ruby code

See merge request gitlab-org/gitlab!25990

.rubocop.yml

@@ -344,3 +344,6 @@ Style/MultilineWhenThen:
 
 Style/FloatDivision:
   Enabled: false
+
+Cop/BanCatchThrow:
+  Enabled: true

app/models/list.rb

@@ -97,6 +97,6 @@ class List < ApplicationRecord
   private
 
   def can_be_destroyed
-    throw(:abort) unless destroyable?
+    throw(:abort) unless destroyable? # rubocop:disable Cop/BanCatchThrow
   end
 end

app/models/project.rb

@@ -2430,7 +2430,7 @@ class Project < ApplicationRecord
 
     if repository_storage.blank? || repository_with_same_path_already_exists?
       errors.add(:base, _('There is already a repository with that name on disk'))
-      throw :abort
+      throw :abort # rubocop:disable Cop/BanCatchThrow
     end
   end
 

app/services/metrics/dashboard/clone_dashboard_service.rb

@@ -22,6 +22,7 @@ module Metrics
         end
       end
 
+      # rubocop:disable Cop/BanCatchThrow
       def execute
         catch(:error) do
           throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized?
@@ -33,6 +34,7 @@ module Metrics
           success(result.merge(http_status: :created, dashboard: dashboard_details))
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
       private
 
@@ -60,6 +62,7 @@ module Metrics
         Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
       end
 
+      # rubocop:disable Cop/BanCatchThrow
       def dashboard_template
         @dashboard_template ||= begin
           throw(:error, error(_('Not found.'), :not_found)) unless self.class.allowed_dashboard_templates.include?(params[:dashboard])
@@ -67,7 +70,9 @@ module Metrics
           params[:dashboard]
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
+      # rubocop:disable Cop/BanCatchThrow
       def branch
         @branch ||= begin
           throw(:error, error(_('There was an error creating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name?
@@ -76,6 +81,7 @@ module Metrics
           params[:branch]
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
       def new_or_default_branch?
         !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch]
@@ -89,6 +95,7 @@ module Metrics
         @new_dashboard_path ||= File.join(USER_DASHBOARDS_DIR, file_name)
       end
 
+      # rubocop:disable Cop/BanCatchThrow
       def file_name
         @file_name ||= begin
           throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid?
@@ -96,6 +103,7 @@ module Metrics
           File.basename(params[:file_name])
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
       def target_file_type_valid?
         File.extname(params[:file_name]) == ALLOWED_FILE_TYPE

app/services/metrics/dashboard/update_dashboard_service.rb

@@ -7,6 +7,7 @@ module Metrics
       ALLOWED_FILE_TYPE = '.yml'
       USER_DASHBOARDS_DIR = ::Metrics::Dashboard::ProjectDashboardService::DASHBOARD_ROOT
 
+      # rubocop:disable Cop/BanCatchThrow
       def execute
         catch(:error) do
           throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized?
@@ -17,6 +18,7 @@ module Metrics
           success(result.merge(http_status: :created, dashboard: dashboard_details))
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
       private
 
@@ -44,6 +46,7 @@ module Metrics
         Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
       end
 
+      # rubocop:disable Cop/BanCatchThrow
       def branch
         @branch ||= begin
           throw(:error, error(_('There was an error updating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name?
@@ -52,6 +55,7 @@ module Metrics
           params[:branch]
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
       def new_or_default_branch?
         !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch]
@@ -69,6 +73,7 @@ module Metrics
         File.extname(params[:file_name]) == ALLOWED_FILE_TYPE
       end
 
+      # rubocop:disable Cop/BanCatchThrow
       def file_name
         @file_name ||= begin
           throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid?
@@ -76,6 +81,7 @@ module Metrics
           File.basename(CGI.unescape(params[:file_name]))
         end
       end
+      # rubocop:enable Cop/BanCatchThrow
 
       def update_dashboard_content
         ::PerformanceMonitoring::PrometheusDashboard.from_json(params[:file_content]).to_yaml

app/services/test_hooks/base_service.rb

@@ -18,7 +18,7 @@ module TestHooks
         return error('Testing not available for this hook')
       end
 
-      error_message = catch(:validation_error) do
+      error_message = catch(:validation_error) do # rubocop:disable Cop/BanCatchThrow
         sample_data = self.__send__(trigger_data_method) # rubocop:disable GitlabSecurity/PublicSend
 
         return hook.execute(sample_data, trigger_key) # rubocop:disable Cop/AvoidReturnFromBlocks

app/services/test_hooks/project_service.rb

@@ -11,7 +11,7 @@ module TestHooks
     private
 
     def push_events_data
-      throw(:validation_error, s_('TestHooks|Ensure the project has at least one commit.')) if project.empty_repo?
+      throw(:validation_error, s_('TestHooks|Ensure the project has at least one commit.')) if project.empty_repo? # rubocop:disable Cop/BanCatchThrow
 
       Gitlab::DataBuilder::Push.build_sample(project, current_user)
     end
@@ -20,14 +20,14 @@ module TestHooks
 
     def note_events_data
       note = project.notes.first
-      throw(:validation_error, s_('TestHooks|Ensure the project has notes.')) unless note.present?
+      throw(:validation_error, s_('TestHooks|Ensure the project has notes.')) unless note.present? # rubocop:disable Cop/BanCatchThrow
 
       Gitlab::DataBuilder::Note.build(note, current_user)
     end
 
     def issues_events_data
       issue = project.issues.first
-      throw(:validation_error, s_('TestHooks|Ensure the project has issues.')) unless issue.present?
+      throw(:validation_error, s_('TestHooks|Ensure the project has issues.')) unless issue.present? # rubocop:disable Cop/BanCatchThrow
 
       issue.to_hook_data(current_user)
     end
@@ -36,21 +36,21 @@ module TestHooks
 
     def merge_requests_events_data
       merge_request = project.merge_requests.first
-      throw(:validation_error, s_('TestHooks|Ensure the project has merge requests.')) unless merge_request.present?
+      throw(:validation_error, s_('TestHooks|Ensure the project has merge requests.')) unless merge_request.present? # rubocop:disable Cop/BanCatchThrow
 
       merge_request.to_hook_data(current_user)
     end
 
     def job_events_data
       build = project.builds.first
-      throw(:validation_error, s_('TestHooks|Ensure the project has CI jobs.')) unless build.present?
+      throw(:validation_error, s_('TestHooks|Ensure the project has CI jobs.')) unless build.present? # rubocop:disable Cop/BanCatchThrow
 
       Gitlab::DataBuilder::Build.build(build)
     end
 
     def pipeline_events_data
       pipeline = project.ci_pipelines.first
-      throw(:validation_error, s_('TestHooks|Ensure the project has CI pipelines.')) unless pipeline.present?
+      throw(:validation_error, s_('TestHooks|Ensure the project has CI pipelines.')) unless pipeline.present? # rubocop:disable Cop/BanCatchThrow
 
       Gitlab::DataBuilder::Pipeline.build(pipeline)
     end
@@ -58,7 +58,7 @@ module TestHooks
     def wiki_page_events_data
       page = project.wiki.list_pages(limit: 1).first
       if !project.wiki_enabled? || page.blank?
-        throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.'))
+        throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.')) # rubocop:disable Cop/BanCatchThrow
       end
 
       Gitlab::DataBuilder::WikiPage.build(page, current_user, 'create')

app/services/test_hooks/system_service.rb

@@ -18,7 +18,7 @@ module TestHooks
 
     def merge_requests_events_data
       merge_request = MergeRequest.of_projects(current_user.projects.select(:id)).first
-      throw(:validation_error, s_('TestHooks|Ensure one of your projects has merge requests.')) unless merge_request.present?
+      throw(:validation_error, s_('TestHooks|Ensure one of your projects has merge requests.')) unless merge_request.present? # rubocop:disable Cop/BanCatchThrow
 
       merge_request.to_hook_data(current_user)
     end

lib/gitlab/request_profiler/middleware.rb

@@ -51,7 +51,7 @@ module Gitlab
       def call_with_call_stack_profiling(env)
         ret = nil
         report = RubyProf::Profile.profile do
-          ret = catch(:warden) do
+          ret = catch(:warden) do # rubocop:disable Cop/BanCatchThrow
             @app.call(env)
           end
         end
@@ -67,7 +67,7 @@ module Gitlab
       def call_with_memory_profiling(env)
         ret = nil
         report = MemoryProfiler.report do
-          ret = catch(:warden) do
+          ret = catch(:warden) do # rubocop:disable Cop/BanCatchThrow
             @app.call(env)
           end
         end
@@ -99,7 +99,7 @@ module Gitlab
         if ret.is_a?(Array)
           ret
         else
-          throw(:warden, ret)
+          throw(:warden, ret) # rubocop:disable Cop/BanCatchThrow
         end
       end
     end

rubocop/cop/ban_catch_throw.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Bans the use of 'catch/throw', as exceptions are better for errors and
+    # they are equivalent to 'goto' for flow control, with all the problems
+    # that implies.
+    #
+    # @example
+    #   # bad
+    #   catch(:error) do
+    #     throw(:error)
+    #   end
+    #
+    #   # good
+    #   begin
+    #     raise StandardError
+    #   rescue StandardError => err
+    #     # ...
+    #   end
+    #
+    class BanCatchThrow < RuboCop::Cop::Cop
+      MSG = "Do not use catch or throw unless a gem's API demands it."
+
+      def on_send(node)
+        receiver, method_name, _ = *node
+
+        return unless receiver.nil? && %i[catch throw].include?(method_name)
+
+        add_offense(node, location: :expression)
+      end
+    end
+  end
+end

rubocop/cop/gitlab/keys-first-and-values-first.rb

@@ -26,7 +26,7 @@ module RuboCop
                            elsif node.descendants.first.method_name == :keys
                              '.each_key'
                            else
-                             throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first")
+                             throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first") # rubocop:disable Cop/BanCatchThrow
                            end
 
             upto_including_keys_or_values = node.descendants.first.source_range

spec/rubocop/cop/ban_catch_throw_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../rubocop/cop/ban_catch_throw'
+
+describe RuboCop::Cop::BanCatchThrow do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense when `catch` or `throw` are used' do
+    inspect_source("catch(:foo) {\n  throw(:foo)\n}")
+
+    aggregate_failures do
+      expect(cop.offenses.size).to eq(2)
+      expect(cop.offenses.map(&:line)).to eq([1, 2])
+      expect(cop.highlights).to eq(['catch(:foo)', 'throw(:foo)'])
+    end
+  end
+
+  it 'does not register an offense for a method called catch or throw' do
+    inspect_source("foo.catch(:foo) {\n  foo.throw(:foo)\n}")
+
+    expect(cop.offenses).to be_empty
+  end
+end