Repository: adobe/elixir-styler
Pull request #20 (2023-04-20 20:58:40)

bugfix: moduledirectives weird parents
Found this while looking into unifying "wrap this node in a `:__block__` parent" code. Extracting that from the Pipes rule and using it universally will fix this bug in module directives


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ed407d2..0870c9e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Fixes
 
+* `ModuleDirectives` handles even weirder places to hide your aliases (anon functions, in this case)
 * `Pipes` tries even harder to keep single-pipe rewrites of invocations on one line
 
 ## v0.3.1
diff --git a/lib/style.ex b/lib/style.ex
index 294a484..6943ed3 100644
--- a/lib/style.ex
+++ b/lib/style.ex
@@ -31,6 +31,36 @@ defmodule Styler.Style do
   """
   @callback run(Zipper.zipper(), context()) :: {Zipper.command(), Zipper.zipper(), context()}
 
+  @doc """
+  Ensure the parent node can have multiple children.
+
+  If a context-changing node (a `do end` block or an `->` arrow block) is encountered
+  the child is wrapped in a `:__block__`
+
+  Other nodes (pipes, assignments) can only have a fixed number of children. This function
+  will recursively traverse up the zipper until it's found the parents of those nodes.
+  """
+  def ensure_block_parent(zipper) do
+    case Zipper.up(zipper) do
+      # Pipes and assignments have exactly two children - keep going up
+      {{:|>, _, _}, _} = parent -> ensure_block_parent(parent)
+      {{:=, _, _}, _} = parent -> ensure_block_parent(parent)
+      # the current zipper is an only-child of an arrow ala `true -> :ok`
+      # we need to change the body of the arrow to be a `:__block__` so our `:ok` can have siblings
+      {{:->, _, _}, _} -> wrap_in_block(zipper)
+      # parent is an only-child of a `do` block
+      {{_, _}, _} -> wrap_in_block(zipper)
+      # a snippet or script where the zipper is a single child with no parent above it
+      nil -> wrap_in_block(zipper)
+      # since its parent isn't one of the problem AST above, the current zipper's parent can have multiple children, so we're done
+      # could be `:def`, `:__block__`, ...
+      _ -> zipper
+    end
+  end
+
+  # give it a block parent, then step back to the child - we can insert next to it now that it's in a block
+  defp wrap_in_block(zipper), do: zipper |> Zipper.update(&{:__block__, [], [&1]}) |> Zipper.down()
+
   @doc """
   Set the line of all comments with `line` in `range_start..range_end` to instead have line `range_start`
   """
diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex
index 6339306..d18b9c5 100644
--- a/lib/style/module_directives.ex
+++ b/lib/style/module_directives.ex
@@ -65,6 +65,7 @@ defmodule Styler.Style.ModuleDirectives do
   """
   @behaviour Styler.Style
 
+  alias Styler.Style
   alias Styler.Zipper
 
   @directives ~w(alias import require use)a
@@ -109,19 +110,8 @@ defmodule Styler.Style.ModuleDirectives do
     end
   end
 
-  def run({{d, _, _} = directive, _} = zipper, ctx) when d in @directives do
-    parent =
-      case Zipper.up(zipper) do
-        nil ->
-          Zipper.replace(zipper, {:__block__, [], [directive]})
-
-        {{{:__block__, _, [:do]}, _only_child}, _} ->
-          Zipper.replace(zipper, {:__block__, [], [directive]})
-
-        parent ->
-          parent
-      end
-
+  def run({{directive, _, _}, _} = zipper, ctx) when directive in @directives do
+    parent = zipper |> Style.ensure_block_parent() |> Zipper.up()
     {:skip, organize_directives(parent), ctx}
   end
 
diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex
index 9e36340..6e74c1f 100644
--- a/lib/style/pipes.ex
+++ b/lib/style/pipes.ex
@@ -21,6 +21,7 @@ defmodule Styler.Style.Pipes do
 
   @behaviour Styler.Style
 
+  alias Styler.Style
   alias Styler.Zipper
 
   @blocks ~w(case if with cond for unless)a
@@ -87,7 +88,7 @@ defmodule Styler.Style.Pipes do
 
     zipper
     |> Zipper.replace({:|>, pipe_meta, [variable, rhs]})
-    |> find_valid_assignment_location()
+    |> Style.ensure_block_parent()
     |> Zipper.insert_left({:=, [], [variable, expression]})
   end
 
@@ -107,62 +108,6 @@ defmodule Styler.Style.Pipes do
     zipper |> Zipper.replace({:|>, pipe_meta, [lhs_rewrite, rhs]}) |> Zipper.next()
   end
 
-  # this really needs a better name.
-  # essentially what we're doing is walking up the tree in search of a parent where it would be syntactically valid
-  # to insert a new "assignment" node (`x = y`)
-  # as we walk up the tree, our parent will be either
-  # 1. an invalid node for an assignment (still in the pipeline or in another assignment)
-  # 2. the start of the context (function def start)
-  # 3. something else!
-  # for 1, we keep going up
-  # for 2, we wrap ourselves in a new block parent (where we can insert a sibling node)
-  # for 3, we're done - wherever it is we are, our parent already supports us inserting a sibling node
-  defp find_valid_assignment_location(zipper) do
-    case Zipper.up(zipper) do
-      # still trying to find our way up the pipe, keep walking...
-      {{:|>, _, _}, _} = parent -> find_valid_assignment_location(parent)
-      # the parent of this pipe is an assignment like
-      #
-      #   baz =
-      #     block do ... end
-      #     |> ...
-      #
-      # so we need to step up again and see what the assignment's parent is, with the goal of inserting our new
-      # assignment before the assignment built from the pipe chain, like:
-      #
-      #   block_result = block do ... end
-      #   baz =
-      #     block_result
-      #     |> ...
-      {{:=, _, _}, _} = parent -> find_valid_assignment_location(parent)
-      # we're in a function which is an immediate pipeline, like:
-      #
-      # def fun do
-      #   block do end
-      #   |> f()
-      # end
-      {{{:__block__, _, _}, {:|>, _, _}}, _} -> wrap_in_block(zipper)
-      # similar to the function definition, except it's an anonymous function this time
-      #
-      # fn ->
-      #   case do end
-      #   |> b()
-      # end
-      {{:->, _, [_, {:|>, _, _} | _]}, _} -> wrap_in_block(zipper)
-      # a snippet or script where the problem block has no parent
-      nil -> wrap_in_block(zipper)
-      # since its parent isn't one of the problem AST above, the current zipper must be a valid place to insert the node
-      _ -> zipper
-    end
-  end
-
-  # give it a block parent, then step back to the pipe - we can insert next to it now that it's in a block
-  defp wrap_in_block({node, _} = zipper) do
-    zipper
-    |> Zipper.replace({:__block__, [], [node]})
-    |> Zipper.next()
-  end
-
   # literal wrapper
   defp valid_pipe_start?({:__block__, _, _}), do: true
   defp valid_pipe_start?({:__aliases__, _, _}), do: true


Test patch:
diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs
index b12b7b4..3c5a566 100644
--- a/test/style/module_directives_test.exs
+++ b/test/style/module_directives_test.exs
@@ -201,7 +201,16 @@ defmodule Styler.Style.ModuleDirectivesTest do
     end
   end
 
-  describe "quote blocks" do
+  describe "strange parents!" do
+    test "anon function" do
+      assert_style("fn -> alias A.{C, B} end", """
+      fn ->
+        alias A.B
+        alias A.C
+      end
+      """)
+    end
+
     test "quote do with one child" do
       assert_style(
         """