improve with statement replacements

CHANGELOG.md

@@ -6,6 +6,7 @@ they can and will change without that change being reflected in Styler's semanti
 
 ### Improvements
 
+- `with do: body` and variations with no arrows in the head will be rewritten to just `body`
 - `# styler:sort` will sort arbitrary ast nodes within a `do end` block:
 
     Given:
@@ -30,8 +31,6 @@ they can and will change without that change being reflected in Styler's semanti
           another_macro :y
         end
 
-
-
 ### Fixes
 
 - fix a bug in comment-movement when multiple `# styler:sort` directives are added to a file at the same time

lib/style/blocks.ex

@@ -75,103 +75,32 @@ defmodule Styler.Style.Blocks do
     {:cont, Zipper.replace(zipper, {:if, m, children}), ctx}
   end
 
-  # Credo.Check.Refactor.WithClauses
-  def run({{:with, with_meta, children}, _} = zipper, ctx) when is_list(children) do
-    # a std lib `with` block will have at least one left arrow and a `do` body. anything else we skip ¯\_(ツ)_/¯
-    arrow_or_match? = &(left_arrow?(&1) || match?({:=, _, _}, &1))
-
-    if Enum.any?(children, arrow_or_match?) and Enum.any?(children, &Style.do_block?/1) do
-      {preroll, children} =
-        children
-        |> Enum.map(fn
-          # `_ <- rhs` => `rhs`
-          {:<-, _, [{:_, _, _}, rhs]} -> rhs
-          # `lhs <- rhs` => `lhs = rhs`
-          {:<-, m, [{atom, _, nil} = lhs, rhs]} when is_atom(atom) -> {:=, m, [lhs, rhs]}
-          child -> child
-        end)
-        |> Enum.split_while(&(not left_arrow?(&1)))
-
-      # after rewriting `x <- y()` to `x = y()` there are no more arrows.
-      # this never should've been a with statement at all! we can just replace it with assignments
-      if Enum.empty?(children) do
-        {:cont, replace_with_statement(zipper, preroll), ctx}
-      else
-        [[{{_, do_meta, _} = do_block, do_body} | elses] | reversed_clauses] = Enum.reverse(children)
-        {postroll, reversed_clauses} = Enum.split_while(reversed_clauses, &(not left_arrow?(&1)))
-        [{:<-, final_clause_meta, [lhs, rhs]} = _final_clause | rest] = reversed_clauses
-
-        # drop singleton identity else clauses like `else foo -> foo end`
-        elses =
-          case elses do
-            [{{_, _, [:else]}, [{:->, _, [[left], right]}]}] -> if nodes_equivalent?(left, right), do: [], else: elses
-            _ -> elses
-          end
-
-        {reversed_clauses, do_body} =
-          cond do
-            # Put the postroll into the body
-            Enum.any?(postroll) ->
-              {node, do_body_meta, do_children} = do_body
-              do_children = if node == :__block__, do: do_children, else: [do_body]
-              do_body = {:__block__, Keyword.take(do_body_meta, [:line]), Enum.reverse(postroll, do_children)}
-              {reversed_clauses, do_body}
-
-            # Credo.Check.Refactor.RedundantWithClauseResult
-            Enum.empty?(elses) and nodes_equivalent?(lhs, do_body) ->
-              {rest, rhs}
-
-            # no change
-            true ->
-              {reversed_clauses, do_body}
-          end
-
-        do_line = do_meta[:line]
-        final_clause_line = final_clause_meta[:line]
-
-        do_line =
-          cond do
-            do_meta[:format] == :keyword && final_clause_line + 1 >= do_line -> do_line
-            do_meta[:format] == :keyword -> final_clause_line + 1
-            true -> final_clause_line
-          end
-
-        do_block = Macro.update_meta(do_block, &Keyword.put(&1, :line, do_line))
-        # disable keyword `, do:` since there will be multiple statements in the body
-        with_meta =
-          if Enum.any?(postroll),
-            do: Keyword.merge(with_meta, do: [line: with_meta[:line]], end: [line: Style.max_line(children) + 1]),
-            else: with_meta
-
-        with_children = Enum.reverse(reversed_clauses, [[{do_block, do_body} | elses]])
-        zipper = Zipper.replace(zipper, {:with, with_meta, with_children})
+  def run({{:with, _, [[{{:__block__, _, [:do]}, body} | _]]}, _} = zipper, ctx) do
+    {:cont, Zipper.replace(zipper, body), ctx}
+  end
 
-        cond do
-          # oops! RedundantWithClauseResult removed the final arrow in this. no more need for a with statement!
-          Enum.empty?(reversed_clauses) ->
-            {:cont, replace_with_statement(zipper, preroll ++ with_children), ctx}
-
-          # recurse if the # of `<-` have changed (this `with` could now be eligible for a `case` rewrite)
-          Enum.any?(preroll) ->
-            # put the preroll before the with statement in either a block we create or the existing parent block
-            zipper
-            |> Style.find_nearest_block()
-            |> Zipper.prepend_siblings(preroll)
-            |> run(ctx)
-
-          # the # of `<-` canged, so we should have another look at this with statement
-          Enum.any?(postroll) ->
-            run(zipper, ctx)
+  # Credo.Check.Refactor.WithClauses
+  def run({{:with, _, children}, _} = zipper, ctx) when is_list(children) do
+    do_block? = Enum.any?(children, &Style.do_block?/1)
+    arrow_or_match? = Enum.any?(children, &(left_arrow?(&1) || match?({:=, _, _}, &1)))
+
+    cond do
+      # we can style this!
+      do_block? and arrow_or_match? ->
+        style_with_statement(zipper, ctx)
+
+      # `with (head_statements) do: x (else ...)`
+      do_block? ->
+        # head statements can be the empty list, if it matters
+        {head_statements, [[{{:__block__, _, [:do]}, body} | _]]} = Enum.split_while(children, &(not Style.do_block?(&1)))
+        [first | rest] = head_statements ++ [body]
+        # replace this `with` statement with its headers + body
+        zipper = zipper |> Zipper.replace(first) |> Zipper.insert_siblings(rest)
+        {:cont, zipper, ctx}
 
-          true ->
-            # of clauess didn't change, so don't reecurse or we'll loop FOREEEVEERR
-            {:cont, zipper, ctx}
-        end
-      end
-    else
-      # maybe this isn't a with statement - could be a function named `with`
-      # or it's just a with statement with no arrows, but that's too saddening to imagine
-      {:cont, zipper, ctx}
+      # maybe this isn't a with statement - could be a function named `with` or something.
+      true ->
+        {:cont, zipper, ctx}
     end
   end
 
@@ -217,6 +146,97 @@ defmodule Styler.Style.Blocks do
 
   def run(zipper, ctx), do: {:cont, zipper, ctx}
 
+  # with statements can do _a lot_, so this beast of a function likewise does a lot.
+  defp style_with_statement({{:with, with_meta, children}, _} = zipper, ctx) do
+    {preroll, children} =
+      children
+      |> Enum.map(fn
+        # `_ <- rhs` => `rhs`
+        {:<-, _, [{:_, _, _}, rhs]} -> rhs
+        # `lhs <- rhs` => `lhs = rhs`
+        {:<-, m, [{atom, _, nil} = lhs, rhs]} when is_atom(atom) -> {:=, m, [lhs, rhs]}
+        child -> child
+      end)
+      |> Enum.split_while(&(not left_arrow?(&1)))
+
+    # after rewriting `x <- y()` to `x = y()` there are no more arrows.
+    # this never should've been a with statement at all! we can just replace it with assignments
+    if Enum.empty?(children) do
+      {:cont, replace_with_statement(zipper, preroll), ctx}
+    else
+      [[{{_, do_meta, _} = do_block, do_body} | elses] | reversed_clauses] = Enum.reverse(children)
+      {postroll, reversed_clauses} = Enum.split_while(reversed_clauses, &(not left_arrow?(&1)))
+      [{:<-, final_clause_meta, [lhs, rhs]} = _final_clause | rest] = reversed_clauses
+
+      # drop singleton identity else clauses like `else foo -> foo end`
+      elses =
+        case elses do
+          [{{_, _, [:else]}, [{:->, _, [[left], right]}]}] -> if nodes_equivalent?(left, right), do: [], else: elses
+          _ -> elses
+        end
+
+      {reversed_clauses, do_body} =
+        cond do
+          # Put the postroll into the body
+          Enum.any?(postroll) ->
+            {node, do_body_meta, do_children} = do_body
+            do_children = if node == :__block__, do: do_children, else: [do_body]
+            do_body = {:__block__, Keyword.take(do_body_meta, [:line]), Enum.reverse(postroll, do_children)}
+            {reversed_clauses, do_body}
+
+          # Credo.Check.Refactor.RedundantWithClauseResult
+          Enum.empty?(elses) and nodes_equivalent?(lhs, do_body) ->
+            {rest, rhs}
+
+          # no change
+          true ->
+            {reversed_clauses, do_body}
+        end
+
+      do_line = do_meta[:line]
+      final_clause_line = final_clause_meta[:line]
+
+      do_line =
+        cond do
+          do_meta[:format] == :keyword && final_clause_line + 1 >= do_line -> do_line
+          do_meta[:format] == :keyword -> final_clause_line + 1
+          true -> final_clause_line
+        end
+
+      do_block = Macro.update_meta(do_block, &Keyword.put(&1, :line, do_line))
+      # disable keyword `, do:` since there will be multiple statements in the body
+      with_meta =
+        if Enum.any?(postroll),
+          do: Keyword.merge(with_meta, do: [line: with_meta[:line]], end: [line: Style.max_line(children) + 1]),
+          else: with_meta
+
+      with_children = Enum.reverse(reversed_clauses, [[{do_block, do_body} | elses]])
+      zipper = Zipper.replace(zipper, {:with, with_meta, with_children})
+
+      cond do
+        # oops! RedundantWithClauseResult removed the final arrow in this. no more need for a with statement!
+        Enum.empty?(reversed_clauses) ->
+          {:cont, replace_with_statement(zipper, preroll ++ with_children), ctx}
+
+        # recurse if the # of `<-` have changed (this `with` could now be eligible for a `case` rewrite)
+        Enum.any?(preroll) ->
+          # put the preroll before the with statement in either a block we create or the existing parent block
+          zipper
+          |> Style.find_nearest_block()
+          |> Zipper.prepend_siblings(preroll)
+          |> run(ctx)
+
+        # the # of `<-` canged, so we should have another look at this with statement
+        Enum.any?(postroll) ->
+          run(zipper, ctx)
+
+        true ->
+          # of clauess didn't change, so don't reecurse or we'll loop FOREEEVEERR
+          {:cont, zipper, ctx}
+      end
+    end
+  end
+
   # `with a <- b(), c <- d(), do: :ok, else: (_ -> :error)`
   # =>
   # `a = b(); c = d(); :ok`

lib/zipper.ex

@@ -172,18 +172,18 @@ defmodule Styler.Zipper do
   top level.
   """
   @spec insert_left(zipper, tree) :: zipper
-  def insert_left({_, nil}, _), do: raise(ArgumentError, message: "Can't insert siblings at the top level.")
-  def insert_left({tree, meta}, child), do: {tree, %{meta | l: [child | meta.l]}}
+  def insert_left(zipper, child), do: prepend_siblings(zipper, [child])
 
   @doc """
   Inserts many siblings to the left.
+  If the node is at the top of the tree, builds a new root `:__block__` while maintaining focus on the current node.
 
   Equivalent to
 
       Enum.reduce(siblings, zipper, &Zipper.insert_left(&2, &1))
   """
   @spec prepend_siblings(zipper, [tree]) :: zipper
-  def prepend_siblings({_, nil}, _), do: raise(ArgumentError, message: "Can't insert siblings at the top level.")
+  def prepend_siblings({node, nil}, siblings), do: {:__block__, [], siblings ++ [node]} |> zip() |> down() |> rightmost()
   def prepend_siblings({tree, meta}, siblings), do: {tree, %{meta | l: Enum.reverse(siblings, meta.l)}}
 
   @doc """
@@ -192,18 +192,18 @@ defmodule Styler.Zipper do
   top level.
   """
   @spec insert_right(zipper, tree) :: zipper
-  def insert_right({_, nil}, _), do: raise(ArgumentError, message: "Can't insert siblings at the top level.")
-  def insert_right({tree, meta}, child), do: {tree, %{meta | r: [child | meta.r]}}
+  def insert_right(zipper, child), do: insert_siblings(zipper, [child])
 
   @doc """
   Inserts many siblings to the right.
+  If the node is at the top of the tree, builds a new root `:__block__` while maintaining focus on the current node.
 
   Equivalent to
 
       Enum.reduce(siblings, zipper, &Zipper.insert_right(&2, &1))
   """
   @spec insert_siblings(zipper, [tree]) :: zipper
-  def insert_siblings({_, nil}, _), do: raise(ArgumentError, message: "Can't insert siblings at the top level.")
+  def insert_siblings({node, nil}, siblings), do: {:__block__, [], [node | siblings]} |> zip() |> down()
   def insert_siblings({tree, meta}, siblings), do: {tree, %{meta | r: siblings ++ meta.r}}
 
   @doc """

test/style/blocks_test.exs

@@ -384,6 +384,11 @@ defmodule Styler.Style.BlocksTest do
         z
         """
       )
+
+      assert_style "with do: x", "x"
+      assert_style "with do x end", "x"
+      assert_style "with do x else foo -> bar end", "x"
+      assert_style "with foo() do bar() else _ -> baz() end", "foo()\nbar()"
     end
 
     test "doesn't false positive with vars" do
@@ -430,7 +435,7 @@ defmodule Styler.Style.BlocksTest do
         """
       )
 
-      for nontrivial_head <- ["foo", ":ok <- foo, :ok <- bar"] do
+      for nontrivial_head <- [":ok <- foo, :ok <- bar"] do
         assert_style("""
         with #{nontrivial_head} do
           :success

test/zipper_test.exs

@@ -470,9 +470,9 @@ defmodule StylerTest.ZipperTest do
              |> Zipper.root() == [1, :left, 2, :right, 3]
     end
 
-    test "raise when attempting to insert a sibling at the root" do
-      assert_raise ArgumentError, fn -> 42 |> Zipper.zip() |> Zipper.insert_left(:nope) end
-      assert_raise ArgumentError, fn -> 42 |> Zipper.zip() |> Zipper.insert_right(:nope) end
+    test "builds a new root node made of a block" do
+      assert {42, %{l: [:nope], ptree: {{:__block__, _, _}, nil}}} = 42 |> Zipper.zip() |> Zipper.insert_left(:nope)
+      assert {42, %{r: [:nope], ptree: {{:__block__, _, _}, nil}}} = 42 |> Zipper.zip() |> Zipper.insert_right(:nope)
     end
   end