From adfe654733eff87d69b1aefe5c64bf2b96cf87e1 Mon Sep 17 00:00:00 2001
From: "V. Elenhaupt" <3624712+veelenga@users.noreply.github.com>
Date: Mon, 6 Nov 2017 20:54:58 +0200
Subject: [PATCH] Performance improvements (#15)

* Performance improvements

Two main changes:

1. Cache parsed AST in a Source. So Parser will parse content only once.
2. Use one unified visitor with multiple callbacks instead of multiple
visitors to traverse AST.

This improves performance significantly, for example running it on
Crystal repository takes ~1 second, which 6 times faster than before.

* Change readme example
---
 README.md                                       | 15 +++++++--------
 spec/ameba/ast/traverse_spec.cr                 |  2 +-
 src/ameba/ast/traverse.cr                       | 15 ++++++---------
 src/ameba/rule.cr                               |  2 +-
 src/ameba/rules/comparison_to_boolean.cr        |  2 +-
 src/ameba/rules/constant_names.cr               |  2 +-
 src/ameba/rules/debugger_statement.cr           |  2 +-
 src/ameba/rules/literal_in_condition.cr         |  4 +---
 src/ameba/rules/literal_in_interpolation.cr     |  2 +-
 src/ameba/rules/method_names.cr                 |  2 +-
 src/ameba/rules/negated_conditions_in_unless.cr |  2 +-
 src/ameba/rules/predicate_name.cr               |  2 +-
 src/ameba/rules/type_names.cr                   |  8 +-------
 src/ameba/rules/unless_else.cr                  |  2 +-
 src/ameba/rules/variable_names.cr               |  6 +-----
 src/ameba/source.cr                             |  8 ++++++++
 16 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/README.md b/README.md
index d899b076..6083850e 100644
--- a/README.md
+++ b/README.md
@@ -67,22 +67,21 @@ your logic to detect a problem:
 ```crystal
 struct DebuggerStatement < Rule
   # This is a required method to be implemented by the rule.
-  # Source will pass here. If rule finds an issue in this source,
-  # it adds an error: 
+  # Source will be passed here. If rule finds an issue in this source,
+  # it reports an error: 
   # 
   #   source.error rule, line_number, message
   #
   def test(source)
     # This line deletegates verification to a particular AST visitor.
-    AST::CallVisitor.new self, source
+    AST::Visitor.new self, source
   end
 
-  # This method is called once our visitor finds a required node.
-  # 
-  # It reports an error, once there is `debugger` method call
-  # without arguments and a receiver. That's it, somebody forgot
-  # to remove debugger statement.
+  # This method is called once the visitor finds a required node.
   def test(source, node : Crystal::Call)
+    # It reports an error, if there is `debugger` method call
+    # without arguments and a receiver. That's it, somebody forgot
+    # to remove a debugger statement.
     return unless node.name == "debugger" && node.args.empty? && node.obj.nil?
 
     source.error self, node.location.try &.line_number,
diff --git a/spec/ameba/ast/traverse_spec.cr b/spec/ameba/ast/traverse_spec.cr
index 3c14a9be..5d32ed6e 100644
--- a/spec/ameba/ast/traverse_spec.cr
+++ b/spec/ameba/ast/traverse_spec.cr
@@ -8,7 +8,7 @@ module Ameba::AST
     {% for name in NODE_VISITORS %}
       describe "{{name}}" do
         it "allow to visit {{name}} node" do
-          visitor = {{name}}Visitor.new rule, source
+          visitor = Visitor.new rule, source
           nodes = Crystal::Parser.new("").parse
           nodes.accept visitor
         end
diff --git a/src/ameba/ast/traverse.cr b/src/ameba/ast/traverse.cr
index e46bd1c2..a7d9bb2b 100644
--- a/src/ameba/ast/traverse.cr
+++ b/src/ameba/ast/traverse.cr
@@ -19,26 +19,23 @@ module Ameba::AST
     Var,
   ]
 
-  abstract class Visitor < Crystal::Visitor
+  class Visitor < Crystal::Visitor
     @rule : Rule
     @source : Source
 
     def initialize(@rule, @source)
-      parser = Crystal::Parser.new(@source.content)
-      parser.filename = @source.path
-      parser.parse.accept self
+      @source.ast.accept self
     end
 
     def visit(node : Crystal::ASTNode)
       true
     end
-  end
 
-  {% for name in NODE_VISITORS %}
-    class {{name}}Visitor < Visitor
+    {% for name in NODE_VISITORS %}
       def visit(node : Crystal::{{name}})
         @rule.test @source, node
+        true
       end
-    end
-  {% end %}
+    {% end %}
+  end
 end
diff --git a/src/ameba/rule.cr b/src/ameba/rule.cr
index 1ced1c2a..8bb1abd9 100644
--- a/src/ameba/rule.cr
+++ b/src/ameba/rule.cr
@@ -3,7 +3,7 @@ module Ameba
     abstract def test(source : Source)
 
     def test(source : Source, node : Crystal::ASTNode)
-      raise "Unimplemented"
+      # can't be abstract
     end
 
     def catch(source : Source)
diff --git a/src/ameba/rules/comparison_to_boolean.cr b/src/ameba/rules/comparison_to_boolean.cr
index a85914cd..18bcd303 100644
--- a/src/ameba/rules/comparison_to_boolean.cr
+++ b/src/ameba/rules/comparison_to_boolean.cr
@@ -14,7 +14,7 @@ module Ameba::Rules
   #
   struct ComparisonToBoolean < Rule
     def test(source)
-      AST::CallVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Call)
diff --git a/src/ameba/rules/constant_names.cr b/src/ameba/rules/constant_names.cr
index bcaa8fbb..7a37646d 100644
--- a/src/ameba/rules/constant_names.cr
+++ b/src/ameba/rules/constant_names.cr
@@ -17,7 +17,7 @@ module Ameba::Rules
   #
   struct ConstantNames < Rule
     def test(source)
-      AST::AssignVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Assign)
diff --git a/src/ameba/rules/debugger_statement.cr b/src/ameba/rules/debugger_statement.cr
index 902808b1..04700013 100644
--- a/src/ameba/rules/debugger_statement.cr
+++ b/src/ameba/rules/debugger_statement.cr
@@ -6,7 +6,7 @@ module Ameba::Rules
   #
   struct DebuggerStatement < Rule
     def test(source)
-      AST::CallVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Call)
diff --git a/src/ameba/rules/literal_in_condition.cr b/src/ameba/rules/literal_in_condition.cr
index b42fd20d..54244cb1 100644
--- a/src/ameba/rules/literal_in_condition.cr
+++ b/src/ameba/rules/literal_in_condition.cr
@@ -17,9 +17,7 @@ module Ameba::Rules
     include AST::Util
 
     def test(source)
-      AST::IfVisitor.new self, source
-      AST::UnlessVisitor.new self, source
-      AST::CaseVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def check_node(source, node)
diff --git a/src/ameba/rules/literal_in_interpolation.cr b/src/ameba/rules/literal_in_interpolation.cr
index cfa303ba..40664280 100644
--- a/src/ameba/rules/literal_in_interpolation.cr
+++ b/src/ameba/rules/literal_in_interpolation.cr
@@ -13,7 +13,7 @@ module Ameba::Rules
     include AST::Util
 
     def test(source)
-      AST::StringInterpolationVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::StringInterpolation)
diff --git a/src/ameba/rules/method_names.cr b/src/ameba/rules/method_names.cr
index 8f6c53ed..129bd1d6 100644
--- a/src/ameba/rules/method_names.cr
+++ b/src/ameba/rules/method_names.cr
@@ -32,7 +32,7 @@ module Ameba::Rules
   # ```
   struct MethodNames < Rule
     def test(source)
-      AST::DefVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Def)
diff --git a/src/ameba/rules/negated_conditions_in_unless.cr b/src/ameba/rules/negated_conditions_in_unless.cr
index 5b0f855a..27181bf8 100644
--- a/src/ameba/rules/negated_conditions_in_unless.cr
+++ b/src/ameba/rules/negated_conditions_in_unless.cr
@@ -22,7 +22,7 @@ module Ameba::Rules
   #
   struct NegatedConditionsInUnless < Rule
     def test(source)
-      AST::UnlessVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Unless)
diff --git a/src/ameba/rules/predicate_name.cr b/src/ameba/rules/predicate_name.cr
index 96d92220..4e7708d4 100644
--- a/src/ameba/rules/predicate_name.cr
+++ b/src/ameba/rules/predicate_name.cr
@@ -24,7 +24,7 @@ module Ameba::Rules
   #
   struct PredicateName < Rule
     def test(source)
-      AST::DefVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Def)
diff --git a/src/ameba/rules/type_names.cr b/src/ameba/rules/type_names.cr
index e29df1dd..bcc08490 100644
--- a/src/ameba/rules/type_names.cr
+++ b/src/ameba/rules/type_names.cr
@@ -47,13 +47,7 @@ module Ameba::Rules
   #
   struct TypeNames < Rule
     def test(source)
-      [
-        AST::ClassDefVisitor,
-        AST::EnumDefVisitor,
-        AST::ModuleDefVisitor,
-        AST::AliasVisitor,
-        AST::LibDefVisitor,
-      ].each(&.new self, source)
+      AST::Visitor.new self, source
     end
 
     private def check_node(source, node)
diff --git a/src/ameba/rules/unless_else.cr b/src/ameba/rules/unless_else.cr
index 3b4905d7..fe85f4c2 100644
--- a/src/ameba/rules/unless_else.cr
+++ b/src/ameba/rules/unless_else.cr
@@ -38,7 +38,7 @@ module Ameba::Rules
   #
   struct UnlessElse < Rule
     def test(source)
-      AST::UnlessVisitor.new self, source
+      AST::Visitor.new self, source
     end
 
     def test(source, node : Crystal::Unless)
diff --git a/src/ameba/rules/variable_names.cr b/src/ameba/rules/variable_names.cr
index f4e095cc..2a0d7aac 100644
--- a/src/ameba/rules/variable_names.cr
+++ b/src/ameba/rules/variable_names.cr
@@ -26,11 +26,7 @@ module Ameba::Rules
   #
   struct VariableNames < Rule
     def test(source)
-      [
-        AST::VarVisitor,
-        AST::InstanceVarVisitor,
-        AST::ClassVarVisitor,
-      ].each &.new(self, source)
+      AST::Visitor.new self, source
     end
 
     private def check_node(source, node)
diff --git a/src/ameba/source.cr b/src/ameba/source.cr
index 7f92803b..2b3348ae 100644
--- a/src/ameba/source.cr
+++ b/src/ameba/source.cr
@@ -15,6 +15,7 @@ module Ameba
     getter errors = [] of Error
     getter path : String?
     getter content : String
+    getter ast : Crystal::ASTNode?
 
     def initialize(@content : String, @path = nil)
     end
@@ -30,5 +31,12 @@ module Ameba
     def lines
       @lines ||= @content.split("\n")
     end
+
+    def ast
+      @ast ||=
+        Crystal::Parser.new(content)
+                       .tap { |parser| parser.filename = @path }
+                       .parse
+    end
   end
 end