From abe5237802e6693da3e2338538cc38bf7a1f8591 Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Fri, 30 Jun 2023 03:00:01 +0200
Subject: [PATCH] Add `Performance/ExcessiveAllocations` rule

---
 .../performance/excessive_allocations_spec.cr | 57 +++++++++++++++
 .../rule/performance/excessive_allocations.cr | 70 +++++++++++++++++++
 2 files changed, 127 insertions(+)
 create mode 100644 spec/ameba/rule/performance/excessive_allocations_spec.cr
 create mode 100644 src/ameba/rule/performance/excessive_allocations.cr

diff --git a/spec/ameba/rule/performance/excessive_allocations_spec.cr b/spec/ameba/rule/performance/excessive_allocations_spec.cr
new file mode 100644
index 00000000..010c5064
--- /dev/null
+++ b/spec/ameba/rule/performance/excessive_allocations_spec.cr
@@ -0,0 +1,57 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Performance
+  subject = ExcessiveAllocations.new
+
+  describe ExcessiveAllocations do
+    it "passes if there is no potential performance improvements" do
+      expect_no_issues subject, <<-CRYSTAL
+        "Alice".chars.each(arg) { |c| puts c }
+        "Alice".chars(arg).each { |c| puts c }
+        "Alice\nBob".lines.each(arg) { |l| puts l }
+        "Alice\nBob".lines(arg).each { |l| puts l }
+        CRYSTAL
+    end
+
+    it "reports if there is a collection method followed by each" do
+      source = expect_issue subject, <<-CRYSTAL
+        "Alice".chars.each { |c| puts c }
+              # ^^^^^^^^^^ error: Use `each_char {...}` instead of `chars.each {...}` to avoid excessive allocation
+        "Alice\nBob".lines.each { |l| puts l }
+           # ^^^^^^^^^^ error: Use `each_line {...}` instead of `lines.each {...}` to avoid excessive allocation
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        "Alice".each_char { |c| puts c }
+        "Alice\nBob".each_line { |l| puts l }
+        CRYSTAL
+    end
+
+    it "does not report if source is a spec" do
+      expect_no_issues subject, <<-CRYSTAL, "source_spec.cr"
+        "Alice".chars.each { |c| puts c }
+        CRYSTAL
+    end
+
+    context "properties" do
+      it "#call_names" do
+        rule = ExcessiveAllocations.new
+        rule.call_names = {
+          "children" => "each_child",
+        }
+
+        expect_no_issues rule, <<-CRYSTAL
+          "Alice".chars.each { |c| puts c }
+          CRYSTAL
+      end
+    end
+
+    context "macro" do
+      it "doesn't report in macro scope" do
+        expect_no_issues subject, <<-CRYSTAL
+          {{ "Alice".chars.each { |c| puts c } }}
+          CRYSTAL
+      end
+    end
+  end
+end
diff --git a/src/ameba/rule/performance/excessive_allocations.cr b/src/ameba/rule/performance/excessive_allocations.cr
new file mode 100644
index 00000000..db8ad968
--- /dev/null
+++ b/src/ameba/rule/performance/excessive_allocations.cr
@@ -0,0 +1,70 @@
+require "./base"
+
+module Ameba::Rule::Performance
+  # This rule is used to identify excessive collection allocations,
+  # that can be avoided by using `each_<member>` instead of `<collection>.each`.
+  #
+  # For example, this is considered inefficient:
+  #
+  # ```
+  # "Alice".chars.each { |c| puts c }
+  # "Alice\nBob".lines.each { |l| puts l }
+  # ```
+  #
+  # And can be written as this:
+  #
+  # ```
+  # "Alice".each_char { |c| puts c }
+  # "Alice\nBob".each_line { |l| puts l }
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Performance/ExcessiveAllocations:
+  #   Enabled: true
+  #   CallNames:
+  #     codepoints: each_codepoint
+  #     graphemes: each_grapheme
+  #     chars: each_char
+  #     lines: each_line
+  # ```
+  class ExcessiveAllocations < Base
+    include AST::Util
+
+    properties do
+      description "Identifies usage of excessive collection allocations"
+      call_names({
+        "codepoints" => "each_codepoint",
+        "graphemes"  => "each_grapheme",
+        "chars"      => "each_char",
+        "lines"      => "each_line",
+        # "keys"       => "each_key",
+        # "values"     => "each_value",
+        # "children"   => "each_child",
+      })
+    end
+
+    MSG = "Use `%s {...}` instead of `%s.each {...}` to avoid excessive allocation"
+
+    def test(source)
+      AST::NodeVisitor.new self, source, skip: :macro
+    end
+
+    def test(source, node : Crystal::Call)
+      return unless node.name == "each" && node.args.empty?
+      return unless (obj = node.obj).is_a?(Crystal::Call)
+      return unless obj.args.empty? && obj.block.nil?
+      return unless method = call_names[obj.name]?
+
+      return unless name_location = obj.name_location
+      return unless end_location = name_end_location(node)
+
+      msg = MSG % {method, obj.name}
+
+      issue_for name_location, end_location, msg do |corrector|
+        corrector.replace(name_location, end_location, method)
+      end
+    end
+  end
+end