Repository: rrrene/credo (elixir)

Issue:
False positive on MultiAliasImportRequireUse when using multiple modules in same file
### Precheck

* Proposals for new features should be submitted via: https://github.com/rrrene/credo-proposals
* For bugs, please do a quick search and make sure the bug has not yet been reported here
   * I have not found any other issue about MultiAlias used in files with multiple modules

### Environment

* Credo version (`mix credo -v`): 1.6.7
* Erlang/Elixir version (`elixir -v`): 1.13.4-otp-25 and 1.12.3-otp-24
* Operating system: Ubuntu 22 and macOS Ventura

### What were you trying to do?

Example repository: https://github.com/ZeLarpMaster/CredoMultiAliasExample

TL;DR in a single file:

```elixir
defmodule CredoMultiAliasExample.Bar do
  @moduledoc "This module aliases another module"

  alias CredoMultiAliasExample.Foo
end

defmodule CredoMultiAliasExample.Baz do
  @moduledoc "This module aliases a different module under the same parent"

  alias CredoMultiAliasExample.Bar
end
```

Moving the 2nd `defmodule` into its own file fixes the issue even though that's effectively the same thing.

### Expected outcome

No issues found

### Actual outcome

>   Consistency
┃
┃ [C] ↗ Most of the time you are using the multi-alias/require/import/use syntax, but here you are using multiple single
┃       directives
┃       lib/credo_multi_alias_example.ex:16 #(CredoMultiAliasExample.Baz)

Pull request:
fix: MultiAliasImportRequireUse collection and false positive
Fixes #1023 

When you have files that contain multiple modules this rule will raise a false positive and force a user to disable the rule for the file. These changes solve that, but they _do not_ solve the issue where you have mixed usage in just a single file. We could do that, but the scope was increase quite a bit and most likely isn't super important.

Patch:
diff --git a/lib/credo/check/consistency/multi_alias_import_require_use/collector.ex b/lib/credo/check/consistency/multi_alias_import_require_use/collector.ex
index fc6f6af2..0c499780 100644
--- a/lib/credo/check/consistency/multi_alias_import_require_use/collector.ex
+++ b/lib/credo/check/consistency/multi_alias_import_require_use/collector.ex
@@ -7,20 +7,34 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUse.Collector do
 
   def collect_matches(source_file, _params) do
     source_file
-    |> Credo.Code.prewalk(&traverse/2, [])
-    |> group_usages
-    |> count_occurrences
+    |> Credo.Code.prewalk(&traverse/2, {%{}, nil})
+    |> elem(0)
+    |> Enum.map(fn {_mod_name, aliases} ->
+      aliases
+      |> group_usages
+      |> count_occurrences
+    end)
+    |> merge_module_stats()
   end
 
   def find_locations_not_matching(expected, source_file) do
     source_file
-    |> Credo.Code.prewalk(&traverse/2, [])
-    |> group_usages
-    |> drop_locations(expected)
+    |> Credo.Code.prewalk(&traverse/2, {%{}, nil})
+    |> elem(0)
+    |> Enum.map(fn {_mod_name, aliases} ->
+      aliases
+      |> group_usages
+      |> drop_locations(expected)
+    end)
+    |> List.flatten()
   end
 
-  defp traverse({directive, meta, arguments} = ast, acc)
-       when directive in @directives do
+  defp traverse({:defmodule, _, [{:__aliases__, _, mod_name} | _]} = ast, {acc, _}) do
+    {ast, {Map.put(acc, mod_name, []), mod_name}}
+  end
+
+  defp traverse({directive, meta, arguments} = ast, {acc, current_module})
+       when directive in @directives and not is_nil(current_module) do
     aliases =
       case arguments do
         [{:__aliases__, _, nested_modules}] when length(nested_modules) > 1 ->
@@ -35,9 +49,12 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUse.Collector do
       end
 
     if aliases do
-      {ast, [{directive, aliases, meta[:line]} | acc]}
+      updated_acc = Map.update(acc, current_module, [], fn entries ->
+        [{directive, aliases, meta[:line]} | entries]
+      end)
+      {ast, {updated_acc, current_module}}
     else
-      {ast, acc}
+      {ast, {acc, current_module}}
     end
   end
 
@@ -94,4 +111,12 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUse.Collector do
 
     {:lists.reverse(acc1), :lists.reverse(acc2)}
   end
+
+  defp merge_module_stats(stats_list) do
+    Enum.reduce(stats_list, %{}, fn stats, acc ->
+      Enum.reduce(stats, acc, fn {key, val}, acc ->
+        Map.update(acc, key, val, &(&1 + val))
+      end)
+    end)
+  end
 end


Test patch:
diff --git a/test/credo/check/consistency/multi_alias_import_require_use_test.exs b/test/credo/check/consistency/multi_alias_import_require_use_test.exs
index 010d4043..27f55f06 100644
--- a/test/credo/check/consistency/multi_alias_import_require_use_test.exs
+++ b/test/credo/check/consistency/multi_alias_import_require_use_test.exs
@@ -24,6 +24,32 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUseTest do
     require Foo.Quux
   end
   """
+  @multi_module_same_file """
+  defmodule CredoMultiAliasExample.SetMultiAliasToSingles do
+    @moduledoc "This modules does many aliases to set the consistency to multi-alias"
+
+    alias Credo.CLI.{Command, Output}
+    alias Config.{Reader, Provider}
+  end
+
+  defmodule CredoMultiAliasExample.Foo do
+    @moduledoc "This module has a function"
+
+    def test, do: :ok
+  end
+
+  defmodule CredoMultiAliasExample.Bar do
+    @moduledoc "This module aliases another module"
+
+    alias CredoMultiAliasExample.Foo
+  end
+
+  defmodule CredoMultiAliasExample.Baz do
+    @moduledoc "This module aliases a different module under the same parent"
+
+    alias CredoMultiAliasExample.Bar
+  end
+  """
 
   #
   # cases NOT raising issues
@@ -36,6 +62,13 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUseTest do
     |> refute_issues()
   end
 
+  test "it should NOT report errors when there are multiple modules in the same file when the multi syntax is used consistently" do
+    [@multi_module_same_file]
+    |> to_source_files
+    |> run_check(@described_check)
+    |> refute_issues()
+  end
+
   test "it should NOT report errors when the single syntax is used consistently" do
     [@single]
     |> to_source_files
@@ -50,6 +83,13 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUseTest do
     |> refute_issues()
   end
 
+  test "it should not report errors when the single syntax is used consistently" do
+    [@single]
+    |> to_source_files
+    |> run_check(@described_check)
+    |> refute_issues()
+  end
+
   #
   # cases raising issues
   #
@@ -61,10 +101,10 @@ defmodule Credo.Check.Consistency.MultiAliasImportRequireUseTest do
     |> assert_issue()
   end
 
-  test "it should not report errors when the single syntax is used consistently" do
-    [@single]
+  test "it should report errors when the multi and single syntaxes are mixed (two files, one multi-module)" do
+    [@single, @multi_module_same_file]
     |> to_source_files
     |> run_check(@described_check)
-    |> refute_issues()
+    |> assert_issue()
   end
 end