Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Performance/MinMaxAfterMap rule #387

Merged
merged 2 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions spec/ameba/rule/performance/minmax_after_map_spec.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
require "../../../spec_helper"

module Ameba::Rule::Performance
subject = MinMaxAfterMap.new

describe MinMaxAfterMap do
it "passes if there are 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
67 changes: 67 additions & 0 deletions src/ameba/rule/performance/minmax_after_map.cr
Original file line number Diff line number Diff line change
@@ -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
Loading