From f1e462cc864183165f009be6bb2b88ea54f68ae2 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 1 Nov 2017 00:47:29 +0200 Subject: [PATCH] Remove dsl & refactor ast visitors closes #4 --- src/ameba/ast.cr | 27 +++++++ src/ameba/dsl.cr | 32 --------- src/ameba/rule.cr | 4 ++ src/ameba/rules/comparison_to_boolean.cr | 49 +++++++------ src/ameba/rules/line_length.cr | 16 +++-- src/ameba/rules/trailing_blank_lines.cr | 15 ++-- src/ameba/rules/trailing_whitespace.cr | 16 +++-- src/ameba/rules/unless_else.cr | 91 ++++++++++++------------ src/ameba/source.cr | 14 ++-- 9 files changed, 142 insertions(+), 122 deletions(-) create mode 100644 src/ameba/ast.cr delete mode 100644 src/ameba/dsl.cr diff --git a/src/ameba/ast.cr b/src/ameba/ast.cr new file mode 100644 index 00000000..5397a610 --- /dev/null +++ b/src/ameba/ast.cr @@ -0,0 +1,27 @@ +require "compiler/crystal/syntax/*" + +module Ameba + NODE_VISITORS = [ + Unless, + Call, + ] + + {% for name in NODE_VISITORS %} + class {{name}}Visitor < Crystal::Visitor + @rule : Rule + @source : Source + + def initialize(@rule, @source) + @source.ast.accept self + end + + def visit(node : Crystal::ASTNode) + true + end + + def visit(node : Crystal::{{name}}) + @rule.test @source, node + end + end + {% end %} +end diff --git a/src/ameba/dsl.cr b/src/ameba/dsl.cr deleted file mode 100644 index 34aed15e..00000000 --- a/src/ameba/dsl.cr +++ /dev/null @@ -1,32 +0,0 @@ -module Ameba - macro rule(name, &block) - module Ameba::Rules - struct {{name.id}} < Rule - def test(source) - {{block.body}} - end - end - end - end - - macro visitor(name, node, &block) - module Ameba::Rules - class {{name.id}}Visitor < Crystal::Visitor - @rule : Rule - @source : Source - - def initialize(@rule, @source) - @source.ast.accept self - end - - def visit(node : Crystal::ASTNode) - true - end - - def visit(node : {{node.id}}) - {{block.body}} - end - end - end - end -end diff --git a/src/ameba/rule.cr b/src/ameba/rule.cr index f4a0ec66..bfa779aa 100644 --- a/src/ameba/rule.cr +++ b/src/ameba/rule.cr @@ -10,6 +10,10 @@ module Ameba abstract struct Rule abstract def test(source : Source) + def test(source : Source, node : Crystal::ASTNode) + raise "Unimplemented" + end + def catch(source : Source) source.tap { |s| test s } end diff --git a/src/ameba/rules/comparison_to_boolean.cr b/src/ameba/rules/comparison_to_boolean.cr index af4e5aaa..bd767db3 100644 --- a/src/ameba/rules/comparison_to_boolean.cr +++ b/src/ameba/rules/comparison_to_boolean.cr @@ -1,26 +1,29 @@ -# A rule that disallows comparison to booleans. -# -# For example, these are considered invalid: -# -# ``` -# foo == true -# bar != false -# false === baz -# ``` -# This is because these expressions evaluate to `true` or `false`, so you -# could get the same result by using either the variable directly, or negating -# the variable. +module Ameba::Rules + # A rule that disallows comparison to booleans. + # + # For example, these are considered invalid: + # + # ``` + # foo == true + # bar != false + # false === baz + # ``` + # This is because these expressions evaluate to `true` or `false`, so you + # could get the same result by using either the variable directly, or negating + # the variable. + struct ComparisonToBoolean < Rule + def test(source) + CallVisitor.new self, source + end -Ameba.rule ComparisonToBoolean do |source| - ComparisonToBooleanVisitor.new self, source -end - -Ameba.visitor ComparisonToBoolean, Crystal::Call do |node| - if %w(== != ===).includes?(node.name) && ( - node.args.first?.try &.is_a?(Crystal::BoolLiteral) || - node.obj.is_a?(Crystal::BoolLiteral) - ) - @source.error @rule, node.location.try &.line_number, - "Comparison to a boolean is pointless" + def test(source, node : Crystal::Call) + if %w(== != ===).includes?(node.name) && ( + node.args.first?.try &.is_a?(Crystal::BoolLiteral) || + node.obj.is_a?(Crystal::BoolLiteral) + ) + source.error self, node.location.try &.line_number, + "Comparison to a boolean is pointless" + end + end end end diff --git a/src/ameba/rules/line_length.cr b/src/ameba/rules/line_length.cr index 8c627002..6ca165e7 100644 --- a/src/ameba/rules/line_length.cr +++ b/src/ameba/rules/line_length.cr @@ -1,8 +1,12 @@ -# A rule that disallows lines longer than 79 symbols. - -Ameba.rule LineLength do |source| - source.lines.each_with_index do |line, index| - next unless line.size > 79 - source.error self, index + 1, "Line too long (#{line.size} symbols)" +module Ameba::Rules + # A rule that disallows lines longer than 79 symbols. + struct LineLength < Rule + def test(source) + source.lines.each_with_index do |line, index| + next unless line.size > 79 + source.error self, index + 1, + "Line too long (#{line.size} symbols)" + end + end end end diff --git a/src/ameba/rules/trailing_blank_lines.cr b/src/ameba/rules/trailing_blank_lines.cr index 26892e27..07774de0 100644 --- a/src/ameba/rules/trailing_blank_lines.cr +++ b/src/ameba/rules/trailing_blank_lines.cr @@ -1,8 +1,11 @@ -# A rule that disallows trailing blank lines at the end of the source file. - -Ameba.rule TrailingBlankLines do |source| - if source.lines.size > 1 && source.lines[-2, 2].join.strip.empty? - source.error self, source.lines.size, - "Blank lines detected at the end of the file" +module Ameba::Rules + # A rule that disallows trailing blank lines at the end of the source file. + struct TrailingBlankLines < Rule + def test(source) + if source.lines.size > 1 && source.lines[-2, 2].join.strip.empty? + source.error self, source.lines.size, + "Blank lines detected at the end of the file" + end + end end end diff --git a/src/ameba/rules/trailing_whitespace.cr b/src/ameba/rules/trailing_whitespace.cr index 53db5892..4f364e56 100644 --- a/src/ameba/rules/trailing_whitespace.cr +++ b/src/ameba/rules/trailing_whitespace.cr @@ -1,8 +1,12 @@ -# A rule that disallows trailing whitespace at the end of a line. - -Ameba.rule TrailingWhitespace do |source| - source.lines.each_with_index do |line, index| - next unless line =~ /\s$/ - source.error self, index + 1, "Trailing whitespace detected" +module Ameba::Rules + # A rule that disallows trailing whitespaces. + struct TrailingWhitespace < Rule + def test(source) + source.lines.each_with_index do |line, index| + next unless line =~ /\s$/ + source.error self, index + 1, + "Trailing whitespace detected" + end + end end end diff --git a/src/ameba/rules/unless_else.cr b/src/ameba/rules/unless_else.cr index 7e46c917..3a60766f 100644 --- a/src/ameba/rules/unless_else.cr +++ b/src/ameba/rules/unless_else.cr @@ -1,47 +1,50 @@ -# A rule that disallows the use of an `else` block with the `unless`. -# -# For example, the rule considers these valid: -# -# ``` -# unless something -# :ok -# end -# -# if something -# :one -# else -# :two -# end -# ``` -# -# But it considers this one invalid as it is an `unless` with an `else`: -# -# ``` -# unless something -# :one -# else -# :two -# end -# ``` -# -# The solution is to swap the order of the blocks, and change the `unless` to -# an `if`, so the previous invalid example would become this: -# -# ``` -# if something -# :two -# else -# :one -# end -# ``` +module Ameba::Rules + # A rule that disallows the use of an `else` block with the `unless`. + # + # For example, the rule considers these valid: + # + # ``` + # unless something + # :ok + # end + # + # if something + # :one + # else + # :two + # end + # ``` + # + # But it considers this one invalid as it is an `unless` with an `else`: + # + # ``` + # unless something + # :one + # else + # :two + # end + # ``` + # + # The solution is to swap the order of the blocks, and change the `unless` to + # an `if`, so the previous invalid example would become this: + # + # ``` + # if something + # :two + # else + # :one + # end + # ``` + struct UnlessElse < Rule + def test(source) + UnlessVisitor.new self, source + end -Ameba.rule UnlessElse do |source| - UnlessElseVisitor.new self, source -end - -Ameba.visitor UnlessElse, Crystal::Unless do |node| - unless node.else.is_a?(Crystal::Nop) - @source.error @rule, node.location.try &.line_number, - "Favour if over unless with else" + def test(source, node : Crystal::Unless) + unless node.else.is_a?(Crystal::Nop) + source.error self, node.location.try &.line_number, + "Favour if over unless with else" + end + end end end diff --git a/src/ameba/source.cr b/src/ameba/source.cr index 82e296f5..ba3dcc4b 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -1,5 +1,3 @@ -require "compiler/crystal/syntax/*" - module Ameba # An entity that represents a Crystal source file. # Has path, lines of code and errors reported by rules. @@ -13,13 +11,13 @@ module Ameba pos : Int32?, message : String - getter lines : Array(String) + getter lines : Array(String)? getter errors = [] of Error getter path : String? getter content : String + getter ast : Crystal::ASTNode? def initialize(@content : String, @path = nil) - @lines = @content.split("\n") end def error(rule : Rule, line_number : Int32?, message : String) @@ -30,8 +28,14 @@ module Ameba errors.empty? end + def lines + @lines ||= @content.split("\n") + end + def ast - Crystal::Parser.new(@content).tap { |p| p.filename = @path }.parse + @ast ||= Crystal::Parser.new(@content) + .tap { |p| p.filename = @path } + .parse end end end