Repository: rrrene/credo
Pull request #744 (2020-02-10 10:47:23)

Add WithToken check
This PR brings in a new readability check which detects `with` token placeholder pattern (not sure if this is the correct name):

```elixir
with {:resource, {:ok, resource}} <- {:resource, Resource.fetch(user)},
      {:authz, :ok} <- {:authz, Resource.authorize(resource, user)} do
  do_something_with(resource)
else
  {:resource, _} -> {:error, :not_found}
  {:authz, _} -> {:error, :unauthorized}
end
```

I've introduced this check a few months ago at my client's projects, because they have been overusing the pattern quite aggressively. The check is therefore tested in production, and it has been working well for us.

The main reason I didn't submit it earlier is b/c I was worried that it might be controversial. But after a few people asked me about it, I figured we could include the check as a low-priority controversial check, so it's turned off by default

I'm not really sure about the name (is there official name for this pattern?). Back in our projects we called this `WithPlaceholder`, and here I went for `WithToken`, but I'm pretty much ok with anything else, especially if there is some more established name.


Patch:
diff --git a/.credo.exs b/.credo.exs
index 6904aca4..fd0fc2bb 100644
--- a/.credo.exs
+++ b/.credo.exs
@@ -163,6 +163,7 @@
         {Credo.Check.Readability.MultiAlias, false},
         {Credo.Check.Readability.Specs, false},
         {Credo.Check.Readability.SinglePipe, false},
+        {Credo.Check.Readability.WithTaggedTuple, false},
         {Credo.Check.Refactor.ABCSize, false},
         {Credo.Check.Refactor.AppendSingleItem, false},
         {Credo.Check.Refactor.DoubleBooleanNegation, false},
diff --git a/lib/credo/check/readability/with_tagged_tuple.ex b/lib/credo/check/readability/with_tagged_tuple.ex
new file mode 100644
index 00000000..bd8c9544
--- /dev/null
+++ b/lib/credo/check/readability/with_tagged_tuple.ex
@@ -0,0 +1,79 @@
+defmodule Credo.Check.Readability.WithTaggedTuple do
+  @moduledoc false
+
+  @checkdoc """
+  Avoid using tags for error reporting.
+
+  Consider the following code:
+
+      with {:resource, {:ok, resource}} <- {:resource, Resource.fetch(user)},
+           {:authz, :ok} <- {:authz, Resource.authorize(resource, user)} do
+        do_something_with(resource)
+      else
+        {:resource, _} -> {:error, :not_found}
+        {:authz, _} -> {:error, :unauthorized}
+      end
+
+  This code injects placeholder tags such as `:resource` and `:authz` for the purpose of error
+  reporting.
+
+  Instead, extract each validation into a separate helper function which returns error
+  information immediately:
+
+      defp find_resource(user) do
+        with :error <- Resource.fetch(user), do: {:error, :not_found}
+      end
+
+      defp authorize(resource, user) do
+        with :error <- Resource.authorize(resource, user), do: {:error, :unauthorized}
+      end
+
+  At this point, the validation chain in `with` becomes clearer and easier to understand:
+
+      with {:ok, resource} <- find_resource(user),
+           :ok <- authorize(resource, user),
+           do: do_something(user)
+  """
+  @explanation [check: @checkdoc]
+
+  use Credo.Check, category: :warning, base_priority: :low
+
+  alias Credo.Code
+
+  def run(source_file, params \\ []) do
+    source_file
+    |> errors()
+    |> Enum.map(&credo_error(&1, IssueMeta.for(source_file, params)))
+  end
+
+  defp errors(source_file) do
+    {_ast, errors} = Macro.prewalk(Code.ast(source_file), MapSet.new(), &traverse/2)
+    Enum.sort_by(errors, &{&1.line, &1.column})
+  end
+
+  defp traverse({:with, _meta, args}, errors) do
+    errors =
+      args
+      |> Stream.map(&placeholder/1)
+      |> Enum.reject(&is_nil/1)
+      |> Enum.into(errors)
+
+    {args, errors}
+  end
+
+  defp traverse(ast, state), do: {ast, state}
+
+  defp placeholder({:<-, meta, [{placeholder, _}, {placeholder, _}]}) when is_atom(placeholder),
+    do: %{placeholder: placeholder, line: meta[:line], column: meta[:column]}
+
+  defp placeholder(_), do: nil
+
+  defp credo_error(error, issue_meta) do
+    format_issue(
+      issue_meta,
+      message: "Invalid usage of placeholder `#{inspect(error.placeholder)}` in with",
+      line_no: error.line,
+      column: error.column
+    )
+  end
+end


Test patch:
diff --git a/test/credo/check/readability/with_tagged_tuple_test.exs b/test/credo/check/readability/with_tagged_tuple_test.exs
new file mode 100644
index 00000000..1a7b7cf6
--- /dev/null
+++ b/test/credo/check/readability/with_tagged_tuple_test.exs
@@ -0,0 +1,45 @@
+defmodule Credo.Check.Readability.WithTaggedTupleTest do
+  use Credo.TestHelper
+
+  @described_check Credo.Check.Readability.WithTaggedTuple
+
+  test "it should NOT report violation" do
+    """
+    defmodule Test do
+      def run(user, resource) do
+        with {:ok, resource} <- Resource.fetch(user),
+             :ok <- Resource.authorize(resource, user),
+             do: SomeMod.do_something(resource)
+      end
+    end
+    """
+    |> to_source_file()
+    |> refute_issues(@described_check)
+  end
+
+  test "it should report a violation" do
+    issue_messages =
+      """
+      defmodule Test do
+        def run(user, resource) do
+          with {:resource, {:ok, resource}} <- {:resource, Resource.fetch(user)},
+               {:authz, :ok} <- {:authz, Resource.authorize(resource, user)} do
+            SomeMod.do_something(resource)
+          else
+            {:resource, _} -> {:error, :not_found}
+            {:authz, _} -> {:error, :unauthorized}
+          end
+        end
+      end
+      """
+      |> to_source_file()
+      |> assert_issues(@described_check)
+      |> Enum.map(& &1.message)
+
+    assert MapSet.new(issue_messages) ==
+             MapSet.new([
+               "Invalid usage of placeholder `:resource` in with",
+               "Invalid usage of placeholder `:authz` in with"
+             ])
+  end
+end