From 57a93165e019ecf031d537171a9cd57da5d4d5c8 Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 21:36:13 -0400 Subject: [PATCH 1/4] Styler-ify more Credo rules: FilterReject, RejectFilter, MapMap, and !is_* guards Adds pipe-collapsing for Enum.filter |> Enum.reject (and the reverse), Enum.map |> Enum.map, and rewrites !is_nil/is_*/etc. to use `not`. Also extends the empty-check rewrite to cover String.length and byte_size. --- docs/credo.md | 2 + lib/style/pipes.ex | 228 +++++++++++++++++++++++++++++++- lib/style/single_node.ex | 27 +++- test/style/pipes_test.exs | 172 ++++++++++++++++++++++++ test/style/single_node_test.exs | 39 ++++++ 5 files changed, 464 insertions(+), 4 deletions(-) diff --git a/docs/credo.md b/docs/credo.md index 41ea257..4130d3b 100644 --- a/docs/credo.md +++ b/docs/credo.md @@ -37,12 +37,14 @@ Disabling the rules means updating your `.credo.exs` depending on your configura {Credo.Check.Refactor.CondStatements, false}, {Credo.Check.Refactor.FilterCount, false}, {Credo.Check.Refactor.FilterFilter, false}, +{Credo.Check.Refactor.FilterReject, false}, {Credo.Check.Refactor.MapInto, false}, {Credo.Check.Refactor.MapJoin, false}, {Credo.Check.Refactor.NegatedConditionsInUnless, false}, {Credo.Check.Refactor.NegatedConditionsWithElse, false}, {Credo.Check.Refactor.PipeChainStart, false}, {Credo.Check.Refactor.RedundantWithClauseResult, false}, +{Credo.Check.Refactor.RejectFilter, false}, {Credo.Check.Refactor.RejectReject, false}, {Credo.Check.Refactor.UnlessWithElse, false}, {Credo.Check.Refactor.WithClauses, false}, diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 68a6219..40ef342 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -20,9 +20,12 @@ defmodule Styler.Style.Pipes do * Credo.Check.Readability.SinglePipe * Credo.Check.Refactor.FilterCount * Credo.Check.Refactor.FilterFilter + * Credo.Check.Refactor.FilterReject * Credo.Check.Refactor.MapInto * Credo.Check.Refactor.MapJoin + * Credo.Check.Refactor.MapMap * Credo.Check.Refactor.PipeChainStart, excluded_functions: ["from"] + * Credo.Check.Refactor.RejectFilter * Credo.Check.Refactor.RejectReject """ @@ -393,6 +396,59 @@ defmodule Styler.Style.Pipes do ), do: {:|>, pm, [lhs, {reject, fm, [combined_predicate(f1, f2, :||, fm)]}]} + # `lhs |> Enum.filter(f1) |> Enum.reject(f2)` => `lhs |> Enum.filter(fn item -> f1.(item) && !f2.(item) 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)]}]} + + # `lhs |> Enum.reject(f1) |> Enum.filter(f2)` => `lhs |> Enum.filter(fn item -> !f1.(item) && f2.(item) 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) + defp fix_pipe( + pipe_chain( + pm, + lhs, + {{:., _, [{_, _, [:Enum]}, :reject]}, fm, [f1]}, + {{:., _, [{_, _, [:Enum]}, :filter]} = filter, _, [f2]} + ) + ), + do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm, negate_f1: true)]}]} + + # `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 rest of the pipe chain. If either side can't be cleanly inlined, + # f1 doesn't pipify (e.g. it inlined to an operator), or f2 doesn't put its placeholder in position + # 1 (so the seed pipe wouldn't unfold), skip — leaving the original two-map chain. + # (Credo.Check.Refactor.MapMap) + defp fix_pipe( + pipe_chain( + pm, + lhs, + {{:., _, [{_, _, [:Enum]}, :map]} = map, fm, [f1]}, + {{:., _, [{_, _, [:Enum]}, :map]}, _, [f2]} + ) = node + ) do + with true <- inlineable?(f1) and inlineable?(f2) and placeholder_in_first_position?(f2), + item_name = iteration_var_name(f1), + item = {item_name, [line: fm[:line]], nil}, + inlined_f1 = inline_capture(f1, item, fm[:line]), + {:|>, _, _} = f1_seed <- pipify(inlined_f1) do + body = inline_capture(f2, f1_seed, fm[:line]) + lambda = {:fn, [closing: [line: fm[:line]], line: fm[:line]], [{:->, [line: fm[:line]], [[item], body]}]} + {:|>, pm, [lhs, {map, fm, [lambda]}]} + else + _ -> node + end + end + # `lhs |> Stream.map(fun) |> Stream.run()` => `lhs |> Enum.each(fun)` # `lhs |> Stream.each(fun) |> Stream.run()` => `lhs |> Enum.each(fun)` defp fix_pipe( @@ -504,15 +560,181 @@ defmodule Styler.Style.Pipes do # 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: `&&`) and RejectReject (op: `||`). - defp combined_predicate(f1, f2, op, m) do + # or an explicit `fn x -> ... end`. Used by FilterFilter (op: `&&`), RejectReject (op: `||`), and + # the mixed FilterReject / RejectFilter rules (op: `&&` with one side wrapped in `!`). + defp combined_predicate(f1, f2, op, m, opts \\ []) do line = m[:line] item = {:item, [line: line], nil} - body = {op, [line: line], [predicate_call(f1, item, line), predicate_call(f2, item, line)]} + 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]}]} end + defp maybe_negate(call, true, line), do: {:!, [line: line], [call]} + defp maybe_negate(call, false, _line), do: call + defp predicate_call(fun, arg, line) do {{:., [line: line], [fun]}, [closing: [line: line], line: line], [arg]} end + + # &Mod.fun/1 → Mod.fun(arg). The `:closing` meta is what tells Styler's `f(pipe, args)` rule + # this is a real call (not a macro) and is safe to pipify. + defp inline_capture( + {:&, _, [{:/, _, [{{:., _, [{:__aliases__, _, mods}, name]}, _, []}, {:__block__, _, [1]}]}]}, + arg, + line + ) do + {{:., [line: line], [{:__aliases__, [line: line], mods}, name]}, [closing: [line: line], line: line], [arg]} + end + + # &fun/1 → fun(arg) + defp inline_capture({:&, _, [{:/, _, [{name, _, ctx}, {:__block__, _, [1]}]}]}, arg, line) + when is_atom(name) and is_atom(ctx) do + {name, [closing: [line: line], line: line], [arg]} + end + + # &expr — safe to inline iff `&1` appears exactly once, no `&n` for n > 1, and there are + # no nested `&(...)` capture forms in the body (their `&1`s belong to a different scope). + defp inline_capture({:&, _, [body]}, arg, _line) do + case placeholder_uses(body) do + {1, false, false} -> substitute_placeholder(body, arg) + _ -> nil + end + end + + # `fn x -> body end` — safe to inline iff `x` appears exactly once in body, no nested `fn`/`&` + # could shadow it, and `x` isn't `_` (which we'd be substituting into ignore-position). + defp inline_capture({:fn, _, [{:->, _, [[{name, _, ctx}], body]}]}, arg, _line) + when is_atom(name) and is_atom(ctx) and name != :_ do + case fn_var_uses(body, name) do + {1, false} -> substitute_fn_var(body, name, arg) + _ -> nil + end + end + + defp inline_capture(_, _, _), do: nil + + # Mirrors the inline_capture clauses above — returns true exactly when inline_capture would succeed. + defp inlineable?({:&, _, [{:/, _, [{{:., _, [{:__aliases__, _, _}, _]}, _, []}, {:__block__, _, [1]}]}]}), do: true + + defp inlineable?({:&, _, [{:/, _, [{name, _, ctx}, {:__block__, _, [1]}]}]}) when is_atom(name) and is_atom(ctx), + do: true + + defp inlineable?({:&, _, [body]}), do: match?({1, false, false}, placeholder_uses(body)) + + defp inlineable?({:fn, _, [{:->, _, [[{name, _, ctx}], body]}]}) when is_atom(name) and is_atom(ctx) and name != :_, + do: match?({1, false}, fn_var_uses(body, name)) + + defp inlineable?(_), do: false + + # If either side is an inline `fn x -> ...`, prefer that var name for the merged lambda — the + # source already named the iteration value. Otherwise, fall back to `arg1`. + defp iteration_var_name({:fn, _, [{:->, _, [[{name, _, ctx}], _]}]}) when is_atom(name) and is_atom(ctx) and name != :_, + do: name + + defp iteration_var_name(_), do: :arg1 + + # The seed-pipe trick only unfolds when f2's placeholder lands in arg position 1 of an outer call. + # If it lands in position 2+, we'd produce something like `Mod.fun(other, pipe)`, which Styler's + # `f(pipe, args)` rule won't touch and leaves an awkward partial pipe stranded inside an arg list. + defp placeholder_in_first_position?({:&, _, [{:/, _, _}]}), do: true + + defp placeholder_in_first_position?({:&, _, [{name, _, [{:&, _, [1]} | _]}]}) + when is_atom(name) and name not in @special_ops, + do: true + + defp placeholder_in_first_position?({:&, _, [{{:., _, _}, _, [{:&, _, [1]} | _]}]}), do: true + + defp placeholder_in_first_position?({:fn, _, [{:->, _, [[{name, _, ctx}], {fname, _, [{var, _, vctx} | _]}]}]}) + when is_atom(name) and is_atom(ctx) and name != :_ and var == name and is_atom(vctx) and is_atom(fname) and + fname not in @special_ops, + do: true + + defp placeholder_in_first_position?({:fn, _, [{:->, _, [[{name, _, ctx}], {{:., _, _}, _, [{var, _, vctx} | _]}]}]}) + when is_atom(name) and is_atom(ctx) and name != :_ and var == name and is_atom(vctx), + do: true + + defp placeholder_in_first_position?(_), do: false + + defp fn_var_uses(ast, name) do + {_, acc} = + Macro.prewalk(ast, {0, false}, fn + {:fn, _, _} = node, {count, _} -> + {node, {count, true}} + + {:&, _, _} = node, {count, _} -> + {node, {count, true}} + + {var, _, ctx} = node, {count, has_nested} when var == name and is_atom(ctx) -> + {node, {count + 1, has_nested}} + + node, acc -> + {node, acc} + end) + + acc + end + + # Mirrors substitute_placeholder/2 — replace the var without descending into substituted `arg` or + # into nested `fn`/`&` (which have their own scoping). + defp substitute_fn_var({:fn, _, _} = node, _name, _arg), do: node + defp substitute_fn_var({:&, _, _} = node, _name, _arg), do: node + defp substitute_fn_var({var, _, ctx}, name, arg) when var == name and is_atom(ctx), do: arg + + defp substitute_fn_var({a, m, args}, name, arg) when is_list(args), + do: {substitute_fn_var(a, name, arg), m, Enum.map(args, &substitute_fn_var(&1, name, arg))} + + defp substitute_fn_var({a, b}, name, arg), do: {substitute_fn_var(a, name, arg), substitute_fn_var(b, name, arg)} + + defp substitute_fn_var(list, name, arg) when is_list(list), do: Enum.map(list, &substitute_fn_var(&1, name, arg)) + + defp substitute_fn_var(other, _name, _arg), do: other + + # Convert a nested function-call AST (e.g. `f(g(h(x), y), z)`) into pipe form (`x |> h(y) |> g(z) |> f()`). + # Stops at non-call nodes, at operator atoms (`arg + 1` shouldn't become `arg |> +(1)`), and at + # already-piped subtrees (which are already in the desired shape). + defp pipify({:|>, _, _} = pipe), do: pipe + + defp pipify({{:., _, _} = dot, m, [first | rest]}), do: {:|>, [line: m[:line]], [pipify(first), {dot, m, rest}]} + + defp pipify({name, m, [first | rest]}) when is_atom(name) and is_list(rest) and name not in @special_ops, + do: {:|>, [line: m[:line]], [pipify(first), {name, m, rest}]} + + defp pipify(other), do: other + + # Returns `{count_of_&1, saw_higher_index?, saw_nested_capture?}`. The third flag prevents us + # from inlining cases where the body contains a nested `&(...)` — its `&1`s are scoped to that + # inner capture, not to the body we're inlining. + defp placeholder_uses(ast) do + {_, acc} = + Macro.prewalk(ast, {0, false, false}, fn + {:&, _, [n]} = node, {count, higher, has_capture} when is_integer(n) -> + if n == 1, + do: {node, {count + 1, higher, has_capture}}, + else: {node, {count, true, has_capture}} + + {:&, _, [_body]} = node, {count, higher, _} -> + {node, {count, higher, true}} + + node, acc -> + {node, acc} + end) + + acc + end + + # Replaces every `&1` in `ast` with `arg`, *without* descending into the substituted-in `arg` + # (whose `&1`s, if any, are not in our scope) or into nested `&(...)` capture forms. + defp substitute_placeholder({:&, _, [1]}, arg), do: arg + defp substitute_placeholder({:&, _, _} = capture, _arg), do: capture + + defp substitute_placeholder({a, m, args}, arg) when is_list(args), + do: {substitute_placeholder(a, arg), m, Enum.map(args, &substitute_placeholder(&1, arg))} + + defp substitute_placeholder({a, b}, arg), do: {substitute_placeholder(a, arg), substitute_placeholder(b, arg)} + + defp substitute_placeholder(list, arg) when is_list(list), do: Enum.map(list, &substitute_placeholder(&1, arg)) + + defp substitute_placeholder(other, _arg), do: other end diff --git a/lib/style/single_node.ex b/lib/style/single_node.ex index 8e0f464..cefb6af 100644 --- a/lib/style/single_node.ex +++ b/lib/style/single_node.ex @@ -24,12 +24,21 @@ defmodule Styler.Style.SingleNode do * Credo.Check.Refactor.RedundantWithClauseResult * Credo.Check.Refactor.WithClauses * Credo.Check.Warning.ExpensiveEmptyEnumCheck + + Also rewrites `!is_nil(x)` (and the other `is_*` guard predicates) to `not is_nil(x)`, + matching our existing preference for `not` over `!` on guard-style predicates. """ @behaviour Styler.Style @closing_delimiters [~s|"|, ")", "}", "|", "]", "'", ">", "/"] + @is_guards ~w( + is_atom is_binary is_bitstring is_boolean is_exception is_float is_function + is_integer is_list is_map is_map_key is_nil is_number is_pid is_port + is_reference is_struct is_tuple + )a + # `|> Timex.now()` => `|> Timex.now()` # skip over pipes into `Timex.now/1` so that we don't accidentally rewrite it as DateTime.utc_now/1 def run({{:|>, _, [_, {{:., _, [{:__aliases__, _, [:Timex]}, :now]}, _, []}]}, _} = zipper, ctx), @@ -37,6 +46,10 @@ defmodule Styler.Style.SingleNode do def run({node, meta}, ctx), do: {:cont, {style(node), meta}, ctx} + # `!is_nil(x)` => `not is_nil(x)` (and same for the other built-in `is_*` guard predicates). + # Style preference: `not` reads more naturally with type guards. + defp style({:!, m, [{guard, _, _} = check]}) when guard in @is_guards, do: {:not, m, [check]} + defp style({:assert, meta, [{:!=, _, [x, {:__block__, _, [nil]}]}]}), do: style({:assert, meta, [x]}) # refute nilly -> assert defp style({:refute, meta, [{:is_nil, _, [x]}]}), do: style({:assert, meta, [x]}) @@ -207,7 +220,9 @@ defmodule Styler.Style.SingleNode do # `length(x) 0|1` => `x == []` or `x != []`. Avoids walking the whole list to check emptiness. # `Enum.count(x) 0|1` => `Enum.empty?(x)` or `not Enum.empty?(x)` (same reason). - # (Credo.Check.Warning.ExpensiveEmptyEnumCheck) + # `String.length(x) 0|1` => `x == ""` or `x != ""`. Avoids walking the whole string. + # `byte_size(x) 0|1` => `x == ""` or `x != ""`. More idiomatic. + # (Credo.Check.Warning.ExpensiveEmptyEnumCheck, plus the String/binary equivalents) defp style({op, m, [lhs, rhs]} = ast) when op in [:==, :!=, :===, :!==, :>, :<, :>=, :<=] do rewrite_empty_check(op, lhs, rhs, m) || ast end @@ -359,6 +374,8 @@ defmodule Styler.Style.SingleNode do defp size_call({:length, _, [x]}), do: {:length, x} defp size_call({{:., _, [{:__aliases__, _, [:Enum]}, :count]}, _, [x]}), do: {:enum_count, x} + defp size_call({{:., _, [{:__aliases__, _, [:String]}, :length]}, _, [x]}), do: {:string_length, x} + defp size_call({:byte_size, _, [x]}), do: {:byte_size, x} defp size_call(_), do: nil defp int_literal({:__block__, _, [n]}) when n in [0, 1], do: n @@ -398,4 +415,12 @@ defmodule Styler.Style.SingleNode do nil -> nil end end + + defp emit_empty_check(op, {kind, x}, n, m) when kind in [:string_length, :byte_size] do + case empty_class(op, n) do + :empty -> {:==, m, [x, {:__block__, [line: m[:line]], [""]}]} + :not_empty -> {:!=, m, [x, {:__block__, [line: m[:line]], [""]}]} + nil -> nil + end + end end diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 1055d7b..991ae50 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -696,6 +696,178 @@ defmodule Styler.Style.PipesTest do ) end + test "FilterReject: collapses Enum.filter |> Enum.reject into one filter with the second negated" do + assert_style( + """ + list + |> Enum.filter(f1) + |> Enum.reject(f2) + """, + """ + Enum.filter(list, fn item -> f1.(item) && !f2.(item) end) + """ + ) + end + + test "FilterReject: leaves chains alone when the modules don't both match Enum" do + assert_style(""" + list + |> Stream.filter(f1) + |> Enum.reject(f2) + """) + + assert_style(""" + list + |> Enum.filter(f1) + |> Stream.reject(f2) + """) + end + + test "RejectFilter: collapses Enum.reject |> Enum.filter into one filter with the first negated" do + assert_style( + """ + list + |> Enum.reject(f1) + |> Enum.filter(f2) + """, + """ + Enum.filter(list, fn item -> !f1.(item) && f2.(item) end) + """ + ) + end + + test "RejectFilter: leaves chains alone when the modules don't both match Enum" do + assert_style(""" + list + |> Stream.reject(f1) + |> Enum.filter(f2) + """) + + assert_style(""" + list + |> Enum.reject(f1) + |> Stream.filter(f2) + """) + end + + test "MapMap: collapses two named-capture maps into a pipe" do + assert_style( + """ + list + |> Enum.map(&Mod.foo/1) + |> Enum.map(&Mod.bar/1) + """, + """ + Enum.map(list, fn arg1 -> arg1 |> Mod.foo() |> Mod.bar() end) + """ + ) + + assert_style( + """ + list + |> Enum.map(&foo/1) + |> Enum.map(&bar/1) + """, + """ + Enum.map(list, fn arg1 -> arg1 |> foo() |> bar() end) + """ + ) + end + + test "MapMap: collapses &fun(&1) shorthand into a pipe" do + assert_style( + """ + list + |> Enum.map(&foo(&1)) + |> Enum.map(&Mod.bar(&1)) + """, + """ + Enum.map(list, fn arg1 -> arg1 |> foo() |> Mod.bar() end) + """ + ) + end + + test "MapMap: pivots on the first arg of f1 when its placeholder isn't there" do + # User case from billing_core/.../process_bloomberg_import.ex. Both sides are inlineable; + # f1 puts the placeholder in position 2, so the merge pivots on `import_record` (f1's first + # arg) and the iter-var defaults to `arg1` since neither side is an inline `fn x -> ...`. + assert_style( + """ + list + |> Enum.map(&build_changeset(import_record, &1)) + |> Enum.map(fn changeset -> Repo.insert(changeset, on_conflict: :nothing) end) + """, + """ + Enum.map(list, fn arg1 -> import_record |> build_changeset(arg1) |> Repo.insert(on_conflict: :nothing) end) + """ + ) + end + + test "MapMap: skips when either side can't be cleanly inlined" do + # Variables (could be any function), multi-`&1` captures (would call f1 twice), and inline + # fns whose body uses the var more than once or zero times — leave those alone. + assert_style(""" + list + |> Enum.map(f1) + |> Enum.map(f2) + """) + + assert_style(""" + list + |> Enum.map(&foo/1) + |> Enum.map(&(&1 + &1)) + """) + + assert_style(""" + list + |> Enum.map(fn x -> x + x end) + |> Enum.map(&Mod.fun/1) + """) + end + + test "MapMap: regression — extends an already-piped fn body without producing a malformed single-arg pipe" do + # Repro from a real codebase: f1's body is itself a pipe (`element |> String.replace(...) |> + # ...`). The merge needs to leave that pipe alone and just extend it with f2's call. Earlier, + # `pipify` mis-treated the inner `:|>` as a regular call and produced `{:|>, _, [single_arg]}`, + # which crashed `fix_pipe_start`. + assert_style( + """ + status + |> Enum.map(fn element -> + element |> String.replace(~r/([A-Z])/, "_\\\\1") |> String.downcase() + end) + |> Enum.map(fn element -> String.to_atom(element) end) + """, + """ + Enum.map(status, fn element -> element |> String.replace(~r/([A-Z])/, "_\\\\1") |> String.downcase() |> String.to_atom() end) + """ + ) + end + + test "MapMap: skips when the merged body wouldn't make a 2+ stage pipe" do + # `&(&1 + 1)` inlines to `arg + 1` which isn't a pipify-able call, so the merge would only + # produce a single pipe (which Styler immediately undoes). Don't bother starting. + assert_style(""" + list + |> Enum.map(&(&1 + 1)) + |> Enum.map(&foo/1) + """) + end + + test "MapMap: leaves non-Enum.map chains alone" do + assert_style(""" + list + |> Stream.map(f1) + |> Stream.map(f2) + """) + + assert_style(""" + list + |> Enum.map(f1) + |> Enum.filter(f2) + """) + end + test "filter/count" do for enum <- ~w(Enum Stream) do assert_style( diff --git a/test/style/single_node_test.exs b/test/style/single_node_test.exs index cf46442..58b34d2 100644 --- a/test/style/single_node_test.exs +++ b/test/style/single_node_test.exs @@ -413,6 +413,45 @@ defmodule Styler.Style.SingleNodeTest do assert_style("foo == 0") assert_style("Enum.count(x, fun) == 0") end + + test "rewrites `String.length(x) 0|1` to comparisons against `\"\"`" do + assert_style(~s|String.length(s) == 0|, ~s|s == ""|) + assert_style(~s|String.length(s) === 0|, ~s|s == ""|) + assert_style(~s|String.length(s) != 0|, ~s|s != ""|) + assert_style(~s|String.length(s) > 0|, ~s|s != ""|) + assert_style(~s|String.length(s) <= 0|, ~s|s == ""|) + assert_style(~s|String.length(s) >= 1|, ~s|s != ""|) + assert_style(~s|String.length(s) < 1|, ~s|s == ""|) + assert_style(~s|0 < String.length(s)|, ~s|s != ""|) + end + + test "rewrites `byte_size(x) 0|1` to comparisons against `\"\"`" do + assert_style(~s|byte_size(s) == 0|, ~s|s == ""|) + assert_style(~s|byte_size(s) != 0|, ~s|s != ""|) + assert_style(~s|byte_size(s) > 0|, ~s|s != ""|) + assert_style(~s|0 == byte_size(s)|, ~s|s == ""|) + end + end + + describe "negated is_* guards" do + test "!is_nil(x) => not is_nil(x)" do + assert_style("!is_nil(x)", "not is_nil(x)") + end + + test "covers other built-in is_* guards" do + assert_style("!is_atom(x)", "not is_atom(x)") + assert_style("!is_binary(x)", "not is_binary(x)") + assert_style("!is_list(x)", "not is_list(x)") + assert_style("!is_map(x)", "not is_map(x)") + assert_style("!is_struct(x, Foo)", "not is_struct(x, Foo)") + assert_style("!is_integer(x)", "not is_integer(x)") + end + + test "leaves unrelated `!` calls alone" do + assert_style("!x") + assert_style("!some_predicate(x)") + assert_style("!Map.has_key?(map, :foo)") + end end describe "to_timeout" do From f20b7ba7749fd2085fe0665844b004af383aa18a Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 22:12:48 -0400 Subject: [PATCH 2/4] Add check for shadowed vars --- lib/style/pipes.ex | 40 +++++++++++++++++++++++++++++++-------- test/style/pipes_test.exs | 23 ++++++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 40ef342..591e38c 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -422,12 +422,11 @@ defmodule Styler.Style.Pipes do ), do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm, negate_f1: true)]}]} - # `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 rest of the pipe chain. If either side can't be cleanly inlined, - # f1 doesn't pipify (e.g. it inlined to an operator), or f2 doesn't put its placeholder in position - # 1 (so the seed pipe wouldn't unfold), skip — leaving the original two-map chain. - # (Credo.Check.Refactor.MapMap) + # `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 + # rest of the pipe chain. If either side can't be cleanly inlined, f1 doesn't pipify (e.g. it inlined to an operator), + # or f2 doesn't put its placeholder in position 1 (so the seed pipe wouldn't unfold), skip — leaving the original + # two-map chain. (Credo.Check.Refactor.MapMap) defp fix_pipe( pipe_chain( pm, @@ -438,6 +437,7 @@ defmodule Styler.Style.Pipes do ) do with true <- inlineable?(f1) and inlineable?(f2) and placeholder_in_first_position?(f2), item_name = iteration_var_name(f1), + false <- shadows_free_var?(item_name, f1, f2), item = {item_name, [line: fm[:line]], nil}, inlined_f1 = inline_capture(f1, item, fm[:line]), {:|>, _, _} = f1_seed <- pipify(inlined_f1) do @@ -628,13 +628,37 @@ defmodule Styler.Style.Pipes do defp inlineable?(_), do: false - # If either side is an inline `fn x -> ...`, prefer that var name for the merged lambda — the - # source already named the iteration value. Otherwise, fall back to `arg1`. + # If either side is an inline `fn x -> ...`, prefer that var name for the merged lambda - the source already named the + # iteration value. Otherwise, fall back to `arg1`. defp iteration_var_name({:fn, _, [{:->, _, [[{name, _, ctx}], _]}]}) when is_atom(name) and is_atom(ctx) and name != :_, do: name defp iteration_var_name(_), do: :arg1 + # The merged lambda introduces a fresh binding for `name`. If that same name appears as a free variable in either + # side's body, it referred to a closure binding in the source - after merging, the new lambda's parameter would shadow + # it, silently changing semantics. Conservatively report any reference to `name` outside the side's own parameter as a + # shadow risk; refs inside a nested `fn`/`&` are technically rebindable but `inlineable?` already rejects most such + # cases. + defp shadows_free_var?(name, f1, f2), do: free_var_in?(name, f1) or free_var_in?(name, f2) + + defp free_var_in?(name, {:fn, _, [{:->, _, [[{param, _, ctx}], body]}]}) when is_atom(param) and is_atom(ctx), + do: param != name and var_in_ast?(body, name) + + defp free_var_in?(name, {:&, _, [body]}), do: var_in_ast?(body, name) + defp free_var_in?(_, _), do: false + + defp var_in_ast?(ast, name) do + {_, found} = + Macro.prewalk(ast, false, fn + node, true -> {node, true} + {var, _, ctx} = node, false when var == name and is_atom(ctx) -> {node, true} + node, acc -> {node, acc} + end) + + found + end + # The seed-pipe trick only unfolds when f2's placeholder lands in arg position 1 of an outer call. # If it lands in position 2+, we'd produce something like `Mod.fun(other, pipe)`, which Styler's # `f(pipe, args)` rule won't touch and leaves an awkward partial pipe stranded inside an arg list. diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 991ae50..6b337f2 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -854,6 +854,29 @@ defmodule Styler.Style.PipesTest do """) end + test "MapMap: skips when the iteration var name collides with a closure var in f2" do + # Reported by Cursor Bugbot: picking f1's param name as the merged lambda's parameter would + # shadow a same-named closure variable referenced in f2's body, silently changing semantics. + assert_style(""" + list + |> Enum.map(fn config -> transform(config) end) + |> Enum.map(fn x -> apply_with(x, config) end) + """) + + assert_style(""" + list + |> Enum.map(fn config -> transform(config) end) + |> Enum.map(&apply_with(&1, config)) + """) + + # Default `:arg1` iter-var case: a closure named `arg1` in f1 must also block the merge. + assert_style(""" + list + |> Enum.map(&build_changeset(arg1, &1)) + |> Enum.map(fn cs -> Repo.insert(cs) end) + """) + end + test "MapMap: leaves non-Enum.map chains alone" do assert_style(""" list From 1a55af9116dc30746547c4a7882b64794eb6609d Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 22:34:51 -0400 Subject: [PATCH 3/4] Drop byte_size empty-check rewrite byte_size(x) > 0 and x != "" are equivalent and equally efficient on binaries (byte_size reads the size header in O(1), and binary inequality short-circuits on that same header). Neither form is more idiomatic than the other, so there's no clear win to rewriting one to the other. Keep the String.length rewrite, which is a real perf win. --- lib/style/single_node.ex | 6 ++---- test/style/single_node_test.exs | 7 ------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/lib/style/single_node.ex b/lib/style/single_node.ex index cefb6af..8ac68a8 100644 --- a/lib/style/single_node.ex +++ b/lib/style/single_node.ex @@ -221,8 +221,7 @@ defmodule Styler.Style.SingleNode do # `length(x) 0|1` => `x == []` or `x != []`. Avoids walking the whole list to check emptiness. # `Enum.count(x) 0|1` => `Enum.empty?(x)` or `not Enum.empty?(x)` (same reason). # `String.length(x) 0|1` => `x == ""` or `x != ""`. Avoids walking the whole string. - # `byte_size(x) 0|1` => `x == ""` or `x != ""`. More idiomatic. - # (Credo.Check.Warning.ExpensiveEmptyEnumCheck, plus the String/binary equivalents) + # (Credo.Check.Warning.ExpensiveEmptyEnumCheck, plus the String equivalent) defp style({op, m, [lhs, rhs]} = ast) when op in [:==, :!=, :===, :!==, :>, :<, :>=, :<=] do rewrite_empty_check(op, lhs, rhs, m) || ast end @@ -375,7 +374,6 @@ defmodule Styler.Style.SingleNode do defp size_call({:length, _, [x]}), do: {:length, x} defp size_call({{:., _, [{:__aliases__, _, [:Enum]}, :count]}, _, [x]}), do: {:enum_count, x} defp size_call({{:., _, [{:__aliases__, _, [:String]}, :length]}, _, [x]}), do: {:string_length, x} - defp size_call({:byte_size, _, [x]}), do: {:byte_size, x} defp size_call(_), do: nil defp int_literal({:__block__, _, [n]}) when n in [0, 1], do: n @@ -416,7 +414,7 @@ defmodule Styler.Style.SingleNode do end end - defp emit_empty_check(op, {kind, x}, n, m) when kind in [:string_length, :byte_size] do + defp emit_empty_check(op, {:string_length, x}, n, m) do case empty_class(op, n) do :empty -> {:==, m, [x, {:__block__, [line: m[:line]], [""]}]} :not_empty -> {:!=, m, [x, {:__block__, [line: m[:line]], [""]}]} diff --git a/test/style/single_node_test.exs b/test/style/single_node_test.exs index 58b34d2..9cccb30 100644 --- a/test/style/single_node_test.exs +++ b/test/style/single_node_test.exs @@ -424,13 +424,6 @@ defmodule Styler.Style.SingleNodeTest do assert_style(~s|String.length(s) < 1|, ~s|s == ""|) assert_style(~s|0 < String.length(s)|, ~s|s != ""|) end - - test "rewrites `byte_size(x) 0|1` to comparisons against `\"\"`" do - assert_style(~s|byte_size(s) == 0|, ~s|s == ""|) - assert_style(~s|byte_size(s) != 0|, ~s|s != ""|) - assert_style(~s|byte_size(s) > 0|, ~s|s != ""|) - assert_style(~s|0 == byte_size(s)|, ~s|s == ""|) - end end describe "negated is_* guards" do From 33551cc14082ad791de68a84f422e191c7ddf8ae Mon Sep 17 00:00:00 2001 From: Jesse Herrick Date: Wed, 6 May 2026 22:40:29 -0400 Subject: [PATCH 4/4] MapMap: prefer f2's iter-var name when only f2 names it Reported by Cursor Bugbot: the comment on iteration_var_name said "if either side is an inline fn x -> ...", but the function only inspected f1. When f1 was a capture (e.g. &Mod.foo/1) and f2 was fn descriptive_name -> ..., the merged lambda defaulted to :arg1 instead of using f2's more meaningful name. --- lib/style/pipes.ex | 10 ++++++---- test/style/pipes_test.exs | 10 ++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 591e38c..c41bcf1 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -436,7 +436,7 @@ defmodule Styler.Style.Pipes do ) = node ) do with true <- inlineable?(f1) and inlineable?(f2) and placeholder_in_first_position?(f2), - item_name = iteration_var_name(f1), + item_name = iteration_var_name(f1, f2), false <- shadows_free_var?(item_name, f1, f2), item = {item_name, [line: fm[:line]], nil}, inlined_f1 = inline_capture(f1, item, fm[:line]), @@ -629,11 +629,13 @@ defmodule Styler.Style.Pipes do defp inlineable?(_), do: false # If either side is an inline `fn x -> ...`, prefer that var name for the merged lambda - the source already named the - # iteration value. Otherwise, fall back to `arg1`. - defp iteration_var_name({:fn, _, [{:->, _, [[{name, _, ctx}], _]}]}) when is_atom(name) and is_atom(ctx) and name != :_, + # iteration value. Prefer f1's name when both are named. Otherwise, fall back to `arg1`. + defp iteration_var_name(f1, f2), do: fn_var_name(f1) || fn_var_name(f2) || :arg1 + + defp fn_var_name({:fn, _, [{:->, _, [[{name, _, ctx}], _]}]}) when is_atom(name) and is_atom(ctx) and name != :_, do: name - defp iteration_var_name(_), do: :arg1 + defp fn_var_name(_), do: nil # The merged lambda introduces a fresh binding for `name`. If that same name appears as a free variable in either # side's body, it referred to a closure binding in the source - after merging, the new lambda's parameter would shadow diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 6b337f2..5349462 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -790,7 +790,8 @@ defmodule Styler.Style.PipesTest do test "MapMap: pivots on the first arg of f1 when its placeholder isn't there" do # User case from billing_core/.../process_bloomberg_import.ex. Both sides are inlineable; # f1 puts the placeholder in position 2, so the merge pivots on `import_record` (f1's first - # arg) and the iter-var defaults to `arg1` since neither side is an inline `fn x -> ...`. + # arg). f1 is a capture so contributes no name; the merged lambda picks up `changeset` from + # f2's `fn changeset -> ...`. assert_style( """ list @@ -798,7 +799,7 @@ defmodule Styler.Style.PipesTest do |> Enum.map(fn changeset -> Repo.insert(changeset, on_conflict: :nothing) end) """, """ - Enum.map(list, fn arg1 -> import_record |> build_changeset(arg1) |> Repo.insert(on_conflict: :nothing) end) + Enum.map(list, fn changeset -> import_record |> build_changeset(changeset) |> Repo.insert(on_conflict: :nothing) end) """ ) end @@ -869,11 +870,12 @@ defmodule Styler.Style.PipesTest do |> Enum.map(&apply_with(&1, config)) """) - # Default `:arg1` iter-var case: a closure named `arg1` in f1 must also block the merge. + # Default `:arg1` iter-var case (neither side names its iter-var): a closure named `arg1` in + # f1 must also block the merge. assert_style(""" list |> Enum.map(&build_changeset(arg1, &1)) - |> Enum.map(fn cs -> Repo.insert(cs) end) + |> Enum.map(&Repo.insert/1) """) end