Repository: rrrene/credo (elixir)

Issue:
Proposal: Checking "with" clauses
I have been playing with a check that I believe is important, I called it `WithClauses`.

In a lot of codebases I've seen examples like this:

```elixir
with {:ok, result} <- fun() do
  # ...
else
  {:error, reason} ->
    # ...
end
```

I always complain about this because using a `with` in this case makes the code more obscure and harder to read since I search for the semantics of `with` when what is actually happening is a `case`:

```elixir
case fun() do
  {:ok, result} ->
    # ...

  {:error, reason} ->
    # ...
end
```

Furthermore, I sometimes see this:

```elixir
with var1 = fun1(),
     {:ok, var2} <- fun2(),
     {:ok, var3} <- fun3(),
     fun3() do
  # ...
end
```

This is confusing to read: I will think that there's a reason why the first `=` clause and the last `fun4()` clause are part of the clauses of the `with`, when in fact there's no reason. I think it's much better to be explicit and only write `with`s where the first and last clause are `<-` clauses. The clauses before the first `<-` clause can be moved outside of the `with`, and the ones after can be moved in the body of the `with`.

```elixir
var1 = fun1()

with {:ok, var2} <- fun2(),
     {:ok, var3} <- fun3() do
  fun4()
  # ...
end
```

@rrrene are you interested in a check like this being introduced into Credo itself? I have the code ready and working and we're using it already, but wanted to see if it could be contributed upstream :)

Pull request description:
Add the WithClauses refactor check
Closes #658.

\cc @rrrene 🎉 

Patch:
diff --git a/.credo.exs b/.credo.exs
index 63d6acd5..5e69d4fb 100644
--- a/.credo.exs
+++ b/.credo.exs
@@ -115,6 +115,7 @@
            excluded_functions: []
          ]},
         {Credo.Check.Refactor.UnlessWithElse, []},
+        {Credo.Check.Refactor.WithClauses, []},
 
         #
         ## Warnings
diff --git a/lib/credo/check/refactor/with_clauses.ex b/lib/credo/check/refactor/with_clauses.ex
new file mode 100644
index 00000000..f32b60db
--- /dev/null
+++ b/lib/credo/check/refactor/with_clauses.ex
@@ -0,0 +1,147 @@
+defmodule Credo.Check.Refactor.WithClauses do
+  @moduledoc false
+
+  @checkdoc ~S"""
+  `with` statements are useful when you need to chain a sequence
+  of pattern matches, stopping at the first one that fails.
+
+  However, there are a few cases where a `with` can be used incorrectly.
+
+  ## Starting or ending with non-pattern-matching clauses
+
+  If the `with` starts or ends with clauses that are not `<-` clauses,
+  then those clauses should be moved either outside of the `with` (if
+  they're the first ones) or inside the body of the `with` (if they're the
+  last ones). For example look at this code:
+
+      with ref = make_ref(),
+           {:ok, user} <- User.create(ref),
+           Logger.debug("Created user: #{inspect(user)}") do
+        user
+      end
+
+  This `with` should be refactored like this:
+
+      ref = make_ref()
+
+      with {:ok, user} <- User.create(ref) do
+        Logger.debug("Created user: #{inspect(user)}")
+        user
+      end
+
+  # Using only one pattern matching clause with `else`
+
+  If the `with` has a single pattern matching clause and no `else`
+  branch, it means that if the clause doesn't match than the whole
+  `with` will return the value of that clause. However, if that
+  `with` has also an `else` clause, then you're using `with` exactly
+  like a `case` and a `case` should be used instead. Take this code:
+
+      with {:ok, user} <- User.create(make_ref()) do
+        user
+      else
+        {:error, :db_down} ->
+          raise "DB is down!"
+
+        {:error, reason} ->
+          raise "error: #{inspect(reason)}"
+      end
+
+  It can be rewritten with a clearer use of `case`:
+
+      case User.create(make_ref()) do
+        {:ok, user} ->
+          user
+
+        {:error, :db_down} ->
+          raise "DB is down!"
+
+        {:error, reason} ->
+          raise "error: #{inspect(reason)}"
+      end
+
+  Like all `Readability` issues, this one is not a technical concern.
+  But you can improve the odds of others reading and liking your code by making
+  it easier to follow.
+  """
+
+  @explanation [
+    check: @checkdoc,
+    params: []
+  ]
+
+  @message_only_one_pattern_clause "\"with\" contains only one <- clause and an \"else\" " <>
+                                     "branch, use a \"case\" instead"
+  @message_first_clause_not_pattern "\"with\" doesn't start with a <- clause, " <>
+                                      "move the non-pattern <- clauses outside of the \"with\""
+  @message_last_clause_not_pattern "\"with\" doesn't end with a <- clause, move " <>
+                                     "the non-pattern <- clauses inside the body of the \"with\""
+
+  use Credo.Check, base_priority: :high, category: :refactor, exit_status: 1
+
+  alias Credo.Code
+
+  @doc false
+  def run(source_file, params \\ []) do
+    issue_meta = IssueMeta.for(source_file, params)
+    Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+  end
+
+  defp traverse({:with, meta, [_, _ | _] = clauses_and_body} = ast, issues, issue_meta)
+       when is_list(clauses_and_body) do
+    # If clauses_and_body is a list with at least two elements in it, we think
+    # this might be a call to the special form "with". To be sure of that,
+    # we get the last element of clauses_and_body and check that it's a keyword
+    # list with a :do key in it (the body).
+
+    # We can hard-match on [maybe_body] here since we know that clauses_and_body
+    # has at least two elements.
+    {maybe_clauses, [maybe_body]} = Enum.split(clauses_and_body, -1)
+
+    if Keyword.keyword?(maybe_body) and Keyword.has_key?(maybe_body, :do) do
+      {ast, issues_for_with(maybe_clauses, maybe_body, meta[:line], issue_meta) ++ issues}
+    else
+      {ast, issues}
+    end
+  end
+
+  defp traverse(ast, issues, _issue_meta) do
+    {ast, issues}
+  end
+
+  defp issues_for_with(clauses, body, line, issue_meta) do
+    issue_if_one_pattern_clause_with_else(clauses, body, line, issue_meta) ++
+      issue_if_not_starting_with_pattern_clause(clauses, line, issue_meta) ++
+      issue_if_not_ending_with_pattern_clause(clauses, line, issue_meta)
+  end
+
+  defp issue_if_one_pattern_clause_with_else(clauses, body, line, issue_meta) do
+    pattern_clauses_count = Enum.count(clauses, &match?({:<-, _, _}, &1))
+
+    if pattern_clauses_count <= 1 and Keyword.has_key?(body, :else) do
+      [format_issue(issue_meta, message: @message_only_one_pattern_clause, line_no: line)]
+    else
+      []
+    end
+  end
+
+  defp issue_if_not_starting_with_pattern_clause(
+         [{:<-, _meta, _args} | _rest],
+         _line,
+         _issue_meta
+       ) do
+    []
+  end
+
+  defp issue_if_not_starting_with_pattern_clause(_clauses, line, issue_meta) do
+    [format_issue(issue_meta, message: @message_first_clause_not_pattern, line_no: line)]
+  end
+
+  defp issue_if_not_ending_with_pattern_clause(clauses, line, issue_meta) do
+    if length(clauses) > 1 and not match?({:<-, _, _}, Enum.at(clauses, -1)) do
+      [format_issue(issue_meta, message: @message_last_clause_not_pattern, line_no: line)]
+    else
+      []
+    end
+  end
+end


Test patch:
diff --git a/test/credo/check/refactor/with_clauses_test.exs b/test/credo/check/refactor/with_clauses_test.exs
new file mode 100644
index 00000000..62a0a69f
--- /dev/null
+++ b/test/credo/check/refactor/with_clauses_test.exs
@@ -0,0 +1,103 @@
+defmodule Credo.Check.Refactor.WithClausesTest do
+  use Credo.TestHelper
+
+  @described_check Credo.Check.Refactor.WithClauses
+
+  #
+  # cases NOT raising issues
+  #
+
+  test "it should NOT report expected code" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter1, parameter2) do
+        with :ok <- parameter1,
+             _ref = make_ref(),
+             IO.puts("Imperative operation"),
+             :ok <- parameter2 do
+          :ok
+        end
+      end
+    end
+    """
+    |> to_source_file
+    |> refute_issues(@described_check)
+
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter1, parameter2) do
+        with :ok <- parameter1 do
+          parameter2
+        end
+      end
+    end
+    """
+    |> to_source_file
+    |> refute_issues(@described_check)
+  end
+
+  #
+  # cases raising issues
+  #
+
+  test "it should report a violation for a single <- clause if there's an else branch" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter1, parameter2) do
+        with :ok <- parameter1 do
+          parameter2
+        else
+          :error ->
+            :error
+        end
+      end
+    end
+    """
+    |> to_source_file
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation if the with doesn't start with <- clauses" do
+    """
+    def some_function(parameter1, parameter2) do
+      with IO.puts("not a <- clause"),
+           :ok <- parameter1 do
+        parameter2
+      end
+    end
+    """
+    |> to_source_file
+    |> assert_issue(@described_check, fn issue ->
+      assert issue.message =~ ~s("with" doesn't start with a <- clause)
+    end)
+  end
+
+  test "it should report a violation if the with doesn't end with <- clauses" do
+    """
+    def some_function(parameter1, parameter2) do
+      with :ok <- parameter1,
+           IO.puts("not a <- clause") do
+        parameter2
+      end
+    end
+    """
+    |> to_source_file
+    |> assert_issue(@described_check, fn issue ->
+      assert issue.message =~ ~s("with" doesn't end with a <- clause)
+    end)
+  end
+
+  #
+  # cases that should not be examined at all
+  #
+
+  test "it shouldn't check calls to functions called \"with\"" do
+    """
+    def some_function(parameter1, parameter2) do
+      with(parameter1, parameter2)
+    end
+    """
+    |> to_source_file
+    |> refute_issues(@described_check)
+  end
+end