Repository: adobe/elixir-styler
Pull request #194 (2024-09-09 17:17:11)

Remove unless from codebases
[unless is being deprecated](https://github.com/elixir-lang/elixir/pull/13769#issuecomment-2334878315)

[unless is complex to read](https://github.com/adobe/elixir-styler/issues/190)


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ce20b8d..616ca03 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,9 @@ they can and will change without that change being reflected in Styler's semanti
 
 ### Improvements
 
+The big change here is the rewrite/removal of `unless` due to [unless "eventually" being deprecated](https://github.com/elixir-lang/elixir/pull/13769#issuecomment-2334878315). Thanks to @janpieper and @ypconstante for bringing this up in #190.
+
+* `unless`: rewrite all `unless` to `if` (#190)
 * `pipes`: optimize `|> Stream.{each|map}(fun) |> Stream.run()` to `|> Enum.each(fun)`
 
 ### Fixes
diff --git a/docs/control_flow_macros.md b/docs/control_flow_macros.md
index 3924303..70a8546 100644
--- a/docs/control_flow_macros.md
+++ b/docs/control_flow_macros.md
@@ -11,13 +11,15 @@ The number of "blocks" in Elixir means there are many ways to write semantically
 
 We believe readability is enhanced by using the simplest api possible, whether we're talking about internal module function calls or standard-library macros.
 
-### use `case`, `if`, or `unless` when...
+### use `case`, `if`, or `cond` when...
 
 We advocate for `case` and `if` as the first tools to be considered for any control flow as they are the two simplest blocks. If a branch _can_ be expressed with an `if` statement, it _should_ be. Otherwise, `case` is the next best choice. In situations where developers might reach for an `if/elseif/else` block in other languages, `cond do` should be used.
 
 (`cond do` seems to see a paucity of use in the language, but many complex nested expressions or with statements can be improved by replacing them with a `cond do`).
 
-`unless` is a special case of `if` meant to make code read as natural-language (citation needed). While it sometimes succeeds in this goal, its absence in most programming languages often makes it feel cumbersome to programmers with non-Ruby backgrounds. Thankfully, with Styler's help developers don't need to ever reach for `unless` - expressions that are "simpler" with its use are automatically rewritten to use it.
+### use `unless` when...
+
+Never! `unless` [is being deprecated](https://github.com/elixir-lang/elixir/pull/13769#issuecomment-2334878315) and so should not be used.
 
 ### use `with` when...
 
@@ -25,7 +27,6 @@ We advocate for `case` and `if` as the first tools to be considered for any cont
 >
 > - Uncle Ben
 
-
 As the most powerful of the Kernel control-flow expressions, `with` requires the most cognitive overhead to understand. Its power means that we can use it as a replacement for anything we might express using a `case`, `if`, or `cond` (especially with the liberal application of small private helper functions).
 
 Unfortunately, this has lead to a proliferation of `with` in codebases where simpler expressions would have sufficed, meaning a lot of Elixir code ends up being harder for readers to understand than it needs to be.
diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex
index ffb0ba0..af2d6a8 100644
--- a/lib/style/blocks.ex
+++ b/lib/style/blocks.ex
@@ -176,31 +176,24 @@ defmodule Styler.Style.Blocks do
     end
   end
 
-  def run({{:unless, m, children}, _} = zipper, ctx) do
-    case children do
-      # Credo.Check.Refactor.UnlessWithElse
-      [{_, hm, _} = head, [_, _] = do_else] ->
-        zipper |> Zipper.replace({:if, m, [{:!, hm, [head]}, do_else]}) |> run(ctx)
-
-      # Credo.Check.Refactor.NegatedConditionsInUnless
-      [negator, [{do_, do_body}]] when is_negator(negator) ->
-        zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, do_body}]]}) |> run(ctx)
-
-      _ ->
-        {:cont, zipper, ctx}
-    end
+  def run({{:unless, m, [head, do_else]}, _} = zipper, ctx) do
+    zipper
+    |> Zipper.replace({:if, m, [invert(head), do_else]})
+    |> run(ctx)
   end
 
   def run({{:if, m, children}, _} = zipper, ctx) do
     case children do
+      # double negator
+      # if !!x, do: y[, else: ...] => if x, do: y[, else: ...]
+      [{_, _, [nb]} = na, do_else] when is_negator(na) and is_negator(nb) ->
+        zipper |> Zipper.replace({:if, m, [invert(nb), do_else]}) |> run(ctx)
+
       # Credo.Check.Refactor.NegatedConditionsWithElse
+      # if !x, do: y, else: z => if x, do: z, else: y
       [negator, [{do_, do_body}, {else_, else_body}]] when is_negator(negator) ->
         zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, else_body}, {else_, do_body}]]}) |> run(ctx)
 
-      # if not x, do: y => unless x, do: y
-      [negator, [do_block]] when is_negator(negator) ->
-        zipper |> Zipper.replace({:unless, m, [invert(negator), [do_block]]}) |> run(ctx)
-
       # drop `else end`
       [head, [do_block, {_, {:__block__, _, []}}]] ->
         {:cont, Zipper.replace(zipper, {:if, m, [head, [do_block]]}), ctx}
@@ -268,7 +261,7 @@ defmodule Styler.Style.Blocks do
         #     c
         #     d
         #   )
-        # @TODO would be nice to changeto
+        # @TODO would be nice to change to
         # a
         # b
         # c
@@ -331,5 +324,16 @@ defmodule Styler.Style.Blocks do
 
   defp invert({:!=, m, [a, b]}), do: {:==, m, [a, b]}
   defp invert({:!==, m, [a, b]}), do: {:===, m, [a, b]}
-  defp invert({_, _, [expr]}), do: expr
+  defp invert({:==, m, [a, b]}), do: {:!=, m, [a, b]}
+  defp invert({:===, m, [a, b]}), do: {:!==, m, [a, b]}
+  defp invert({:!, _, [condition]}), do: condition
+  defp invert({:not, _, [condition]}), do: condition
+
+  defp invert({fun, m, _} = ast) do
+    meta = [line: m[:line]]
+
+    if fun == :|>,
+      do: {:|>, meta, [ast, {{:., meta, [Kernel, :!]}, meta, []}]},
+      else: {:!, meta, [ast]}
+  end
 end
diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex
index e385eb9..623ce5f 100644
--- a/lib/style/module_directives.ex
+++ b/lib/style/module_directives.ex
@@ -135,7 +135,7 @@ defmodule Styler.Style.ModuleDirectives do
   defp moduledoc({:__aliases__, m, aliases}) do
     name = aliases |> List.last() |> to_string()
     # module names ending with these suffixes will not have a default moduledoc appended
-    unless String.ends_with?(name, ~w(Test Mixfile MixProject Controller Endpoint Repo Router Socket View HTML JSON)) do
+    if !String.ends_with?(name, ~w(Test Mixfile MixProject Controller Endpoint Repo Router Socket View HTML JSON)) do
       Style.set_line(@moduledoc_false, m[:line] + 1)
     end
   end
diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex
index d297465..20761b3 100644
--- a/lib/style/pipes.ex
+++ b/lib/style/pipes.ex
@@ -128,6 +128,8 @@ defmodule Styler.Style.Pipes do
       # |> ...
       var_name =
         case fun do
+          # unless will be rewritten to `if` statements in the Blocks Style
+          :unless -> :if
           fun when is_atom(fun) -> fun
           {:., _, [{:__aliases__, _, _}, fun]} when is_atom(fun) -> fun
           _ -> "block"


Test patch:
diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs
index 400fd99..14fd9e8 100644
--- a/test/style/blocks_test.exs
+++ b/test/style/blocks_test.exs
@@ -822,42 +822,25 @@ defmodule Styler.Style.BlocksTest do
     end
   end
 
-  describe "if/unless" do
-    test "drops if else nil" do
-      assert_style("if a, do: b, else: nil", "if a, do: b")
-
-      assert_style("if a do b else nil end", """
-      if a do
-        b
-      end
-      """)
-    end
-
-    test "if not => unless" do
-      assert_style("if not x, do: y", "unless x, do: y")
-      assert_style("if !x, do: y", "unless x, do: y")
-      assert_style("if !!x, do: y", "if x, do: y")
-    end
-
-    test "regression: negation with empty do body" do
+  describe "unless to if" do
+    test "inverts all the things" do
       assert_style(
         """
-        if a != b do
-          # comment
+        unless !! not true do
+          a
         else
-          :ok
+          b
         end
         """,
         """
-        if a == b do
-          # comment
-          :ok
+        if true do
+          a
+        else
+          b
         end
         """
       )
-    end
 
-    test "Credo.Check.Refactor.UnlessWithElse" do
       for negator <- ["!", "not "] do
         assert_style(
           """
@@ -912,9 +895,7 @@ defmodule Styler.Style.BlocksTest do
         end
         """
       )
-    end
 
-    test "Credo.Check.Refactor.NegatedConditionsInUnless" do
       for negator <- ["!", "not "] do
         assert_style("unless #{negator} foo, do: :bar", "if foo, do: :bar")
 
@@ -950,7 +931,69 @@ defmodule Styler.Style.BlocksTest do
       end
     end
 
-    test "Credo.Check.Refactor.NegatedConditionsWithElse" do
+    test "unless with pipes" do
+      assert_style "unless a |> b() |> c(), do: x", "if a |> b() |> c() |> Kernel.!(), do: x"
+    end
+  end
+
+  describe "if" do
+    test "drops else nil" do
+      assert_style("if a, do: b, else: nil", "if a, do: b")
+
+      assert_style("if a do b else nil end", """
+      if a do
+        b
+      end
+      """)
+
+      assert_style(
+        """
+        if a != b do
+          # comment
+        else
+          :ok
+        end
+        """,
+        """
+        if a == b do
+          # comment
+          :ok
+        end
+        """
+      )
+    end
+
+    test "double negator rewrites" do
+      for a <- ~w(not !), block <- ["do: z", "do: z, else: zz"] do
+        assert_style "if #{a} (x != y), #{block}", "if x == y, #{block}"
+        assert_style "if #{a} (x !== y), #{block}", "if x === y, #{block}"
+        assert_style "if #{a} ! x, #{block}", "if x, #{block}"
+        assert_style "if #{a} not x, #{block}", "if x, #{block}"
+      end
+
+      assert_style("if not x, do: y", "if not x, do: y")
+      assert_style("if !x, do: y", "if !x, do: y")
+
+      assert_style(
+        """
+        if !!val do
+          a
+        else
+          b
+        end
+        """,
+        """
+        if val do
+          a
+        else
+          b
+        end
+        """
+      )
+    end
+
+    test "single negator do/else swaps" do
+      # covers Credo.Check.Refactor.NegatedConditionsWithElse
       for negator <- ["!", "not "] do
         assert_style("if #{negator}foo, do: :bar, else: :baz", "if foo, do: :baz, else: :bar")
 
@@ -994,44 +1037,6 @@ defmodule Styler.Style.BlocksTest do
       end
     end
 
-    test "recurses" do
-      assert_style(
-        """
-        if !!val do
-          a
-        else
-          b
-        end
-        """,
-        """
-        if val do
-          a
-        else
-          b
-        end
-        """
-      )
-
-      assert_style(
-        """
-        unless !! not true do
-          a
-        else
-          b
-        end
-        """,
-        """
-        if true do
-          a
-        else
-          b
-        end
-        """
-      )
-
-      assert_style("if not (a != b), do: c", "if a == b, do: c")
-    end
-
     test "comments and flips" do
       assert_style(
         """
diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs
index 5bd24c9..49763ea 100644
--- a/test/style/pipes_test.exs
+++ b/test/style/pipes_test.exs
@@ -228,12 +228,12 @@ defmodule Styler.Style.PipesTest do
         |> wee()
         """,
         """
-        unless_result =
-          unless foo do
+        if_result =
+          if !foo do
             bar
           end
 
-        wee(unless_result)
+        wee(if_result)
         """
       )
     end
diff --git a/test/support/style_case.ex b/test/support/style_case.ex
index 61a11f2..f87c7bd 100644
--- a/test/support/style_case.ex
+++ b/test/support/style_case.ex
@@ -70,7 +70,7 @@ defmodule Styler.StyleCase do
           # body blocks - for example, the block node for an anonymous function - don't have line meta
           # yes, i just did `&& case`. sometimes it's funny to write ugly things in my project that's all about style.
           # i believe they calls that one "irony"
-          is_body_block? =
+          body_block? =
             node == :__block__ &&
               case up && Zipper.node(up) do
                 # top of a snippet
@@ -99,7 +99,7 @@ defmodule Styler.StyleCase do
             end
           end
 
-          unless line || is_body_block? do
+          if is_nil(line) and not body_block? do
             IO.puts("missing `:line` meta in node:")
             dbg(ast)