FilterFilter et al: inline predicates, skip multi-statement bodies

lib/style/pipes.ex

@@ -372,43 +372,55 @@ 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(
            pm,
            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(
            pm,
            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(
            pm,
            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,61 @@ 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) <op> 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 <op> 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]}]}
+    # 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
 
+  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
 
@@ -648,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

test/style/pipes_test.exs

@@ -656,19 +656,70 @@ 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
         |> Enum.filter(&String.contains?(&1, "x"))
         |> 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 s -> String.contains?(s, "x") && String.contains?(s, "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: 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("""
+      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