Fix more triggers

lib/credo/check/design/tag_helper.ex

@@ -36,18 +36,19 @@ defmodule Credo.Check.Design.TagHelper do
     end
   end
 
-  defp traverse({:@, _, [{name, meta, [string]} | _]} = ast, issues, regex)
+  defp traverse({:@, _, [{name, meta, [string]} | _]} = ast, memo, regex)
        when name in @doc_attribute_names and is_binary(string) do
     if string =~ regex do
       trimmed = String.trim_trailing(string)
-      {nil, issues ++ [{meta[:line], trimmed, trimmed}]}
+
+      {nil, memo ++ [{meta[:line], trimmed, trimmed}]}
     else
-      {ast, issues}
+      {ast, memo}
     end
   end
 
-  defp traverse(ast, issues, _regex) do
-    {ast, issues}
+  defp traverse(ast, memo, _regex) do
+    {ast, memo}
   end
 
   defp find_tag_in_line({line, index}, regex) do

lib/credo/check/warning/dbg.ex

@@ -12,76 +12,78 @@ defmodule Credo.Check.Warning.Dbg do
       """
     ]
 
-  @call_string "dbg"
-
   @doc false
   @impl true
   def run(%SourceFile{} = source_file, params) do
     issue_meta = IssueMeta.for(source_file, params)
     Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
   end
 
+  defp traverse(
+         {:@, _, [{:dbg, _, _}]},
+         issues,
+         _issue_meta
+       ) do
+    {nil, issues}
+  end
+
   defp traverse(
          {:dbg, meta, []} = ast,
          issues,
          issue_meta
        ) do
-    {ast, issues_for_call(meta, issues, issue_meta)}
+    {ast, [issue_for(issue_meta, meta[:line]) | issues]}
   end
 
   defp traverse(
          {:dbg, meta, [_single_param]} = ast,
          issues,
          issue_meta
        ) do
-    {ast, issues_for_call(meta, issues, issue_meta)}
+    {ast, [issue_for(issue_meta, meta[:line]) | issues]}
   end
 
   defp traverse(
          {:dbg, meta, [_first_param, _second_param]} = ast,
          issues,
          issue_meta
        ) do
-    {ast, issues_for_call(meta, issues, issue_meta)}
+    {ast, [issue_for(issue_meta, meta[:line]) | issues]}
   end
 
   defp traverse(
          {{:., _, [{:__aliases__, _, [:"Elixir", :Kernel]}, :dbg]}, meta, _args} = ast,
          issues,
          issue_meta
        ) do
-    {ast, issues_for_call(meta, issues, issue_meta)}
+    {ast, [issue_for(issue_meta, meta[:line]) | issues]}
   end
 
   defp traverse(
          {{:., _, [{:__aliases__, _, [:Kernel]}, :dbg]}, meta, _args} = ast,
          issues,
          issue_meta
        ) do
-    {ast, issues_for_call(meta, issues, issue_meta)}
+    {ast, [issue_for(issue_meta, meta[:line]) | issues]}
   end
 
   defp traverse(
          {:|>, _, [_, {:dbg, meta, nil}]} = ast,
          issues,
          issue_meta
        ) do
-    {ast, issues_for_call(meta, issues, issue_meta)}
+    {ast, [issue_for(issue_meta, meta[:line]) | issues]}
   end
 
   defp traverse(ast, issues, _issue_meta) do
     {ast, issues}
   end
 
-  defp issues_for_call(meta, issues, issue_meta) do
-    [issue_for(issue_meta, meta[:line], @call_string) | issues]
-  end
-
-  defp issue_for(issue_meta, line_no, trigger) do
+  defp issue_for(issue_meta, line_no) do
     format_issue(
       issue_meta,
       message: "There should be no calls to dbg.",
-      trigger: trigger,
+      trigger: "dbg",
       line_no: line_no
     )
   end

lib/credo/check/warning/leaky_environment.ex

@@ -27,13 +27,13 @@ defmodule Credo.Check.Warning.LeakyEnvironment do
     Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
   end
 
-  defp traverse({{:., _loc, call}, meta, args} = ast, issues, issue_meta) do
+  defp traverse({{:., _, call}, meta, args} = ast, issues, issue_meta) do
     case get_forbidden_call(call, args) do
       nil ->
         {ast, issues}
 
-      bad ->
-        {ast, issues_for_call(bad, meta, issue_meta, issues)}
+      trigger ->
+        {ast, [issue_for(issue_meta, meta[:line], trigger) | issues]}
     end
   end
 
@@ -42,38 +42,33 @@ defmodule Credo.Check.Warning.LeakyEnvironment do
   end
 
   defp get_forbidden_call([{:__aliases__, _, [:System]}, :cmd], [_, _]) do
-    "System.cmd/2"
+    "System.cmd"
   end
 
   defp get_forbidden_call([{:__aliases__, _, [:System]}, :cmd], [_, _, opts])
        when is_list(opts) do
-    if Keyword.has_key?(opts, :env) do
-      nil
-    else
-      "System.cmd/3"
+    if not Keyword.has_key?(opts, :env) do
+      "System.cmd"
     end
   end
 
   defp get_forbidden_call([:erlang, :open_port], [_, opts])
        when is_list(opts) do
-    if Keyword.has_key?(opts, :env) do
-      nil
-    else
-      ":erlang.open_port/2"
+    if not Keyword.has_key?(opts, :env) do
+      ":erlang.open_port"
     end
   end
 
   defp get_forbidden_call(_, _) do
     nil
   end
 
-  defp issues_for_call(call, meta, issue_meta, issues) do
-    options = [
-      message: "When using #{call}, clear or overwrite sensitive environment variables",
-      trigger: call,
-      line_no: meta[:line]
-    ]
-
-    [format_issue(issue_meta, options) | issues]
+  defp issue_for(issue_meta, line_no, trigger) do
+    format_issue(
+      issue_meta,
+      message: "When using #{trigger}, clear or overwrite sensitive environment variables",
+      trigger: trigger,
+      line_no: line_no
+    )
   end
 end

lib/credo/check/warning/unsafe_exec.ex

@@ -36,8 +36,8 @@ defmodule Credo.Check.Warning.UnsafeExec do
 
   defp traverse({{:., _loc, call}, meta, args} = ast, issues, issue_meta) do
     case get_forbidden_call(call, args) do
-      {bad, suggestion} ->
-        {ast, issues_for_call(bad, suggestion, meta, issue_meta, issues)}
+      {bad, suggestion, trigger} ->
+        {ast, [issue_for(bad, suggestion, trigger, meta[:line], issue_meta) | issues]}
 
       nil ->
         {ast, issues}
@@ -49,28 +49,27 @@ defmodule Credo.Check.Warning.UnsafeExec do
   end
 
   defp get_forbidden_call([:os, :cmd], [_]) do
-    {":os.cmd/1", "System.cmd/2,3"}
+    {":os.cmd/1", "System.cmd/2,3", "System.cmd"}
   end
 
   defp get_forbidden_call([:os, :cmd], [_, _]) do
-    {":os.cmd/2", "System.cmd/2,3"}
+    {":os.cmd/2", "System.cmd/2,3", "System.cmd"}
   end
 
   defp get_forbidden_call([:erlang, :open_port], [{:spawn, _}, _]) do
-    {":erlang.open_port/2 with `:spawn`", ":erlang.open_port/2 with `:spawn_executable`"}
+    {":erlang.open_port/2 with `:spawn`", ":erlang.open_port/2 with `:spawn_executable`",
+     ":erlang.open_port"}
   end
 
   defp get_forbidden_call(_, _) do
     nil
   end
 
-  defp issues_for_call(call, suggestion, meta, issue_meta, issues) do
-    options = [
+  defp issue_for(call, suggestion, trigger, line_no, issue_meta) do
+    format_issue(issue_meta,
       message: "Prefer #{suggestion} over #{call} to prevent command injection",
-      trigger: call,
-      line_no: meta[:line]
-    ]
-
-    [format_issue(issue_meta, options) | issues]
+      trigger: trigger,
+      line_no: line_no
+    )
   end
 end

lib/credo/check/warning/unused_enum_operation.ex

@@ -10,9 +10,9 @@ defmodule Credo.Check.Warning.UnusedEnumOperation do
       While this is correct ...
 
           def prepend_my_username(my_username, usernames) do
-            valid_usernames = Enum.reject(usernames, &is_nil/1)
+            usernames = Enum.reject(usernames, &is_nil/1)
 
-            [my_username] ++ valid_usernames
+            [my_username] ++ usernames
           end
 
       ... we forgot to save the downcased username in this example:
@@ -22,7 +22,7 @@ defmodule Credo.Check.Warning.UnusedEnumOperation do
           def prepend_my_username(my_username, usernames) do
             Enum.reject(usernames, &is_nil/1)
 
-            [my_username] ++ valid_usernames
+            [my_username] ++ usernames
           end
 
       Since Elixir variables are immutable, Enum operations never work on the

test/credo/check/warning/dbg_test.exs

@@ -10,7 +10,10 @@ defmodule Credo.Check.Warning.DbgTest do
   test "it should NOT report expected code" do
     """
     defmodule CredoSampleModule do
+      @dbg "this should be found"
+
       def some_function(parameter1, parameter2) do
+        dbg = "variables should also not be a problem"
         parameter1 + parameter2
       end
     end
@@ -193,6 +196,9 @@ defmodule Credo.Check.Warning.DbgTest do
     '''
     |> to_source_file
     |> run_check(@described_check)
-    |> assert_issue()
+    |> assert_issue(fn issue ->
+      assert issue.line_no == 3
+      assert issue.trigger == "dbg"
+    end)
   end
 end

test/credo/check/warning/forbidden_module_test.exs

@@ -106,15 +106,16 @@ defmodule Credo.Check.Warning.ForbiddenModuleTest do
   test "it should display a custom message" do
     """
     defmodule CredoSampleModule do
-      def some_function, do: CredoSampleModule.ForbiddenModule.another_function()
+      def some_function, do:
+        CredoSampleModule.ForbiddenModule.another_function()
     end
     """
     |> to_source_file
     |> run_check(@described_check, modules: [{CredoSampleModule.ForbiddenModule, "my message"}])
     |> assert_issue(fn issue ->
-      expected_message = "my message"
-
-      assert issue.message == expected_message
+      assert issue.line_no == 3
+      assert issue.trigger == "CredoSampleModule.ForbiddenModule"
+      assert issue.message == "my message"
     end)
   end
 

test/credo/check/warning/leaky_environment_test.exs

@@ -38,7 +38,10 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do
     """
     |> to_source_file()
     |> run_check(@described_check)
-    |> assert_issue()
+    |> assert_issue(fn issue ->
+      assert issue.line_no == 3
+      assert issue.trigger == "System.cmd"
+    end)
   end
 
   test "it should report a violation /2" do
@@ -51,7 +54,10 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do
     """
     |> to_source_file()
     |> run_check(@described_check)
-    |> assert_issue()
+    |> assert_issue(fn issue ->
+      assert issue.line_no == 3
+      assert issue.trigger == "System.cmd"
+    end)
   end
 
   test "it should report a violation /3" do
@@ -64,6 +70,9 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do
     """
     |> to_source_file()
     |> run_check(@described_check)
-    |> assert_issue()
+    |> assert_issue(fn issue ->
+      assert issue.line_no == 3
+      assert issue.trigger == ":erlang.open_port"
+    end)
   end
 end

test/credo/check/warning/raise_inside_rescue_test.exs

@@ -117,4 +117,39 @@ defmodule Credo.Check.Warning.RaiseInsideRescueTest do
       assert 8 == issue.line_no
     end)
   end
+
+  test "it should report multiple violations" do
+    """
+    defmodule CredoSampleModule do
+      use ExUnit.Case
+
+      def catcher do
+        try do
+          raise "oops"
+        rescue
+          e ->
+            if is_nil(e) do
+              Logger.warn("Something bad happened") && raise e
+            else
+              raise e
+            end
+        end
+      end
+
+      def catcher do
+        raise "oops"
+      rescue
+        e ->
+          if is_nil(e) do
+            Logger.warn("Something bad happened") && raise e
+          else
+            raise e
+          end
+      end
+    end
+    """
+    |> to_source_file
+    |> run_check(@described_check)
+    |> assert_issues(fn issues -> assert Enum.count(issues) == 4 end)
+  end
 end