From 95d340c3ad9aa44b79a7ce3377718f2b25fc95db Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Sun, 24 Jan 2021 20:23:32 +0100
Subject: [PATCH] Add Style/IsAFilter rule

---
 spec/ameba/rule/style/is_a_filter_spec.cr | 64 ++++++++++++++++++++
 src/ameba/rule/style/is_a_filter.cr       | 74 +++++++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 spec/ameba/rule/style/is_a_filter_spec.cr
 create mode 100644 src/ameba/rule/style/is_a_filter.cr

diff --git a/spec/ameba/rule/style/is_a_filter_spec.cr b/spec/ameba/rule/style/is_a_filter_spec.cr
new file mode 100644
index 00000000..1a49c2d1
--- /dev/null
+++ b/spec/ameba/rule/style/is_a_filter_spec.cr
@@ -0,0 +1,64 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Style
+  subject = IsAFilter.new
+
+  describe IsAFilter do
+    it "passes if there is no potential performance improvements" do
+      source = Source.new %(
+        [1, 2, nil].select(Int32)
+        [1, 2, nil].reject(Nil)
+      )
+      subject.catch(source).should be_valid
+    end
+
+    it "reports if there is .is_a? call within select" do
+      source = Source.new %(
+        [1, 2, nil].select(&.is_a?(Int32))
+      )
+      subject.catch(source).should_not be_valid
+    end
+
+    it "reports if there is .nil? call within reject" do
+      source = Source.new %(
+        [1, 2, nil].reject(&.nil?)
+      )
+      subject.catch(source).should_not be_valid
+    end
+
+    context "properties" do
+      it "allows to configure filter_names" do
+        source = Source.new %(
+          [1, 2, nil].reject(&.nil?)
+        )
+        rule = IsAFilter.new
+        rule.filter_names = %w(select)
+        rule.catch(source).should be_valid
+      end
+    end
+
+    context "macro" do
+      it "reports in macro scope" do
+        source = Source.new %(
+          {{ [1, 2, nil].reject(&.nil?) }}
+        )
+        subject.catch(source).should_not be_valid
+      end
+    end
+
+    it "reports rule, pos and message" do
+      source = Source.new path: "source.cr", code: %(
+        [1, 2, nil].reject(&.nil?)
+      )
+      subject.catch(source).should_not be_valid
+      source.issues.size.should eq 1
+
+      issue = source.issues.first
+      issue.rule.should_not be_nil
+      issue.location.to_s.should eq "source.cr:1:13"
+      issue.end_location.to_s.should eq "source.cr:1:26"
+
+      issue.message.should eq "Use `reject(Nil)` instead of `reject {...}`"
+    end
+  end
+end
diff --git a/src/ameba/rule/style/is_a_filter.cr b/src/ameba/rule/style/is_a_filter.cr
new file mode 100644
index 00000000..fedeedec
--- /dev/null
+++ b/src/ameba/rule/style/is_a_filter.cr
@@ -0,0 +1,74 @@
+module Ameba::Rule::Style
+  # This rule is used to identify usage of `is_a?/nil?` calls within filters.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # matches = %w[Alice Bob].map(&.match(/^A./))
+  #
+  # matches.any?(&.is_a?(Regex::MatchData)) # => true
+  # matches.one?(&.nil?)                    # => true
+  #
+  # typeof(matches.reject(&.nil?))                    # => Array(Regex::MatchData | Nil)
+  # typeof(matches.select(&.is_a?(Regex::MatchData))) # => Array(Regex::MatchData | Nil)
+  # ```
+  #
+  # And it should be written as this:
+  #
+  # ```
+  # matches = %w[Alice Bob].map(&.match(/^A./))
+  #
+  # matches.any?(Regex::MatchData) # => true
+  # matches.one?(Nil)              # => true
+  #
+  # typeof(matches.reject(Nil))              # => Array(Regex::MatchData)
+  # typeof(matches.select(Regex::MatchData)) # => Array(Regex::MatchData)
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Style/IsAFilter:
+  #   Enabled: true
+  #   FilterNames:
+  #     - select
+  #     - reject
+  #     - any?
+  #     - all?
+  #     - none?
+  #     - one?
+  # ```
+  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?)
+    end
+
+    MSG = "Use `%s(%s)` instead of `%s {...}`"
+
+    def test(source, node : Crystal::Call)
+      return unless node.name.in?(filter_names)
+      return unless (block = node.block)
+      return unless (body = block.body).is_a?(Crystal::IsA)
+      return unless (path = body.const).is_a?(Crystal::Path)
+      return unless body.obj.is_a?(Crystal::Var)
+
+      name = path.names.join("::")
+      name = "::#{name}" if path.global? && !body.nil_check?
+
+      end_location = node.end_location
+      if !end_location || end_location.try(&.column_number.zero?)
+        if end_location = path.end_location
+          end_location = Crystal::Location.new(
+            end_location.filename,
+            end_location.line_number,
+            end_location.column_number + 1
+          )
+        end
+      end
+
+      issue_for node.name_location, end_location,
+        MSG % {node.name, name, node.name}
+    end
+  end
+end