From fd78d17c40183f2bd4d097e22a5f1a7f811e867c Mon Sep 17 00:00:00 2001 From: "V. Elenhaupt" <3624712+veelenga@users.noreply.github.com> Date: Thu, 6 Sep 2018 17:59:11 +0300 Subject: [PATCH] New rule: Performance/FirstLastAfterFilter (#76) --- .../first_last_after_filter_spec.cr | 125 ++++++++++++++++++ .../performance/first_last_after_filter.cr | 51 +++++++ 2 files changed, 176 insertions(+) create mode 100644 spec/ameba/rule/performance/first_last_after_filter_spec.cr create mode 100644 src/ameba/rule/performance/first_last_after_filter.cr diff --git a/spec/ameba/rule/performance/first_last_after_filter_spec.cr b/spec/ameba/rule/performance/first_last_after_filter_spec.cr new file mode 100644 index 00000000..b3192443 --- /dev/null +++ b/spec/ameba/rule/performance/first_last_after_filter_spec.cr @@ -0,0 +1,125 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = FirstLastAfterFilter.new + + describe FirstLastAfterFilter do + it "passes if there is no potential performance improvements" do + source = Source.new %( + [1, 2, 3].select { |e| e > 1 } + [1, 2, 3].reverse.select { |e| e > 1 } + [1, 2, 3].reverse.last + [1, 2, 3].reverse.first + [1, 2, 3].reverse.first + ) + subject.catch(source).should be_valid + end + + it "reports if there is select followed by last" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.last + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is select followed by last?" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.last? + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is select followed by first" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.first + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is select followed by first?" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.first? + ) + subject.catch(source).should_not be_valid + end + + it "does not report if there is select followed by any other call" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.size + [1, 2, 3].select { |e| e > 2 }.any? + ) + subject.catch(source).should be_valid + end + + context "properties" do + it "allows to configure object_call_names" do + source = Source.new %( + [1, 2, 3].select { |e| e > 2 }.first + ) + rule = Rule::Performance::FirstLastAfterFilter.new + rule.filter_names = %w(reject) + rule.catch(source).should be_valid + end + end + + it "reports rule, pos and message" do + s = Source.new %( + [1, 2, 3].select { |e| e > 2 }.first + ), "source.cr" + subject.catch(s).should_not be_valid + s.issues.size.should eq 1 + + issue = s.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:2:19" + issue.end_location.to_s.should eq "source.cr:2:45" + + issue.message.should eq "Use `find {...}` instead of `select {...}.first`" + end + + it "reports a correct message for first?" do + s = Source.new %( + [1, 2, 3].select { |e| e > 2 }.first? + ), "source.cr" + subject.catch(s).should_not be_valid + s.issues.size.should eq 1 + + issue = s.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:2:19" + issue.end_location.to_s.should eq "source.cr:2:46" + + issue.message.should eq "Use `find {...}` instead of `select {...}.first?`" + end + + it "reports rule, pos and reverse message" do + s = Source.new %( + [1, 2, 3].select { |e| e > 2 }.last + ), "source.cr" + subject.catch(s).should_not be_valid + s.issues.size.should eq 1 + + issue = s.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:2:19" + issue.end_location.to_s.should eq "source.cr:2:44" + + issue.message.should eq "Use `reverse_each.find {...}` instead of `select {...}.last`" + end + + it "reports a correct message for last?" do + s = Source.new %( + [1, 2, 3].select { |e| e > 2 }.last? + ), "source.cr" + subject.catch(s).should_not be_valid + s.issues.size.should eq 1 + + issue = s.issues.first + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:2:19" + issue.end_location.to_s.should eq "source.cr:2:45" + + issue.message.should eq "Use `reverse_each.find {...}` instead of `select {...}.last?`" + end + end +end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr new file mode 100644 index 00000000..2cfb9ec5 --- /dev/null +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -0,0 +1,51 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of `first/last/first?/last?` calls that follow filters. + # + # For example, this is considered inefficient: + # + # ``` + # [-1, 0, 1, 2].select { |e| e > 0 }.first? + # [-1, 0, 1, 2].select { |e| e > 0 }.last? + # ``` + # + # And can be written as this: + # + # ``` + # [-1, 0, 1, 2].find { |e| e > 0 } + # [-1, 0, 1, 2].reverse_each.find { |e| e > 0 } + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/FirstLastAfterFilter + # Enabled: true + # FilterNames: + # - select + # ``` + # + struct FirstLastAfterFilter < Base + CALL_NAMES = %w(first last first? last?) + MSG = "Use `find {...}` instead of `%s {...}.%s`" + MSG_REVERSE = "Use `reverse_each.find {...}` instead of `%s {...}.%s`" + + properties do + filter_names : Array(String) = %w(select) + description "Identifies usage of `first/last/first?/last?` calls that follow filters." + end + + def test(source) + AST::NodeVisitor.new self, source + end + + def test(source, node : Crystal::Call) + return unless CALL_NAMES.includes?(node.name) && (obj = node.obj) + + if node.block.nil? && obj.is_a?(Crystal::Call) && + filter_names.includes?(obj.name) && !obj.block.nil? + message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE + issue_for obj.name_location, node.name_end_location, message % {obj.name, node.name} + end + end + end +end