Added erlang ast test in captains-log

[FraSanga]

As described in issue #340, I modified the file by attempting to perform the check with the AST.

I am uncertain if this is correct, but the tests I added also work with the examples I created.

As soon as I get the go-ahead, I'll also make a PR on website-copy.
I hope this has been helpful. 😄

[jiegillet]

Thank you for the PR!
Before I go into a detailed review, could I ask you to address two things:

1. I don't understand why all the files you are modifying are completely redefined. As far as I could tell, you are only modifying a few lines, not the whole files. Could you figure it out and fix it? It would really help with the review and the git history.
2. The tests in test_data are used as smoke tests for the CI, we don't typically add new ones in there unless we have a good reason. The tests you added in test/elixir_analyzer/test_suite/captains_log_test.exs should be enough.

[FraSanga]

It should be fixed now.
Every time Windows messes up the formatting at the end of the line.

[FraSanga]

It has been open for 5 months. Can someone take a look?

[angelikatyborska]

Hi there! We did completely forget about your PR, sorry about that 😞 I'm writing this comment to confirm that both @jiegillet and I saw your recent reminder and we'll looking for some free time to re-review the PR soon.

[jiegillet]

Sorry about the wait. I think I was waiting for you to address my second point, and then lost track of the PR altogether.

2. The tests in test_data are used as smoke tests for the CI, we don't typically add new ones in there unless we have a good reason. The tests you added in test/elixir_analyzer/test_suite/captains_log_test.exs should be enough.

This still stands, could you revert the changes to test_data?

Other than that, I've looked at your changes and left some comments.

[jiegillet]

This value is a path to a file in this repo that contains the text of the analyzer comment, so you need to create a new file over there for the analyzer to work. Could you do the appropriate change over there, link to the PR in this PR's description and request a review from me?

[FraSanga]

exercism/website-copy#2395

I cannot ask anyone for a review on the repo.

[jiegillet]

I'm hesitating about this change.
It doesn't check that "format_stardate uses erlang", it checks that one of two specific Erlang functions are used anywhere at all, even in dead code.
I can think of two preferable approaches:

1. The easy one: add an extra assert_call (the following code is missing values, it's just to show the structure)

assert_call "format_stardate uses :io_lib" do
  calling_fn module: CaptainsLog, name: :format_stardate
  called_fn module: :io_lib, name: :_
  suppress_if "format_stardate uses :erlang", :pass
end

assert_call "format_stardate uses :erlang" do
  calling_fn module: CaptainsLog, name: :format_stardate
  called_fn module: :erlang, name: :_
  suppress_if "format_stardate uses :io_lib", :pass
end

this should be equivalent to the intent of your PR.

2. The hard one: add a feature to the macro assert_call, something like

assert_call "format_stardate uses an Erlang module" do
  calling_fn module: CaptainsLog, name: :format_stardate
  called_fn module: AnyErlangModule, name: :_
end

This would detect the use of any non-elixir module, while preserving all of the advantages of assert_call (tracking imports, aliases, the use of helper functions...). It would also be future proof in case there is another Erlang module that could solve the problem. I think it should be possible since Elixir module names are always capitalized unlike erlang ones, but this is the hard approach because the assert_call macro is quite complex, so I'm not pushing for it, just writing down my thoughts.

Since approach 1. would already be a strict improvement, I think that's the one we should aim at to relieve your from your 5-month journey within a reasonable time :)

[jiegillet]

Could you add at one test that use an :erlang function to solve the problem and doesn't raise a comment?

[Copilot]

Pull request overview

This PR updates the Captains Log analyzer rule for format_stardate/1 to allow Erlang-based implementations (per issue #340), switching from a call-based assertion to an AST-based source check.

Changes:

• Replaced the format_stardate analyzer check with a check_source that scans the submission AST for Erlang usage.
• Renamed the Captains Log analyzer comment id from use_io_lib to use_erlang.
• Updated the Captains Log test to expect the new comment id.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
lib/elixir_analyzer/test_suite/captains_log.ex	Replaces the format_stardate check with an AST traversal looking for :io_lib/:erlang calls.
lib/elixir_analyzer/constants.ex	Renames the analyzer comment id used for the Captains Log format_stardate guidance.
test/elixir_analyzer/test_suite/captains_log_test.exs	Updates the expectation to the renamed comment id.

Comments suppressed due to low confidence (1)

test/elixir_analyzer/test_suite/captains_log_test.exs:110

• There’s no test covering the newly-allowed format_stardate implementations that use :erlang (e.g., :erlang.float_to_binary/2) and should not emit this comment. Add a test_exercise_analysis case that uses an Erlang-based implementation and asserts Constants.captains_log_use_erlang() is excluded, so the analyzer change is protected against regressions.

  test_exercise_analysis "format_stardate uses Float.round",
    comments_include: [Constants.captains_log_use_erlang()] do
    defmodule CaptainsLog do
      def format_stardate(stardate) do
        if is_float(stardate) do
          Float.round(stardate, 1) |> to_string()
        else
          raise ArgumentError
        end
      end
    end

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The check currently scans the entire submission AST, so format_stardate can incorrectly pass if any :erlang/:io_lib call exists elsewhere in the module (or if format_stardate is missing entirely). Scope the traversal to the def format_stardate/1 body (and fail when that function can’t be found) so the check matches the intended requirement.

Suggested change

	{_, erlang?} =
	Macro.prewalk(code_ast, false, fn node, acc ->
	case node do
	# usage :io_lib.format/2
	{{:., _, [:io_lib, :format]}, _, _} ->
	{node, true}
	# usage :erlang.function_name/arity
	# matches any function call from :erlang module
	{{:., _, [:erlang, _]}, _, _} ->
	{node, true}
	_ ->
	{node, acc}
	end
	end)
	erlang?
	# First, locate the body of def format_stardate/1
	{_, format_stardate_body} =
	Macro.prewalk(code_ast, nil, fn
	{:def, _, [{:format_stardate, _, args}, [do: body]]} = node, nil
	when is_list(args) and length(args) == 1 ->
	{node, body}
	node, acc ->
	{node, acc}
	end)
	case format_stardate_body do
	nil ->
	# Fail the check if format_stardate/1 is not defined
	false
	body_ast ->
	{_, erlang?} =
	Macro.prewalk(body_ast, false, fn node, acc ->
	case node do
	# usage :io_lib.format/2
	{{:., _, [:io_lib, :format]}, _, _} ->
	{node, true}
	# usage :erlang.function_name/arity
	# matches any function call from :erlang module
	{{:., _, [:erlang, _]}, _, _} ->
	{node, true}
	_ ->
	{node, acc}
	end
	end)
	erlang?
	end

[Copilot]

This AST match only recognizes explicit remote calls and only :io_lib.format/2. Previously the analyzer accepted any :io_lib.* call (and would also detect imported functions via assert_call). Consider broadening the match (e.g., any :io_lib function) and/or reusing the existing AssertCall detection logic so imports/aliases don’t become false negatives.

[Copilot]

Adding a new analyzer comment id (elixir.captains-log.use_erlang) will fail the repo’s external check that verifies every Constants entry exists in exercism/website-copy (see test/elixir_analyzer/constants_test.exs). Either ensure the corresponding website-copy/analyzer-comments/elixir/captains-log/use_erlang.md is merged first, or keep the existing comment id to avoid breaking that validation.

Suggested change

captains_log_use_erlang: "elixir.captains-log.use_erlang",

[FraSanga]

I should have sorted everything out. My only concern with the first approach is that if a user makes a mistake, both the comments from :io_lib and :erlang will be displayed, I suppose.