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)
+      def visit(node : Crystal::If | Crystal::Unless)
         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)
-        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)
+      def visit(node : Crystal::While | Crystal::Until)
         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)
-        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
 
-    getter branches = [] of Crystal::ASTNode
-
-    # The actual Crystal node.
-    getter node : Crystal::ASTNode
-
     # Parent branchable (if any)
     getter parent : Branchable?
 
+    # Array of branches
+    getter branches = [] of Crystal::ASTNode
+
+    # The actual Crystal node
+    getter node : Crystal::ASTNode
+
     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,7 +145,8 @@ 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
@@ -149,6 +154,7 @@ module Ameba::AST
     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,21 +26,25 @@ 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
@@ -47,11 +52,12 @@ module Ameba::AST
     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,6 +102,7 @@ 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
 
@@ -103,6 +110,7 @@ module Ameba::AST
     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
 
@@ -110,6 +118,7 @@ module Ameba::AST
     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

src/ameba/config.cr

@@ -246,7 +246,7 @@ class Ameba::Config
       {% if block.body.is_a? Assign %}
         {% definitions << {var: block.body.target, value: block.body.value} %}
       {% elsif block.body.is_a? Call %}
-          {% definitions << {var: block.body.name, value: block.body.args.first} %}
+        {% definitions << {var: block.body.name, value: block.body.args.first} %}
       {% elsif block.body.is_a? TypeDeclaration %}
         {% definitions << {var: block.body.var, value: block.body.value, type: block.body.type} %}
       {% elsif block.body.is_a? Expressions %}
@@ -274,7 +274,7 @@ class Ameba::Config
           {% converter = SeverityYamlConverter %}
         {% end %}
 
-        {% if type == nil %}
+        {% unless type %}
           {% if value.is_a? BoolLiteral %}
             {% type = Bool %}
           {% elsif value.is_a? StringLiteral %}
@@ -284,23 +284,23 @@ class Ameba::Config
               {% type = Int32 %}
             {% elsif value.kind == :i64 %}
               {% type = Int64 %}
+            {% elsif value.kind == :i128 %}
+              {% type = Int128 %}
             {% elsif value.kind == :f32 %}
               {% type = Float32 %}
             {% elsif value.kind == :f64 %}
               {% type = Float64 %}
             {% end %}
           {% end %}
-
-          {% type = Nil if type == nil %}
         {% end %}
 
         {% properties[name] = {key: key, default: value, type: type, converter: converter} %}
 
-        @[YAML::Field(key: {{ key }}, converter: {{ converter }}, type: {{ type }})]
+        @[YAML::Field(key: {{ key }}, converter: {{ converter }})]
         {% if type == Bool %}
-          property? {{ name }} : {{ type }} = {{ value }}
+          property? {{ name }}{{ " : #{type}".id if type }} = {{ value }}
         {% else %}
-          property {{ name }} : {{ type }} = {{ value }}
+          property {{ name }}{{ " : #{type}".id if type }} = {{ value }}
         {% end %}
       {% end %}
 

src/ameba/rule/lint/debug_calls.cr

@@ -18,7 +18,7 @@ module Ameba::Rule::Lint
   class DebugCalls < Base
     properties do
       description "Disallows debug-related calls"
-      method_names : Array(String) = %w(p p! pp pp!)
+      method_names %w(p p! pp pp!)
     end
 
     MSG = "Possibly forgotten debug-related `%s` call detected"

src/ameba/rule/lint/documentation.cr

@@ -7,6 +7,12 @@ module Ameba::Rule::Lint
   # ```
   # Lint/Documentation:
   #   Enabled: true
+  #   IgnoreClasses: false
+  #   IgnoreModules: true
+  #   IgnoreEnums: false
+  #   IgnoreDefs: true
+  #   IgnoreMacros: false
+  #   IgnoreMacroHooks: true
   # ```
   class Documentation < Base
     properties do
@@ -18,6 +24,7 @@ module Ameba::Rule::Lint
       ignore_enums false
       ignore_defs true
       ignore_macros false
+      ignore_macro_hooks true
     end
 
     MSG = "Missing documentation"
@@ -50,14 +57,21 @@ module Ameba::Rule::Lint
     end
 
     def test(source, node : Crystal::Macro, scope : AST::Scope)
-      node.name.in?(MACRO_HOOK_NAMES) ||
-        ignore_macros? || check_missing_doc(source, node, scope)
+      return if ignore_macro_hooks? && node.name.in?(MACRO_HOOK_NAMES)
+
+      ignore_macros? || check_missing_doc(source, node, scope)
     end
 
     private def check_missing_doc(source, node, scope)
-      visibility = scope.visibility
+      # bail out if the node is not public,
+      # i.e. `private def foo`
+      return if !node.visibility.public?
 
-      return if visibility && !visibility.public?
+      # bail out if the scope is not public,
+      # i.e. `def bar` inside `private class Foo`
+      return if (visibility = scope.visibility) && !visibility.public?
+
+      # bail out if the node has the documentation present
       return if node.doc.presence
 
       issue_for(node, MSG)

src/ameba/rule/lint/unused_argument.cr

@@ -42,23 +42,26 @@ module Ameba::Rule::Lint
     end
 
     def test(source, node : Crystal::ProcLiteral, scope : AST::Scope)
-      ignore_procs? || find_unused_arguments source, scope
+      ignore_procs? || find_unused_arguments(source, scope)
     end
 
     def test(source, node : Crystal::Block, scope : AST::Scope)
-      ignore_blocks? || find_unused_arguments source, scope
+      ignore_blocks? || find_unused_arguments(source, scope)
     end
 
     def test(source, node : Crystal::Def, scope : AST::Scope)
+      arguments = scope.arguments.dup
+
       # `Lint/UnusedBlockArgument` rule covers this case explicitly
       if block_arg = node.block_arg
-        scope.arguments.reject!(&.node.== block_arg)
+        arguments.reject!(&.node.== block_arg)
       end
-      ignore_defs? || find_unused_arguments source, scope
+
+      ignore_defs? || find_unused_arguments(source, scope, arguments)
     end
 
-    private def find_unused_arguments(source, scope)
-      scope.arguments.each do |argument|
+    private def find_unused_arguments(source, scope, arguments = scope.arguments)
+      arguments.each do |argument|
         next if argument.anonymous? || argument.ignored?
         next if scope.references?(argument.variable)
 

src/ameba/rule/lint/useless_condition_in_when.cr

@@ -37,11 +37,11 @@ module Ameba::Rule::Lint
 
     MSG = "Useless condition in when detected"
 
-    # TODO: condition.cond may be a complex ASTNode with
+    # TODO: condition *cond* may be a complex ASTNode with
     # useless inner conditions. We might need to improve this
     # simple implementation in future.
     protected def check_node(source, when_node, cond)
-      cond_s = cond.to_s
+      return unless cond_s = cond.to_s.presence
       return if when_node.conds.none?(&.to_s.==(cond_s))
 
       issue_for cond, MSG

src/ameba/rule/performance/any_after_filter.cr

@@ -29,7 +29,7 @@ module Ameba::Rule::Performance
   class AnyAfterFilter < Base
     properties do
       description "Identifies usage of `any?` calls that follow filters"
-      filter_names : Array(String) = %w(select reject)
+      filter_names %w(select reject)
     end
 
     ANY_NAME = "any?"

src/ameba/rule/performance/chained_call_with_no_bang.cr

@@ -45,7 +45,7 @@ module Ameba::Rule::Performance
       # All of those have bang method variants returning `self`
       # and are not modifying the receiver type (like `compact` does),
       # thus are safe to switch to the bang variant.
-      call_names : Array(String) = %w(uniq sort sort_by shuffle reverse)
+      call_names %w(uniq sort sort_by shuffle reverse)
     end
 
     # All these methods are allocating a new object

src/ameba/rule/performance/first_last_after_filter.cr

@@ -28,7 +28,7 @@ module Ameba::Rule::Performance
   class FirstLastAfterFilter < Base
     properties do
       description "Identifies usage of `first/last/first?/last?` calls that follow filters"
-      filter_names : Array(String) = %w(select)
+      filter_names %w(select)
     end
 
     CALL_NAMES  = %w(first last first? last?)

src/ameba/rule/performance/size_after_filter.cr

@@ -35,7 +35,7 @@ module Ameba::Rule::Performance
   class SizeAfterFilter < Base
     properties do
       description "Identifies usage of `size` calls that follow filter"
-      filter_names : Array(String) = %w(select reject)
+      filter_names %w(select reject)
     end
 
     SIZE_NAME = "size"

src/ameba/rule/style/guard_clause.cr

@@ -63,7 +63,9 @@ module Ameba::Rule::Style
           "code inside a conditional expression."
 
     def test(source)
-      AST::NodeVisitor.new self, source, skip: [Crystal::Assign]
+      AST::NodeVisitor.new self, source, skip: [
+        Crystal::Assign,
+      ]
     end
 
     def test(source, node : Crystal::Def)
@@ -118,6 +120,7 @@ module Ameba::Rule::Style
 
         issue_for keyword_loc, keyword_end_loc, MSG % example do |corrector|
           replacement = "#{scope_exiting_keyword} #{conditional_keyword}"
+
           corrector.replace(keyword_loc, keyword_end_loc, replacement)
           corrector.remove(end_loc, end_end_loc)
         end

src/ameba/rule/style/is_a_filter.cr

@@ -41,7 +41,7 @@ module Ameba::Rule::Style
   class IsAFilter < Base
     properties do
       description "Identifies usage of `is_a?/nil?` calls within filters"
-      filter_names : Array(String) = %w(select reject any? all? none? one?)
+      filter_names %w(select reject any? all? none? one?)
     end
 
     MSG = "Use `%s` instead of `%s`"