Repository: adobe/elixir-styler
Pull request #72 (2023-09-10 17:39:48)

with statement rewrites
rewrite two credo rules:

- Credo.Check.Readability.WithSingleClause
- Credo.Check.Refactor.WithClauses

### Testing

There's a lot of weird ways `with` statements can be written.
Need to run this on a big codebase and see what goes kaboom... (I don't have any `with ..., do:` tests, for one)

**update**: Ran against a large codebase without errors: :shipit: 


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f58bdb9..64577c3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 * Added right-hand-pattern-matching rewrites to `for` and `with` left arrow expressions `<-`
   (ex: `with map = %{} <- foo()` => `with %{} = map <- foo`)
+* `with` statement rewrites, solving the following credo rules
+  * `Credo.Check.Readability.WithSingleClause`
+  * `Credo.Check.Refactor.RedundantWithClauseResult`
+  * `Credo.Check.Refactor.WithClauses`
 
 ## v0.8.5
 
diff --git a/README.md b/README.md
index fe596f9..226c21f 100644
--- a/README.md
+++ b/README.md
@@ -81,11 +81,14 @@ If you're using Credo and Styler, **we recommend disabling these rules in `.cred
 | `Credo.Check.Readability.SinglePipe` | |
 | `Credo.Check.Readability.StrictModuleLayout` | **potentially breaks compilation** (see notes above) |
 | `Credo.Check.Readability.UnnecessaryAliasExpansion` | |
+| `Credo.Check.Readability.WithSingleClause` | |
 | `Credo.Check.Refactor.CaseTrivialMatches` | |
 | `Credo.Check.Refactor.FilterCount` | in pipes only |
 | `Credo.Check.Refactor.MapInto` | in pipes only |
 | `Credo.Check.Refactor.MapJoin` | in pipes only |
 | `Credo.Check.Refactor.PipeChainStart` | allows ecto's `from`|
+| `Credo.Check.Refactor.RedundantWithClauseResult` | |
+| `Credo.Check.Refactor.WithClauses` | |
 
 ## Your first Styling
 
diff --git a/lib/style/single_node.ex b/lib/style/single_node.ex
index 573fb3b..69c5c16 100644
--- a/lib/style/single_node.ex
+++ b/lib/style/single_node.ex
@@ -18,7 +18,10 @@ defmodule Styler.Style.SingleNode do
   * Credo.Check.Readability.LargeNumbers
   * Credo.Check.Readability.ParenthesesOnZeroArityDefs
   * Credo.Check.Readability.PreferImplicitTry
+  * Credo.Check.Readability.WithSingleClause
   * Credo.Check.Refactor.CaseTrivialMatches
+  * Credo.Check.Refactor.RedundantWithClauseResult
+  * Credo.Check.Refactor.WithClauses
   """
 
   @behaviour Styler.Style
@@ -141,6 +144,63 @@ defmodule Styler.Style.SingleNode do
   defp style(trivial_case(head, {:__block__, _, [true]}, do_body, {:_, _, _}, else_body)),
     do: if_ast(head, do_body, else_body)
 
+  # Credo.Check.Readability.WithSingleClause
+  # rewrite `with success <- single_statement do body else ...elses end`
+  # to `case single_statement do success -> body; ...elses end`
+  defp style({:with, m, [{:<-, am, [success, single_statement]}, [body, elses]]}) do
+    {{:__block__, do_meta, [:do]}, body} = body
+    {{:__block__, _else_meta, [:else]}, elses} = elses
+    clauses = [{{:__block__, am, [:do]}, [{:->, do_meta, [[success], body]} | elses]}]
+    style({:case, m, [single_statement, clauses]})
+  end
+
+  # Credo.Check.Refactor.WithClauses
+  # Credo.Check.Refactor.RedundantWithClauseResult
+  defp style({:with, m, children} = with) when is_list(children) do
+    if Enum.any?(children, &left_arrow?/1) do
+      {preroll, children} = Enum.split_while(children, &(not left_arrow?(&1)))
+      # the do/else keyword macro of the with statement is the last element of the list
+      {[{do_block, body} | elses], clauses} = List.pop_at(children, -1)
+
+      {postroll, reversed_clauses} =
+        clauses
+        |> Enum.reverse()
+        |> Enum.split_while(&(not left_arrow?(&1)))
+
+      [{:<-, _, [lhs, rhs]} = _final_clause | rest] = reversed_clauses
+
+      # Credo.Check.Refactor.RedundantWithClauseResult
+      rewrite_body? = Enum.empty?(postroll) and Enum.empty?(elses) and nodes_equivalent?(lhs, body)
+
+      {reversed_clauses, body} =
+        if rewrite_body?,
+          do: {rest, [rhs]},
+          else: {reversed_clauses, Enum.reverse(postroll, [body])}
+
+      do_else = [{do_block, {:__block__, [], body}} | elses]
+      clauses = Enum.reverse(reversed_clauses, [do_else])
+      # @TODO check for redundant final node
+      # - can only be redundant if the body itself is a single ast node (after body has postroll added)
+      # - can only be redundant if there's no else
+      rewritten_with = {:with, m, clauses}
+      # only rewrite if it needs rewriting!
+      cond do
+        Enum.any?(preroll) ->
+          {:__block__, m, preroll ++ [rewritten_with]}
+
+        rewrite_body? or Enum.any?(postroll) ->
+          rewritten_with
+
+        true ->
+          with
+      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
+      with
+    end
+  end
+
   # ARROW REWRITES
   # `with`, `for` left arrow - if only we could write something this trivial for `->`!
   defp style({:<-, cm, [lhs, rhs]}), do: {:<-, cm, [put_matches_on_right(lhs), rhs]}
@@ -173,6 +233,14 @@ defmodule Styler.Style.SingleNode do
     |> Zipper.node()
   end
 
+  defp left_arrow?({:<-, _, _}), do: true
+  defp left_arrow?(_), do: false
+
+  defp nodes_equivalent?(a, b) do
+    # compare nodes without metadata
+    Style.update_all_meta(a, fn _ -> nil end) == Style.update_all_meta(b, fn _ -> nil end)
+  end
+
   # don't write an else clause if it's `false -> nil`
   defp if_ast(head, do_body, {:__block__, _, [nil]}), do: {:if, [do: []], [head, [{{:__block__, [], [:do]}, do_body}]]}
 


Test patch:
diff --git a/test/style/single_node_test.exs b/test/style/single_node_test.exs
index 0cd6920..a3c24fc 100644
--- a/test/style/single_node_test.exs
+++ b/test/style/single_node_test.exs
@@ -213,6 +213,27 @@ defmodule Styler.Style.SingleNodeTest do
       end
       """)
     end
+
+    test "with statements" do
+      assert_style(
+        """
+        with ok = :ok <- foo, :ok <- yeehaw() do
+          ok
+        else
+          error = :error -> error
+          other -> other
+        end
+        """,
+        """
+        with :ok = ok <- foo, :ok <- yeehaw() do
+          ok
+        else
+          :error = error -> error
+          other -> other
+        end
+        """
+      )
+    end
   end
 
   describe "numbers" do
@@ -354,4 +375,110 @@ defmodule Styler.Style.SingleNodeTest do
       )
     end
   end
+
+  describe "with statements" do
+    test "doesn't false positive with vars" do
+      assert_style("""
+      if naming_is_hard, do: with
+      """)
+    end
+
+    test "Credo.Check.Readability.WithSingleClause" do
+      assert_style(
+        """
+        with :ok <- foo do
+          :success
+        else
+          error = :error -> error
+          :fail -> :failure
+        end
+        """,
+        """
+        case foo do
+          :ok -> :success
+          :error = error -> error
+          :fail -> :failure
+        end
+        """
+      )
+
+      for nontrivial_head <- ["foo", ":ok = foo", ":ok <- foo, :ok <- bar"] do
+        assert_style("""
+        with #{nontrivial_head} do
+          :success
+        else
+          :fail -> :failure
+        end
+        """)
+      end
+    end
+
+    test "moves non-arrow clauses from the beginning & end" do
+      assert_style(
+        """
+        with foo, bar, :ok <- baz, :ok <- boz, a = bop, boop do
+          :horay!
+        else
+          :error -> :error
+        end
+        """,
+        """
+        foo
+        bar
+
+        with :ok <- baz, :ok <- boz do
+          a = bop
+          boop
+          :horay!
+        else
+          :error -> :error
+        end
+        """
+      )
+
+      assert_style(
+        """
+        with :ok <- baz, :ok <- boz, a = bop, boop do
+          :horay!
+        else
+          :error -> :error
+        end
+        """,
+        """
+        with :ok <- baz, :ok <- boz do
+          a = bop
+          boop
+          :horay!
+        else
+          :error -> :error
+        end
+        """
+      )
+    end
+
+    test "Credo.Check.Refactor.RedundantWithClauseResult" do
+      assert_style(
+        """
+        with {:ok, a} <- foo(),
+             {:ok, b} <- bar(a) do
+          {:ok, b}
+        end
+        """,
+        """
+        with {:ok, a} <- foo() do
+          bar(a)
+        end
+        """
+      )
+
+      assert_style("""
+      with {:ok, a} <- foo(),
+           {:ok, b} <- bar(a) do
+        {:ok, b}
+      else
+        error -> handle(error)
+      end
+      """)
+    end
+  end
 end