Repository: rrrene/credo (elixir)

Issue:
Proposal: warn on Mix.env usage in runtime files
Hi, I hope this proposal makes sense, I'm relatively new to Elixir!

### Environment

* Credo version (`mix credo -v`): 1.1.4
* Erlang/Elixir version (`elixir -v`): Elixir 1.7.4 (compiled with Erlang/OTP 21)
* Operating system: Ubuntu Linux

### What were you trying to do?

My program failed upon deployment because `Mix.env` was called inside runtime code. This works fine in dev and test environments because Mix is available, as well as in the config during build. So it can cause unexpected failures if Mix is unavailable in production as [recommend](https://elixir-lang.org/getting-started/mix-otp/introduction-to-mix.html#environments). It seems like Credo would be a good place to catch such a mistake?

### Expected outcome

Credo emits a warning that `Mix.env` is being used in code where it won't be available at runtime.

### Actual outcome

Credo is silent on the issue.


Pull request:
Add Credo.Check.Warning.MixEnv. Warns on Mix.env usage in app code.
Fixes #699.

Patch:
diff --git a/.credo.exs b/.credo.exs
index 231035d2..716f70d3 100644
--- a/.credo.exs
+++ b/.credo.exs
@@ -124,6 +124,7 @@
         {Credo.Check.Warning.IExPry, []},
         {Credo.Check.Warning.IoInspect, []},
         {Credo.Check.Warning.LazyLogging, []},
+        {Credo.Check.Warning.MixEnv, []},
         {Credo.Check.Warning.OperationOnSameValues, []},
         {Credo.Check.Warning.OperationWithConstantResult, []},
         {Credo.Check.Warning.RaiseInsideRescue, []},
diff --git a/lib/credo/check/warning/mix_env.ex b/lib/credo/check/warning/mix_env.ex
new file mode 100644
index 00000000..f4e41633
--- /dev/null
+++ b/lib/credo/check/warning/mix_env.ex
@@ -0,0 +1,80 @@
+defmodule Credo.Check.Warning.MixEnv do
+  @moduledoc false
+
+  alias Credo.SourceFile
+
+  @checkdoc """
+  From the elixir-lang guide:
+
+  Mix is a build tool and, as such, it is not expected to be available in production.
+  Therefore, it is recommended to access Mix.env only in configuration files and inside
+  mix.exs, never in your application code (lib).
+
+  """
+  @explanation [
+    check: @checkdoc,
+    params: [
+      excluded_paths: "List of paths or regex to exclude from this check"
+    ]
+  ]
+
+  @default_params [excluded_paths: []]
+  @call_string "Mix.env"
+
+  use Credo.Check, base_priority: :high
+
+  @doc false
+  def run(%SourceFile{filename: filename} = source_file, params \\ []) do
+    excluded_paths = Params.get(params, :excluded_paths, @default_params)
+
+    case ignore_path?(source_file.filename, excluded_paths) do
+      true ->
+        []
+
+      false ->
+        issue_meta = IssueMeta.for(source_file, params)
+
+        filename
+        |> Path.extname()
+        |> case do
+          ".exs" -> []
+          _ -> Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+        end
+    end
+  end
+
+  # Check if analyzed module path is within ignored paths
+  defp ignore_path?(filename, excluded_paths) do
+    directory = Path.dirname(filename)
+
+    Enum.any?(excluded_paths, &matches?(directory, &1))
+  end
+
+  defp matches?(directory, %Regex{} = regex), do: Regex.match?(regex, directory)
+  defp matches?(directory, path) when is_binary(path), do: String.starts_with?(directory, path)
+
+  defp traverse(
+         {{:., _, [{:__aliases__, _, [:Mix]}, :env]}, meta, _arguments} = ast,
+         issues,
+         issue_meta
+       ) do
+    {ast, issues_for_call(meta, issues, issue_meta)}
+  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
+    format_issue(
+      issue_meta,
+      message: "There should be no calls to Mix.env in application code.",
+      trigger: trigger,
+      line_no: line_no
+    )
+  end
+end


Test patch:
diff --git a/test/credo/check/warning/mix_env_test.exs b/test/credo/check/warning/mix_env_test.exs
new file mode 100644
index 00000000..a2f21147
--- /dev/null
+++ b/test/credo/check/warning/mix_env_test.exs
@@ -0,0 +1,89 @@
+defmodule Credo.Check.Warning.MixEnvTest do
+  use Credo.TestHelper
+
+  @described_check Credo.Check.Warning.MixEnv
+
+  #
+  # cases NOT raising issues
+  #
+
+  test "it should NOT report expected code" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter1, parameter2) do
+        parameter1 + parameter2
+      end
+    end
+    """
+    |> to_source_file
+    |> refute_issues(@described_check)
+  end
+
+  test "it should NOT report on instance in exs file" do
+    """
+    defmodule CredoSampleModule do
+      def some_function do
+        Mix.env()
+      end
+    end
+    """
+    |> to_source_file("foo.exs")
+    |> refute_issues(@described_check)
+  end
+
+  test "it should NOT report on instance in excluded string path" do
+    """
+    defmodule CredoSampleModule do
+      def some_function do
+        Mix.env()
+      end
+    end
+    """
+    |> to_source_file("foo/dangerous_stuff/bar.ex")
+    |> refute_issues(@described_check,
+      excluded_paths: ["foo/dangerous_stuff"]
+    )
+  end
+
+  test "it should NOT report on instance in excluded regex path" do
+    """
+    defmodule CredoSampleModule do
+      def some_function do
+        Mix.env()
+      end
+    end
+    """
+    |> to_source_file("foo/dangerous_stuff/bar.ex")
+    |> refute_issues(@described_check,
+      excluded_paths: [~r"danger"]
+    )
+  end
+
+  #
+  # cases raising issues
+  #
+
+  test "it should report a violation" do
+    """
+    defmodule CredoSampleModule do
+      def some_function do
+        Mix.env()
+      end
+    end
+    """
+    |> to_source_file
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /2" do
+    """
+    defmodule CredoSampleModule do
+      def some_function do
+        &Mix.env/0
+      end
+    end
+    """
+    |> to_source_file
+    |> assert_issue(@described_check)
+  end
+end