New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiple dialyzer warnings with OTP 19 #1872
Comments
Erlang 19 had a regression and it was reporting warnings in cases it should not. It was fixed in OTP's maint branch but it was not released yet: erlang/otp@784f845 It should fix all of the cases for the variable |
Thanks @josevalim, issues filed. What about:
|
Dialyzer bug as well. |
Ah, same one or different one? Is that one filed somewhere? Thanks! |
Same. This is the one: erlang/otp@784f845 |
@josevalim sorry, another clarification, is It seems like dialyzer is seeing |
@josevalim @chrismccord would it be possible to reopen this with the idea that we can suppress these warnings in phoenix temporarily? It'd be great if we had a clean dialyzer bill of health out of the gate. As it stands, some things are impossible for user-land to suppress AFAICT since they are in generated modules. |
@aaronjensen we should not suppress warnings because that just leads to the situation we will keep on suppressing warnings while we should fix them. The static_url is also due to the dialyzer bug. |
@josevalim I'm not sure I understand that reasoning. Right now it's impossible to get a clean dialyzer run on phoenix w/ erlang 19. That means that we've not been running dialyzer because we haven't set up a hack like https://github.com/basho/riak/blob/develop/Makefile#L161-L162 yet. Allowing end-users to run dialyzer cleanly seems orthogonal to a project's commitment to fixing warnings that are temporarily suppressed. An issue could be left open to track the suppressed warnings. In the meantime, we can be encouraging dialyzer usage. I would love to see dialyzer usage be the default, not the exception. Heck maybe one day it can be included in |
Except that's not what happens in practice. Once we suppress some warnings, we will be giving us an implicit excuse to suppress warnings instead of fixing them in the future. A bit like in the Broken Windows theory. Plus, if we suppress warnings, we will be unable to know further offences since warnings won't be reported. At least I believe Erlang 19.1 will be coming out soon. If you prefer, you can fork the latest stable Phoenix release you are running on and add the annotations directly. |
You'd know better than I. My sense is that this is totally up to the project maintainer(s) to enforce or not, to be diligent or not.
You can be pretty specific about the suppression: @dialyzer({:no_match, phoenix_controller_pipeline: 2})
Yeah, this is what I plan on doing for now. I'll post a link here when I'm done in case anyone else wants clean dialyzer for now. |
❤️ 💚 💙 💛 💜 |
This was enough (after fixing other dependency issues) to get dialyzer working with OTP 19 and Elixir 1.3.2 https://github.com/aaronjensen/phoenix/tree/suppress-dialyzer-warnings Add this to your # Based on 1.2.1
# Should be able to upgrade after Erlang 19.1 and *maybe* Elixir 1.4 too
# https://github.com/phoenixframework/phoenix/issues/1872
{:phoenix,
github: "aaronjensen/phoenix",
ref: "suppress-dialyzer-warnings",
override: true},
# Based on 2.0.5
# https://github.com/elixir-ecto/ecto/pull/1689
{:ecto, github: "aaronjensen/ecto", override: true}, Also add these to your
|
@aaronjensen beautiful. We should have a new Ecto release relatively soon too. |
@josevalim fyi, an update, these are the errors w/ OTP 19.1, Elixir 1.3.3, phoenix 1.2.1, and ecto 2.1.0-rc.0:
|
@aaronjensen given the reports, I am thinking they are all coming from here: https://github.com/elixir-lang/plug/blob/master/lib/plug/builder.ex#L225-L226 Can you fork plug (or change plug in your deps/plug) to remove that line, then run |
@josevalim that's the line that causes the errors. I can only think of a few ways to not warn on that clause:
|
@aaronjensen let me try something out and I should have a code snippet for you to try out soon. Thank you for the help! ❤️ |
@aaronjensen can you please try replacing the whole defp quote_plug({plug_type, plug, opts, guards}, acc, env, builder_opts) do
call = quote_plug_call(plug_type, plug, opts)
error_message = case plug_type do
:module -> "expected #{inspect plug}.call/2 to return a Plug.Conn"
:function -> "expected #{plug}/2 to return a Plug.Conn"
end <> ", all plugs must receive a connection (conn) and return a connection"
{fun, meta, [arg, [do: clauses]]} =
quote do
case unquote(compile_guards(call, guards)) do
%Plug.Conn{halted: true} = conn ->
unquote(log_halt(plug_type, plug, env, builder_opts))
conn
%Plug.Conn{} = conn ->
unquote(acc)
_ ->
raise unquote(error_message)
end
end
clauses =
Enum.map(clauses, fn {:->, meta, args} ->
{:->, [generated: true] ++ Keyword.put(meta, :line, -1), args}
end)
{fun, meta, [arg, [do: clauses]]}
end and then please let me know if it works as expected. :) |
@josevalim awesome I knew there was a 4. 😄 That worked, now the only warnings left on https://github.com/aaronjensen/dialyzer_repro are gettext:
|
Yay! 🎉 I am closing this issue then because we already have an open one for the gettext warnings: elixir-gettext/gettext#115 ❤️ 💚 💙 💛 💜 |
So to summarize the solution here, either:
Correct? |
@novaugust yes. erlang 19.1 is out and ecto 2.1 already has a rc out. gettext is not fixed yet i believe. |
@josevalim I think the fix you linked is in 19.1.1 (tagged 7 days ago), not just 19.1 (15 days ago). I still see the |
Brand new phoenix app created with
mix phoenix.new
on OTP 19 has these warnings.Here's a repro app: https://github.com/aaronjensen/dialyzer_repro
Some of these may be plug or ecto, so let me know if you'd like me to open issues in the respective places.
The text was updated successfully, but these errors were encountered: