New rule: redundant begin

config/ameba.yml

@@ -50,6 +50,10 @@ PredicateName:
   # the prefix `has_` or the prefix `is_`.
   Enabled: true
 
+RedundantBegin:
+  # Disallows redundant begin blocks.
+  Enabled: true
+
 TrailingBlankLines:
   # Disallows trailing blank lines at the end of the source file.
   Enabled: true

spec/ameba/rule/redundant_begin_spec.cr

@@ -0,0 +1,213 @@
+require "../../spec_helper"
+
+module Ameba::Rule
+  describe RedundantBegin do
+    subject = RedundantBegin.new
+
+    it "passes if there is no redundant begin blocks" do
+      s = Source.new %(
+        def method
+          do_something
+        rescue
+          do_something_else
+        end
+
+        def method
+          do_something
+          do_something_else
+        ensure
+          handle_something
+        end
+
+        def method
+          yield
+        rescue
+        end
+
+        def method; end
+        def method; a = 1; rescue; end
+        def method; begin; rescue; end; end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "passes if there is a correct begin block in a handler" do
+      s = Source.new %q(
+        def handler_and_expression
+          begin
+            open_file
+          rescue
+            close_file
+          end
+          do_some_stuff
+        end
+
+        def multiple_handlers
+          begin
+            begin1
+          rescue
+          end
+
+          begin
+            begin2
+          rescue
+          end
+        rescue
+          do_something_else
+        end
+
+        def assign_and_begin
+          @result ||=
+            begin
+              do_something
+              do_something_else
+              returnit
+            end
+        rescue
+        end
+
+        def inner_handler
+          s = begin
+              rescue
+              end
+        rescue
+        end
+
+        def begin_and_expression
+          begin
+            a = 1
+            b = 2
+          end
+          expr
+        end
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "fails if there is a redundant begin block" do
+      s = Source.new %q(
+        def method(a : String) : String
+          begin
+            open_file
+            do_some_stuff
+          ensure
+            close_file
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is a redundant begin block in a method without args" do
+      s = Source.new %q(
+        def method
+          begin
+            open_file
+          ensure
+            close_file
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is a redundant block in a method with return type" do
+      s = Source.new %q(
+        def method : String
+          begin
+            open_file
+          ensure
+            close_file
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is a redundant block in a method with multiple args" do
+      s = Source.new %q(
+        def method(a : String,
+                  b : String)
+          begin
+            open_file
+          ensure
+            close_file
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is a redundant block in a method with multiple args" do
+      s = Source.new %q(
+        def method(a : String,
+                  b : String
+        )
+          begin
+            open_file
+          ensure
+            close_file
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is an inner redundant block" do
+      s = Source.new %q(
+        def method
+          begin
+            open_file
+          ensure
+            close_file
+          end
+        rescue
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is a redundant block with yield" do
+      s = Source.new %q(
+        def method
+          begin
+            yield
+          ensure
+            close_file
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "fails if there is top level redundant block in a method" do
+      s = Source.new %q(
+        def method
+          begin
+            a = 1
+            b = 2
+          end
+        end
+      )
+      subject.catch(s).should_not be_valid
+    end
+
+    it "reports rule, pos and message" do
+      s = Source.new %q(
+        def method
+          begin
+            open_connection
+          ensure
+            close_connection
+          end
+        end
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+
+      error = s.errors.first
+      error.rule.should_not be_nil
+      error.location.to_s.should eq "source.cr:2:9"
+      error.message.should eq "Redundant `begin` block detected."
+    end
+  end
+end

src/ameba/ast/util.cr

@@ -1,15 +1,20 @@
 # Utility module for Ameba's rules.
 module Ameba::AST::Util
-  # Returns true if current `node` is a literal, false - otherwise.
+  # Returns true if current `node` is a literal, false otherwise.
   def literal?(node)
     node.try &.class.name.ends_with? "Literal"
   end
 
-  # Returns true if current `node` is a string literal, false - otherwise.
+  # Returns true if current `node` is a string literal, false otherwise.
   def string_literal?(node)
     node.is_a? Crystal::StringLiteral
   end
 
+  # Returns true if current `node` is an exception handler, false otherwise.
+  def exception_handler?(node)
+    node.is_a? Crystal::ExceptionHandler
+  end
+
   # Returns a source code for the current node.
   # This method uses `node.location` and `node.end_location`
   # to determine and cut a piece of source of the node.

src/ameba/rule/redundant_begin.cr

@@ -0,0 +1,117 @@
+module Ameba::Rule
+  # A rule that disallows redundant begin blocks.
+  #
+  # Currently it is able to detect:
+  #
+  # 1. Exception handler block that can be used as a part of the method.
+  #
+  # For example, this:
+  #
+  # ```
+  # def method
+  #   begin
+  #     read_content
+  #   rescue
+  #     close_file
+  #   end
+  # end
+  # ```
+  #
+  # should be rewritten as:
+  #
+  # ```
+  # def method
+  #   read_content
+  # rescue
+  #   close_file
+  # end
+  # ```
+  #
+  # 2. begin..end block as a top level block in a method.
+  #
+  # For example this is considered invalid:
+  #
+  # ```
+  # def method
+  #   begin
+  #     a = 1
+  #     b = 2
+  #   end
+  # end
+  # ```
+  #
+  # and has to be written as the following:
+  #
+  # ```
+  # def method
+  #   a = 1
+  #   b = 2
+  # end
+  # ```
+  #
+  struct RedundantBegin < Base
+    include AST::Util
+
+    def test(source)
+      AST::Visitor.new self, source
+    end
+
+    def test(source, node : Crystal::Def)
+      return unless redundant_begin?(source, node)
+
+      source.error self, node.location, "Redundant `begin` block detected."
+    end
+
+    private def redundant_begin?(source, node)
+      case body = node.body
+      when Crystal::ExceptionHandler
+        redundant_begin_in_handler?(source, body, node)
+      when Crystal::Expressions
+        redundant_begin_in_expressions?(body)
+      end
+    end
+
+    private def redundant_begin_in_expressions?(node)
+      node.keyword == :begin
+    end
+
+    private def redundant_begin_in_handler?(source, handler, node)
+      return false if begin_exprs_in_handler?(handler)
+
+      code = node_source(node, source.lines).try &.join("\n")
+      def_redundant_begin? code if code
+    rescue
+      false
+    end
+
+    private def begin_exprs_in_handler?(handler)
+      if (body = handler.body).is_a?(Crystal::Expressions)
+        exception_handler?(body.expressions.first)
+      end
+    end
+
+    private def def_redundant_begin?(code)
+      lexer = Crystal::Lexer.new code
+      in_body? = in_argument_list? = false
+      while true
+        token = lexer.next_token
+
+        case token.type
+        when :EOF
+          break
+        when :IDENT
+          return token.value == :begin if in_body?
+        when :"("
+          in_argument_list? = true
+        when :")"
+          in_argument_list? = false
+        when :NEWLINE
+          in_body? = true unless in_argument_list?
+        when :SPACE
+        else
+          return false if in_body?
+        end
+      end
+    end
+  end
+end