Elixir: Extra compiler warnings in ExUnit assert_receive(timeout, msg) and assert_received(msg)

[gasparch]

Environment

Erlang/OTP 20 [erts-9.0] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.5.0
Ubuntu 16.04

Not a regression, because same was in previous versions as well

Current behavior

When running following test

defmodule Test do
  use ExUnit.Case

  test "the truth" do
    send self(), 123
    assert_receive(msg, 100, "got message")
    assert msg == 123
  end
end

extra warning messages are generated

> mix test test/elixir_test.exs
warning: this check/guard will always yield the same result
  test/learn_test.exs:7

warning: this clause cannot match because a previous clause at line 7 always matches
  test/learn_test.exs:7

Which are generated by line assert_receive(msg, 100, "got message")

Same applies to assert_received as well.

Expected behavior

Test should run without any warnings.

[gasparch]

Also

refute_receive(msg, 100, "got message")

this construct should do not compile at all, as it makes no sense. Either variable should be pinned or it should be _ in there.

[bschaeffer]

assert_receive is for testing that specific msg was received (same for refute_receive). This is a valid compiler warning to me because it's basically telling you the test is pointless, which I would want to know.

[gasparch]

Please come here for discussion - https://groups.google.com/forum/#!msg/elixir-lang-core/Av1XM8cCERE/ZvjCm4QXAAAJ

[gasparch]

What you say also have a point, so this is why I think having separate assert may help. But in other way there will be too much clutter.

But test is not pointless - it just tests something different - it tests that _some_ message was received in during specified time.

[gasparch]

Looking on documentation is is not 100% clear which way is right. Because msg is also pattern, which just happens to match everything.

May be to following may make sense. Then assert_receive should not accept anything besides pattern with pinned variable or '_' and always return received message.

msg = assert_receive(_, timeout, "message") 

This will break a lot of existing tests of course, if enforced.

[michalmuskala]

I agree that assert_receive works correclty for the current behaviour - it does not return the message, so matching on a variable should warn, since it is unused, to silence one should match on _.

For me this should not compile - the msg variable is not defined on the second line.

    assert_receive(msg, 100, "got message")
    assert msg == 123

[gasparch]

So what you say is that it basically leaks variable from macro context?

I would be totally happy if it returned received value.

[whatyouhide]

"leaking variable for macro context" is necessary for the semantics of assert_receive because many tests will do

assert_receive {:heres_a_keyword, kw}
assert Keyword.keyword?(kw)

and it's a valid use case.

[gasparch]

so then following this logic assert_receive msg is also valid one :)

It will be inconsistent to let variables leak if they are part of the pattern, but not when they are alone.

[jerel]

It looks like v1.5.2 was released without this fix, is it large enough that it needs to wait for a minor or could we get it in v1.5.3? All our instances of assert_receive that have a descriptive message are throwing warnings which end up hiding real warnings

[josevalim]

Backported to v1.5.