From efc71c99ecca9bb3cb2f9e55783ffa15e43d3fcb Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 23:38:44 -0400 Subject: [PATCH 1/2] FilterFilter et al: inline predicates, skip multi-statement bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four pipe-collapse rules (FilterFilter, RejectReject, FilterReject, RejectFilter) used to wrap each predicate as `f.(item)` no matter what, producing nasty output like `(&(&1 in list)).(item) && (fn x -> ... end).(item)` whenever the source used a capture or anonymous fn. Now they reuse the MapMap rule's inlining machinery to substitute the iteration variable directly into the predicate body, and skip the rewrite entirely when either side has a multi-statement `fn` body — combining those under `&&`/`||` would change short-circuit semantics and read worse than the original two-pipe chain. --- lib/style/pipes.ex | 97 +++++++++++++++++++++++++++++---------- test/style/pipes_test.exs | 39 +++++++++++++++- 2 files changed, 110 insertions(+), 26 deletions(-) diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index c41bcf1..e479efa 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -372,7 +372,7 @@ defmodule Styler.Style.Pipes do when mod in @enum, do: {:|>, pm, [lhs, {count, meta, [filterer]}]} - # `lhs |> Enum.filter(f1) |> Enum.filter(f2)` => `lhs |> Enum.filter(fn item -> f1.(item) && f2.(item) end)` + # `lhs |> Enum.filter(f1) |> Enum.filter(f2)` => `lhs |> Enum.filter(fn item -> f1_inlined && f2_inlined end)` # (Credo.Check.Refactor.FilterFilter) defp fix_pipe( pipe_chain( @@ -380,11 +380,15 @@ defmodule Styler.Style.Pipes do lhs, {{:., _, [{_, _, [:Enum]}, :filter]} = filter, fm, [f1]}, {{:., _, [{_, _, [:Enum]}, :filter]}, _, [f2]} - ) - ), - do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm)]}]} + ) = node + ) do + case combined_predicate(f1, f2, :&&, fm) do + nil -> node + combined -> {:|>, pm, [lhs, {filter, fm, [combined]}]} + end + end - # `lhs |> Enum.reject(f1) |> Enum.reject(f2)` => `lhs |> Enum.reject(fn item -> f1.(item) || f2.(item) end)` + # `lhs |> Enum.reject(f1) |> Enum.reject(f2)` => `lhs |> Enum.reject(fn item -> f1_inlined || f2_inlined end)` # (Credo.Check.Refactor.RejectReject) defp fix_pipe( pipe_chain( @@ -392,11 +396,15 @@ defmodule Styler.Style.Pipes do lhs, {{:., _, [{_, _, [:Enum]}, :reject]} = reject, fm, [f1]}, {{:., _, [{_, _, [:Enum]}, :reject]}, _, [f2]} - ) - ), - do: {:|>, pm, [lhs, {reject, fm, [combined_predicate(f1, f2, :||, fm)]}]} + ) = node + ) do + case combined_predicate(f1, f2, :||, fm) do + nil -> node + combined -> {:|>, pm, [lhs, {reject, fm, [combined]}]} + end + end - # `lhs |> Enum.filter(f1) |> Enum.reject(f2)` => `lhs |> Enum.filter(fn item -> f1.(item) && !f2.(item) end)` + # `lhs |> Enum.filter(f1) |> Enum.reject(f2)` => `lhs |> Enum.filter(fn item -> f1_inlined && !f2_inlined end)` # (Credo.Check.Refactor.FilterReject) defp fix_pipe( pipe_chain( @@ -404,11 +412,15 @@ defmodule Styler.Style.Pipes do lhs, {{:., _, [{_, _, [:Enum]}, :filter]} = filter, fm, [f1]}, {{:., _, [{_, _, [:Enum]}, :reject]}, _, [f2]} - ) - ), - do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm, negate_f2: true)]}]} + ) = node + ) do + case combined_predicate(f1, f2, :&&, fm, negate_f2: true) do + nil -> node + combined -> {:|>, pm, [lhs, {filter, fm, [combined]}]} + end + end - # `lhs |> Enum.reject(f1) |> Enum.filter(f2)` => `lhs |> Enum.filter(fn item -> !f1.(item) && f2.(item) end)` + # `lhs |> Enum.reject(f1) |> Enum.filter(f2)` => `lhs |> Enum.filter(fn item -> !f1_inlined && f2_inlined end)` # The merged call collapses to `Enum.filter` (as Credo recommends) — `f1` was the original reject, # so we negate it; `f2` was the original filter, so it stays. # (Credo.Check.Refactor.RejectFilter) @@ -418,9 +430,13 @@ defmodule Styler.Style.Pipes do lhs, {{:., _, [{_, _, [:Enum]}, :reject]}, fm, [f1]}, {{:., _, [{_, _, [:Enum]}, :filter]} = filter, _, [f2]} - ) - ), - do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm, negate_f1: true)]}]} + ) = node + ) do + case combined_predicate(f1, f2, :&&, fm, negate_f1: true) do + nil -> node + combined -> {:|>, pm, [lhs, {filter, fm, [combined]}]} + end + end # `lhs |> Enum.map(f1) |> Enum.map(f2)` => single `Enum.map` whose body is the inlined nested call. We seed the body # with a one-step pipe inside f1's slot - Styler's existing `f(pipe, args)` walk then unfolds the f2 call into the @@ -558,19 +574,52 @@ defmodule Styler.Style.Pipes do defp valid_pipe_start?({fun, _, _args}) when is_atom(fun), do: String.match?("#{fun}", ~r/^sigil_[a-zA-Z]$/) defp valid_pipe_start?(_), do: true - # Combines two 1-arity predicates into a single anonymous function: `fn item -> f1.(item) f2.(item) end`. - # Universal form that's correct regardless of whether each predicate is a capture, an `&(...)` shortform, - # or an explicit `fn x -> ... end`. Used by FilterFilter (op: `&&`), RejectReject (op: `||`), and - # the mixed FilterReject / RejectFilter rules (op: `&&` with one side wrapped in `!`). + # Combines two 1-arity predicates into a single anonymous function: `fn item -> f1_term f2_term end`. + # Returns nil if either side can't be combined cleanly — the caller skips the rewrite. We inline + # captures and inline-fn predicates whose body is a single expression (so combining with `&&`/`||` + # is sound). Bare references (like `f1`) fall back to `f1.(item)`. Anything else (multi-statement + # `fn` bodies, captures with multiple `&1` uses, nested closures) is left alone — the original + # two-pipe form is more readable than what we'd produce. Used by FilterFilter (op: `&&`), + # RejectReject (op: `||`), and the mixed FilterReject / RejectFilter rules. defp combined_predicate(f1, f2, op, m, opts \\ []) do line = m[:line] item = {:item, [line: line], nil} - call_f1 = maybe_negate(predicate_call(f1, item, line), opts[:negate_f1] == true, line) - call_f2 = maybe_negate(predicate_call(f2, item, line), opts[:negate_f2] == true, line) - body = {op, [line: line], [call_f1, call_f2]} - {:fn, [closing: [line: line], line: line], [{:->, [line: line], [[item], body]}]} + + with call_f1 when not is_nil(call_f1) <- predicate_term(f1, item, line), + call_f2 when not is_nil(call_f2) <- predicate_term(f2, item, line) do + call_f1 = maybe_negate(call_f1, opts[:negate_f1] == true, line) + call_f2 = maybe_negate(call_f2, opts[:negate_f2] == true, line) + body = {op, [line: line], [call_f1, call_f2]} + {:fn, [closing: [line: line], line: line], [{:->, [line: line], [[item], body]}]} + end + end + + defp predicate_term(fun, item, line) do + cond do + inlineable_simple?(fun) -> Style.set_line(inline_capture(fun, item, line), line) + bare_reference?(fun) -> predicate_call(fun, item, line) + true -> nil + end end + # Inlineable AND the body is a single expression. Multi-statement `fn`/`&` bodies don't compose + # cleanly under `&&`/`||` — short-circuit semantics would change and the formatted output is uglier + # than the original two-pipe chain. + defp inlineable_simple?({:&, _, [{:/, _, _}]} = fun), do: inlineable?(fun) + + defp inlineable_simple?({:&, _, [body]} = fun), do: inlineable?(fun) and not multi_statement_block?(body) + + defp inlineable_simple?({:fn, _, [{:->, _, [_, body]}]} = fun), + do: inlineable?(fun) and not multi_statement_block?(body) + + defp inlineable_simple?(_), do: false + + defp multi_statement_block?({:__block__, _, [_, _ | _]}), do: true + defp multi_statement_block?(_), do: false + + defp bare_reference?({var, _, ctx}) when is_atom(var) and is_atom(ctx), do: true + defp bare_reference?(_), do: false + defp maybe_negate(call, true, line), do: {:!, [line: line], [call]} defp maybe_negate(call, false, _line), do: call diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 5349462..83b0fbc 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -656,7 +656,7 @@ defmodule Styler.Style.PipesTest do ) end - test "FilterFilter: combines captures and inline predicates" do + test "FilterFilter: inlines captures and inline-fn predicates" do assert_style( """ list @@ -664,11 +664,46 @@ defmodule Styler.Style.PipesTest do |> Enum.filter(fn s -> String.contains?(s, "a") end) """, """ - Enum.filter(list, fn item -> (&String.contains?(&1, "x")).(item) && (fn s -> String.contains?(s, "a") end).(item) end) + Enum.filter(list, fn item -> String.contains?(item, "x") && String.contains?(item, "a") end) """ ) end + test "RejectFilter: inlines `&fun/1` style captures" do + assert_style( + """ + list + |> Enum.reject(&is_nil/1) + |> Enum.filter(&is_map/1) + """, + """ + Enum.filter(list, fn item -> not is_nil(item) && is_map(item) end) + """ + ) + end + + test "FilterFilter / RejectFilter: skips rewrite when a predicate has a multi-statement fn body" do + # The collapsed lambda would be uglier than the original chain — leave it alone. + assert_style(""" + schema_modules + |> Enum.reject(&(&1 in context.ignore_list)) + |> Enum.filter(fn schema_module -> + schema = schema_module.schema() + schema_title = schema.title + not is_nil(schema.example) and not MapSet.member?(set, schema_title) + end) + """) + + assert_style(""" + list + |> Enum.filter(fn x -> + y = x.a + y > 0 + end) + |> Enum.filter(f2) + """) + end + test "FilterFilter: leaves non-Enum.filter chains alone" do assert_style(""" list From ac3c7528d71b322f5a560b5eaeddf8834a4de472 Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Thu, 7 May 2026 00:57:28 -0400 Subject: [PATCH 2/2] combined_predicate: shadow check + prefer source's iteration name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bugbot caught that combined_predicate hardcoded `:item` as the merged lambda's parameter without checking whether either predicate closed over a variable named `item` — the new lambda would silently shadow it. Apply the same machinery MapMap already uses: prefer the source's own iteration name (from a `fn x -> ...` predicate) when present, and check shadows_free_var? before committing. If the chosen name would shadow a free var on either side, bail out and leave the chain untouched. Also extend free_var_in? to recognize bare-reference predicates (e.g. `Enum.filter(list, item)`) as shadow risks — MapMap already filters these out before reaching the check, so its behavior is unchanged. --- lib/style/pipes.ex | 28 ++++++++++++++++++++-------- test/style/pipes_test.exs | 18 +++++++++++++++++- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index e479efa..fea4d5b 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -583,14 +583,23 @@ defmodule Styler.Style.Pipes do # RejectReject (op: `||`), and the mixed FilterReject / RejectFilter rules. defp combined_predicate(f1, f2, op, m, opts \\ []) do line = m[:line] - item = {:item, [line: line], nil} - - with call_f1 when not is_nil(call_f1) <- predicate_term(f1, item, line), - call_f2 when not is_nil(call_f2) <- predicate_term(f2, item, line) do - call_f1 = maybe_negate(call_f1, opts[:negate_f1] == true, line) - call_f2 = maybe_negate(call_f2, opts[:negate_f2] == true, line) - body = {op, [line: line], [call_f1, call_f2]} - {:fn, [closing: [line: line], line: line], [{:->, [line: line], [[item], body]}]} + # Prefer the source's own iteration name (like MapMap), defaulting to `:item` for filter/reject + # convention. If the chosen name appears as a free variable in either predicate, the merged + # lambda's parameter would silently shadow that closure — bail out. + item_name = fn_var_name(f1) || fn_var_name(f2) || :item + + if shadows_free_var?(item_name, f1, f2) do + nil + else + item = {item_name, [line: line], nil} + + with call_f1 when not is_nil(call_f1) <- predicate_term(f1, item, line), + call_f2 when not is_nil(call_f2) <- predicate_term(f2, item, line) do + call_f1 = maybe_negate(call_f1, opts[:negate_f1] == true, line) + call_f2 = maybe_negate(call_f2, opts[:negate_f2] == true, line) + body = {op, [line: line], [call_f1, call_f2]} + {:fn, [closing: [line: line], line: line], [{:->, [line: line], [[item], body]}]} + end end end @@ -697,6 +706,9 @@ defmodule Styler.Style.Pipes do do: param != name and var_in_ast?(body, name) defp free_var_in?(name, {:&, _, [body]}), do: var_in_ast?(body, name) + # bare reference predicate (e.g., `Enum.filter(list, f1)`) — if the chosen lambda param name + # matches the reference, we'd shadow it + defp free_var_in?(name, {var, _, ctx}) when is_atom(var) and is_atom(ctx), do: var == name defp free_var_in?(_, _), do: false defp var_in_ast?(ast, name) do diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 83b0fbc..24b7041 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -664,7 +664,7 @@ defmodule Styler.Style.PipesTest do |> Enum.filter(fn s -> String.contains?(s, "a") end) """, """ - Enum.filter(list, fn item -> String.contains?(item, "x") && String.contains?(item, "a") end) + Enum.filter(list, fn s -> String.contains?(s, "x") && String.contains?(s, "a") end) """ ) end @@ -682,6 +682,22 @@ defmodule Styler.Style.PipesTest do ) end + test "FilterFilter: skips rewrite when chosen var name would shadow a closure" do + # `item` is closed over by f1; merging with hardcoded `:item` would silently shadow it. + assert_style(""" + list + |> Enum.filter(&(&1.thing == item)) + |> Enum.filter(f2) + """) + + # Bare-reference predicate named `:item` — same trap. + assert_style(""" + list + |> Enum.filter(item) + |> Enum.filter(f2) + """) + end + test "FilterFilter / RejectFilter: skips rewrite when a predicate has a multi-statement fn body" do # The collapsed lambda would be uglier than the original chain — leave it alone. assert_style("""