Merge branch '42889-avoid-return-inside-block' into 'master'

Resolve "Make a Rubocop that forbids returning from a block"

Closes #42889

See merge request gitlab-org/gitlab-ce!18000

app/controllers/concerns/notes_actions.rb

@@ -217,7 +217,7 @@ module NotesActions
 
   def note_project
     strong_memoize(:note_project) do
-      return nil unless project
+      next nil unless project
 
       note_project_id = params[:note_project_id]
 
@@ -228,7 +228,7 @@ module NotesActions
           project
         end
 
-      return access_denied! unless can?(current_user, :create_note, the_project)
+      next access_denied! unless can?(current_user, :create_note, the_project)
 
       the_project
     end

app/controllers/groups/variables_controller.rb

@@ -15,7 +15,7 @@ module Groups
     def update
       if @group.update(group_variables_params)
         respond_to do |format|
-          format.json { return render_group_variables }
+          format.json { render_group_variables }
         end
       else
         respond_to do |format|

app/controllers/projects/merge_requests_controller.rb

@@ -60,13 +60,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       end
 
       format.patch  do
-        return render_404 unless @merge_request.diff_refs
+        break render_404 unless @merge_request.diff_refs
 
         send_git_patch @project.repository, @merge_request.diff_refs
       end
 
       format.diff do
-        return render_404 unless @merge_request.diff_refs
+        break render_404 unless @merge_request.diff_refs
 
         send_git_diff @project.repository, @merge_request.diff_refs
       end

app/controllers/projects/variables_controller.rb

@@ -12,7 +12,7 @@ class Projects::VariablesController < Projects::ApplicationController
   def update
     if @project.update(variables_params)
       respond_to do |format|
-        format.json { return render_variables }
+        format.json { render_variables }
       end
     else
       respond_to do |format|

app/models/ci/build.rb

@@ -479,7 +479,7 @@ module Ci
 
     def user_variables
       Gitlab::Ci::Variables::Collection.new.tap do |variables|
-        return variables if user.blank?
+        break variables if user.blank?
 
         variables.append(key: 'GITLAB_USER_ID', value: user.id.to_s)
         variables.append(key: 'GITLAB_USER_EMAIL', value: user.email)
@@ -594,7 +594,7 @@ module Ci
 
     def persisted_variables
       Gitlab::Ci::Variables::Collection.new.tap do |variables|
-        return variables unless persisted?
+        break variables unless persisted?
 
         variables
           .append(key: 'CI_JOB_ID', value: id.to_s)
@@ -643,7 +643,7 @@ module Ci
 
     def persisted_environment_variables
       Gitlab::Ci::Variables::Collection.new.tap do |variables|
-        return variables unless persisted? && persisted_environment.present?
+        break variables unless persisted? && persisted_environment.present?
 
         variables.concat(persisted_environment.predefined_variables)
 

app/models/notification_recipient.rb

@@ -83,14 +83,14 @@ class NotificationRecipient
 
   def has_access?
     DeclarativePolicy.subject_scope do
-      return false unless user.can?(:receive_notifications)
-      return true if @skip_read_ability
+      break false unless user.can?(:receive_notifications)
+      break true if @skip_read_ability
 
-      return false if @target && !user.can?(:read_cross_project)
-      return false if @project && !user.can?(:read_project, @project)
+      break false if @target && !user.can?(:read_cross_project)
+      break false if @project && !user.can?(:read_project, @project)
 
-      return true unless read_ability
-      return true unless DeclarativePolicy.has_policy?(@target)
+      break true unless read_ability
+      break true unless DeclarativePolicy.has_policy?(@target)
 
       user.can?(read_ability, @target)
     end

app/models/project.rb

@@ -1637,7 +1637,7 @@ class Project < ActiveRecord::Base
 
   def container_registry_variables
     Gitlab::Ci::Variables::Collection.new.tap do |variables|
-      return variables unless Gitlab.config.registry.enabled
+      break variables unless Gitlab.config.registry.enabled
 
       variables.append(key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port)
 

app/services/ci/register_job_service.rb

@@ -44,7 +44,7 @@ module Ci
             build.run!
             register_success(build)
 
-            return Result.new(build, true)
+            return Result.new(build, true) # rubocop:disable Cop/AvoidReturnFromBlocks
           rescue Ci::Build::MissingDependenciesError
             build.drop!(:missing_dependency_failure)
           end

app/services/clusters/gcp/verify_provision_status_service.rb

@@ -17,7 +17,7 @@ module Clusters
           when 'DONE'
             finalize_creation
           else
-            return provider.make_errored!("Unexpected operation status; #{operation.status} #{operation.status_message}")
+            provider.make_errored!("Unexpected operation status; #{operation.status} #{operation.status_message}")
           end
         end
       end

app/services/create_deployment_service.rb

@@ -19,8 +19,8 @@ class CreateDeploymentService
 
       environment.fire_state_event(action)
 
-      return unless environment.save
-      return if environment.stopped?
+      break unless environment.save
+      break if environment.stopped?
 
       deploy.tap(&:update_merge_request_metrics!)
     end

app/services/import_export_clean_up_service.rb

@@ -10,7 +10,7 @@ class ImportExportCleanUpService
 
   def execute
     Gitlab::Metrics.measure(:import_export_clean_up) do
-      return unless File.directory?(path)
+      next unless File.directory?(path)
 
       clean_up_export_files
     end

app/services/projects/destroy_service.rb

@@ -137,7 +137,7 @@ module Projects
       return true unless Gitlab.config.registry.enabled
 
       ContainerRepository.build_root_repository(project).tap do |repository|
-        return repository.has_tags? ? repository.delete_tags! : true
+        break repository.has_tags? ? repository.delete_tags! : true
       end
     end
 

app/services/repository_archive_clean_up_service.rb

@@ -10,7 +10,7 @@ class RepositoryArchiveCleanUpService
 
   def execute
     Gitlab::Metrics.measure(:repository_archive_clean_up) do
-      return unless File.directory?(path)
+      next unless File.directory?(path)
 
       clean_up_old_archives
       clean_up_empty_directories

app/services/test_hooks/base_service.rb

@@ -19,7 +19,7 @@ module TestHooks
       error_message = catch(:validation_error) do
         sample_data = self.__send__(trigger_data_method) # rubocop:disable GitlabSecurity/PublicSend
 
-        return hook.execute(sample_data, trigger_key)
+        return hook.execute(sample_data, trigger_key) # rubocop:disable Cop/AvoidReturnFromBlocks
       end
 
       error(error_message)

app/workers/post_receive.rb

@@ -33,7 +33,7 @@ class PostReceive
 
       unless @user
         log("Triggered hook for non-existing user \"#{post_received.identifier}\"")
-        return false
+        return false # rubocop:disable Cop/AvoidReturnFromBlocks
       end
 
       if Gitlab::Git.tag_ref?(ref)

app/workers/stuck_ci_jobs_worker.rb

@@ -38,7 +38,7 @@ class StuckCiJobsWorker
 
   def drop_stuck(status, timeout)
     search(status, timeout) do |build|
-      return unless build.stuck?
+      break unless build.stuck?
 
       drop_build :stuck, build, status, timeout
     end

changelogs/unreleased/42889-avoid-return-inside-block.yml

+---
+title: Rubocop rule to avoid returning from a block
+merge_request: 18000
+author: Jacopo Beschi @jacopo-beschi
+type: added

lib/api/discussions.rb

@@ -25,7 +25,7 @@ module API
         get ":id/#{noteables_str}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          return not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
+          break not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
 
           notes = noteable.notes
             .inc_relations_for_view
@@ -50,7 +50,7 @@ module API
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
           if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
-            return not_found!("Discussion")
+            break not_found!("Discussion")
           end
 
           discussion = Discussion.build(notes, noteable)
@@ -98,7 +98,7 @@ module API
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
           if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
-            return not_found!("Notes")
+            break not_found!("Notes")
           end
 
           present notes, with: Entities::Note
@@ -117,8 +117,8 @@ module API
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          return not_found!("Discussion") if notes.empty?
-          return bad_request!("Discussion is an individual note.") unless notes.first.part_of_discussion?
+          break not_found!("Discussion") if notes.empty?
+          break bad_request!("Discussion is an individual note.") unless notes.first.part_of_discussion?
 
           opts = {
             note: params[:body],

lib/api/group_variables.rb

@@ -31,7 +31,7 @@ module API
         key = params[:key]
         variable = user_group.variables.find_by(key: key)
 
-        return not_found!('GroupVariable') unless variable
+        break not_found!('GroupVariable') unless variable
 
         present variable, with: Entities::Variable
       end
@@ -67,7 +67,7 @@ module API
       put ':id/variables/:key' do
         variable = user_group.variables.find_by(key: params[:key])
 
-        return not_found!('GroupVariable') unless variable
+        break not_found!('GroupVariable') unless variable
 
         variable_params = declared_params(include_missing: false).except(:key)
 

lib/api/internal.rb

@@ -50,7 +50,7 @@ module API
           access_checker.check(params[:action], params[:changes])
           @project ||= access_checker.project
         rescue Gitlab::GitAccess::UnauthorizedError, Gitlab::GitAccess::NotFoundError => e
-          return { status: false, message: e.message }
+          break { status: false, message: e.message }
         end
 
         log_user_activity(actor)
@@ -142,21 +142,21 @@ module API
         if key
           key.update_last_used_at
         else
-          return { 'success' => false, 'message' => 'Could not find the given key' }
+          break { 'success' => false, 'message' => 'Could not find the given key' }
         end
 
         if key.is_a?(DeployKey)
-          return { success: false, message: 'Deploy keys cannot be used to retrieve recovery codes' }
+          break { success: false, message: 'Deploy keys cannot be used to retrieve recovery codes' }
         end
 
         user = key.user
 
         unless user
-          return { success: false, message: 'Could not find a user for the given key' }
+          break { success: false, message: 'Could not find a user for the given key' }
         end
 
         unless user.two_factor_enabled?
-          return { success: false, message: 'Two-factor authentication is not enabled for this user' }
+          break { success: false, message: 'Two-factor authentication is not enabled for this user' }
         end
 
         codes = nil

lib/api/issues.rb

@@ -310,7 +310,7 @@ module API
 
         issue = find_project_issue(params[:issue_iid])
 
-        return not_found!('UserAgentDetail') unless issue.user_agent_detail
+        break not_found!('UserAgentDetail') unless issue.user_agent_detail
 
         present issue.user_agent_detail, with: Entities::UserAgentDetail
       end

lib/api/job_artifacts.rb

@@ -77,7 +77,7 @@ module API
 
         build = find_build!(params[:job_id])
         authorize!(:update_build, build)
-        return not_found!(build) unless build.artifacts?
+        break not_found!(build) unless build.artifacts?
 
         build.keep_artifacts!
 

lib/api/jobs.rb

@@ -120,7 +120,7 @@ module API
 
         build = find_build!(params[:job_id])
         authorize!(:update_build, build)
-        return forbidden!('Job is not retryable') unless build.retryable?
+        break forbidden!('Job is not retryable') unless build.retryable?
 
         build = Ci::Build.retry(build, current_user)
 
@@ -138,7 +138,7 @@ module API
 
         build = find_build!(params[:job_id])
         authorize!(:erase_build, build)
-        return forbidden!('Job is not erasable!') unless build.erasable?
+        break forbidden!('Job is not erasable!') unless build.erasable?
 
         build.erase(erased_by: current_user)
         present build, with: Entities::Job

lib/api/project_snippets.rb

@@ -145,7 +145,7 @@ module API
 
         snippet = Snippet.find_by!(id: params[:snippet_id], project_id: params[:id])
 
-        return not_found!('UserAgentDetail') unless snippet.user_agent_detail
+        break not_found!('UserAgentDetail') unless snippet.user_agent_detail
 
         present snippet.user_agent_detail, with: Entities::UserAgentDetail
       end

lib/api/projects.rb

@@ -402,7 +402,7 @@ module API
         end
 
         unless user_project.allowed_to_share_with_group?
-          return render_api_error!("The project sharing with group is disabled", 400)
+          break render_api_error!("The project sharing with group is disabled", 400)
         end
 
         link = user_project.project_group_links.new(declared_params(include_missing: false))

lib/api/runner.rb

@@ -29,7 +29,7 @@ module API
             project.runners.create(attributes)
           end
 
-        return forbidden! unless runner
+        break forbidden! unless runner
 
         if runner.id
           present runner, with: Entities::RunnerRegistrationDetails
@@ -83,7 +83,7 @@ module API
         if current_runner.runner_queue_value_latest?(params[:last_update])
           header 'X-GitLab-Last-Update', params[:last_update]
           Gitlab::Metrics.add_event(:build_not_found_cached)
-          return no_content!
+          break no_content!
         end
 
         new_update = current_runner.ensure_runner_queue_value
@@ -152,7 +152,7 @@ module API
 
         stream_size = job.trace.append(request.body.read, content_range[0].to_i)
         if stream_size < 0
-          return error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{-stream_size}" })
+          break error!('416 Range Not Satisfiable', 416, { 'Range' => "0-#{-stream_size}" })
         end
 
         status 202

lib/api/snippets.rb

@@ -94,7 +94,7 @@ module API
       end
       put ':id' do
         snippet = snippets_for_current_user.find_by(id: params.delete(:id))
-        return not_found!('Snippet') unless snippet
+        break not_found!('Snippet') unless snippet
 
         authorize! :update_personal_snippet, snippet
 
@@ -120,7 +120,7 @@ module API
       end
       delete ':id' do
         snippet = snippets_for_current_user.find_by(id: params.delete(:id))
-        return not_found!('Snippet') unless snippet
+        break not_found!('Snippet') unless snippet
 
         authorize! :destroy_personal_snippet, snippet
 
@@ -135,7 +135,7 @@ module API
       end
       get ":id/raw" do
         snippet = snippets_for_current_user.find_by(id: params.delete(:id))
-        return not_found!('Snippet') unless snippet
+        break not_found!('Snippet') unless snippet
 
         env['api.format'] = :txt
         content_type 'text/plain'
@@ -153,7 +153,7 @@ module API
 
         snippet = Snippet.find_by!(id: params[:id])
 
-        return not_found!('UserAgentDetail') unless snippet.user_agent_detail
+        break not_found!('UserAgentDetail') unless snippet.user_agent_detail
 
         present snippet.user_agent_detail, with: Entities::UserAgentDetail
       end

lib/api/triggers.rb

@@ -62,7 +62,7 @@ module API
         authorize! :admin_build, user_project
 
         trigger = user_project.triggers.find(params.delete(:trigger_id))
-        return not_found!('Trigger') unless trigger
+        break not_found!('Trigger') unless trigger
 
         present trigger, with: Entities::Trigger
       end
@@ -99,7 +99,7 @@ module API
         authorize! :admin_build, user_project
 
         trigger = user_project.triggers.find(params.delete(:trigger_id))
-        return not_found!('Trigger') unless trigger
+        break not_found!('Trigger') unless trigger
 
         if trigger.update(declared_params(include_missing: false))
           present trigger, with: Entities::Trigger
@@ -119,7 +119,7 @@ module API
         authorize! :admin_build, user_project
 
         trigger = user_project.triggers.find(params.delete(:trigger_id))
-        return not_found!('Trigger') unless trigger
+        break not_found!('Trigger') unless trigger
 
         if trigger.update(owner: current_user)
           status :ok
@@ -140,7 +140,7 @@ module API
         authorize! :admin_build, user_project
 
         trigger = user_project.triggers.find(params.delete(:trigger_id))
-        return not_found!('Trigger') unless trigger
+        break not_found!('Trigger') unless trigger
 
         destroy_conditionally!(trigger)
       end

lib/api/v3/builds.rb

@@ -51,7 +51,7 @@ module API
         get ':id/repository/commits/:sha/builds' do
           authorize_read_builds!
 
-          return not_found! unless user_project.commit(params[:sha])
+          break not_found! unless user_project.commit(params[:sha])
 
           pipelines = user_project.pipelines.where(sha: params[:sha])
           builds = user_project.builds.where(pipeline: pipelines).order('id DESC')
@@ -153,7 +153,7 @@ module API
 
           build = get_build!(params[:build_id])
           authorize!(:update_build, build)
-          return forbidden!('Build is not retryable') unless build.retryable?
+          break forbidden!('Build is not retryable') unless build.retryable?
 
           build = Ci::Build.retry(build, current_user)
 
@@ -171,7 +171,7 @@ module API
 
           build = get_build!(params[:build_id])
           authorize!(:erase_build, build)
-          return forbidden!('Build is not erasable!') unless build.erasable?
+          break forbidden!('Build is not erasable!') unless build.erasable?
 
           build.erase(erased_by: current_user)
           present build, with: ::API::V3::Entities::Build
@@ -188,7 +188,7 @@ module API
 
           build = get_build!(params[:build_id])
           authorize!(:update_build, build)
-          return not_found!(build) unless build.artifacts?
+          break not_found!(build) unless build.artifacts?
 
           build.keep_artifacts!
 

lib/api/v3/projects.rb

@@ -423,7 +423,7 @@ module API
           end
 
           unless user_project.allowed_to_share_with_group?
-            return render_api_error!("The project sharing with group is disabled", 400)
+            break render_api_error!("The project sharing with group is disabled", 400)
           end
 
           link = user_project.project_group_links.new(declared_params(include_missing: false))

lib/api/v3/snippets.rb

@@ -90,7 +90,7 @@ module API
         end
         put ':id' do
           snippet = snippets_for_current_user.find_by(id: params.delete(:id))
-          return not_found!('Snippet') unless snippet
+          break not_found!('Snippet') unless snippet
 
           authorize! :update_personal_snippet, snippet
 
@@ -114,7 +114,7 @@ module API
         end
         delete ':id' do
           snippet = snippets_for_current_user.find_by(id: params.delete(:id))
-          return not_found!('Snippet') unless snippet
+          break not_found!('Snippet') unless snippet
 
           authorize! :destroy_personal_snippet, snippet
           snippet.destroy
@@ -129,7 +129,7 @@ module API
         end
         get ":id/raw" do
           snippet = snippets_for_current_user.find_by(id: params.delete(:id))
-          return not_found!('Snippet') unless snippet
+          break not_found!('Snippet') unless snippet
 
           env['api.format'] = :txt
           content_type 'text/plain'

lib/api/v3/triggers.rb

@@ -72,7 +72,7 @@ module API
           authorize! :admin_build, user_project
 
           trigger = user_project.triggers.find_by(token: params[:token].to_s)
-          return not_found!('Trigger') unless trigger
+          break not_found!('Trigger') unless trigger
 
           present trigger, with: ::API::V3::Entities::Trigger
         end
@@ -100,7 +100,7 @@ module API
           authorize! :admin_build, user_project
 
           trigger = user_project.triggers.find_by(token: params[:token].to_s)
-          return not_found!('Trigger') unless trigger
+          break not_found!('Trigger') unless trigger
 
           trigger.destroy
 

lib/api/variables.rb

@@ -31,7 +31,7 @@ module API
         key = params[:key]
         variable = user_project.variables.find_by(key: key)
 
-        return not_found!('Variable') unless variable
+        break not_found!('Variable') unless variable
 
         present variable, with: Entities::Variable
       end
@@ -67,7 +67,7 @@ module API
       put ':id/variables/:key' do
         variable = user_project.variables.find_by(key: params[:key])
 
-        return not_found!('Variable') unless variable
+        break not_found!('Variable') unless variable
 
         variable_params = declared_params(include_missing: false).except(:key)
 

lib/declarative_policy/runner.rb

@@ -77,7 +77,7 @@ module DeclarativePolicy
       @state = State.new
 
       steps_by_score do |step, score|
-        return if !debug && @state.prevented?
+        break if !debug && @state.prevented?
 
         passed = nil
         case step.action

lib/gitlab/auth.rb

@@ -51,7 +51,7 @@ module Gitlab
         Gitlab::Auth::UniqueIpsLimiter.limit_user! do
           user = User.by_login(login)
 
-          return if user && !user.active?
+          break if user && !user.active?
 
           authenticators = []
 

lib/gitlab/ci/trace.rb

@@ -45,7 +45,7 @@ module Gitlab
       def append(data, offset)
         write do |stream|
           current_length = stream.size
-          return -current_length unless current_length == offset
+          break -current_length unless current_length == offset
 
           data = job.hide_secrets(data)
           stream.append(data, offset)

lib/gitlab/ci/trace/stream.rb

@@ -87,7 +87,7 @@ module Gitlab
 
             match = matches.flatten.last
             coverage = match.gsub(/\d+(\.\d+)?/).first
-            return coverage if coverage.present?
+            return coverage if coverage.present? # rubocop:disable Cop/AvoidReturnFromBlocks
           end
 
           nil

lib/gitlab/daemon.rb

@@ -30,7 +30,7 @@ module Gitlab
       return unless enabled?
 
       @mutex.synchronize do
-        return thread if thread?
+        break thread if thread?
 
         @thread = Thread.new { start_working }
       end
@@ -38,7 +38,7 @@ module Gitlab
 
     def stop
       @mutex.synchronize do
-        return unless thread?
+        break unless thread?
 
         stop_working
 

lib/gitlab/gfm/uploads_rewriter.rb

@@ -21,7 +21,7 @@ module Gitlab
 
         @text.gsub(@pattern) do |markdown|
           file = find_file(@source_project, $~[:secret], $~[:file])
-          return markdown unless file.try(:exists?)
+          break markdown unless file.try(:exists?)
 
           new_uploader = FileUploader.new(target_project)
           with_link_in_tmp_dir(file.file) do |open_tmp_file|

lib/gitlab/git/diff.rb

@@ -249,7 +249,7 @@ module Gitlab
 
             if size >= SIZE_LIMIT
               too_large!
-              return true
+              return true # rubocop:disable Cop/AvoidReturnFromBlocks
             end
           end
         end

lib/gitlab/git/popen.rb

@@ -25,7 +25,9 @@ module Gitlab
           stdin.close
 
           if lazy_block
-            return [lazy_block.call(stdout.lazy), 0]
+            cmd_output = lazy_block.call(stdout.lazy)
+            cmd_status = 0
+            break
           else
             cmd_output << stdout.read
           end

lib/gitlab/git/repository_mirroring.rb

@@ -26,7 +26,7 @@ module Gitlab
           # When the remote repo does not have tags.
           if target.nil? || path.nil?
             Rails.logger.info "Empty or invalid list of tags for remote: #{remote}. Output: #{output}"
-            return []
+            break []
           end
 
           name = path.split('/', 3).last

lib/gitlab/optimistic_locking.rb

@@ -3,18 +3,15 @@ module Gitlab
     module_function
 
     def retry_lock(subject, retries = 100, &block)
-      loop do
-        begin
-          ActiveRecord::Base.transaction do
-            return yield(subject)
-          end
-        rescue ActiveRecord::StaleObjectError
-          retries -= 1
-          raise unless retries >= 0
-
-          subject.reload
-        end
+      ActiveRecord::Base.transaction do
+        yield(subject)
       end
+    rescue ActiveRecord::StaleObjectError
+      retries -= 1
+      raise unless retries >= 0
+
+      subject.reload
+      retry
     end
 
     alias_method :retry_optimistic_lock, :retry_lock

lib/gitlab/shell.rb

@@ -340,7 +340,7 @@ module Gitlab
         if enabled
           gitaly_namespace_client(storage).rename(old_name, new_name)
         else
-          return false if exists?(storage, new_name) || !exists?(storage, old_name)
+          break false if exists?(storage, new_name) || !exists?(storage, old_name)
 
           FileUtils.mv(full_path(storage, old_name), full_path(storage, new_name))
         end

lib/gitlab/sidekiq_middleware/shutdown.rb

@@ -25,7 +25,7 @@ module Gitlab
       # can be only one shutdown thread in the process.
       def self.create_shutdown_thread
         mu_synchronize do
-          return unless @shutdown_thread.nil?
+          break unless @shutdown_thread.nil?
 
           @shutdown_thread = Thread.new { yield }
         end

lib/tasks/gitlab/storage.rake

@@ -111,7 +111,7 @@ namespace :gitlab do
 
           puts "  - #{project.full_path} (id: #{project.id})".color(:red)
 
-          return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator
+          return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator, Cop/AvoidReturnFromBlocks
         end
       end
     end
@@ -132,7 +132,7 @@ namespace :gitlab do
 
           puts "  - #{upload.path} (id: #{upload.id})".color(:red)
 
-          return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator
+          return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator, Cop/AvoidReturnFromBlocks
         end
       end
     end

qa/qa/factory/base.rb

@@ -22,7 +22,7 @@ module QA
 
           factory.fabricate!(*args)
 
-          return Factory::Product.populate!(factory)
+          break Factory::Product.populate!(factory)
         end
       end
 

qa/qa/page/group/show.rb

@@ -29,7 +29,7 @@ module QA
           filter_by_name(name)
 
           wait(reload: false) do
-            return false if page.has_content?('Sorry, no groups or projects matched your search')
+            break false if page.has_content?('Sorry, no groups or projects matched your search')
 
             page.has_link?(name)
           end

qa/qa/page/project/pipeline/show.rb

@@ -20,14 +20,14 @@ module QA::Page
 
       def running?
         within('.ci-header-container') do
-          return page.has_content?('running')
+          page.has_content?('running')
         end
       end
 
       def has_build?(name, status: :success)
         within('.pipeline-graph') do
           within('.ci-job-component', text: name) do
-            return has_selector?(".ci-status-icon-#{status}")
+            has_selector?(".ci-status-icon-#{status}")
           end
         end
       end

qa/qa/scenario/template.rb

@@ -4,7 +4,7 @@ module QA
       def self.perform(*args)
         new.tap do |scenario|
           yield scenario if block_given?
-          return scenario.perform(*args)
+          break scenario.perform(*args)
         end
       end
 

rubocop/cop/avoid_break_from_strong_memoize.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Checks for break inside strong_memoize blocks.
+    # For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/42889
+    #
+    # @example
+    #   # bad
+    #   strong_memoize(:result) do
+    #     break if something
+    #
+    #     do_an_heavy_calculation
+    #   end
+    #
+    #   # good
+    #   strong_memoize(:result) do
+    #     next if something
+    #
+    #     do_an_heavy_calculation
+    #   end
+    #
+    class AvoidBreakFromStrongMemoize < RuboCop::Cop::Cop
+      MSG = 'Do not use break inside strong_memoize, use next instead.'
+
+      def on_block(node)
+        block_body = node.body
+
+        return unless block_body
+        return unless node.method_name == :strong_memoize
+
+        block_body.each_node(:break) do |break_node|
+          next if container_block_for(break_node) != node
+
+          add_offense(break_node)
+        end
+      end
+
+      private
+
+      def container_block_for(current_node)
+        current_node = current_node.parent until current_node.type == :block && current_node.method_name == :strong_memoize
+
+        current_node
+      end
+    end
+  end
+end

rubocop/cop/avoid_return_from_blocks.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Checks for return inside blocks.
+    # For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/42889
+    #
+    # @example
+    #   # bad
+    #   call do
+    #     return if something
+    #
+    #     do_something_else
+    #   end
+    #
+    #   # good
+    #   call do
+    #     break if something
+    #
+    #     do_something_else
+    #   end
+    #
+    class AvoidReturnFromBlocks < RuboCop::Cop::Cop
+      MSG = 'Do not return from a block, use next or break instead.'
+      DEF_METHODS = %i[define_method lambda].freeze
+      WHITELISTED_METHODS = %i[each each_filename times loop].freeze
+
+      def on_block(node)
+        block_body = node.body
+
+        return unless block_body
+        return unless top_block?(node)
+
+        block_body.each_node(:return) do |return_node|
+          next if parent_blocks(node, return_node).all?(&method(:whitelisted?))
+
+          add_offense(return_node)
+        end
+      end
+
+      private
+
+      def top_block?(node)
+        current_node = node
+        top_block = nil
+
+        while current_node && current_node.type != :def
+          top_block = current_node if current_node.type == :block
+          current_node = current_node.parent
+        end
+
+        top_block == node
+      end
+
+      def parent_blocks(node, current_node)
+        blocks = []
+
+        until node == current_node || def?(current_node)
+          blocks << current_node if current_node.type == :block
+          current_node = current_node.parent
+        end
+
+        blocks << node if node == current_node && !def?(node)
+        blocks
+      end
+
+      def def?(node)
+        node.type == :def || node.type == :defs ||
+          (node.type == :block && DEF_METHODS.include?(node.method_name))
+      end
+
+      def whitelisted?(block_node)
+        WHITELISTED_METHODS.include?(block_node.method_name)
+      end
+    end
+  end
+end

rubocop/cop/migration/safer_boolean_column.rb

@@ -61,7 +61,7 @@ module RuboCop
           return true unless opts
 
           each_hash_node_pair(opts) do |key, value|
-            return value == 'nil' if key == :default
+            break value == 'nil' if key == :default
           end
         end
 
@@ -69,7 +69,7 @@ module RuboCop
           return true unless opts
 
           each_hash_node_pair(opts) do |key, value|
-            return value != 'false' if key == :null
+            break value != 'false' if key == :null
           end
         end
 

rubocop/rubocop.rb

@@ -4,6 +4,8 @@ require_relative 'cop/gitlab/httparty'
 require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/gitlab/predicate_memoization'
 require_relative 'cop/include_sidekiq_worker'
+require_relative 'cop/avoid_return_from_blocks'
+require_relative 'cop/avoid_break_from_strong_memoize'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_concurrent_foreign_key'

spec/lib/gitlab/ci/trace_spec.rb

@@ -458,7 +458,7 @@ describe Gitlab::Ci::Trace do
     context 'when job does not have trace artifact' do
       context 'when trace file stored in default path' do
         let!(:build) { create(:ci_build, :success, :trace_live) }
-        let!(:src_path) { trace.read { |s| return s.path } }
+        let!(:src_path) { trace.read { |s| s.path } }
         let!(:src_checksum) { Digest::SHA256.file(src_path).hexdigest }
 
         it_behaves_like 'archive trace file'

spec/lib/gitlab/shell_spec.rb

@@ -727,7 +727,7 @@ describe Gitlab::Shell do
 
   def find_in_authorized_keys_file(key_id)
     gitlab_shell.batch_read_key_ids do |ids|
-      return true if ids.include?(key_id)
+      return true if ids.include?(key_id) # rubocop:disable Cop/AvoidReturnFromBlocks
     end
 
     false

spec/rubocop/cop/avoid_break_from_strong_memoize_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/avoid_break_from_strong_memoize'
+
+describe RuboCop::Cop::AvoidBreakFromStrongMemoize do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'flags violation for break inside strong_memoize' do
+    expect_offense(<<~RUBY)
+      strong_memoize(:result) do
+        break if something
+        ^^^^^ Do not use break inside strong_memoize, use next instead.
+
+        do_an_heavy_calculation
+      end
+    RUBY
+  end
+
+  it 'flags violation for break inside strong_memoize nested blocks' do
+    expect_offense(<<~RUBY)
+      strong_memoize do
+        items.each do |item|
+          break item
+          ^^^^^^^^^^ Do not use break inside strong_memoize, use next instead.
+        end
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation for next inside strong_memoize" do
+    expect_no_offenses(<<~RUBY)
+      strong_memoize(:result) do
+        next if something
+
+        do_an_heavy_calculation
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation for break inside blocks" do
+    expect_no_offenses(<<~RUBY)
+      call do
+        break if something
+
+        do_an_heavy_calculation
+      end
+    RUBY
+  end
+
+  it "doesn't call add_offense twice for nested blocks" do
+    source = <<~RUBY
+      call do
+        strong_memoize(:result) do
+          break if something
+
+          do_an_heavy_calculation
+        end
+      end
+    RUBY
+    expect_any_instance_of(described_class).to receive(:add_offense).once
+
+    inspect_source(source)
+  end
+
+  it "doesn't check when block is empty" do
+    expect_no_offenses(<<~RUBY)
+      strong_memoize(:result) do
+      end
+    RUBY
+  end
+end

spec/rubocop/cop/avoid_return_from_blocks_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/avoid_return_from_blocks'
+
+describe RuboCop::Cop::AvoidReturnFromBlocks do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'flags violation for return inside a block' do
+    expect_offense(<<~RUBY)
+      call do
+        do_something
+        return if something_else
+        ^^^^^^ Do not return from a block, use next or break instead.
+      end
+    RUBY
+  end
+
+  it "doesn't call add_offense twice for nested blocks" do
+    source = <<~RUBY
+      call do
+        call do
+          something
+          return if something_else
+        end
+      end
+    RUBY
+    expect_any_instance_of(described_class).to receive(:add_offense).once
+
+    inspect_source(source)
+  end
+
+  it 'flags violation for return inside included > def > block' do
+    expect_offense(<<~RUBY)
+      included do
+        def a_method
+          return if something
+
+          call do
+            return if something_else
+            ^^^^^^ Do not return from a block, use next or break instead.
+          end
+        end
+      end
+    RUBY
+  end
+
+  shared_examples 'examples with whitelisted method' do |whitelisted_method|
+    it "doesn't flag violation for return inside #{whitelisted_method}" do
+      expect_no_offenses(<<~RUBY)
+        items.#{whitelisted_method} do |item|
+          do_something
+          return if something_else
+        end
+      RUBY
+    end
+  end
+
+  %i[each each_filename times loop].each do |whitelisted_method|
+    it_behaves_like 'examples with whitelisted method', whitelisted_method
+  end
+
+  shared_examples 'examples with def methods' do |def_method|
+    it "doesn't flag violation for return inside #{def_method}" do
+      expect_no_offenses(<<~RUBY)
+        helpers do
+          #{def_method} do
+            return if something
+
+            do_something_more
+          end
+        end
+      RUBY
+    end
+  end
+
+  %i[define_method lambda].each do |def_method|
+    it_behaves_like 'examples with def methods', def_method
+  end
+
+  it "doesn't flag violation for return inside a lambda" do
+    expect_no_offenses(<<~RUBY)
+      lambda do
+        do_something
+        return if something_else
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation for return used inside a method definition" do
+    expect_no_offenses(<<~RUBY)
+      describe Klass do
+        def a_method
+          do_something
+          return if something_else
+        end
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation for next inside a block" do
+    expect_no_offenses(<<~RUBY)
+      call do
+        do_something
+        next if something_else
+      end
+    RUBY
+  end
+
+  it "doesn't flag violation for break inside a block" do
+    expect_no_offenses(<<~RUBY)
+      call do
+        do_something
+        break if something_else
+      end
+    RUBY
+  end
+
+  it "doesn't check when block is empty" do
+    expect_no_offenses(<<~RUBY)
+      call do
+      end
+    RUBY
+  end
+end

spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb

@@ -4,7 +4,7 @@ shared_examples "matches the method pattern" do |method|
   let(:pattern) { patterns[method] }
 
   it do
-    return skip "No pattern provided, skipping." unless pattern
+    skip "No pattern provided, skipping." unless pattern
 
     expect(target.method(method).call(*args)).to match(pattern)
   end