ExUnit assert swallowing unused variable warning

[novaugust]

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.2.2] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Elixir 1.18.2 (compiled with Erlang/OTP 27)

Operating system

macos

Current behavior

Some refactoring surfaced an unused variable warning that should have also been present in the original code. Shrinking it down, it seems assert is swallowing unused variable warnings when the assignment is happening deep in.

all of these tests have an unused variable x, but the first does not warn about it.

  test "missing warning" do
    assert Enum.member?(1..100, x = 1)
  end

  test "correctly warns without assert" do
    Enum.member?(1..100, x = 1)
  end

  test "correctly warns with assert in" do
    assert (x = 1) in 1..100
  end

output:

    warning: variable "x" is unused (if the variable is not meant to be used, prefix it with an underscore)
    │
 xx │     Enum.member?(1..100, x = 1)
    │                          ~

    warning: variable "x" is unused (if the variable is not meant to be used, prefix it with an underscore)
    │
 xx │     assert (x = 1) in 1..100
    │             ~

Expected behavior

warn for all unused variables =)

[josevalim]

Do you have capture_io(:stderr somewhere?

[josevalim]

Oh ignore me, I am barking at the wrong tree. This is something about ExUnit capturing the variables for internal reports... assert as a macro is using the variables... I am not sure if we can fix it. :(

[novaugust]

it's not a big stressor. i was just surprised when a refactor that should've been 1-1 code generated warnings, and then realized the warnings should've (ideally!) been there all along. but not like unused variables are breaking anything except prettiness (by the time the test's committed, anyways)

[josevalim]

I will sleep on it but most likely it cannot be fixed.

[sabiwara]

@josevalim Actually it seems to be the generated clause here: https://github.com/elixir-lang/elixir/blob/main/lib/ex_unit/lib/ex_unit/assertions.ex#L179. Once removed, it warns.

The expanded code is fine and x wouldn't be considered as used.

case value =
       (
         {arg1, arg2} = {1..100, x = 1}
         Enum.member?(arg1, arg2)
       ) do
  x when Kernel.in(x, [false, nil]) ->
    raise ExUnit.AssertionError,
      args: [arg1, arg2],
      expr:
        {:assert, [],
         [
           {{:., [], [{:__aliases__, [], [:Enum]}, :member?]}, [],
            [{:.., [], [1, 100]}, {:=, [], [{:x, [], Elixir}, 1]}]}
         ]},
      message: "Expected truthy, got #{inspect(value)}"

  _ ->
    value
end

Perhaps we could improve macro expansion so that the propagation of generated stops at unquotes instead? Not sure if that's feasible but that would prevent similar issues.

Right now an if everything within a generated if is marked as such:

This might be intentional? 7a90dbf

[sabiwara]

Found a fix in the meantime that doesn't need it: #14677.

@novaugust sorry for asking, just curious. Why would you write it with Enum.member? rather than in, given in can generate better assertion errors in the case of tests, and sometimes more efficient code in the case of source code? My personal advice would be stick to using in as much as possible. (also more readable IMHO, but that's subjective)

[novaugust]

Why would you write it with Enum.member? rather than in, given in can generate better assertion errors in the case of tests

Personally, I wouldn't! We're on the same team -- you've misread which way the translation went hehe. Styler 1.6 does a bunch of assert rewrites that surfaced this. When it went from member to in i got the warnings and said "wha? how did styler introduce unused vars?! i must have a serious bug in that rewrite"

[sabiwara]

you've misread which way the translation went hehe

😌 Nice!
Actually the reason behind my question was a worry that Styler would do it the other way ^^

[novaugust]

yeah i could read the sadness and disappointment in me you were trying to mask 😂 "to think i thought he knew what he was doing..."

anyways thanks + neat job on finding a (the?) fix. i'm sure jose'll be psyched to see that in the morning