New common check: Helper functions should be private (#207)

* Implement check

* Add Source to read and hold exemploid files

* Add tests for missing/corrupted example files

* Send proper file name to compiler warnings

* Read practice exercice example files in test cases

* Add tests, fix old tests with public helpers

* update submodule

* mix format and mix credo

* Typos and formatting

* Refactor, remove undocumented :file and :module parameters

* Add tests for private helpers

* Change check to consider arity

* Rework check test to show comment details

* Update externel test CI and submodule

* typo

* Update submodule with cleaned up exemploids

* Arguments to _, show first failure

* Hide functions after @doc false or @impl true

* Revert "Hide functions after @doc false or @impl true"

This reverts commit 6e5c3a7.

* Annotate AST to keep track of modules

Co-authored-by: Angelika Tyborska <[email protected]>

Author: jiegillet

.github/workflows/elixir_test_external.yml

@@ -10,11 +10,25 @@ jobs:
       image: hexpm/elixir:1.12.1-erlang-24.0.1-ubuntu-focal-20210325
 
     steps:
-      - uses: actions/checkout@v1
+      - name: Install git
+        run: |
+          apt-get update
+          apt-get install -y git
+
+      - name: Checkout repository and submodules
+        uses: actions/checkout@v2
+        with:
+          submodules: recursive
+
+      - name: Update submodules
+        run: |
+          git submodule update --recursive --remote
+
       - name: Install Dependencies
         run: |
           mix local.rebar --force
           mix local.hex --force
           mix deps.get
+
       - name: Run Tests
         run: mix test --only external

elixir

@@ -9,6 +9,7 @@ defmodule ElixirAnalyzer do
   alias ElixirAnalyzer.Constants
   alias ElixirAnalyzer.Submission
   alias ElixirAnalyzer.Comment
+  alias ElixirAnalyzer.Source
 
   import ElixirAnalyzer.Summary, only: [summary: 2]
 
@@ -85,8 +86,6 @@ defmodule ElixirAnalyzer do
     defaults = [
       {:exercise, exercise},
       {:path, input_path},
-      {:file, nil},
-      {:module, nil},
       {:output_path, output_path},
       {:output_file, @output_file},
       {:exercise_config, default_exercise_config()},
@@ -100,22 +99,28 @@ defmodule ElixirAnalyzer do
   # Do init work
   # -read config, create the initial Submission struct
   defp init(params) do
-    submission = %Submission{
+    source = %Source{
       path: params.path,
-      code_path: nil,
-      code_file: nil,
+      slug: params.exercise
+    }
+
+    submission = %Submission{
+      source: source,
       analysis_module: nil
     }
 
     try do
       Logger.debug("Getting the exercise config")
       exercise_config = params.exercise_config[params.exercise]
-      {code_path, code_file, exemplar_path, analysis_module} = do_init(params, exercise_config)
+
+      {code_path, exercise_type, exemploid_path, analysis_module} =
+        do_init(params, exercise_config)
 
       Logger.debug("Initialization successful",
         path: params.path,
         code_path: code_path,
-        exemplar_path: exemplar_path,
+        exercise_type: exercise_type,
+        exemploid_path: exemploid_path,
         analysis_module: analysis_module
       )
 
@@ -128,11 +133,16 @@ defmodule ElixirAnalyzer do
           Logger.info("Exercise test suite '#{m}' found and loaded.")
       end
 
+      source = %{
+        source
+        | code_path: code_path,
+          exercise_type: exercise_type,
+          exemploid_path: exemploid_path
+      }
+
       %{
         submission
-        | code_path: code_path,
-          code_file: code_file,
-          exemplar_path: exemplar_path,
+        | source: source,
           analysis_module: analysis_module
       }
     rescue
@@ -159,99 +169,78 @@ defmodule ElixirAnalyzer do
     end
   end
 
-  # When file is nil, pull code params from config file
-  defp do_init(%{file: nil} = params, exercise_config) do
+  defp do_init(params, exercise_config) do
     meta_config = Path.join(params.path, @meta_config) |> File.read!() |> Jason.decode!()
     relative_code_path = meta_config["files"]["solution"] |> hd()
-    full_code_path = Path.join(params.path, relative_code_path)
+    code_path = Path.join(params.path, relative_code_path)
 
-    code_path = Path.dirname(full_code_path)
-    code_file = Path.basename(full_code_path)
-
-    exemplar_path =
-      case meta_config["files"]["exemplar"] do
-        [path | _] -> Path.join(params.path, path)
-        _ -> nil
+    {exercise_type, exemploid_path} =
+      case meta_config["files"] do
+        %{"exemplar" => [path | _]} -> {:concept, Path.join(params.path, path)}
+        %{"example" => [path | _]} -> {:practice, Path.join(params.path, path)}
       end
 
-    {code_path, code_file, exemplar_path,
+    {code_path, exercise_type, exemploid_path,
      exercise_config[:analyzer_module] || ElixirAnalyzer.TestSuite.Default}
   end
 
-  # Else, use passed in params to analyze
-  defp do_init(params, _exercise_config) do
-    {
-      params.path,
-      params.file,
-      String.to_existing_atom("ElixirAnalyzer.ExerciseTest.#{params.module}")
-    }
-  end
-
   # Check
   # - check if the file exists
   # - read in the code
-  # - check if there is an exemplar
-  # - read in the exemplar
-  # - parse the exemplar into an AST
+  # - check if there is an exemploid
+  # - read in the exemploid
+  # - parse the exemploid into an AST
   defp check(%Submission{halted: true} = submission, _params) do
     Logger.warning("Check not performed, halted previously")
     submission
   end
 
-  defp check(%Submission{} = submission, _params) do
-    with path_to_code <- Path.join(submission.code_path, submission.code_file),
-         :ok <- Logger.info("Attempting to read code file", code_file_path: path_to_code),
-         {:code_read, {:ok, code_str}} <- {:code_read, File.read(path_to_code)},
-         :ok <- Logger.info("Code file read successfully"),
-         submission <- %{submission | code: code_str},
-         :ok <- Logger.info("Check if exemplar exists", exemplar_path: submission.exemplar_path),
-         {:exemplar_exists, submission, exemplar_path} when not is_nil(exemplar_path) <-
-           {:exemplar_exists, submission, submission.exemplar_path},
-         :ok <-
-           Logger.info("Exemplar file exists, attempting to read", exemplar_path: exemplar_path),
-         {:exemplar_read, submission, {:ok, exemplar_code}} <-
-           {:exemplar_read, submission, File.read(exemplar_path)},
-         :ok <- Logger.info("Exemplar file read successfully, attempting to parse"),
-         {:exemplar_ast, submission, {:ok, exemplar_code}} <-
-           {:exemplar_ast, submission, Code.string_to_quoted(exemplar_code)},
-         :ok <- Logger.info("Exemplar file parsed successfully"),
-         submission <- %{submission | exemplar_code: exemplar_code} do
-      submission
+  defp check(%Submission{source: source} = submission, _params) do
+    Logger.info("Attempting to read code file", code_file_path: source.code_path)
+
+    with {:code_read, {:ok, code_string}} <- {:code_read, File.read(source.code_path)},
+         source <- %{source | code_string: code_string},
+         Logger.info("Code file read successfully"),
+         Logger.info("Attempting to read exemploid", exemploid_path: source.exemploid_path),
+         {:exemploid_read, _, {:ok, exemploid_string}} <-
+           {:exemploid_read, source, File.read(source.exemploid_path)},
+         Logger.info("Exemploid file read successfully, attempting to parse"),
+         {:exemploid_ast, _, {:ok, exemploid_ast}} <-
+           {:exemploid_ast, source, Code.string_to_quoted(exemploid_string)} do
+      Logger.info("Exemploid file parsed successfully")
+      source = %{source | exemploid_string: exemploid_string, exemploid_ast: exemploid_ast}
+      %{submission | source: source}
     else
       {:code_read, {:error, reason}} ->
         Logger.warning("TestSuite halted: Code file not found. Reason: #{reason}",
-          path: submission.path,
-          file_name: submission.code_file
+          path: source.path,
+          code_path: source.code_path
         )
 
         submission
         |> Submission.halt()
         |> Submission.append_comment(%Comment{
           comment: Constants.general_file_not_found(),
           params: %{
-            "file_name" => submission.code_file,
-            "path" => submission.path
+            "file_name" => Path.basename(source.code_path),
+            "path" => source.path
           },
           type: :essential
         })
 
-      {:exemplar_exists, submission, nil} ->
-        Logger.info("There is no exemplar file for this exercise")
-        submission
-
-      {:exemplar_read, submission, {:error, reason}} ->
-        Logger.warning("Exemplar file not found. Reason: #{reason}",
-          exemplar_path: submission.exemplar_path
+      {:exemploid_read, source, {:error, reason}} ->
+        Logger.warning("Exemploid file not found. Reason: #{reason}",
+          exemploid_path: source.exemploid_path
         )
 
-        submission
+        %{submission | source: source}
 
-      {:exemplar_ast, submission, {:error, reason}} ->
-        Logger.warning("Exemplar file could not be parsed. Reason: #{inspect(reason)}",
-          exemplar_code: submission.exemplar_code
+      {:exemploid_ast, source, {:error, reason}} ->
+        Logger.warning("Exemploid file could not be parsed. Reason: #{inspect(reason)}",
+          exemploid_path: source.exemploid_path
         )
 
-        submission
+        %{submission | source: source}
     end
   end
 
@@ -267,7 +256,7 @@ defmodule ElixirAnalyzer do
 
     submission =
       submission
-      |> submission.analysis_module.analyze(submission.code, submission.exemplar_code)
+      |> submission.analysis_module.analyze(submission.source)
       |> Submission.set_analyzed(true)
 
     Logger.info("Analyzing code complete")

lib/elixir_analyzer.ex

@@ -35,6 +35,7 @@ defmodule ElixirAnalyzer.Constants do
     solution_no_integer_literal: "elixir.solution.no_integer_literal",
     solution_boilerplate_comment: "elixir.solution.boilerplate_comment",
     solution_todo_comment: "elixir.solution.todo_comment",
+    solution_private_helper_functions: "elixir.solution.private_helper_functions",
 
     # Concept exercises
 

lib/elixir_analyzer/constants.ex

@@ -9,6 +9,7 @@ defmodule ElixirAnalyzer.ExerciseTest do
   alias ElixirAnalyzer.Submission
   alias ElixirAnalyzer.Constants
   alias ElixirAnalyzer.Comment
+  alias ElixirAnalyzer.Source
 
   @doc false
   defmacro __using__(opts) do
@@ -21,7 +22,7 @@ defmodule ElixirAnalyzer.ExerciseTest do
 
       import unquote(__MODULE__)
       @before_compile unquote(__MODULE__)
-      @dialyzer no_match: {:do_analyze, 4}
+      @dialyzer no_match: {:do_analyze, 2}
     end
   end
 
@@ -38,7 +39,7 @@ defmodule ElixirAnalyzer.ExerciseTest do
 
     # placeholders for submission code
     code_ast = quote do: code_ast
-    code_as_string = quote do: code_as_string
+    code_string = quote do: code_string
 
     # compile each feature to a test
     feature_tests = Enum.map(feature_test_data, &FeatureCompiler.compile(&1, code_ast))
@@ -48,28 +49,34 @@ defmodule ElixirAnalyzer.ExerciseTest do
 
     # compile each check_source to a test
     check_source_tests =
-      Enum.map(check_source_data, &CheckSourceCompiler.compile(&1, code_as_string))
+      Enum.map(check_source_data, &CheckSourceCompiler.compile(&1, code_string))
 
     quote do
-      @spec analyze(Submission.t(), String.t(), nil | Macro.t()) :: Submission.t()
-      def analyze(%Submission{} = submission, code_as_string, exemplar_ast) do
-        case Code.string_to_quoted(code_as_string) do
+      @spec analyze(Submission.t(), Source.t()) :: Submission.t()
+      def analyze(%Submission{} = submission, %Source{code_string: code_string} = source) do
+        case Code.string_to_quoted(code_string) do
           {:ok, code_ast} ->
-            do_analyze(submission, code_ast, code_as_string, exemplar_ast)
+            source = %{source | code_ast: code_ast}
+            do_analyze(submission, source)
 
           {:error, e} ->
             append_analysis_failure(submission, e)
         end
       end
 
-      defp do_analyze(%Submission{} = submission, code_ast, code_as_string, exemplar_ast)
-           when is_binary(code_as_string) do
+      defp do_analyze(
+             %Submission{} = submission,
+             %Source{
+               code_string: code_string,
+               code_ast: code_ast
+             } = source
+           ) do
         results =
           Enum.concat([
             unquote(feature_tests),
             unquote(assert_call_tests),
             unquote(check_source_tests),
-            CommonChecks.run(code_ast, code_as_string, exemplar_ast)
+            CommonChecks.run(source)
           ])
           |> filter_suppressed_results()
 

lib/elixir_analyzer/exercise_test.ex

@@ -12,10 +12,12 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks do
     BooleanFunctions,
     ExemplarComparison,
     Indentation,
+    PrivateHelperFunctions,
     Comments
   }
 
   alias ElixirAnalyzer.Comment
+  alias ElixirAnalyzer.Source
 
   # CommonChecks that use feature or assert_call should be called here
   defmacro __using__(_opts) do
@@ -27,18 +29,25 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks do
     end
   end
 
-  @spec run(Macro.t(), String.t(), nil | Macro.t()) :: [{:pass | :fail | :skip, %Comment{}}]
-  def run(code_ast, code_as_string, exemplar_ast) when is_binary(code_as_string) do
+  @spec run(Source.t()) :: [{:pass | :fail | :skip, %Comment{}}]
+  def run(%Source{
+        code_path: code_path,
+        code_ast: code_ast,
+        code_string: code_string,
+        exercise_type: type,
+        exemploid_ast: exemploid_ast
+      }) do
     [
       FunctionNames.run(code_ast),
       VariableNames.run(code_ast),
       ModuleAttributeNames.run(code_ast),
       ModulePascalCase.run(code_ast),
-      CompilerWarnings.run(code_ast),
+      CompilerWarnings.run(code_path, code_ast),
       BooleanFunctions.run(code_ast),
-      ExemplarComparison.run(code_ast, exemplar_ast),
-      Indentation.run(code_ast, code_as_string),
-      Comments.run(code_ast, code_as_string)
+      ExemplarComparison.run(code_ast, type, exemploid_ast),
+      Indentation.run(code_ast, code_string),
+      PrivateHelperFunctions.run(code_ast, exemploid_ast),
+      Comments.run(code_ast, code_string)
     ]
     |> List.flatten()
   end

lib/elixir_analyzer/exercise_test/common_checks.ex

@@ -5,14 +5,14 @@ defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.CompilerWarnings do
   alias ElixirAnalyzer.Constants
   alias ElixirAnalyzer.Comment
 
-  def run(code_ast) do
+  def run(code_path, code_ast) do
     import ExUnit.CaptureIO
     Application.put_env(:elixir, :ansi_enabled, false)
 
     warnings =
       capture_io(:stderr, fn ->
         try do
-          Code.compile_quoted(code_ast)
+          Code.compile_quoted(code_ast, Path.basename(code_path))
           |> Enum.each(fn {module, _binary} ->
             :code.delete(module)
             :code.purge(module)