`with` expression support and clause reachability

[eksperimental]

This is a reduced case of The clause on line X cannot be reached, when calling with/1

defmodule Unreacheable do
  @spec foo(term) :: pos_integer()
  def foo(bar) do
    with _value <- bar do
      1
    else
      _ ->
        2
    end
  end
end

will generate:

lib/debug_gradient/unreacheable.ex: The clause on line 4 cannot be reached

[erszcz]

Hmm, it seems we're not handling the code generated for the with statement properly. This needs some deeper research on our end.

[eksperimental]

Removing the spec or replacing it with @spec foo(term) :: pos_integer() emits no warning.

[erszcz]

Ok, here we have some findings:

    with _value <- bar do
      1
    else
      _ ->
        2
    end

compiles to

    case _bar@1 of
        __value@1 -> 1;
        _ -> 2
    end.

which shows what's already visible in the original example - _value matches everything, so the latter clause actually is unreachable. In other words, this is a valid warning.

Just to make sure, I also checked the following:

    with {:ok, 3} <- bar do
      1
    else
      _ ->
        2
    end

    case _bar@1 of
        {ok, 3} -> 1;
        _ -> 2
    end.

which, as expected, doesn't generate the warning.

@eksperimental thanks for the report! In this case we're doing the right thing, though :)

[eksperimental]

Maybe I oversimplified the example. I will try to analyze the real case I had and get back with more info. Thank you @erszcz

[eksperimental]

Here's an updated example that fails

defmodule Unreacheable do
  @spec foo(term) :: {:ok, term} | :error
  def foo(bar) do
    with pid when is_pid(pid) <- pid_or_nil(bar),
         build <- build(bar) do
      {:ok, build}
    else
      _ ->
        :error
    end
  end

  @spec pid_or_nil(term) :: pid | nil
  def pid_or_nil(_term) do
    case Enum.random(1..2) do
      1 -> nil
      2 -> self()
    end
  end

  @spec build(key) :: {:build, key} when key: term
  def build(key) do
    {:build, key}
  end
end

[eksperimental]

This one is even more simplified.

defmodule Unreacheable do
  require Integer

  @spec the_odds(term) :: {:even | :odd, pos_integer()}
  def the_odds(bar) do
    with integer when Integer.is_odd(integer) <- Enum.random(1..2),
         build <- identity(bar) do
      {:odd, build}
    else
      integer ->
        {:even, integer}
    end
  end

  def identity(key), do: key
end

[eksperimental]

Converting the latter example to generated Erlang code, and back to Elixir, it looks like this.

defmodule UnreacheableExpanded do
  require Integer
  import Bitwise

  @spec the_odds(term) :: {:even | :odd, pos_integer()}
  def the_odds(bar) do
    case Enum.random(1..2) do
      integer2 when is_integer(integer2) and band(integer2, 1) == 1 ->
        case identity(bar) do
          build ->
            {:odd, build}

          integer1 ->
            {:even, integer1}
        end

      integer1 ->
        {:even, integer1}
    end
  end

  def identity(key), do: key
end

which generates the following warning on compilation

warning: this clause cannot match because a previous clause at line 10 always matches lib/debug_gradient/unreacheable_expanded.ex:13

Which is correct. Then Gradualizer is correct.

I wonder if this is considered a bad practice in Elixir. (Ab)using with to define variables.

[eksperimental]

The warning will be gone when Elixir supports OTP25+ exclusively as it's planned to port with to use the recently introduced maybe in Erlang. https://www.erlang.org/doc/reference_manual/expressions.html#maybe

[xxdavid]

@eksperimental I've tried both of the examples (1, 2) and it seems Gradualizer no longer reports an error (I'm not sure why, though).

I wonder if this is considered a bad practice in Elixir. (Ab)using with to define variables.

I think you should use <- only when you want the with behaviour in case the pattern doesn't match. If you just bind a variable (without the pin operator), it will always match and thus there is no reason for using <- and you can just use =. Like so:

    with pid when is_pid(pid) <- pid_or_nil(bar),
         build = build(bar) do
      {:ok, build}
    else
      _ ->
        :error
    end

But because it's the last clause, it also makes more sense to it move in the body. We even get a Credo warning for this:

with doesn't end with a <- clause, move the non-pattern <- clauses inside the body of the with

Which gets us here:

    with pid when is_pid(pid) <- pid_or_nil(bar) do
      build = build(bar)
      {:ok, build}
    else
      _ ->
        :error
    end

I think this version is a bit more idiomatic than the original one.