Fix: Dialyzer

[filipeherculano]

The problem

When running Dialyzer with the code snippet below, it results errors of unused guards after all the quotes have been compiled.

use Nebulex.Caching.Decorators
alias Adapters.Cache

[...]
@decorator cache(
 cache: Cache, 
 key: {:fetch, param}, 
 opts:[ttl: get_ttl()]
)
def fetch(param) do
...
end

defp get_ttl(...) do
...
end

▶ mix dialyzer --format dialyxir
[...]
Total errors: 7, Skipped: 5, Unnecessary Skips: 0
done in 0m22.75s
lib/str_integration_engine/rest.ex:1:guard_fail
The guard clause:

when _ :: true === nil

can never succeed.

________________________________________________________________________________
lib/str_integration_engine/rest.ex:1:guard_fail
The guard clause:

when _ :: {:fetch, _} === nil

can never succeed.

________________________________________________________________________________
done (warnings were emitted)
Halting VM with exit status 2

The reason

When the function caching_action is compiled, the IR (Intermediate Representation), that is generated, has control flow code that will never run (as the error Dialyzer error says: "The guard clause: (...) can never succeed."). But when does it happen?

After careful testing on my working code, the following scenarios occurred:

When passing the key: value to the decorator, when it hits line 244, the generated code is a case statement, which has both true or false flows. The false or nil case will never happen, since at compilation time its known by the Dialyzer that the code will always get the unquote(key_var) value. We have below a IR of the line 244. This also happens if we don't use a key: when calling the decorator.

__@4 = case {fetch, _param@1}
               of
             __@2 when __@2 =:= false orelse __@2 =:= nil ->
                 erlang:phash2({'Elixir.STRIntegrationEngine.REST',
                                fetch});
             __@3 -> __@3
           end,

This also happens, later in the quote, when line 260-264 are compiled to (and I'm assuming that is the same on lines 290-293):

case erlang:apply(__@5, [__@11]) of
            __@12 when __@12 =:= false orelse __@12 =:= nil ->
                __@11;
            _ -> __@1:set(__@2, __@11, __@4)
          end;

On both cases the problem is the same (as mentioned above). On this case, the default match function will always evaluate to true, so the Dialyzer will bitch about it. This will not be true if we create our own matching functions and pass them with variables (this will prevent that at compilation time the Dialyzer figures out useless control flow code). But since this is an optional functionality, it shouldn't work as an essential one.

PS: You can look at the IR by running inside a iex shell:

f = '<PATH_TO_BEAM>'
result = :beam_lib.chunks(f,[:abstract_code])
{:ok,{_,[{:abstract_code,{_,ac}}]}} = result
IO.puts :erl_prettypr.format(:erl_syntax.form_list(ac))

The proposed solution

My proposed solution is to create a way that those IR will not have useless code when using the default values of match, or simply by using or not the key. To do this I replaced the usage of Kernel.|| to Keyword.get/3, but just enhancing the already used Keyword.get on line 231. With this, at compile time, the generated IR code is now:

__@2 = {fetch, _param@1}

Which has no unused control flow code. The second fix was to replace the if-else statement with a cond statement. This will be a little trickier, since credo has a bad temper with only one clause on a cond. With this replacement, now the second IR code is:

 case erlang:apply(__@9, [__@15]) of
            __@16 when __@16 /= nil andalso __@16 /= false ->
                __@1:set(__@4, __@15, __@6);
            _ -> erlang:error(cond_clause)
          end

Which also has comparisons, but it does compare equality with nil or false, but rather the opposite. I created a PR with this solution, since it was rather fast to code.

Waiting for reviews. Cheers!

[cabol]

@filipeherculano first of all, thanks a lot for this great contribution 😄 !! I've replied to your PR, looks very good overall, just let you a couple of silly comments and also run the CI tests before, just to ensure the CI runs without errors.

Thanks 😉 !!

[filipeherculano]

Could you bump or commit a tag to v1.2.2 @cabol so I can incorporate the change safely on my project? Thanks again!

[cabol]

Just published version bump 1.2.2 🚀