Repository: rrrene/credo (elixir)

Issue:
NegatedIsNil check can consume all available memory
### 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

### Environment

* Credo version (`mix credo -v`): 1.7.0
* Erlang/Elixir version (`elixir -v`): 1.14.2
* Operating system: Any

### What were you trying to do?

When using the `NegatedIsNil` check and a file has a violation, the ast traversal is mishandling the issues list and will concatenate the list with itself for every other function with a non violating guard in the file after the violation is found.
The results will range from reporting the same line multiple times up to consuming all available memory until it's killed, depending on the number of functions with guards that come after a violating function in a file.

A benign example:

```elixir
defmodule Credobomb do
  def hello(a) when not is_nil(a), do: a

  def foo1(x) when is_integer(x), do: x

  def foo2(x) when is_integer(x), do: x
end
```

```
mix credo --strict 
Checking 3 source files ...

  Refactoring opportunities                                                                                                                           
┃ 
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:2 #(Credobomb.hello)
```

The issues list ends up containing 3^n entries where n = the number of guarded functions after a violating function. Multiple violating functions will increase this. A further illustration can be seen by reordering the functions in the file:

```elixir
defmodule Credobomb do
  def foo1(x) when is_integer(x), do: x

  def foo2(x) when is_integer(x), do: x

  def hello(a) when not is_nil(a), do: a
end
```

```
mix credo --strict 
Checking 3 source files ...

  Refactoring opportunities                                                                                                                           
┃ 
┃ [F] ↘ Negated is_nil in guard clause found
┃       lib/credobomb.ex:6 #(Credobomb.hello)
```



Pull request:
fix: NegatedIsNil memory leak
Fixes #1056.

Prior to this, this example test would raise 9 issues:

```elixir
test "regression test for issue list duplication" do
  """
  defmodule CredoSampleModule do
    def hello(a) when not is_nil(a), do: a
    def foo1(x) when is_integer(x), do: x
    def foo2(x) when is_integer(x), do: x
    def foo3(x) when is_integer(x), do: x
  end
  """
  |> to_source_file()
  |> run_check(@described_check)
  |> assert_issue()
end
```

Patch:
diff --git a/lib/credo/check/refactor/negated_is_nil.ex b/lib/credo/check/refactor/negated_is_nil.ex
index 1038bc18..1ed1da3a 100644
--- a/lib/credo/check/refactor/negated_is_nil.ex
+++ b/lib/credo/check/refactor/negated_is_nil.ex
@@ -65,8 +65,8 @@ defmodule Credo.Check.Refactor.NegatedIsNil do
   end
 
   defp traverse({:when, meta, [fun, {_, _, [first_op | second_op]}]} = ast, issues, issue_meta) do
-    {_, first_op_issues} = traverse({:when, meta, [fun, first_op]}, issues, issue_meta)
-    {_, second_op_issues} = traverse({:when, meta, [fun, second_op]}, issues, issue_meta)
+    {_, first_op_issues} = traverse({:when, meta, [fun, first_op]}, [], issue_meta)
+    {_, second_op_issues} = traverse({:when, meta, [fun, second_op]}, [], issue_meta)
 
     {ast, first_op_issues ++ second_op_issues ++ issues}
   end


Test patch:
diff --git a/test/credo/check/refactor/negated_is_nil_test.exs b/test/credo/check/refactor/negated_is_nil_test.exs
index 7b58f2fa..14a8bb93 100644
--- a/test/credo/check/refactor/negated_is_nil_test.exs
+++ b/test/credo/check/refactor/negated_is_nil_test.exs
@@ -108,4 +108,32 @@ defmodule Credo.Check.Refactor.NegatedIsNilTest do
     |> run_check(@described_check)
     |> refute_issues()
   end
+
+  test "it should report only one violation in a module with multiple functions when only one is problematic" do
+    """
+    defmodule CredoSampleModule do
+      def hello(a) when not is_nil(a), do: a
+      def foo1(x) when is_integer(x), do: x
+      def foo2(x) when is_integer(x), do: x
+    end
+    """
+    |> to_source_file()
+    |> run_check(@described_check)
+    |> assert_issue()
+  end
+
+  test "it should report two violations in a module with multiple functions when two are problematic" do
+    """
+    defmodule CredoSampleModule do
+      def hello(a) when not is_nil(a), do: a
+      def foo1(x) when is_integer(x), do: x
+      def foo2(x) when is_integer(x), do: x
+      def hello2(a) when not is_nil(a), do: a
+      def foo3(x) when is_integer(x), do: x
+    end
+    """
+    |> to_source_file()
+    |> run_check(@described_check)
+    |> assert_issues(fn issues -> assert length(issues) == 2 end)
+  end
 end