safwank/ElixirRetry

Retry causing dialyzer warnings in application code

Closed this issue · 5 comments

seddy commented

We've hit an issue when trying to run dialyxir whilst using Retry.retry. This can be demonstrated with a fairly simple example.

I ran this by creating mix new, then putting the following in my mix.exs:

# mix.exs
...
  defp deps do
    [
      {:dialyxir, ">= 0.0.0", runtime: false},
      {:retry, ">= 0.0.0"}
    ]
  end

With the following sample code:

# lib/retry_poc.ex
defmodule RetryPoc do
  use Retry

  def foo do
    retry with: lin_backoff(1, 1) |> Stream.take(2) do
      {:ok, :bar}
    end
  end
end

When running mix dialyze, I see the following:

18:05:20@/tmp/retry_poc (master *%?):> mix dialyzer
Compiling 1 file (.ex)
Checking PLT...
[:compiler, :elixir, :kernel, :logger, :retry, :stdlib]
PLT is up to date!
Starting Dialyzer
dialyzer args: [
  check_plt: false,
  init_plt: '/tmp/retry_poc/_build/dev/dialyxir_erlang-20.1_elixir-1.6.3_deps-dev.plt',
  files_rec: ['/tmp/retry_poc/_build/dev/lib/retry_poc/ebin'],
  warnings: [:unknown]
]
done in 0m1.34s
lib/retry_poc.ex:5: The pattern _@1 = {'error', _} can never match the type {'ok','bar'}
lib/retry_poc.ex:5: The pattern _@2 = 'error' can never match the type {'ok','bar'}

This example is fairly trivial, but it caused a load of investigation for us earlier where we had function which returned {:ok, data} | {:error, error} but didn't return :error. Consequently we were getting an the fairly obscure error above. We ultimately tracked it down to this bit of code:

          case unquote(block) do
            {:error, _} = result -> {:cont, result}
            :error = result      -> {:cont, result}
            result               -> {:halt, result}
          end

It looks like dialyxir is essentially parsing the block_runner case statement and then complaining because two of the patterns are never matched. Could this potentially be resolved by moving the case section to a function instead of having it in a quote (though there may be very good reason not to do that)? I think so but unfortunately elixir metaprogramming is not my strong point...

It seems strange that library code should cause a dialyzer error like this. Would it be possible to change the implementation somehow so that it doesn't cause this please? Happy to help with that of course, I'm just not sure where to start 😄

I have the same issue

@seddy Apologies for the delay in getting back to you.

This is a tricky one. On the one hand, it's understandably annoying to get these warnings from a dependency. On the other, the "offending" code is perfectly idiomatic as far as I'm concerned. Also, I'm not sure if moving the case clause to to a function will fix the problem. Even if it does, the resulting code will most likely be very verbose and unreadable.

I'm going to reach out to the wider Elixir community for their opinion on this issue. Feel free to do the same. In the meantime, you can ignore the warnings as documented here so you can continue using Dialyzer in your project.

Sorry to say but this is a show stopper for us. We simply can't silence the error the way you describe and trust the output of dialyzer. Your code may be idiomatic but not pragmatic. I have to remove this dependency from our codebase until the issue is resolved. Which is sad because I like the functionality.

@edescourtis That's unfortunate. Have you tried the latest version to see if it solves this issue? If not, you're more than welcome to submit a PR. If you're planning to do so, I can reopen it.

Thanks, I might do that although it might be a little bit. I need to finish some other things first.