From c6b1043e9d1b7fe9912c330b6e7d4342f2a9694e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=99=88=20=20jacopo=20beschi=20=F0=9F=99=89?= Date: Wed, 18 Apr 2018 09:19:40 +0000 Subject: Resolve "Make a Rubocop that forbids returning from a block" --- app/controllers/concerns/notes_actions.rb | 4 +- app/controllers/groups/variables_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 4 +- app/controllers/projects/variables_controller.rb | 2 +- app/models/ci/build.rb | 6 +- app/models/notification_recipient.rb | 12 +- app/models/project.rb | 2 +- app/services/ci/register_job_service.rb | 2 +- .../gcp/verify_provision_status_service.rb | 2 +- app/services/create_deployment_service.rb | 4 +- app/services/import_export_clean_up_service.rb | 2 +- app/services/projects/destroy_service.rb | 2 +- .../repository_archive_clean_up_service.rb | 2 +- app/services/test_hooks/base_service.rb | 2 +- app/workers/post_receive.rb | 2 +- app/workers/stuck_ci_jobs_worker.rb | 2 +- .../unreleased/42889-avoid-return-inside-block.yml | 5 + lib/api/discussions.rb | 10 +- lib/api/group_variables.rb | 4 +- lib/api/internal.rb | 10 +- lib/api/issues.rb | 2 +- lib/api/job_artifacts.rb | 2 +- lib/api/jobs.rb | 4 +- lib/api/project_snippets.rb | 2 +- lib/api/projects.rb | 2 +- lib/api/runner.rb | 6 +- lib/api/snippets.rb | 8 +- lib/api/triggers.rb | 8 +- lib/api/v3/builds.rb | 8 +- lib/api/v3/projects.rb | 2 +- lib/api/v3/snippets.rb | 6 +- lib/api/v3/triggers.rb | 4 +- lib/api/variables.rb | 4 +- lib/declarative_policy/runner.rb | 2 +- lib/gitlab/auth.rb | 2 +- lib/gitlab/ci/trace.rb | 2 +- lib/gitlab/ci/trace/stream.rb | 2 +- lib/gitlab/daemon.rb | 4 +- lib/gitlab/gfm/uploads_rewriter.rb | 2 +- lib/gitlab/git/diff.rb | 2 +- lib/gitlab/git/popen.rb | 4 +- lib/gitlab/git/repository_mirroring.rb | 2 +- lib/gitlab/optimistic_locking.rb | 19 ++- lib/gitlab/shell.rb | 2 +- lib/gitlab/sidekiq_middleware/shutdown.rb | 2 +- lib/tasks/gitlab/storage.rake | 4 +- qa/qa/factory/base.rb | 2 +- qa/qa/page/group/show.rb | 2 +- qa/qa/page/project/pipeline/show.rb | 4 +- qa/qa/scenario/template.rb | 2 +- rubocop/cop/avoid_break_from_strong_memoize.rb | 48 ++++++++ rubocop/cop/avoid_return_from_blocks.rb | 77 +++++++++++++ rubocop/cop/migration/safer_boolean_column.rb | 4 +- rubocop/rubocop.rb | 2 + spec/lib/gitlab/ci/trace_spec.rb | 2 +- spec/lib/gitlab/shell_spec.rb | 2 +- .../cop/avoid_break_from_strong_memoize_spec.rb | 74 ++++++++++++ spec/rubocop/cop/avoid_return_from_blocks_spec.rb | 127 +++++++++++++++++++++ .../uploaders/gitlab_uploader_shared_examples.rb | 2 +- 59 files changed, 434 insertions(+), 102 deletions(-) create mode 100644 changelogs/unreleased/42889-avoid-return-inside-block.yml create mode 100644 rubocop/cop/avoid_break_from_strong_memoize.rb create mode 100644 rubocop/cop/avoid_return_from_blocks.rb create mode 100644 spec/rubocop/cop/avoid_break_from_strong_memoize_spec.rb create mode 100644 spec/rubocop/cop/avoid_return_from_blocks_spec.rb diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index ad4e936a3d4..0c34e49206a 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/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 diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb index 6142e75b4c1..4d8a20de017 100644 --- a/app/controllers/groups/variables_controller.rb +++ b/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| diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 54e7d81de6a..62b739918e6 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 517d0b026c2..bf09ea7e4d8 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/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| diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 3e3ef674dff..1c8d9ca4aa5 100644 --- a/app/models/ci/build.rb +++ b/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) diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index b3ffad00a07..2c3580bbdc6 100644 --- a/app/models/notification_recipient.rb +++ b/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 diff --git a/app/models/project.rb b/app/models/project.rb index ffd78d3ab70..38139a9b137 100644 --- a/app/models/project.rb +++ b/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) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 79eec33ae73..c289b44f885 100644 --- a/app/services/ci/register_job_service.rb +++ b/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 diff --git a/app/services/clusters/gcp/verify_provision_status_service.rb b/app/services/clusters/gcp/verify_provision_status_service.rb index f994aacd086..7cc4324677e 100644 --- a/app/services/clusters/gcp/verify_provision_status_service.rb +++ b/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 diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index 88dfb7a4a90..7e5a77fb056 100644 --- a/app/services/create_deployment_service.rb +++ b/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 diff --git a/app/services/import_export_clean_up_service.rb b/app/services/import_export_clean_up_service.rb index 6442406d77e..74088b970c9 100644 --- a/app/services/import_export_clean_up_service.rb +++ b/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 diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index aa14206db3b..44e869851ca 100644 --- a/app/services/projects/destroy_service.rb +++ b/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 diff --git a/app/services/repository_archive_clean_up_service.rb b/app/services/repository_archive_clean_up_service.rb index aa84d36a206..9a88459b511 100644 --- a/app/services/repository_archive_clean_up_service.rb +++ b/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 diff --git a/app/services/test_hooks/base_service.rb b/app/services/test_hooks/base_service.rb index e9aefb1fb75..aadc1ea644b 100644 --- a/app/services/test_hooks/base_service.rb +++ b/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) diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 3909dbf7d7f..f88b3fdbfb1 100644 --- a/app/workers/post_receive.rb +++ b/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) diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index fb26fa4c515..7ebf69bdc39 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/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 diff --git a/changelogs/unreleased/42889-avoid-return-inside-block.yml b/changelogs/unreleased/42889-avoid-return-inside-block.yml new file mode 100644 index 00000000000..e3e1341ddcc --- /dev/null +++ b/changelogs/unreleased/42889-avoid-return-inside-block.yml @@ -0,0 +1,5 @@ +--- +title: Rubocop rule to avoid returning from a block +merge_request: 18000 +author: Jacopo Beschi @jacopo-beschi +type: added diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 6abd575b6ad..7975f35ab1e 100644 --- a/lib/api/discussions.rb +++ b/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], diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb index 92800ce6450..55d5c7f1606 100644 --- a/lib/api/group_variables.rb +++ b/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) diff --git a/lib/api/internal.rb b/lib/api/internal.rb index fcbc248fc3b..6b72caea8fd 100644 --- a/lib/api/internal.rb +++ b/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 diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 88e7f46c92c..12ff2a1398b 100644 --- a/lib/api/issues.rb +++ b/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 diff --git a/lib/api/job_artifacts.rb b/lib/api/job_artifacts.rb index b1adef49d46..32379d7c8ab 100644 --- a/lib/api/job_artifacts.rb +++ b/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! diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 60911c8d733..54d1acbd412 100644 --- a/lib/api/jobs.rb +++ b/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 diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index 39c03c40bab..1de5551fee9 100644 --- a/lib/api/project_snippets.rb +++ b/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 diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 3ae6fbd1fa9..51b3b0459f3 100644 --- a/lib/api/projects.rb +++ b/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)) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 60aeb69e10a..4d4fbe50f9f 100644 --- a/lib/api/runner.rb +++ b/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 diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index c736cc32021..b30305b4bc9 100644 --- a/lib/api/snippets.rb +++ b/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 diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index b3709455bc3..b29e660c6e0 100644 --- a/lib/api/triggers.rb +++ b/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 diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb index 683b9c993cb..b49448e1e67 100644 --- a/lib/api/v3/builds.rb +++ b/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! diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb index a2df969d819..eb3dd113524 100644 --- a/lib/api/v3/projects.rb +++ b/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)) diff --git a/lib/api/v3/snippets.rb b/lib/api/v3/snippets.rb index 85613c8ed84..1df8a20e74a 100644 --- a/lib/api/v3/snippets.rb +++ b/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' diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index 34f07dfb486..969bb2a05de 100644 --- a/lib/api/v3/triggers.rb +++ b/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 diff --git a/lib/api/variables.rb b/lib/api/variables.rb index d08876ae1b9..a34de9410e8 100644 --- a/lib/api/variables.rb +++ b/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) diff --git a/lib/declarative_policy/runner.rb b/lib/declarative_policy/runner.rb index 77c91817382..87f14b3b0d2 100644 --- a/lib/declarative_policy/runner.rb +++ b/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 diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 2a44e11efb6..8e5a985edd7 100644 --- a/lib/gitlab/auth.rb +++ b/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 = [] diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index cedf4171ab1..47b67930c6d 100644 --- a/lib/gitlab/ci/trace.rb +++ b/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) diff --git a/lib/gitlab/ci/trace/stream.rb b/lib/gitlab/ci/trace/stream.rb index 8f7600f60c6..187ad8b833a 100644 --- a/lib/gitlab/ci/trace/stream.rb +++ b/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 diff --git a/lib/gitlab/daemon.rb b/lib/gitlab/daemon.rb index 633de9f9776..bd14c7eece3 100644 --- a/lib/gitlab/daemon.rb +++ b/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 diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index 1b74f735679..b6eeb5d9a2b 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/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| diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index a203587aec1..b58296375ef 100644 --- a/lib/gitlab/git/diff.rb +++ b/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 diff --git a/lib/gitlab/git/popen.rb b/lib/gitlab/git/popen.rb index c1767046ff0..f9f24ecc48d 100644 --- a/lib/gitlab/git/popen.rb +++ b/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 diff --git a/lib/gitlab/git/repository_mirroring.rb b/lib/gitlab/git/repository_mirroring.rb index dc424a433fb..8a01f92e2af 100644 --- a/lib/gitlab/git/repository_mirroring.rb +++ b/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 diff --git a/lib/gitlab/optimistic_locking.rb b/lib/gitlab/optimistic_locking.rb index 1d9a5d1a20a..d09bce642b0 100644 --- a/lib/gitlab/optimistic_locking.rb +++ b/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 diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 67407b651a5..ac4ac537a8a 100644 --- a/lib/gitlab/shell.rb +++ b/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 diff --git a/lib/gitlab/sidekiq_middleware/shutdown.rb b/lib/gitlab/sidekiq_middleware/shutdown.rb index c2b8d6de66e..b232ac4da33 100644 --- a/lib/gitlab/sidekiq_middleware/shutdown.rb +++ b/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 diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index 8ac73bc8ff2..6e8bd9078c8 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/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 diff --git a/qa/qa/factory/base.rb b/qa/qa/factory/base.rb index afaa96b4541..7a532ce534b 100644 --- a/qa/qa/factory/base.rb +++ b/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 diff --git a/qa/qa/page/group/show.rb b/qa/qa/page/group/show.rb index d215518d316..89125bd2e59 100644 --- a/qa/qa/page/group/show.rb +++ b/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 diff --git a/qa/qa/page/project/pipeline/show.rb b/qa/qa/page/project/pipeline/show.rb index b183552d46c..ec61c47b3bb 100644 --- a/qa/qa/page/project/pipeline/show.rb +++ b/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 diff --git a/qa/qa/scenario/template.rb b/qa/qa/scenario/template.rb index 341998af160..d21a9d52997 100644 --- a/qa/qa/scenario/template.rb +++ b/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 diff --git a/rubocop/cop/avoid_break_from_strong_memoize.rb b/rubocop/cop/avoid_break_from_strong_memoize.rb new file mode 100644 index 00000000000..9b436118db3 --- /dev/null +++ b/rubocop/cop/avoid_break_from_strong_memoize.rb @@ -0,0 +1,48 @@ +# 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 diff --git a/rubocop/cop/avoid_return_from_blocks.rb b/rubocop/cop/avoid_return_from_blocks.rb new file mode 100644 index 00000000000..40b2aed019f --- /dev/null +++ b/rubocop/cop/avoid_return_from_blocks.rb @@ -0,0 +1,77 @@ +# 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 diff --git a/rubocop/cop/migration/safer_boolean_column.rb b/rubocop/cop/migration/safer_boolean_column.rb index dc5c55df6fb..a7d922c752f 100644 --- a/rubocop/cop/migration/safer_boolean_column.rb +++ b/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 diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index c2254332e7d..96061672749 100644 --- a/rubocop/rubocop.rb +++ b/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' diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb index 3a9371ed2e8..6a9c6442282 100644 --- a/spec/lib/gitlab/ci/trace_spec.rb +++ b/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' diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb index 7ff2c0639ec..7f579df1c36 100644 --- a/spec/lib/gitlab/shell_spec.rb +++ b/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 diff --git a/spec/rubocop/cop/avoid_break_from_strong_memoize_spec.rb b/spec/rubocop/cop/avoid_break_from_strong_memoize_spec.rb new file mode 100644 index 00000000000..ac7b1575ec0 --- /dev/null +++ b/spec/rubocop/cop/avoid_break_from_strong_memoize_spec.rb @@ -0,0 +1,74 @@ +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 diff --git a/spec/rubocop/cop/avoid_return_from_blocks_spec.rb b/spec/rubocop/cop/avoid_return_from_blocks_spec.rb new file mode 100644 index 00000000000..a5c280a7adc --- /dev/null +++ b/spec/rubocop/cop/avoid_return_from_blocks_spec.rb @@ -0,0 +1,127 @@ +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 diff --git a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb b/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb index 934d53e7bba..93c21a99e59 100644 --- a/spec/support/shared_examples/uploaders/gitlab_uploader_shared_examples.rb +++ b/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 -- cgit v1.2.1