From ddff8d226b4f8be5a622364e65dbbba9962aae69 Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Mon, 10 Jul 2023 04:26:45 +0200
Subject: [PATCH] Add `Performance/MinMaxAfterMap` rule

---
 .../rule/performance/minmax_after_map_spec.cr | 45 +++++++++++++
 .../rule/performance/minmax_after_map.cr      | 67 +++++++++++++++++++
 2 files changed, 112 insertions(+)
 create mode 100644 spec/ameba/rule/performance/minmax_after_map_spec.cr
 create mode 100644 src/ameba/rule/performance/minmax_after_map.cr

diff --git a/spec/ameba/rule/performance/minmax_after_map_spec.cr b/spec/ameba/rule/performance/minmax_after_map_spec.cr
new file mode 100644
index 00000000..c0f4701f
--- /dev/null
+++ b/spec/ameba/rule/performance/minmax_after_map_spec.cr
@@ -0,0 +1,45 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Performance
+  subject = MinMaxAfterMap.new
+
+  describe MinMaxAfterMap do
+    it "passes if there is no potential performance improvements" do
+      expect_no_issues subject, <<-CRYSTAL
+        %w[Alice Bob].map { |name| name.size }.min(2)
+        %w[Alice Bob].map { |name| name.size }.max(2)
+        CRYSTAL
+    end
+
+    it "reports if there is a `min/max/minmax` call followed by `map`" do
+      source = expect_issue subject, <<-CRYSTAL
+        %w[Alice Bob].map { |name| name.size }.min
+                    # ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Use `min_of {...}` instead of `map {...}.min`.
+        %w[Alice Bob].map(&.size).max.zero?
+                    # ^^^^^^^^^^^^^^^ error: Use `max_of {...}` instead of `map {...}.max`.
+        %w[Alice Bob].map(&.size).minmax?
+                    # ^^^^^^^^^^^^^^^^^^^ error: Use `minmax_of? {...}` instead of `map {...}.minmax?`.
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        %w[Alice Bob].min_of { |name| name.size }
+        %w[Alice Bob].max_of(&.size).zero?
+        %w[Alice Bob].minmax_of?(&.size)
+        CRYSTAL
+    end
+
+    it "does not report if source is a spec" do
+      expect_no_issues subject, path: "source_spec.cr", code: <<-CRYSTAL
+        %w[Alice Bob].map(&.size).min
+        CRYSTAL
+    end
+
+    context "macro" do
+      it "doesn't report in macro scope" do
+        expect_no_issues subject, <<-CRYSTAL
+          {{ %w[Alice Bob].map(&.size).min }}
+          CRYSTAL
+      end
+    end
+  end
+end
diff --git a/src/ameba/rule/performance/minmax_after_map.cr b/src/ameba/rule/performance/minmax_after_map.cr
new file mode 100644
index 00000000..1ed54158
--- /dev/null
+++ b/src/ameba/rule/performance/minmax_after_map.cr
@@ -0,0 +1,67 @@
+require "./base"
+
+module Ameba::Rule::Performance
+  # This rule is used to identify usage of `min/max/minmax` calls that follow `map`.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # %w[Alice Bob].map(&.size).min
+  # %w[Alice Bob].map(&.size).max
+  # %w[Alice Bob].map(&.size).minmax
+  # ```
+  #
+  # And it should be written as this:
+  #
+  # ```
+  # %w[Alice Bob].min_of(&.size)
+  # %w[Alice Bob].max_of(&.size)
+  # %w[Alice Bob].minmax_of(&.size)
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Performance/MinMaxAfterMap:
+  #   Enabled: true
+  # ```
+  class MinMaxAfterMap < Base
+    properties do
+      description "Identifies usage of `min/max/minmax` calls that follow `map`"
+    end
+
+    MSG        = "Use `%s {...}` instead of `map {...}.%s`."
+    CALL_NAMES = %w[min min? max max? minmax minmax?]
+
+    def test(source)
+      AST::NodeVisitor.new self, source, skip: :macro
+    end
+
+    def test(source, node : Crystal::Call)
+      return unless node.name.in?(CALL_NAMES) && node.block.nil? && node.args.empty?
+      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 end_location = node.name_end_location
+
+      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
+
+        # FIXME: switching the order of the below calls breaks the corrector
+        corrector.replace(
+          name_location,
+          name_location.adjust(column_number: {{ "map".size - 1 }}),
+          of_name
+        )
+        corrector.remove(
+          node_name_location.adjust(column_number: -1),
+          end_location
+        )
+      end
+    end
+  end
+end