Merge pull request #422 from crystal-ameba/refactor-adding-issues-with-name-location

Make it easier to add issues for nodes with name location preference

src/ameba/reportable.cr

@@ -1,6 +1,10 @@
+require "./ast/util"
+
 module Ameba
   # Represents a module used to report issues.
   module Reportable
+    include AST::Util
+
     # List of reported issues.
     getter issues = [] of Issue
 
@@ -30,13 +34,19 @@ module Ameba
     end
 
     # Adds a new issue for Crystal AST *node*.
-    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue
+    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil, *, prefer_name_location = false) : Issue
-      add_issue rule, node.location, node.end_location, message, status, block
+      location = name_location(node) if prefer_name_location
+      location ||= node.location
+
+      end_location = name_end_location(node) if prefer_name_location
+      end_location ||= node.end_location
+
+      add_issue rule, location, end_location, message, status, block
     end
 
     # :ditto:
-    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue
+    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, *, prefer_name_location = false, &block : Source::Corrector ->) : Issue
-      add_issue rule, node, message, status, block
+      add_issue rule, node, message, status, block, prefer_name_location: prefer_name_location
     end
 
     # Adds a new issue for Crystal *token*.

src/ameba/rule/lint/missing_block_argument.cr

@@ -20,8 +20,6 @@ module Ameba::Rule::Lint
   #   Enabled: true
   # ```
   class MissingBlockArgument < Base
-    include AST::Util
-
     properties do
       description "Disallows yielding method definitions without block argument"
     end
@@ -36,10 +34,7 @@ module Ameba::Rule::Lint
     def test(source, node : Crystal::Def, scope : AST::Scope)
       return if !scope.yields? || node.block_arg
 
-      return unless location = node.name_location
+      issue_for node, MSG, prefer_name_location: true
-      end_location = name_end_location(node)
-
-      issue_for location, end_location, MSG
     end
   end
 end

src/ameba/rule/lint/not_nil.cr

@@ -26,8 +26,6 @@ module Ameba::Rule::Lint
   #   Enabled: true
   # ```
   class NotNil < Base
-    include AST::Util
-
     properties do
       description "Identifies usage of `not_nil!` calls"
     end
@@ -42,10 +40,7 @@ module Ameba::Rule::Lint
       return unless node.name == "not_nil!"
       return unless node.obj && node.args.empty?
 
-      return unless name_location = node.name_location
+      issue_for node, MSG, prefer_name_location: true
-      return unless end_location = name_end_location(node)
-
-      issue_for name_location, end_location, MSG
     end
   end
 end

src/ameba/rule/lint/not_nil_after_no_bang.cr

@@ -41,7 +41,7 @@ module Ameba::Rule::Lint
       return unless (obj = node.obj).is_a?(Crystal::Call)
       return unless obj.name.in?(obj.block ? BLOCK_CALL_NAMES : CALL_NAMES)
 
-      return unless name_location = obj.name_location
+      return unless name_location = name_location(obj)
       return unless name_location_end = name_end_location(obj)
       return unless end_location = name_end_location(node)
 

src/ameba/rule/lint/redundant_string_coercion.cr

@@ -31,7 +31,7 @@ module Ameba::Rule::Lint
 
     def test(source, node : Crystal::StringInterpolation)
       string_coercion_nodes(node).each do |expr|
-        issue_for expr.name_location, expr.end_location, MSG
+        issue_for name_location(expr), expr.end_location, MSG
       end
     end
 

src/ameba/rule/lint/redundant_with_index.cr

@@ -51,7 +51,7 @@ module Ameba::Rule::Lint
     end
 
     private def report(source, node, msg)
-      issue_for node.name_location, node.name_end_location, msg
+      issue_for node, msg, prefer_name_location: true
     end
   end
 end

src/ameba/rule/lint/redundant_with_object.cr

@@ -40,7 +40,7 @@ module Ameba::Rule::Lint
                 !(block = node.block) ||
                 with_index_arg?(block)
 
-      issue_for node.name_location, node.name_end_location, MSG
+      issue_for node, MSG, prefer_name_location: true
     end
 
     private def with_index_arg?(block : Crystal::Block)

src/ameba/rule/metrics/cyclomatic_complexity.cr

@@ -9,8 +9,6 @@ module Ameba::Rule::Metrics
   #   MaxComplexity: 10
   # ```
   class CyclomaticComplexity < Base
-    include AST::Util
-
     properties do
       description "Disallows methods with a cyclomatic complexity higher than `MaxComplexity`"
       max_complexity 10
@@ -22,10 +20,7 @@ module Ameba::Rule::Metrics
       complexity = AST::CountingVisitor.new(node).count
       return unless complexity > max_complexity
 
-      return unless location = node.name_location
+      issue_for node, MSG % {complexity, max_complexity}, prefer_name_location: true
-      end_location = name_end_location(node)
-
-      issue_for location, end_location, MSG % {complexity, max_complexity}
     end
   end
 end

src/ameba/rule/naming/accessor_method_name.cr

@@ -36,8 +36,6 @@ module Ameba::Rule::Naming
   #   Enabled: true
   # ```
   class AccessorMethodName < Base
-    include AST::Util
-
     properties do
       description "Makes sure that accessor methods are named properly"
     end
@@ -66,24 +64,13 @@ module Ameba::Rule::Naming
     end
 
     private def check_issue(source, node : Crystal::Def)
-      location = name_location(node)
-      end_location = name_end_location(node)
-
       case node.name
       when /^get_([a-z]\w*)$/
         return unless node.args.empty?
-        if location && end_location
+        issue_for node, MSG % {$1, node.name}, prefer_name_location: true
-          issue_for location, end_location, MSG % {$1, node.name}
-        else
-          issue_for node, MSG % {$1, node.name}
-        end
       when /^set_([a-z]\w*)$/
         return unless node.args.size == 1
-        if location && end_location
+        issue_for node, MSG % {"#{$1}=", node.name}, prefer_name_location: true
-          issue_for location, end_location, MSG % {"#{$1}=", node.name}
-        else
-          issue_for node, MSG % {"#{$1}=", node.name}
-        end
       end
     end
   end

src/ameba/rule/naming/ascii_identifiers.cr

@@ -22,8 +22,6 @@ module Ameba::Rule::Naming
   #   Enabled: true
   # ```
   class AsciiIdentifiers < Base
-    include AST::Util
-
     properties do
       description "Disallows non-ascii characters in identifiers"
     end
@@ -43,38 +41,27 @@ module Ameba::Rule::Naming
     end
 
     def test(source, node : Crystal::Def)
-      check_issue_with_location(source, node)
+      check_issue(source, node, prefer_name_location: true)
 
       node.args.each do |arg|
-        check_issue_with_location(source, arg)
+        check_issue(source, arg, prefer_name_location: true)
       end
     end
 
     def test(source, node : Crystal::ClassVar | Crystal::InstanceVar | Crystal::Var | Crystal::Alias)
-      check_issue_with_location(source, node)
+      check_issue(source, node, prefer_name_location: true)
     end
 
     def test(source, node : Crystal::ClassDef | Crystal::ModuleDef | Crystal::EnumDef | Crystal::LibDef)
       check_issue(source, node.name, node.name)
     end
 
-    private def check_issue_with_location(source, node)
-      location = name_location(node)
-      end_location = name_end_location(node)
-
-      if location && end_location
-        check_issue(source, location, end_location, node.name)
-      else
-        check_issue(source, node, node.name)
-      end
-    end
-
     private def check_issue(source, location, end_location, name)
       issue_for location, end_location, MSG unless name.to_s.ascii_only?
     end
 
-    private def check_issue(source, node, name)
+    private def check_issue(source, node, name = node.name, *, prefer_name_location = false)
-      issue_for node, MSG unless name.to_s.ascii_only?
+      issue_for node, MSG, prefer_name_location: prefer_name_location unless name.to_s.ascii_only?
     end
   end
 end

src/ameba/rule/naming/block_parameter_name.cr

@@ -20,7 +20,7 @@ module Ameba::Rule::Naming
   #   Enabled: true
   #   MinNameLength: 3
   #   AllowNamesEndingInNumbers: true
-  #   AllowedNames: [_, e, i, j, k, v, x, y, ex, io, ws, tx, id, k1, k2, v1, v2]
+  #   AllowedNames: [_, e, i, j, k, v, x, y, ex, io, ws, op, tx, id, ip, k1, k2, v1, v2]
   #   ForbiddenNames: []
   # ```
   class BlockParameterName < Base
@@ -28,7 +28,7 @@ module Ameba::Rule::Naming
       description "Disallows non-descriptive block parameter names"
       min_name_length 3
       allow_names_ending_in_numbers true
-      allowed_names %w[_ e i j k v x y ex io ws tx id k1 k2 v1 v2]
+      allowed_names %w[_ e i j k v x y ex io ws op tx id ip k1 k2 v1 v2]
       forbidden_names %w[]
     end
 

src/ameba/rule/naming/method_names.cr

@@ -38,8 +38,6 @@ module Ameba::Rule::Naming
   #   Enabled: true
   # ```
   class MethodNames < Base
-    include AST::Util
-
     properties do
       description "Enforces method names to be in underscored case"
     end
@@ -49,10 +47,7 @@ module Ameba::Rule::Naming
     def test(source, node : Crystal::Def)
       return if (expected = node.name.underscore) == node.name
 
-      return unless location = name_location(node)
+      issue_for node, MSG % {expected, node.name}, prefer_name_location: true
-      return unless end_location = name_end_location(node)
-
-      issue_for location, end_location, MSG % {expected, node.name}
     end
   end
 end

src/ameba/rule/performance/any_after_filter.cr

@@ -27,6 +27,8 @@ module Ameba::Rule::Performance
   #     - reject
   # ```
   class AnyAfterFilter < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `any?` calls that follow filters"
       filter_names %w(select reject)
@@ -39,7 +41,7 @@ module Ameba::Rule::Performance
       return unless obj.is_a?(Crystal::Call) && obj.block && node.block.nil?
       return unless obj.name.in?(filter_names)
 
-      issue_for obj.name_location, node.name_end_location, MSG % obj.name
+      issue_for name_location(obj), name_end_location(node), MSG % obj.name
     end
   end
 end

src/ameba/rule/performance/any_instead_of_empty.cr

@@ -28,8 +28,6 @@ module Ameba::Rule::Performance
   #   Enabled: true
   # ```
   class AnyInsteadOfEmpty < Base
-    include AST::Util
-
     properties do
       description "Identifies usage of arg-less `any?` calls"
     end
@@ -41,10 +39,7 @@ module Ameba::Rule::Performance
       return unless node.block.nil? && node.args.empty?
       return unless node.obj
 
-      return unless name_location = node.name_location
+      issue_for node, MSG, prefer_name_location: true
-      return unless end_location = name_end_location(node)
-
-      issue_for name_location, end_location, MSG
     end
   end
 end

src/ameba/rule/performance/chained_call_with_no_bang.cr

@@ -67,11 +67,12 @@ module Ameba::Rule::Performance
       return unless node.name.in?(call_names)
       return unless obj.name.in?(call_names) || obj.name.in?(ALLOCATING_METHOD_NAMES)
 
-      return unless location = node.name_location
+      if end_location = name_end_location(node)
-      return unless end_location = name_end_location(node)
+        issue_for node, MSG % {node.name, obj.name}, prefer_name_location: true do |corrector|
-
+          corrector.insert_after(end_location, '!')
-      issue_for location, end_location, MSG % {node.name, obj.name} do |corrector|
+        end
-        corrector.insert_after(end_location, '!')
+      else
+        issue_for node, MSG % {node.name, obj.name}, prefer_name_location: true
       end
     end
   end

src/ameba/rule/performance/compact_after_map.cr

@@ -22,6 +22,8 @@ module Ameba::Rule::Performance
   #   Enabled: true
   # ```
   class CompactAfterMap < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `compact` calls that follow `map`"
     end
@@ -37,7 +39,7 @@ module Ameba::Rule::Performance
       return unless obj.is_a?(Crystal::Call) && obj.block
       return unless obj.name == "map"
 
-      issue_for obj.name_location, node.name_end_location, MSG
+      issue_for name_location(obj), name_end_location(node), MSG
     end
   end
 end

src/ameba/rule/performance/excessive_allocations.cr

@@ -57,7 +57,7 @@ module Ameba::Rule::Performance
       return unless obj.args.empty? && obj.block.nil?
       return unless method = call_names[obj.name]?
 
-      return unless name_location = obj.name_location
+      return unless name_location = name_location(obj)
       return unless end_location = name_end_location(node)
 
       msg = MSG % {method, obj.name}

src/ameba/rule/performance/first_last_after_filter.cr

@@ -26,6 +26,8 @@ module Ameba::Rule::Performance
   #     - select
   # ```
   class FirstLastAfterFilter < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `first/last/first?/last?` calls that follow filters"
       filter_names %w(select)
@@ -47,7 +49,7 @@ module Ameba::Rule::Performance
 
       message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE
 
-      issue_for obj.name_location, node.name_end_location,
+      issue_for name_location(obj), name_end_location(node),
         message % {obj.name, node.name}
     end
   end

src/ameba/rule/performance/flatten_after_map.cr

@@ -22,6 +22,8 @@ module Ameba::Rule::Performance
   #   Enabled: true
   # ```
   class FlattenAfterMap < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `flatten` calls that follow `map`"
     end
@@ -37,7 +39,7 @@ module Ameba::Rule::Performance
       return unless obj.is_a?(Crystal::Call) && obj.block
       return unless obj.name == "map"
 
-      issue_for obj.name_location, node.name_end_location, MSG
+      issue_for name_location(obj), name_end_location(node), MSG
     end
   end
 end

src/ameba/rule/performance/map_instead_of_block.cr

@@ -23,6 +23,8 @@ module Ameba::Rule::Performance
   #   Enabled: true
   # ```
   class MapInsteadOfBlock < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `sum/product` calls that follow `map`"
     end
@@ -40,7 +42,7 @@ module Ameba::Rule::Performance
       return unless obj.is_a?(Crystal::Call) && obj.block
       return unless obj.name == MAP_NAME
 
-      issue_for obj.name_location, node.name_end_location,
+      issue_for name_location(obj), name_end_location(node),
         MSG % {node.name, node.name}
     end
   end

src/ameba/rule/performance/minmax_after_map.cr

@@ -26,6 +26,8 @@ module Ameba::Rule::Performance
   #   Enabled: true
   # ```
   class MinMaxAfterMap < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `min/max/minmax` calls that follow `map`"
     end
@@ -42,14 +44,14 @@ module Ameba::Rule::Performance
       return unless (obj = node.obj) && obj.is_a?(Crystal::Call)
       return unless obj.name == "map" && obj.block && obj.args.empty?
 
-      return unless name_location = obj.name_location
+      return unless name_location = name_location(obj)
-      return unless end_location = node.name_end_location
+      return unless end_location = name_end_location(node)
 
       of_name = node.name.sub(/(.+?)(\?)?$/, "\\1_of\\2")
       message = MSG % {of_name, node.name}
 
       issue_for name_location, end_location, message do |corrector|
-        next unless node_name_location = node.name_location
+        next unless node_name_location = name_location(node)
 
         # TODO: switching the order of the below calls breaks the corrector
         corrector.replace(

src/ameba/rule/performance/size_after_filter.cr

@@ -33,6 +33,8 @@ module Ameba::Rule::Performance
   #     - reject
   # ```
   class SizeAfterFilter < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `size` calls that follow filter"
       filter_names %w(select reject)
@@ -49,7 +51,7 @@ module Ameba::Rule::Performance
       return unless obj.is_a?(Crystal::Call) && obj.block
       return unless obj.name.in?(filter_names)
 
-      issue_for obj.name_location, node.name_end_location, MSG % obj.name
+      issue_for name_location(obj), name_end_location(node), MSG % obj.name
     end
   end
 end

src/ameba/rule/style/is_a_filter.cr

@@ -39,6 +39,8 @@ module Ameba::Rule::Style
   #     - one?
   # ```
   class IsAFilter < Base
+    include AST::Util
+
     properties do
       description "Identifies usage of `is_a?/nil?` calls within filters"
       filter_names %w(select reject any? all? none? one?)
@@ -54,7 +56,7 @@ module Ameba::Rule::Style
 
     def test(source, node : Crystal::Call)
       return unless node.name.in?(filter_names)
-      return unless filter_location = node.name_location
+      return unless filter_location = name_location(node)
       return unless block = node.block
       return unless (body = block.body).is_a?(Crystal::IsA)
       return unless (path = body.const).is_a?(Crystal::Path)

src/ameba/rule/style/verbose_block.cr

@@ -47,7 +47,7 @@ module Ameba::Rule::Style
     CALL_PATTERN = "%s(%s&.%s)"
 
     protected def same_location_lines?(a, b)
-      return unless a_location = a.name_location
+      return unless a_location = name_location(a)
       return unless b_location = b.location
 
       a_location.line_number == b_location.line_number
@@ -78,7 +78,7 @@ module Ameba::Rule::Style
 
     protected def valid_line_length?(node, code)
       if max_line_length = self.max_line_length
-        if location = node.name_location
+        if location = name_location(node)
           final_line_length = location.column_number + code.size
           return final_line_length <= max_line_length
         end
@@ -203,7 +203,7 @@ module Ameba::Rule::Style
       return unless valid_line_length?(call, call_code)
       return unless valid_length?(call_code)
 
-      return unless location = call.name_location
+      return unless location = name_location(call)
       return unless end_location = block.end_location
 
       if call_code.includes?("{...}")