From 7460cbddfad59997a205abdc1a2085d8f1b8aed7 Mon Sep 17 00:00:00 2001 From: Tyler Young Date: Sat, 14 Jun 2025 07:04:46 -0500 Subject: [PATCH] feat (FilterFilter and related): Check Map and Keyword versions too This expands the `Credo.Check.Refactor.FilterFilter` and related checks (`FilterReject`, `RejectFilter`, and `RejectReject`) to detect chains of the `Map` and `Keyword` versions of those functions in addition to `Enum`. Thus, we can now detect all three of: ```elixir ints |> Enum.filter(&even?/1) |> Enum.filter(fn val -> val > 0 end) map |> Map.filter(fn {key, _val} -> is_binary(key) end) |> Map.filter(fn {_key, val} -> val > 0 end) keyword |> Keyword.filter(fn {key, _val} -> key in [:a, :b, :c] end) |> Keyword.filter(fn {_key, val} -> val > 0 end) ``` --- lib/credo/check/refactor/filter_filter.ex | 25 ++++-- lib/credo/check/refactor/filter_reject.ex | 25 ++++-- lib/credo/check/refactor/keyword_helpers.ex | 87 +++++++++++++++++++ lib/credo/check/refactor/map_helpers.ex | 87 +++++++++++++++++++ lib/credo/check/refactor/reject_filter.ex | 25 ++++-- lib/credo/check/refactor/reject_reject.ex | 25 ++++-- .../check/refactor/filter_filter_test.exs | 51 +++++++++++ .../check/refactor/filter_reject_test.exs | 51 +++++++++++ .../check/refactor/reject_filter_test.exs | 55 ++++++++++++ .../check/refactor/reject_reject_test.exs | 51 +++++++++++ 10 files changed, 462 insertions(+), 20 deletions(-) create mode 100644 lib/credo/check/refactor/keyword_helpers.ex create mode 100644 lib/credo/check/refactor/map_helpers.ex diff --git a/lib/credo/check/refactor/filter_filter.ex b/lib/credo/check/refactor/filter_filter.ex index 0a8eac69f..c3f051d64 100644 --- a/lib/credo/check/refactor/filter_filter.ex +++ b/lib/credo/check/refactor/filter_filter.ex @@ -3,7 +3,7 @@ defmodule Credo.Check.Refactor.FilterFilter do id: "EX4008", explanations: [ check: """ - One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.filter/2`. + One `Enum.filter/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.filter/2 |> Enum.filter/2`. This should be refactored: @@ -24,17 +24,32 @@ defmodule Credo.Check.Refactor.FilterFilter do ] alias Credo.Check.Refactor.EnumHelpers + alias Credo.Check.Refactor.MapHelpers + alias Credo.Check.Refactor.KeywordHelpers @doc false def run(source_file, params \\ []) do issue_meta = IssueMeta.for(source_file, params) - message = "One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.filter/2`" trigger = "|>" - Credo.Code.prewalk( - source_file, - &EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :filter, :filter, __MODULE__) + Enum.flat_map( + [ + {&EnumHelpers.traverse/8, "Enum"}, + {&MapHelpers.traverse/8, "Map"}, + {&KeywordHelpers.traverse/8, "Keyword"} + ], + fn {traverse, module} -> + message = + "One `#{module}.filter/2` is more efficient than `#{module}.filter/2 |> #{module}.filter/2`" + + Credo.Code.prewalk( + source_file, + fn ast, issues -> + traverse.(ast, issues, issue_meta, message, trigger, :filter, :filter, __MODULE__) + end + ) + end ) end end diff --git a/lib/credo/check/refactor/filter_reject.ex b/lib/credo/check/refactor/filter_reject.ex index acbbc422d..00553c3d1 100644 --- a/lib/credo/check/refactor/filter_reject.ex +++ b/lib/credo/check/refactor/filter_reject.ex @@ -4,7 +4,7 @@ defmodule Credo.Check.Refactor.FilterReject do tags: [:controversial], explanations: [ check: """ - One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.reject/2`. + One `Enum.filter/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.filter/2 |> Enum.reject/2`. This should be refactored: @@ -25,17 +25,32 @@ defmodule Credo.Check.Refactor.FilterReject do ] alias Credo.Check.Refactor.EnumHelpers + alias Credo.Check.Refactor.MapHelpers + alias Credo.Check.Refactor.KeywordHelpers @doc false def run(source_file, params \\ []) do issue_meta = IssueMeta.for(source_file, params) - message = "One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.reject/2`" trigger = "|>" - Credo.Code.prewalk( - source_file, - &EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :filter, :reject, __MODULE__) + Enum.flat_map( + [ + {&EnumHelpers.traverse/8, "Enum"}, + {&MapHelpers.traverse/8, "Map"}, + {&KeywordHelpers.traverse/8, "Keyword"} + ], + fn {traverse, module} -> + message = + "One `#{module}.filter/2` is more efficient than `#{module}.filter/2 |> #{module}.reject/2`" + + Credo.Code.prewalk( + source_file, + fn ast, issues -> + traverse.(ast, issues, issue_meta, message, trigger, :filter, :reject, __MODULE__) + end + ) + end ) end end diff --git a/lib/credo/check/refactor/keyword_helpers.ex b/lib/credo/check/refactor/keyword_helpers.ex new file mode 100644 index 000000000..23fb60fd8 --- /dev/null +++ b/lib/credo/check/refactor/keyword_helpers.ex @@ -0,0 +1,87 @@ +defmodule Credo.Check.Refactor.KeywordHelpers do + def traverse( + {{:., _, [{:__aliases__, meta, [:Keyword]}, second]}, _, + [{{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _}, _]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse( + {:|>, meta, + [ + {{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _}, + {{:., _, [{:__aliases__, _, [:Keyword]}, second]}, _, _} + ]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse( + {{:., meta, [{:__aliases__, _, [:Keyword]}, second]}, _, + [ + {:|>, _, [_, {{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _}]}, + _ + ]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse( + {:|>, meta, + [ + {:|>, _, + [ + _, + {{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _} + ]}, + {{:., _, [{:__aliases__, _, [:Keyword]}, second]}, _, _} + ]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse(ast, issues, _issue_meta, _message, _trigger, _first, _second, _module) do + {ast, issues} + end + + defp issue_for(issue_meta, line_no, message, trigger, module) do + module.format_issue( + issue_meta, + message: message, + trigger: trigger, + line_no: line_no + ) + end +end diff --git a/lib/credo/check/refactor/map_helpers.ex b/lib/credo/check/refactor/map_helpers.ex new file mode 100644 index 000000000..82036e3ea --- /dev/null +++ b/lib/credo/check/refactor/map_helpers.ex @@ -0,0 +1,87 @@ +defmodule Credo.Check.Refactor.MapHelpers do + def traverse( + {{:., _, [{:__aliases__, meta, [:Map]}, second]}, _, + [{{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _}, _]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse( + {:|>, meta, + [ + {{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _}, + {{:., _, [{:__aliases__, _, [:Map]}, second]}, _, _} + ]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse( + {{:., meta, [{:__aliases__, _, [:Map]}, second]}, _, + [ + {:|>, _, [_, {{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _}]}, + _ + ]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse( + {:|>, meta, + [ + {:|>, _, + [ + _, + {{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _} + ]}, + {{:., _, [{:__aliases__, _, [:Map]}, second]}, _, _} + ]} = ast, + issues, + issue_meta, + message, + trigger, + first, + second, + module + ) do + new_issue = issue_for(issue_meta, meta[:line], message, trigger, module) + {ast, issues ++ List.wrap(new_issue)} + end + + def traverse(ast, issues, _issue_meta, _message, _trigger, _first, _second, _module) do + {ast, issues} + end + + defp issue_for(issue_meta, line_no, message, trigger, module) do + module.format_issue( + issue_meta, + message: message, + trigger: trigger, + line_no: line_no + ) + end +end diff --git a/lib/credo/check/refactor/reject_filter.ex b/lib/credo/check/refactor/reject_filter.ex index 3d9954e8c..32fda0ccf 100644 --- a/lib/credo/check/refactor/reject_filter.ex +++ b/lib/credo/check/refactor/reject_filter.ex @@ -4,7 +4,7 @@ defmodule Credo.Check.Refactor.RejectFilter do tags: [:controversial], explanations: [ check: """ - One `Enum.filter/2` is more efficient than `Enum.reject/2 |> Enum.filter/2`. + One `Enum.filter/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.reject/2 |> Enum.filter/2`. This should be refactored: @@ -25,17 +25,32 @@ defmodule Credo.Check.Refactor.RejectFilter do ] alias Credo.Check.Refactor.EnumHelpers + alias Credo.Check.Refactor.MapHelpers + alias Credo.Check.Refactor.KeywordHelpers @doc false def run(source_file, params \\ []) do issue_meta = IssueMeta.for(source_file, params) - message = "One `Enum.filter/2` is more efficient than `Enum.reject/2 |> Enum.filter/2`" trigger = "|>" - Credo.Code.prewalk( - source_file, - &EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :reject, :filter, __MODULE__) + Enum.flat_map( + [ + {&EnumHelpers.traverse/8, "Enum"}, + {&MapHelpers.traverse/8, "Map"}, + {&KeywordHelpers.traverse/8, "Keyword"} + ], + fn {traverse, module} -> + message = + "One `#{module}.reject/2` is more efficient than `#{module}.reject/2 |> #{module}.filter/2`" + + Credo.Code.prewalk( + source_file, + fn ast, issues -> + traverse.(ast, issues, issue_meta, message, trigger, :reject, :filter, __MODULE__) + end + ) + end ) end end diff --git a/lib/credo/check/refactor/reject_reject.ex b/lib/credo/check/refactor/reject_reject.ex index 48f549dbe..d5c6cffb0 100644 --- a/lib/credo/check/refactor/reject_reject.ex +++ b/lib/credo/check/refactor/reject_reject.ex @@ -3,7 +3,7 @@ defmodule Credo.Check.Refactor.RejectReject do id: "EX4026", explanations: [ check: """ - One `Enum.reject/2` is more efficient than `Enum.reject/2 |> Enum.reject/2`. + One `Enum.reject/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.reject/2 |> Enum.reject/2`. This should be refactored: @@ -24,17 +24,32 @@ defmodule Credo.Check.Refactor.RejectReject do ] alias Credo.Check.Refactor.EnumHelpers + alias Credo.Check.Refactor.MapHelpers + alias Credo.Check.Refactor.KeywordHelpers @doc false def run(source_file, params \\ []) do issue_meta = IssueMeta.for(source_file, params) - message = "One `Enum.reject/2` is more efficient than `Enum.reject/2 |> Enum.reject/2`" trigger = "|>" - Credo.Code.prewalk( - source_file, - &EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :reject, :reject, __MODULE__) + Enum.flat_map( + [ + {&EnumHelpers.traverse/8, "Enum"}, + {&MapHelpers.traverse/8, "Map"}, + {&KeywordHelpers.traverse/8, "Keyword"} + ], + fn {traverse, module} -> + message = + "One `#{module}.reject/2` is more efficient than `#{module}.reject/2 |> #{module}.reject/2`" + + Credo.Code.prewalk( + source_file, + fn ast, issues -> + traverse.(ast, issues, issue_meta, message, trigger, :reject, :reject, __MODULE__) + end + ) + end ) end end diff --git a/test/credo/check/refactor/filter_filter_test.exs b/test/credo/check/refactor/filter_filter_test.exs index 57c4febac..ab1633286 100644 --- a/test/credo/check/refactor/filter_filter_test.exs +++ b/test/credo/check/refactor/filter_filter_test.exs @@ -115,4 +115,55 @@ defmodule Credo.Check.Refactor.FilterFilterTest do assert issue.trigger == "|>" end) end + + test "reports chains of Map.filter" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + %{"a" => 1, "b" => 2, "c" => 3} + |> Map.filter(fn {key, value} -> key == "a" end) + |> Map.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Map.filter/2`" + refute issue.message =~ "`Enum.filter/2`" + end) + end + + test "reports chains of Keyword.filter" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + [a: 1, b: 2, c: 3] + |> Keyword.filter(fn {key, value} -> key == :a end) + |> Keyword.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Keyword.filter/2`" + refute issue.message =~ "`Enum.filter/2`" + end) + end + + test "does not report on mix-and-match modules" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + ["a", "b", "c"] + |> Map.filter(&String.contains?(&1, "x")) + |> Enum.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end end diff --git a/test/credo/check/refactor/filter_reject_test.exs b/test/credo/check/refactor/filter_reject_test.exs index 0cd0415ed..2ac5c86a3 100644 --- a/test/credo/check/refactor/filter_reject_test.exs +++ b/test/credo/check/refactor/filter_reject_test.exs @@ -115,4 +115,55 @@ defmodule Credo.Check.Refactor.FilterRejectTest do assert issue.trigger == "|>" end) end + + test "reports chains of Map.filter and Map.reject" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + %{"a" => 1, "b" => 2, "c" => 3} + |> Map.filter(fn {key, value} -> key == "a" end) + |> Map.reject(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Map.filter/2`" + refute issue.message =~ "`Enum.filter/2`" + end) + end + + test "reports chains of Keyword.filter and Keyword.reject" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + [a: 1, b: 2, c: 3] + |> Keyword.filter(fn {key, value} -> key == :a end) + |> Keyword.reject(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Keyword.filter/2`" + refute issue.message =~ "`Enum.filter/2`" + end) + end + + test "does not report on mix-and-match modules" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + ["a", "b", "c"] + |> Map.filter(&String.contains?(&1, "x")) + |> Enum.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end end diff --git a/test/credo/check/refactor/reject_filter_test.exs b/test/credo/check/refactor/reject_filter_test.exs index a745a9aef..b696367e4 100644 --- a/test/credo/check/refactor/reject_filter_test.exs +++ b/test/credo/check/refactor/reject_filter_test.exs @@ -115,4 +115,59 @@ defmodule Credo.Check.Refactor.RejectFilterTest do assert issue.trigger == "|>" end) end + + test "reports chains of Map.reject and Map.filter" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + %{"a" => 1, "b" => 2, "c" => 3} + |> Map.reject(fn {key, value} -> key == "a" end) + |> Map.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Map.reject/2`" + assert issue.message =~ "Map.filter/2" + refute issue.message =~ "`Enum.reject/2`" + refute issue.message =~ "Enum.filter/2" + end) + end + + test "reports chains of Keyword.reject and Keyword.filter" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + [a: 1, b: 2, c: 3] + |> Keyword.reject(fn {key, value} -> key == :a end) + |> Keyword.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Keyword.reject/2`" + assert issue.message =~ "Keyword.filter/2" + refute issue.message =~ "`Enum.reject/2`" + refute issue.message =~ "Enum.filter/2" + end) + end + + test "does not report on mix-and-match modules" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + ["a", "b", "c"] + |> Map.reject(&String.contains?(&1, "x")) + |> Enum.filter(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end end diff --git a/test/credo/check/refactor/reject_reject_test.exs b/test/credo/check/refactor/reject_reject_test.exs index fad1a7547..0ef97b863 100644 --- a/test/credo/check/refactor/reject_reject_test.exs +++ b/test/credo/check/refactor/reject_reject_test.exs @@ -115,4 +115,55 @@ defmodule Credo.Check.Refactor.RejectRejectTest do assert issue.trigger == "|>" end) end + + test "reports chains of Map.reject" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + %{"a" => 1, "b" => 2, "c" => 3} + |> Map.reject(fn {key, value} -> key == "a" end) + |> Map.reject(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Map.reject/2`" + refute issue.message =~ "`Enum.reject/2`" + end) + end + + test "reports chains of Keyword.reject" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + [a: 1, b: 2, c: 3] + |> Keyword.reject(fn {key, value} -> key == :a end) + |> Keyword.reject(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issue(fn issue -> + assert issue.message =~ "`Keyword.reject/2`" + refute issue.message =~ "`Enum.reject/2`" + end) + end + + test "does not report on mix-and-match modules" do + """ + defmodule Credo.Sample.Module do + def some_function(p1, p2, p3, p4, p5) do + ["a", "b", "c"] + |> Map.reject(&String.contains?(&1, "x")) + |> Enum.reject(fn {key, value} -> value == 1 end) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> refute_issues() + end end