Analyzer and using Kernel.function/x (rather than function/x)

[MtlSnk]

Hi there!

The hints for the first step of the RPN Calculator Inspection exercise suggest using Kernel.spawn_link/1 instead of a task. In my submission I used Kernel.spawn_link/1 (explicitly with the module), rather than spawn_link/1 or Task.start_link/1 and the analyzer's test suite doesn't account for that.

I'm still learning Elixir; is it against best practice to call Kernel functions by explicitly using the Kernel module's name? ~refer to functions stemming from the Kernel module?~ (edit: fix sentence) If not, should an assert_call be added to elixir_analyzer/test_suite/rpn_calculator_inspection.ex to allow for usage of Kernel.spawn_link/1? Or should called_fn be changed so that the module keyword defaults to Kernel?

[jiegillet]

Hi @MtlSnk,

Would you mind posting your full solution and the comment you got, or alternatively send us a mentoring request link?

I prefer having the full picture before saying something stupid :p

[MtlSnk]

Sure thing! This is the code that causes the analyzer to disagree:

defmodule RPNCalculatorInspection do                                                
  def start_reliability_check(calculator, input) do                                 
    pid = Kernel.spawn_link(fn -> calculator.(input) end)                           
    %{pid: pid, input: input}                                                       
  end                                                                               

  def await_reliability_check_result(%{pid: pid, input: input}, results) do         
    receive do                                                                      
      {:EXIT, ^pid, :normal} -> results |> Map.put(input, :ok)                      
      {:EXIT, ^pid, _error} -> results |> Map.put(input, :error)                    
    after                                                                           
      100 -> results |> Map.put(input, :timeout)                                    
    end                                                                             
  end                                                                               

  def reliability_check(calculator, inputs) do                                      
    {_trap_exit, init_trap_exit} = Process.info(self(), :trap_exit)                 
    Process.flag(:trap_exit, true)                                                  

    results = inputs                                                                
    |> Enum.map(fn input -> start_reliability_check(calculator, input) end)         
    |> Enum.map(fn check -> await_reliability_check_result(check, %{}) end)         
    |> Enum.flat_map(fn result -> result end)                                       
    |> Enum.into(%{})                                                               

    Process.flag(:trap_exit, init_trap_exit)                                        
    results                                                                         
  end                                                                               

  def correctness_check(calculator, inputs) do                                      
    inputs                                                                          
    |> Enum.map(fn input -> Task.async(fn -> calculator.(input) end) end)           
    |> Enum.map(fn task -> Task.await(task, 100) end)                               
  end                                                                               
end

The analyzer:

And when I remove Kernel. from line 3:

[jiegillet]

Thank you for the full code, I was able to find the bug and fix it in #344.

[MtlSnk]

No worries. I do have a couple of questions:

1. Why was the code I sent included in the test suite? I don't see any other test suites that do the same.
2. Does modifying the assert_call mean that using spawn_link/1 now fails or do they both work? I would've expected an additional assert_call with a suppress_if, but if it works. :relaxed:

I found some more of potentially the same issue, though I haven't checked. There were more still, going by grep -R 'called_fn name:'[1], but I couldn't tell if they were similar issues (only that the module keyword was omitted).

• test_suite/german_sysadmin.ex#L10-L20
• test_suite/list_ops.ex#L27-L49

[1] 41 entries as opposed to grep -R 'called_fn module: Kernel's 6.

---

Wouldn't lines 153-154 in assert_call.ex be where the Kernel could be set as the default?

Omitting the module keyword in the AssertCall.function_signature seems (to me) to be used to check whether the called_fn is a:

• local function;
• Kernel function;
• Kernel.SpecialForms function.

It would be nice if the nil return could be changed to return one of the above. Maybe something along the lines of this:

  def validate_module(signature) do
    case signature[:module] do
      nil ->
        try_fetch_module_from_name(signature[:name])
      _ -> "boo"
    end
  end

  defp try_fetch_module_from_name(name) do
    case kfetch(Kernel.SpecialForms, name) do
      Kernel.SpecialForms -> Kernel.SpecialForms
      _ -> case kfetch(Kernel, name) do
        Kernel -> Kernel
        _ -> nil
      end
    end
  end

  defp kfetch(module, name) do
    case Keyword.fetch(module.module_info(:exports), name) do
      {:ok, _arity} -> module
      _ -> nil
    end
  end

I'd test it myself with the existing code base, but I can't get it to work, so I only tested this in a local project. :sweat_smile: Undoubtedly, there's a better, more Elixir idiomatic way of doing this, but I'm not equipped to provide it. Expanding the function name atom :cond to :"MACRO-cond" would still need to be done, in order for current tests not to break.

These are the tests I ran and it seemed to work:

  test "validate_module matches function name to Kernel" do
    signature = %{name: :inspect}
    assert GoFetch.validate_module(signature) == Kernel
  end

  test "validate_module matches function name to Kernel.SpecialForms" do
    signature = %{name: :"MACRO-cond"}
    assert GoFetch.validate_module(signature) == Kernel.SpecialForms
  end

  test "validate_module matches function name to nil (local function)" do
    signature = %{name: :kfetch}
    assert GoFetch.validate_module(signature) == nil
  end

  test "signature uses module keyword" do
    signature = %{module: List, name: :first}
    assert GoFetch.validate_module(signature) == "boo"
  end

Let me know what you think. :relaxed:

[jiegillet]

1. Why was the code I sent included in the test suite?
   I don't see any other test suites that do the same.

Sorry, I should have asked you if it was OK. We sometimes do it, because it's ideal to have real code it tests. We don't always attribute the code though.

May we use your code in there? I can remove it, anonymize it or attribute it to you if you prefer.

2. Does modifying the `assert_call` mean that using `spawn_link/1` now fails or do they both work?
   I would've expected an additional `assert_call` with a `suppress_if`, but if it works. ☺️

The both work. assert_call is able to keep track of functions imported from the standard library, once it knows that spawn_link is a Kernel function, it will find it either way because Kernel is always imported.

I found some more of potentially the same issue, though I haven't checked. There were more still, going by grep -R 'called_fn name:'[1], but I couldn't tell if they were similar issues (only that the module keyword was omitted).

* [test_suite/german_sysadmin.ex#L10-L20](https://github.com/exercism/elixir-analyzer/blob/main/lib/elixir_analyzer/test_suite/german_sysadmin.ex#L10-L20)

* [test_suite/list_ops.ex#L27-L49](https://github.com/exercism/elixir-analyzer/blob/main/lib/elixir_analyzer/test_suite/list_ops.ex#L27-L49)

Nice find! Yes, I believe those are similar. I will add them to the PR.

Wouldn't lines 153-154 in assert_call.ex be where the Kernel could be set as the default?

I like the idea and I will look into it. Thank you for working on it and sharing your ideas.

I'd test it myself with the existing code base, but I can't get it to work, so I only tested this in a local project. 😅

Why can't you get it to work? The codebase has many tests already, it would be very beneficial to be able to use them.

Undoubtedly, there's a better, more Elixir idiomatic way of doing this, but I'm not equipped to provide it. Expanding the function name atom :cond to :"MACRO-cond" would still need to be done, in order for current tests not to break.

I don't worry too much about the special forms, I have never seen a use of Kernel.SpecialForms.cond ... do in the wild. for the sake a of simpler codebase, I think we could ignore it.

[MtlSnk]

May we use your code in there? I can remove it, anonymize it or attribute it to you if you prefer.

Of course. I don't mind, I was just wondering.

The both work. assert_call is able to keep track of functions imported from the standard library, once it knows that spawn_link is a Kernel function, it will find it either way because Kernel is always imported.

That makes sense, thank you!

I like the idea and I will look into it. Thank you for working on it and sharing your ideas.

Hope it helps! I enjoy Exercism and its Elixir exercises very much, so if this is how I can contribute back, I'll take it. :smile:

Why can't you get it to work? The codebase has many tests already, it would be very beneficial to be able to use them.

It's probably a PEBCAK issue. I tried to run the analyzer against a different directory (which contains my exercism exercises), which might be why it didn't work.

I'll try again when I have some more time. :relaxed:

I don't worry too much about the special forms, I have never seen a use of Kernel.SpecialForms.cond ... do in the wild.

Haha, fair point. I didn't think of that.

[jiegillet]

I made some changes to the PR, now assert_call will find Kernel functions whether they are qualified or not, have a look if you like! It was a bit tedious, I'm very thankful we have so many tests, many edge cases were caught that way.

It's probably a PEBCAK issue. I tried to run the analyzer against a different directory (which contains my exercism exercises), which might be why it didn't work.

That should work, I believe. Make sure you use asdf and that you run bin/build.sh before using it.

[MtlSnk]

I don't fully understand the code, but it seems like a nice solution! I'd review it if I had more authority.

Not being able to run mix test was my mistake, I got it working. The elixer_analyzer escript only seems to work after you create a .meta directory containing a config.json. I'd have expected the readme to mention it, but I figured it out from the test_data directory.

I ran elixir_analyzer from your branch and it seems happy with Kernel.spawn_link/1. Cheers, @jiegillet!