Merge pull request #379 from crystal-ameba/several-tweaks-and-refactors

Several tweaks and refactors

src/ameba/ast/branch.cr

@@ -62,7 +62,7 @@ module Ameba::AST
     # Branch.of(assign_node, def_node)
     # ```
     def self.of(node : Crystal::ASTNode, parent_node : Crystal::ASTNode)
-      BranchVisitor.new(node).tap(&.accept parent_node).branch
+      BranchVisitor.new(node).tap(&.accept(parent_node)).branch
     end
 
     # :nodoc:
@@ -84,6 +84,7 @@ module Ameba::AST
 
         branches.each do |branch_node|
           break if branch # branch found
+
           @current_branch = branch_node if branch_node && !branch_node.nop?
           branch_node.try &.accept(self)
         end
@@ -108,19 +109,11 @@ module Ameba::AST
         true
       end
 
-      def visit(node : Crystal::If)
-        on_branchable_start node, node.cond, node.then, node.else
-      end
-
-      def end_visit(node : Crystal::If)
-        on_branchable_end node
-      end
-
-      def visit(node : Crystal::Unless)
+      def visit(node : Crystal::If | Crystal::Unless)
         on_branchable_start node, node.cond, node.then, node.else
       end
 
-      def end_visit(node : Crystal::Unless)
+      def end_visit(node : Crystal::If | Crystal::Unless)
         on_branchable_end node
       end
 
@@ -140,19 +133,11 @@ module Ameba::AST
         on_branchable_end node
       end
 
-      def visit(node : Crystal::While)
-        on_branchable_start node, node.cond, node.body
-      end
-
-      def end_visit(node : Crystal::While)
-        on_branchable_end node
-      end
-
-      def visit(node : Crystal::Until)
+      def visit(node : Crystal::While | Crystal::Until)
         on_branchable_start node, node.cond, node.body
       end
 
-      def end_visit(node : Crystal::Until)
+      def end_visit(node : Crystal::While | Crystal::Until)
         on_branchable_end node
       end
 

src/ameba/ast/branchable.cr

@@ -15,14 +15,15 @@ module Ameba::AST
   class Branchable
     include Util
 
+    # Parent branchable (if any)
+    getter parent : Branchable?
+
+    # Array of branches
     getter branches = [] of Crystal::ASTNode
 
-    # The actual Crystal node.
+    # The actual Crystal node
     getter node : Crystal::ASTNode
 
-    # Parent branchable (if any)
-    getter parent : Branchable?
-
     delegate to_s, to: @node
     delegate location, to: @node
     delegate end_location, to: @node

src/ameba/ast/flow_expression.cr

@@ -53,8 +53,11 @@ module Ameba::AST
       case current_node = node
       when Crystal::Expressions
         control_flow_found = false
+
         current_node.expressions.each do |exp|
-          unreachable_nodes << exp if control_flow_found
+          if control_flow_found
+            unreachable_nodes << exp
+          end
           control_flow_found ||= !loop?(exp) && flow_expression?(exp, in_loop?)
         end
       when Crystal::BinaryOp

src/ameba/ast/scope.cr

@@ -46,7 +46,7 @@ module Ameba::AST
     # scope = Scope.new(class_node, nil)
     # ```
     def initialize(@node, @outer_scope = nil)
-      @outer_scope.try &.inner_scopes.<<(self)
+      @outer_scope.try &.inner_scopes.<< self
     end
 
     # Creates a new variable in the current scope.
@@ -97,7 +97,8 @@ module Ameba::AST
     # scope.find_variable("foo")
     # ```
     def find_variable(name : String)
-      variables.find(&.name.==(name)) || outer_scope.try &.find_variable(name)
+      variables.find(&.name.==(name)) ||
+        outer_scope.try &.find_variable(name)
     end
 
     # Creates a new assignment for the variable.
@@ -113,7 +114,8 @@ module Ameba::AST
     # Returns `true` if current scope represents a block (or proc),
     # `false` otherwise.
     def block?
-      node.is_a?(Crystal::Block) || node.is_a?(Crystal::ProcLiteral)
+      node.is_a?(Crystal::Block) ||
+        node.is_a?(Crystal::ProcLiteral)
     end
 
     # Returns `true` if current scope represents a spawn block, e. g.
@@ -129,7 +131,9 @@ module Ameba::AST
 
     # Returns `true` if current scope sits inside a macro.
     def in_macro?
-      (node.is_a?(Crystal::Macro) || node.is_a?(Crystal::MacroFor)) ||
+      (node.is_a?(Crystal::Macro) ||
+        node.is_a?(Crystal::MacroIf) ||
+        node.is_a?(Crystal::MacroFor)) ||
         !!outer_scope.try(&.in_macro?)
     end
 
@@ -141,14 +145,16 @@ module Ameba::AST
 
     # Returns `true` if type declaration variable is assigned in this scope.
     def assigns_type_dec?(name)
-      type_dec_variables.any?(&.name.== name) || !!outer_scope.try(&.assigns_type_dec?(name))
+      type_dec_variables.any?(&.name.== name) ||
+        !!outer_scope.try(&.assigns_type_dec?(name))
     end
 
     # Returns `true` if and only if current scope represents some
     # type definition, for example a class.
     def type_definition?
       node.is_a?(Crystal::ClassDef) ||
         node.is_a?(Crystal::ModuleDef) ||
+        node.is_a?(Crystal::EnumDef) ||
         node.is_a?(Crystal::LibDef) ||
         node.is_a?(Crystal::FunDef) ||
         node.is_a?(Crystal::TypeDef) ||
@@ -159,8 +165,8 @@ module Ameba::AST
     # `false` otherwise.
     def references?(variable : Variable, check_inner_scopes = true)
       variable.references.any? do |reference|
-        return true if reference.scope == self
-        check_inner_scopes && inner_scopes.any?(&.references?(variable))
+        (reference.scope == self) ||
+          (check_inner_scopes && inner_scopes.any?(&.references?(variable)))
       end || variable.used_in_macro?
     end
 

src/ameba/ast/variabling/variable.cr

@@ -170,7 +170,7 @@ module Ameba::AST
     private class MacroReferenceFinder < Crystal::Visitor
       property? references = false
 
-      def initialize(node, @reference : String = reference)
+      def initialize(node, @reference : String)
         node.accept self
       end
 
@@ -179,10 +179,6 @@ module Ameba::AST
         val.to_s.includes?(@reference)
       end
 
-      def visit(node : Crystal::ASTNode)
-        true
-      end
-
       def visit(node : Crystal::MacroLiteral)
         !(@references ||= includes_reference?(node.value))
       end
@@ -201,14 +197,20 @@ module Ameba::AST
                           includes_reference?(node.then) ||
                           includes_reference?(node.else))
       end
+
+      def visit(node : Crystal::ASTNode)
+        true
+      end
     end
 
     private def update_assign_reference!
-      if @assign_before_reference.nil? &&
-         references.size <= assignments.size &&
-         assignments.none?(&.op_assign?)
-        @assign_before_reference = assignments.find { |ass| !ass.in_branch? }.try &.node
-      end
+      return if @assign_before_reference
+      return if references.size > assignments.size
+      return if assignments.any?(&.op_assign?)
+
+      @assign_before_reference = assignments.find { |ass|
+        !ass.in_branch?
+      }.try &.node
     end
   end
 end

src/ameba/ast/visitors/flow_expression_visitor.cr

@@ -8,11 +8,6 @@ module Ameba::AST
 
     @loop_stack = [] of Crystal::ASTNode
 
-    # Creates a new flow expression visitor.
-    def initialize(@rule, @source)
-      @source.ast.accept self
-    end
-
     # :nodoc:
     def visit(node)
       if flow_expression?(node, in_loop?)
@@ -22,12 +17,7 @@ module Ameba::AST
     end
 
     # :nodoc:
-    def visit(node : Crystal::While)
-      on_loop_started(node)
-    end
-
-    # :nodoc:
-    def visit(node : Crystal::Until)
+    def visit(node : Crystal::While | Crystal::Until)
       on_loop_started(node)
     end
 
@@ -37,12 +27,7 @@ module Ameba::AST
     end
 
     # :nodoc:
-    def end_visit(node : Crystal::While)
-      on_loop_ended(node)
-    end
-
-    # :nodoc:
-    def end_visit(node : Crystal::Until)
+    def end_visit(node : Crystal::While | Crystal::Until)
       on_loop_ended(node)
     end
 

src/ameba/ast/visitors/scope_visitor.cr

@@ -16,6 +16,7 @@ module Ameba::AST
       ProcLiteral,
       Block,
       Macro,
+      MacroIf,
       MacroFor,
     }
 
@@ -25,33 +26,38 @@ module Ameba::AST
     @scope_queue = [] of Scope
     @current_scope : Scope
     @current_assign : Crystal::ASTNode?
-    @visibility_modifier : Crystal::Visibility?
+    @current_visibility : Crystal::Visibility?
     @skip : Array(Crystal::ASTNode.class)?
 
     def initialize(@rule, @source, skip = nil)
-      @skip = skip.try &.map(&.as(Crystal::ASTNode.class))
       @current_scope = Scope.new(@source.ast) # top level scope
-      @source.ast.accept self
-      @scope_queue.each { |scope| @rule.test @source, scope.node, scope }
+      @skip = skip.try &.map(&.as(Crystal::ASTNode.class))
+
+      super @rule, @source
+
+      @scope_queue.each do |scope|
+        @rule.test @source, scope.node, scope
+      end
     end
 
     private def on_scope_enter(node)
       return if skip?(node)
 
       scope = Scope.new(node, @current_scope)
-      scope.visibility = @visibility_modifier
+      scope.visibility = @current_visibility
 
       @current_scope = scope
     end
 
     private def on_scope_end(node)
       @scope_queue << @current_scope
 
-      @visibility_modifier = nil
+      @current_visibility = nil
 
       # go up if this is not a top level scope
-      return unless outer_scope = @current_scope.outer_scope
-      @current_scope = outer_scope
+      if outer_scope = @current_scope.outer_scope
+        @current_scope = outer_scope
+      end
     end
 
     private def on_assign_end(target, node)
@@ -73,7 +79,7 @@ module Ameba::AST
 
     # :nodoc:
     def visit(node : Crystal::VisibilityModifier)
-      @visibility_modifier = node.exp.visibility = node.modifier
+      @current_visibility = node.exp.visibility = node.modifier
       true
     end
 
@@ -96,20 +102,23 @@ module Ameba::AST
     def end_visit(node : Crystal::Assign | Crystal::OpAssign)
       on_assign_end(node.target, node)
       @current_assign = nil
+
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
     # :nodoc:
     def end_visit(node : Crystal::MultiAssign)
       node.targets.each { |target| on_assign_end(target, node) }
       @current_assign = nil
+
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
     # :nodoc:
     def end_visit(node : Crystal::UninitializedVar)
       on_assign_end(node.var, node)
       @current_assign = nil
+
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
@@ -119,7 +128,8 @@ module Ameba::AST
 
       @current_scope.add_variable(var)
       @current_scope.add_type_dec_variable(node)
-      @current_assign = node.value unless node.value.nil?
+
+      @current_assign = node.value if node.value
     end
 
     # :nodoc:
@@ -128,6 +138,7 @@ module Ameba::AST
 
       on_assign_end(var, node)
       @current_assign = nil
+
       on_scope_end(node) if @current_scope.eql?(node)
     end
 
@@ -165,7 +176,9 @@ module Ameba::AST
         if node.name.in?(SPECIAL_NODE_NAMES) && node.args.empty?
           @current_scope.arguments.each do |arg|
             variable = arg.variable
-            variable.reference(variable.node, @current_scope).explicit = false
+
+            ref = variable.reference(variable.node, @current_scope)
+            ref.explicit = false
           end
         end
         true