fix alias deletion crash

CHANGELOG.md

@@ -9,7 +9,7 @@
 
 ### Fixes
 
-* fix an obscure & unreported bug when styling modules whose only child was a literal
+* various fixes for unreported obscure crashes related to module directive styling
 
 ## v0.11.9
 

lib/style/module_directives.ex

@@ -191,8 +191,7 @@ defmodule Styler.Style.ModuleDirectives do
     uses = (directives[:use] || []) |> Enum.flat_map(&expand_directive/1) |> reset_newlines()
     imports = expand_and_sort(directives[:import] || [])
     requires = expand_and_sort(directives[:require] || [])
-    all_aliases = directives[:alias] || []
-    aliases = expand_and_sort(all_aliases)
+    aliases = expand_and_sort(directives[:alias] || [])
 
     directives =
       [
@@ -207,20 +206,16 @@ defmodule Styler.Style.ModuleDirectives do
       |> Enum.concat()
       |> fix_line_numbers(List.first(nondirectives))
 
-    cond do
-      # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion
-      Enum.empty?(directives) and Enum.empty?(all_aliases) ->
-        parent
-
-      Enum.empty?(nondirectives) ->
-        Zipper.update(parent, &Zipper.replace_children(&1, directives))
-
-      true ->
-        parent
-        |> Zipper.update(&Zipper.replace_children(&1, directives))
-        |> Zipper.down()
-        |> Zipper.rightmost()
-        |> Zipper.insert_siblings(nondirectives)
+    # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion
+    if Enum.empty?(directives) do
+      Zipper.replace_children(parent, nondirectives)
+    else
+      # this ensures we continue the traversal _after_ any directives
+      parent
+      |> Zipper.replace_children(directives)
+      |> Zipper.down()
+      |> Zipper.rightmost()
+      |> Zipper.insert_siblings(nondirectives)
     end
   end
 

lib/styler.ex

@@ -2,10 +2,12 @@
 # This file is licensed to you under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License. You may obtain a copy
 # of the License at http://www.apache.org/licenses/LICENSE-2.0
+
 # Unless required by applicable law or agreed to in writing, software distributed under
 # the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
 # OF ANY KIND, either express or implied. See the License for the specific language
 # governing permissions and limitations under the License.
+
 defmodule Styler do
   @moduledoc """
   Styler is a formatter plugin with stronger opinions on code organization, multi-line defs and other code-style matters.

lib/zipper.ex

@@ -48,14 +48,15 @@ defmodule Styler.Zipper do
   def children({_, _}), do: []
 
   @doc """
-  Returns a new node, given an existing node and new children.
+  Returns the zipper with the current children of the node replaced with `children`
   """
-  @spec replace_children(tree, [tree]) :: tree
-  def replace_children({form, meta, _}, children) when is_atom(form), do: {form, meta, children}
-  def replace_children({_form, meta, args}, [first | rest]) when is_list(args), do: {first, meta, rest}
-  def replace_children({_, _}, [left, right]), do: {left, right}
-  def replace_children({_, _}, children), do: {:{}, [], children}
-  def replace_children(list, children) when is_list(list), do: children
+  @spec replace_children(zipper, [tree]) :: zipper
+  def replace_children({node, meta}, children) when is_list(children), do: {replace_node_children(node, children), meta}
+
+  defp replace_node_children({form, meta, _}, children) when is_atom(form), do: {form, meta, children}
+  defp replace_node_children({_form, meta, args}, [first | rest]) when is_list(args), do: {first, meta, rest}
+  defp replace_node_children({_, _}, [left, right]), do: {left, right}
+  defp replace_node_children(list, children) when is_list(list), do: children
 
   @doc """
   Creates a zipper from a tree node.
@@ -104,7 +105,7 @@ defmodule Styler.Zipper do
   def up({tree, meta}) do
     children = Enum.reverse(meta.l, [tree | meta.r])
     {parent, parent_meta} = meta.ptree
-    {replace_children(parent, children), parent_meta}
+    {replace_node_children(parent, children), parent_meta}
   end
 
   @doc """
@@ -163,7 +164,7 @@ defmodule Styler.Zipper do
   @spec remove(zipper) :: zipper
   def remove({_, nil}), do: raise(ArgumentError, message: "Cannot remove the top level node.")
   def remove({_, %{l: [left | rest]} = meta}), do: prev_down({left, %{meta | l: rest}})
-  def remove({_, %{ptree: {parent, parent_meta}, r: children}}), do: {replace_children(parent, children), parent_meta}
+  def remove({_, %{ptree: {p, p_meta}, r: children}}), do: {replace_node_children(p, children), p_meta}
 
   @doc """
   Inserts the item as the left sibling of the node at this zipper, without

test/style/module_directives_test.exs

@@ -483,6 +483,15 @@ defmodule Styler.Style.ModuleDirectivesTest do
   test "Deletes root level alias" do
     assert_style("alias Foo", "")
 
+    assert_style(
+      """
+      alias Foo
+
+      Foo.bar()
+      """,
+      "Foo.bar()"
+    )
+
     assert_style(
       """
       alias unquote(Foo)

test/zipper_test.exs

@@ -37,24 +37,21 @@ defmodule StylerTest.ZipperTest do
 
   describe "replace_children/2" do
     test "2-tuples" do
-      assert Zipper.replace_children({1, 2}, [3, 4]) == {3, 4}
-    end
-
-    test "changing to 2-tuples arity" do
-      assert Zipper.replace_children({1, 2}, [3, 4, 5]) == {:{}, [], [3, 4, 5]}
-      assert Zipper.replace_children({1, 2}, [3]) == {:{}, [], [3]}
+      assert {1, 2} |> Zipper.zip() |> Zipper.replace_children([3, 4]) |> Zipper.node() == {3, 4}
     end
 
     test "lists" do
-      assert Zipper.replace_children([1, 2, 3], [:a, :b, :c]) == [:a, :b, :c]
+      assert [1, 2, 3] |> Zipper.zip() |> Zipper.replace_children([:a, :b, :c]) |> Zipper.node() == [:a, :b, :c]
     end
 
     test "unqualified calls" do
-      assert Zipper.replace_children({:foo, [], [1, 2]}, [:a, :b]) == {:foo, [], [:a, :b]}
+      assert {:foo, [], [1, 2]} |> Zipper.zip() |> Zipper.replace_children([:a, :b]) |> Zipper.node() ==
+               {:foo, [], [:a, :b]}
     end
 
     test "qualified calls" do
-      assert Zipper.replace_children({{:., [], [1, 2]}, [], [3, 4]}, [:a, :b, :c]) == {:a, [], [:b, :c]}
+      assert {{:., [], [1, 2]}, [], [3, 4]} |> Zipper.zip() |> Zipper.replace_children([:a, :b, :c]) |> Zipper.node() ==
+               {:a, [], [:b, :c]}
     end
   end