Repository: rrrene/credo
Pull request #769 (2020-04-06 13:45:07)

Improve strict module layout
This PR brings in a few minor changes/additions to the `StrictModuleLayout` check. These additions are motivated by the real usage in the project of my client (which is where the original implementation of this check was done).

Note that these changes weren't discussed in any issue so far. However, to better understand the issues, I experimented with a fix, and so I ended up with the solution here, and then I figured I may as well submit it :-) That said, I'm not insisting on these changes, and so if you have some other ideas feel free to propose them.

Anyway, here are the changes:

1. Callback macros and callback functions are bundled under the same key `callback_impl`. In other words, with this change we can't enforce that e.g. callback macros must preceed callback functions. The reason is because a single module could implement multiple behaviours, and then such order becomes counterproductive, because we typically want to group callbacks by the module (e.g. first all callbacks of `Foo`, then all callbacks of `Bar`).
2. Added the `:ignore` option which allows us to ignore some module parts (i.e. allow some parts to appear anywhere in the module). For my client's projects, the concrete example would be private macros.


Patch:
diff --git a/lib/credo/check/readability/strict_module_layout.ex b/lib/credo/check/readability/strict_module_layout.ex
index b081e2ed..3958cd03 100644
--- a/lib/credo/check/readability/strict_module_layout.ex
+++ b/lib/credo/check/readability/strict_module_layout.ex
@@ -44,9 +44,9 @@ defmodule Credo.Check.Readability.StrictModuleLayout do
             - `:module_attribute` - other module attribute
             - `:public_fun` - public function
             - `:private_fun` - private function or a public function marked with `@doc false`
-            - `:callback_fun` - public function marked with `@impl`
             - `:public_macro` - public macro
             - `:private_macro` - private macro or a public macro marked with `@doc false`
+            - `:callback_impl` - public function or macro marked with `@impl`
             - `:public_guard` - public guard
             - `:private_guard` - private guard or a public guard marked with `@doc false`
             - `:module` - inner module definition (`defmodule` expression inside a module)
@@ -54,38 +54,87 @@ defmodule Credo.Check.Readability.StrictModuleLayout do
         Notice that the desired order always starts from the top. For example, if you provide
         the order `~w/public_fun private_fun/a`, it means that everything else (e.g. `@moduledoc`)
         must appear after function definitions.
+        """,
+        ignore: """
+        List of atoms identifying the module parts which are not checked, and may therefore appear
+        anywhere in the module. Allowed values are the same as in the `:order` option.
+        Defaults to an empty list.
         """
       ]
+    ],
+    param_defaults: [
+      order: ~w/shortdoc moduledoc behaviour use import alias require/a,
+      ignore: []
     ]
 
   alias Credo.Code
+  alias Credo.CLI.Output.UI
 
   @doc false
   def run(source_file, params \\ []) do
+    params = normalize_params(params)
+
     source_file
     |> Code.ast()
     |> Credo.Code.Module.analyze()
-    |> all_errors(expected_order(params), IssueMeta.for(source_file, params))
+    |> all_errors(params, IssueMeta.for(source_file, params))
     |> Enum.sort_by(&{&1.line_no, &1.column})
   end
 
-  defp expected_order(params) do
-    params
-    |> Keyword.get(:order, ~w/shortdoc moduledoc behaviour use import alias require/a)
-    |> Enum.with_index()
-    |> Map.new()
+  defp normalize_params(params) do
+    order =
+      params
+      |> Keyword.get(:order, ~w/shortdoc moduledoc behaviour use import alias require/a)
+      |> Enum.map(fn element ->
+        # TODO: This is done for backward compatibility and should be removed in some future version.
+        with :callback_fun <- element do
+          UI.warn([
+            :red,
+            "** (StrictModuleLayout) `:callback_fun` has been deprecated. Use `:callback_impl` instead."
+          ])
+
+          :callback_impl
+        end
+      end)
+
+    Keyword.put(params, :order, order)
   end
 
-  defp all_errors(modules_and_parts, expected_order, issue_meta) do
+  defp all_errors(modules_and_parts, params, issue_meta) do
+    expected_order = expected_order(params)
+    ignored_parts = Keyword.get(params, :ignore, [])
+
     Enum.reduce(
       modules_and_parts,
       [],
       fn {module, parts}, errors ->
+        parts =
+          parts
+          |> Stream.map(fn
+            # Converting `callback_macro` and `callback_fun` into a common `callback_impl`,
+            # because enforcing an internal order between these two kinds is counterproductive if
+            # a module implements multiple behaviours. In such cases, we typically want to group
+            # callbacks by the implementation, not by the kind (fun vs macro).
+            {callback_impl, location} when callback_impl in ~w/callback_macro callback_fun/a ->
+              {:callback_impl, location}
+
+            other ->
+              other
+          end)
+          |> Stream.reject(fn {part, _location} -> part in ignored_parts end)
+
         module_errors(module, parts, expected_order, issue_meta) ++ errors
       end
     )
   end
 
+  defp expected_order(params) do
+    params
+    |> Keyword.fetch!(:order)
+    |> Enum.with_index()
+    |> Map.new()
+  end
+
   defp module_errors(module, parts, expected_order, issue_meta) do
     Enum.reduce(
       parts,
@@ -131,6 +180,6 @@ defmodule Credo.Check.Readability.StrictModuleLayout do
   defp part_to_string(:public_macro), do: "public macro"
   defp part_to_string(:public_fun), do: "public function"
   defp part_to_string(:private_fun), do: "private function"
-  defp part_to_string(:callback_fun), do: "callback implementation"
+  defp part_to_string(:callback_impl), do: "callback implementation"
   defp part_to_string(part), do: "#{part}"
 end
diff --git a/lib/credo/code/module.ex b/lib/credo/code/module.ex
index 1580cdd6..17ad7da6 100644
--- a/lib/credo/code/module.ex
+++ b/lib/credo/code/module.ex
@@ -31,6 +31,7 @@ defmodule Credo.Code.Module do
           | :public_guard
           | :private_guard
           | :callback_fun
+          | :callback_macro
           | :module
 
   @type location :: [line: pos_integer, column: pos_integer]
@@ -404,7 +405,7 @@ defmodule Credo.Code.Module do
   defp code_type(:defp, _), do: :private_fun
 
   defp code_type(:defmacro, nil), do: :public_macro
-  defp code_type(:defmacro, :impl), do: :impl
+  defp code_type(:defmacro, :impl), do: :callback_macro
   defp code_type(macro, _) when macro in ~w/defmacro defmacrop/a, do: :private_macro
 
   defp code_type(:defguard, nil), do: :public_guard


Test patch:
diff --git a/test/credo/check/readability/strict_module_layout_test.exs b/test/credo/check/readability/strict_module_layout_test.exs
index f237225b..2c49492d 100644
--- a/test/credo/check/readability/strict_module_layout_test.exs
+++ b/test/credo/check/readability/strict_module_layout_test.exs
@@ -146,6 +146,32 @@ defmodule Credo.Check.Readability.StrictModuleLayoutTest do
 
       assert issue.message == "alias must appear before require"
     end
+
+    test "callback functions and macros are handled by the `:callback_impl` option" do
+      assert [issue1, issue2] =
+               """
+               defmodule Test do
+                 @impl true
+                 def foo
+
+                 def baz, do: :ok
+
+                 @impl true
+                 defmacro bar
+
+                 def qux, do: :ok
+               end
+               """
+               |> to_source_file
+               |> run_check(@described_check, order: ~w/public_fun callback_impl/a)
+               |> assert_issues
+
+      assert issue1.message == "public function must appear before callback implementation"
+      assert issue1.scope == "Test.baz"
+
+      assert issue2.message == "public function must appear before callback implementation"
+      assert issue2.scope == "Test.qux"
+    end
   end
 
   describe "custom order" do
@@ -180,5 +206,55 @@ defmodule Credo.Check.Readability.StrictModuleLayoutTest do
       assert issue2.message == "public function must appear before private function"
       assert issue2.line_no == 4
     end
+
+    test "treats `:callback_fun` as `:callback_impl` for backward compatibility" do
+      [issue] =
+        """
+        defmodule Test do
+          @impl Foo
+          def foo, do: :ok
+
+          def bar, do: :ok
+        end
+        """
+        |> to_source_file
+        |> run_check(@described_check, order: ~w/public_fun callback_fun/a)
+        |> assert_issue
+
+      assert issue.message == "public function must appear before callback implementation"
+    end
+  end
+
+  describe "ignored parts" do
+    test "no errors are reported on ignored parts" do
+      """
+      defmodule Test do
+        alias Foo
+        import Bar
+        use Baz
+        require Qux
+      end
+      """
+      |> to_source_file
+      |> run_check(@described_check, ignore: ~w/use import/a)
+      |> refute_issues
+    end
+
+    test "reports errors on non-ignored parts" do
+      [issue] =
+        """
+        defmodule Test do
+          require Qux
+          import Bar
+          use Baz
+          alias Foo
+        end
+        """
+        |> to_source_file
+        |> run_check(@described_check, ignore: ~w/use import/a)
+        |> assert_issue
+
+      assert issue.message == "alias must appear before require"
+    end
   end
 end
diff --git a/test/credo/code/module_test.exs b/test/credo/code/module_test.exs
index 10b5d54d..f890934e 100644
--- a/test/credo/code/module_test.exs
+++ b/test/credo/code/module_test.exs
@@ -488,6 +488,11 @@ defmodule Credo.Code.ModuleTest do
                [{Test, [private_macro: [line: 3, column: 3]]}]
     end
 
+    test "interprets defmacro marked with @impl as callback macro" do
+      assert analyze(~s/@impl true\ndefmacro x, do: true/) ==
+               [{Test, [callback_macro: [line: 3, column: 3]]}]
+    end
+
     test "recognizes def" do
       assert analyze(~s/def x, do: true/) == [{Test, [public_fun: [line: 2, column: 3]]}]
     end