Enable `Style/RedundantBegin` cop to avoid newly adding redundant begin block

Currently we sometimes find a redundant begin block in code review
(e.g. https://github.com/rails/rails/pull/33604#discussion_r209784205).

I'd like to enable `Style/RedundantBegin` cop to avoid that, since
rescue/else/ensure are allowed inside do/end blocks in Ruby 2.5
(https://bugs.ruby-lang.org/issues/12906), so we'd probably meets with
that situation than before.

12 files changed, 132 insertions, 176 deletions

diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb
index be9449629f..51fac08749 100644
--- a/actionpack/lib/action_controller/metal/instrumentation.rb
+++ b/actionpack/lib/action_controller/metal/instrumentation.rb
@@ -30,13 +30,11 @@ module ActionController
       ActiveSupport::Notifications.instrument("start_processing.action_controller", raw_payload.dup)
 
       ActiveSupport::Notifications.instrument("process_action.action_controller", raw_payload) do |payload|
-        begin
-          result = super
+        super.tap do
           payload[:status] = response.status
-          result
-        ensure
-          append_info_to_payload(payload)
         end
+      ensure
+        append_info_to_payload(payload)
       end
     end
 
diff --git a/actionpack/lib/action_dispatch/middleware/callbacks.rb b/actionpack/lib/action_dispatch/middleware/callbacks.rb
index 5b2ad36dd5..87fe19225b 100644
--- a/actionpack/lib/action_dispatch/middleware/callbacks.rb
+++ b/actionpack/lib/action_dispatch/middleware/callbacks.rb
@@ -24,10 +24,8 @@ module ActionDispatch
     def call(env)
       error = nil
       result = run_callbacks :call do
-        begin
-          @app.call(env)
-        rescue => error
-        end
+        @app.call(env)
+      rescue => error
       end
       raise error if error
       result
diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
index eadb59173d..61773d97a2 100644
--- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -48,11 +48,9 @@ module ActionDispatch
         wrapper = ExceptionWrapper.new(backtrace_cleaner, exception)
 
         @interceptors.each do |interceptor|
-          begin
-            interceptor.call(request, exception)
-          rescue Exception
-            log_error(request, wrapper)
-          end
+          interceptor.call(request, exception)
+        rescue Exception
+          log_error(request, wrapper)
         end
       end
 
diff --git a/actionpack/lib/action_dispatch/middleware/host_authorization.rb b/actionpack/lib/action_dispatch/middleware/host_authorization.rb
index 48f7c25216..447b70112a 100644
--- a/actionpack/lib/action_dispatch/middleware/host_authorization.rb
+++ b/actionpack/lib/action_dispatch/middleware/host_authorization.rb
@@ -21,13 +21,11 @@ module ActionDispatch
 
       def allows?(host)
         @hosts.any? do |allowed|
-          begin
-            allowed === host
-          rescue
-            # IPAddr#=== raises an error if you give it a hostname instead of
-            # IP. Treat similar errors as blocked access.
-            false
-          end
+          allowed === host
+        rescue
+          # IPAddr#=== raises an error if you give it a hostname instead of
+          # IP. Treat similar errors as blocked access.
+          false
         end
       end
 
diff --git a/actionpack/lib/action_dispatch/middleware/remote_ip.rb b/actionpack/lib/action_dispatch/middleware/remote_ip.rb
index 35158f9062..a5667573f4 100644
--- a/actionpack/lib/action_dispatch/middleware/remote_ip.rb
+++ b/actionpack/lib/action_dispatch/middleware/remote_ip.rb
@@ -162,14 +162,12 @@ module ActionDispatch
         # Split the comma-separated list into an array of strings.
         ips = header.strip.split(/[,\s]+/)
         ips.select do |ip|
-          begin
-            # Only return IPs that are valid according to the IPAddr#new method.
-            range = IPAddr.new(ip).to_range
-            # We want to make sure nobody is sneaking a netmask in.
-            range.begin == range.end
-          rescue ArgumentError
-            nil
-          end
+          # Only return IPs that are valid according to the IPAddr#new method.
+          range = IPAddr.new(ip).to_range
+          # We want to make sure nobody is sneaking a netmask in.
+          range.begin == range.end
+        rescue ArgumentError
+          nil
         end
       end
 
diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb
index 763df3a776..ecb8c37e6b 100644
--- a/actionpack/test/controller/action_pack_assertions_test.rb
+++ b/actionpack/test/controller/action_pack_assertions_test.rb
@@ -276,16 +276,14 @@ class ActionPackAssertionsControllerTest < ActionController::TestCase
   end
 
   def test_assert_redirect_failure_message_with_protocol_relative_url
-    begin
-      process :redirect_external_protocol_relative
-      assert_redirected_to "/foo"
-    rescue ActiveSupport::TestCase::Assertion => ex
-      assert_no_match(
-        /#{request.protocol}#{request.host}\/\/www.rubyonrails.org/,
-        ex.message,
-        "protocol relative url was incorrectly normalized"
-      )
-    end
+    process :redirect_external_protocol_relative
+    assert_redirected_to "/foo"
+  rescue ActiveSupport::TestCase::Assertion => ex
+    assert_no_match(
+      /#{request.protocol}#{request.host}\/\/www.rubyonrails.org/,
+      ex.message,
+      "protocol relative url was incorrectly normalized"
+    )
   end
 
   def test_template_objects_exist
diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb
index 8e117528e2..104c9eeade 100644
--- a/actionpack/test/controller/filters_test.rb
+++ b/actionpack/test/controller/filters_test.rb
@@ -1000,16 +1000,12 @@ class YieldingAroundFiltersTest < ActionController::TestCase
   def test_nested_actions
     controller = ControllerWithNestedFilters
     assert_nothing_raised do
-      begin
-        test_process(controller, "raises_both")
-      rescue Before, After
-      end
+      test_process(controller, "raises_both")
+    rescue Before, After
     end
     assert_raise Before do
-      begin
-        test_process(controller, "raises_both")
-      rescue After
-      end
+      test_process(controller, "raises_both")
+    rescue After
     end
   end
 
diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb
index d2fa0aa16e..fbfe24059b 100644
--- a/actionpack/test/controller/parameters/parameters_permit_test.rb
+++ b/actionpack/test/controller/parameters/parameters_permit_test.rb
@@ -365,17 +365,15 @@ class ParametersPermitTest < ActiveSupport::TestCase
   end
 
   test "permitted takes a default value when Parameters.permit_all_parameters is set" do
-    begin
-      ActionController::Parameters.permit_all_parameters = true
-      params = ActionController::Parameters.new(person: {
-        age: "32", name: { first: "David", last: "Heinemeier Hansson" }
-      })
-
-      assert_predicate params.slice(:person), :permitted?
-      assert_predicate params[:person][:name], :permitted?
-    ensure
-      ActionController::Parameters.permit_all_parameters = false
-    end
+    ActionController::Parameters.permit_all_parameters = true
+    params = ActionController::Parameters.new(person: {
+      age: "32", name: { first: "David", last: "Heinemeier Hansson" }
+    })
+
+    assert_predicate params.slice(:person), :permitted?
+    assert_predicate params[:person][:name], :permitted?
+  ensure
+    ActionController::Parameters.permit_all_parameters = false
   end
 
   test "permitting parameters as an array" do
@@ -396,16 +394,14 @@ class ParametersPermitTest < ActiveSupport::TestCase
   end
 
   test "to_h returns converted hash when .permit_all_parameters is set" do
-    begin
-      ActionController::Parameters.permit_all_parameters = true
-      params = ActionController::Parameters.new(crab: "Senjougahara Hitagi")
-
-      assert_instance_of ActiveSupport::HashWithIndifferentAccess, params.to_h
-      assert_not_kind_of ActionController::Parameters, params.to_h
-      assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_h)
-    ensure
-      ActionController::Parameters.permit_all_parameters = false
-    end
+    ActionController::Parameters.permit_all_parameters = true
+    params = ActionController::Parameters.new(crab: "Senjougahara Hitagi")
+
+    assert_instance_of ActiveSupport::HashWithIndifferentAccess, params.to_h
+    assert_not_kind_of ActionController::Parameters, params.to_h
+    assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_h)
+  ensure
+    ActionController::Parameters.permit_all_parameters = false
   end
 
   test "to_hash raises UnfilteredParameters on unfiltered params" do
@@ -429,17 +425,15 @@ class ParametersPermitTest < ActiveSupport::TestCase
   end
 
   test "to_hash returns converted hash when .permit_all_parameters is set" do
-    begin
-      ActionController::Parameters.permit_all_parameters = true
-      params = ActionController::Parameters.new(crab: "Senjougahara Hitagi")
-
-      assert_instance_of Hash, params.to_hash
-      assert_not_kind_of ActionController::Parameters, params.to_hash
-      assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_hash)
-      assert_equal({ "crab" => "Senjougahara Hitagi" }, params)
-    ensure
-      ActionController::Parameters.permit_all_parameters = false
-    end
+    ActionController::Parameters.permit_all_parameters = true
+    params = ActionController::Parameters.new(crab: "Senjougahara Hitagi")
+
+    assert_instance_of Hash, params.to_hash
+    assert_not_kind_of ActionController::Parameters, params.to_hash
+    assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_hash)
+    assert_equal({ "crab" => "Senjougahara Hitagi" }, params)
+  ensure
+    ActionController::Parameters.permit_all_parameters = false
   end
 
   test "to_unsafe_h returns unfiltered params" do
diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb
index 37399cfd07..c326f276bf 100644
--- a/actionpack/test/dispatch/debug_exceptions_test.rb
+++ b/actionpack/test/dispatch/debug_exceptions_test.rb
@@ -27,14 +27,12 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
     end
 
     def raise_nested_exceptions
+      raise "First error"
+    rescue
       begin
-        raise "First error"
+        raise "Second error"
       rescue
-        begin
-          raise "Second error"
-        rescue
-          raise "Third error"
-        end
+        raise "Third error"
       end
     end
 
@@ -290,22 +288,20 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest
   end
 
   test "rescue with JSON format as fallback if API request format is not supported" do
-    begin
-      Mime::Type.register "text/wibble", :wibble
+    Mime::Type.register "text/wibble", :wibble
 
-      ActionDispatch::IntegrationTest.register_encoder(:wibble,
-        param_encoder: -> params { params })
+    ActionDispatch::IntegrationTest.register_encoder(:wibble,
+      param_encoder: -> params { params })
 
-      @app = ActionDispatch::DebugExceptions.new(Boomer.new(true), RoutesApp, :api)
+    @app = ActionDispatch::DebugExceptions.new(Boomer.new(true), RoutesApp, :api)
 
-      get "/index", headers: { "action_dispatch.show_exceptions" => true }, as: :wibble
-      assert_response 500
-      assert_equal "application/json", response.content_type
-      assert_match(/RuntimeError: puke/, body)
+    get "/index", headers: { "action_dispatch.show_exceptions" => true }, as: :wibble
+    assert_response 500
+    assert_equal "application/json", response.content_type
+    assert_match(/RuntimeError: puke/, body)
 
-    ensure
-      Mime::Type.unregister :wibble
-    end
+  ensure
+    Mime::Type.unregister :wibble
   end
 
   test "does not show filtered parameters" do
diff --git a/actionpack/test/dispatch/mime_type_test.rb b/actionpack/test/dispatch/mime_type_test.rb
index fa264417e1..45d91883c0 100644
--- a/actionpack/test/dispatch/mime_type_test.rb
+++ b/actionpack/test/dispatch/mime_type_test.rb
@@ -96,57 +96,47 @@ class MimeTypeTest < ActiveSupport::TestCase
   end
 
   test "custom type" do
-    begin
-      type = Mime::Type.register("image/foo", :foo)
-      assert_equal type, Mime[:foo]
-    ensure
-      Mime::Type.unregister(:foo)
-    end
+    type = Mime::Type.register("image/foo", :foo)
+    assert_equal type, Mime[:foo]
+  ensure
+    Mime::Type.unregister(:foo)
   end
 
   test "custom type with type aliases" do
-    begin
-      Mime::Type.register "text/foobar", :foobar, ["text/foo", "text/bar"]
-      %w[text/foobar text/foo text/bar].each do |type|
-        assert_equal Mime[:foobar], type
-      end
-    ensure
-      Mime::Type.unregister(:foobar)
+    Mime::Type.register "text/foobar", :foobar, ["text/foo", "text/bar"]
+    %w[text/foobar text/foo text/bar].each do |type|
+      assert_equal Mime[:foobar], type
     end
+  ensure
+    Mime::Type.unregister(:foobar)
   end
 
   test "register callbacks" do
-    begin
-      registered_mimes = []
-      Mime::Type.register_callback do |mime|
-        registered_mimes << mime
-      end
-
-      mime = Mime::Type.register("text/foo", :foo)
-      assert_equal [mime], registered_mimes
-    ensure
-      Mime::Type.unregister(:foo)
+    registered_mimes = []
+    Mime::Type.register_callback do |mime|
+      registered_mimes << mime
     end
+
+    mime = Mime::Type.register("text/foo", :foo)
+    assert_equal [mime], registered_mimes
+  ensure
+    Mime::Type.unregister(:foo)
   end
 
   test "custom type with extension aliases" do
-    begin
-      Mime::Type.register "text/foobar", :foobar, [], [:foo, "bar"]
-      %w[foobar foo bar].each do |extension|
-        assert_equal Mime[:foobar], Mime::EXTENSION_LOOKUP[extension]
-      end
-    ensure
-      Mime::Type.unregister(:foobar)
+    Mime::Type.register "text/foobar", :foobar, [], [:foo, "bar"]
+    %w[foobar foo bar].each do |extension|
+      assert_equal Mime[:foobar], Mime::EXTENSION_LOOKUP[extension]
     end
+  ensure
+    Mime::Type.unregister(:foobar)
   end
 
   test "register alias" do
-    begin
-      Mime::Type.register_alias "application/xhtml+xml", :foobar
-      assert_equal Mime[:html], Mime::EXTENSION_LOOKUP["foobar"]
-    ensure
-      Mime::Type.unregister(:foobar)
-    end
+    Mime::Type.register_alias "application/xhtml+xml", :foobar
+    assert_equal Mime[:html], Mime::EXTENSION_LOOKUP["foobar"]
+  ensure
+    Mime::Type.unregister(:foobar)
   end
 
   test "type should be equal to symbol" do
diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb
index beab8e78b5..2a48a12497 100644
--- a/actionpack/test/dispatch/request/json_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb
@@ -74,17 +74,15 @@ class JsonParamsParsingTest < ActionDispatch::IntegrationTest
 
   test "occurring a parse error if parsing unsuccessful" do
     with_test_routing do
-      begin
-        $stderr = StringIO.new # suppress the log
-        json = "[\"person]\": {\"name\": \"David\"}}"
-        exception = assert_raise(ActionDispatch::Http::Parameters::ParseError) do
-          post "/parse", params: json, headers: { "CONTENT_TYPE" => "application/json", "action_dispatch.show_exceptions" => false }
-        end
-        assert_equal JSON::ParserError, exception.cause.class
-        assert_equal exception.cause.message, exception.message
-      ensure
-        $stderr = STDERR
+      $stderr = StringIO.new # suppress the log
+      json = "[\"person]\": {\"name\": \"David\"}}"
+      exception = assert_raise(ActionDispatch::Http::Parameters::ParseError) do
+        post "/parse", params: json, headers: { "CONTENT_TYPE" => "application/json", "action_dispatch.show_exceptions" => false }
       end
+      assert_equal JSON::ParserError, exception.cause.class
+      assert_equal exception.cause.message, exception.message
+    ensure
+      $stderr = STDERR
     end
   end
 
@@ -157,31 +155,27 @@ class RootLessJSONParamsParsingTest < ActionDispatch::IntegrationTest
   end
 
   test "parses json params after custom json mime type registered" do
-    begin
-      Mime::Type.unregister :json
-      Mime::Type.register "application/json", :json, %w(application/vnd.rails+json)
-      assert_parses(
-        { "user" => { "username" => "meinac" }, "username" => "meinac" },
-        "{\"username\": \"meinac\"}", "CONTENT_TYPE" => "application/json"
-      )
-    ensure
-      Mime::Type.unregister :json
-      Mime::Type.register "application/json", :json, %w( text/x-json application/jsonrequest )
-    end
+    Mime::Type.unregister :json
+    Mime::Type.register "application/json", :json, %w(application/vnd.rails+json)
+    assert_parses(
+      { "user" => { "username" => "meinac" }, "username" => "meinac" },
+      "{\"username\": \"meinac\"}", "CONTENT_TYPE" => "application/json"
+    )
+  ensure
+    Mime::Type.unregister :json
+    Mime::Type.register "application/json", :json, %w( text/x-json application/jsonrequest )
   end
 
   test "parses json params after custom json mime type registered with synonym" do
-    begin
-      Mime::Type.unregister :json
-      Mime::Type.register "application/json", :json, %w(application/vnd.rails+json)
-      assert_parses(
-        { "user" => { "username" => "meinac" }, "username" => "meinac" },
-        "{\"username\": \"meinac\"}", "CONTENT_TYPE" => "application/vnd.rails+json"
-      )
-    ensure
-      Mime::Type.unregister :json
-      Mime::Type.register "application/json", :json, %w( text/x-json application/jsonrequest )
-    end
+    Mime::Type.unregister :json
+    Mime::Type.register "application/json", :json, %w(application/vnd.rails+json)
+    assert_parses(
+      { "user" => { "username" => "meinac" }, "username" => "meinac" },
+      "{\"username\": \"meinac\"}", "CONTENT_TYPE" => "application/vnd.rails+json"
+    )
+  ensure
+    Mime::Type.unregister :json
+    Mime::Type.register "application/json", :json, %w( text/x-json application/jsonrequest )
   end
 
   private
diff --git a/actionpack/test/dispatch/system_testing/screenshot_helper_test.rb b/actionpack/test/dispatch/system_testing/screenshot_helper_test.rb
index de79c05657..097ef8af29 100644
--- a/actionpack/test/dispatch/system_testing/screenshot_helper_test.rb
+++ b/actionpack/test/dispatch/system_testing/screenshot_helper_test.rb
@@ -41,22 +41,20 @@ class ScreenshotHelperTest < ActiveSupport::TestCase
   end
 
   test "display_image return artifact format when specify RAILS_SYSTEM_TESTING_SCREENSHOT environment" do
-    begin
-      original_output_type = ENV["RAILS_SYSTEM_TESTING_SCREENSHOT"]
-      ENV["RAILS_SYSTEM_TESTING_SCREENSHOT"] = "artifact"
+    original_output_type = ENV["RAILS_SYSTEM_TESTING_SCREENSHOT"]
+    ENV["RAILS_SYSTEM_TESTING_SCREENSHOT"] = "artifact"
 
-      new_test = DrivenBySeleniumWithChrome.new("x")
+    new_test = DrivenBySeleniumWithChrome.new("x")
 
-      assert_equal "artifact", new_test.send(:output_type)
+    assert_equal "artifact", new_test.send(:output_type)
 
-      Rails.stub :root, Pathname.getwd do
-        new_test.stub :passed?, false do
-          assert_match %r|url=artifact://.+?tmp/screenshots/failures_x\.png|, new_test.send(:display_image)
-        end
+    Rails.stub :root, Pathname.getwd do
+      new_test.stub :passed?, false do
+        assert_match %r|url=artifact://.+?tmp/screenshots/failures_x\.png|, new_test.send(:display_image)
       end
-    ensure
-      ENV["RAILS_SYSTEM_TESTING_SCREENSHOT"] = original_output_type
     end
+  ensure
+    ENV["RAILS_SYSTEM_TESTING_SCREENSHOT"] = original_output_type
   end
 
   test "image path returns the absolute path from root" do