Improve strict module layout

lib/credo/check/readability/strict_module_layout.ex

@@ -44,48 +44,97 @@ 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)
 
         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

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/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

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