Skip to content

Commit f20b7ba

Browse files
committed
Add check for shadowed vars
1 parent 57a9316 commit f20b7ba

2 files changed

Lines changed: 55 additions & 8 deletions

File tree

lib/style/pipes.ex

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -422,12 +422,11 @@ defmodule Styler.Style.Pipes do
422422
),
423423
do: {:|>, pm, [lhs, {filter, fm, [combined_predicate(f1, f2, :&&, fm, negate_f1: true)]}]}
424424

425-
# `lhs |> Enum.map(f1) |> Enum.map(f2)` => single `Enum.map` whose body is the inlined nested call.
426-
# We seed the body with a one-step pipe inside f1's slot — Styler's existing `f(pipe, args)` walk
427-
# then unfolds the f2 call into the rest of the pipe chain. If either side can't be cleanly inlined,
428-
# f1 doesn't pipify (e.g. it inlined to an operator), or f2 doesn't put its placeholder in position
429-
# 1 (so the seed pipe wouldn't unfold), skip — leaving the original two-map chain.
430-
# (Credo.Check.Refactor.MapMap)
425+
# `lhs |> Enum.map(f1) |> Enum.map(f2)` => single `Enum.map` whose body is the inlined nested call. We seed the body
426+
# with a one-step pipe inside f1's slot - Styler's existing `f(pipe, args)` walk then unfolds the f2 call into the
427+
# rest of the pipe chain. If either side can't be cleanly inlined, f1 doesn't pipify (e.g. it inlined to an operator),
428+
# or f2 doesn't put its placeholder in position 1 (so the seed pipe wouldn't unfold), skip — leaving the original
429+
# two-map chain. (Credo.Check.Refactor.MapMap)
431430
defp fix_pipe(
432431
pipe_chain(
433432
pm,
@@ -438,6 +437,7 @@ defmodule Styler.Style.Pipes do
438437
) do
439438
with true <- inlineable?(f1) and inlineable?(f2) and placeholder_in_first_position?(f2),
440439
item_name = iteration_var_name(f1),
440+
false <- shadows_free_var?(item_name, f1, f2),
441441
item = {item_name, [line: fm[:line]], nil},
442442
inlined_f1 = inline_capture(f1, item, fm[:line]),
443443
{:|>, _, _} = f1_seed <- pipify(inlined_f1) do
@@ -628,13 +628,37 @@ defmodule Styler.Style.Pipes do
628628

629629
defp inlineable?(_), do: false
630630

631-
# If either side is an inline `fn x -> ...`, prefer that var name for the merged lambda the
632-
# source already named the iteration value. Otherwise, fall back to `arg1`.
631+
# If either side is an inline `fn x -> ...`, prefer that var name for the merged lambda - the source already named the
632+
# iteration value. Otherwise, fall back to `arg1`.
633633
defp iteration_var_name({:fn, _, [{:->, _, [[{name, _, ctx}], _]}]}) when is_atom(name) and is_atom(ctx) and name != :_,
634634
do: name
635635

636636
defp iteration_var_name(_), do: :arg1
637637

638+
# The merged lambda introduces a fresh binding for `name`. If that same name appears as a free variable in either
639+
# side's body, it referred to a closure binding in the source - after merging, the new lambda's parameter would shadow
640+
# it, silently changing semantics. Conservatively report any reference to `name` outside the side's own parameter as a
641+
# shadow risk; refs inside a nested `fn`/`&` are technically rebindable but `inlineable?` already rejects most such
642+
# cases.
643+
defp shadows_free_var?(name, f1, f2), do: free_var_in?(name, f1) or free_var_in?(name, f2)
644+
645+
defp free_var_in?(name, {:fn, _, [{:->, _, [[{param, _, ctx}], body]}]}) when is_atom(param) and is_atom(ctx),
646+
do: param != name and var_in_ast?(body, name)
647+
648+
defp free_var_in?(name, {:&, _, [body]}), do: var_in_ast?(body, name)
649+
defp free_var_in?(_, _), do: false
650+
651+
defp var_in_ast?(ast, name) do
652+
{_, found} =
653+
Macro.prewalk(ast, false, fn
654+
node, true -> {node, true}
655+
{var, _, ctx} = node, false when var == name and is_atom(ctx) -> {node, true}
656+
node, acc -> {node, acc}
657+
end)
658+
659+
found
660+
end
661+
638662
# The seed-pipe trick only unfolds when f2's placeholder lands in arg position 1 of an outer call.
639663
# If it lands in position 2+, we'd produce something like `Mod.fun(other, pipe)`, which Styler's
640664
# `f(pipe, args)` rule won't touch and leaves an awkward partial pipe stranded inside an arg list.

test/style/pipes_test.exs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -854,6 +854,29 @@ defmodule Styler.Style.PipesTest do
854854
""")
855855
end
856856

857+
test "MapMap: skips when the iteration var name collides with a closure var in f2" do
858+
# Reported by Cursor Bugbot: picking f1's param name as the merged lambda's parameter would
859+
# shadow a same-named closure variable referenced in f2's body, silently changing semantics.
860+
assert_style("""
861+
list
862+
|> Enum.map(fn config -> transform(config) end)
863+
|> Enum.map(fn x -> apply_with(x, config) end)
864+
""")
865+
866+
assert_style("""
867+
list
868+
|> Enum.map(fn config -> transform(config) end)
869+
|> Enum.map(&apply_with(&1, config))
870+
""")
871+
872+
# Default `:arg1` iter-var case: a closure named `arg1` in f1 must also block the merge.
873+
assert_style("""
874+
list
875+
|> Enum.map(&build_changeset(arg1, &1))
876+
|> Enum.map(fn cs -> Repo.insert(cs) end)
877+
""")
878+
end
879+
857880
test "MapMap: leaves non-Enum.map chains alone" do
858881
assert_style("""
859882
list

0 commit comments

Comments
 (0)