From fb398b505627da68f2051adcd2ba95ebd383b63c Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Thu, 16 Nov 2017 18:22:41 +0200 Subject: [PATCH] New rule: redundant begin --- config/ameba.yml | 4 + spec/ameba/rule/redundant_begin_spec.cr | 213 ++++++++++++++++++++++++ src/ameba/ast/util.cr | 9 +- src/ameba/rule/redundant_begin.cr | 117 +++++++++++++ 4 files changed, 341 insertions(+), 2 deletions(-) create mode 100644 spec/ameba/rule/redundant_begin_spec.cr create mode 100644 src/ameba/rule/redundant_begin.cr diff --git a/config/ameba.yml b/config/ameba.yml index d98cbab8..f2dab120 100644 --- a/config/ameba.yml +++ b/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 diff --git a/spec/ameba/rule/redundant_begin_spec.cr b/spec/ameba/rule/redundant_begin_spec.cr new file mode 100644 index 00000000..48b1bd43 --- /dev/null +++ b/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 diff --git a/src/ameba/ast/util.cr b/src/ameba/ast/util.cr index 7aa22089..d50d3551 100644 --- a/src/ameba/ast/util.cr +++ b/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. diff --git a/src/ameba/rule/redundant_begin.cr b/src/ameba/rule/redundant_begin.cr new file mode 100644 index 00000000..cbdb40d0 --- /dev/null +++ b/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