Why does the compiler throw the warning "the variable is unsafe"?

Consider this code:

def test_function(a) do
  if (b = a) && (c = b), do: c
end

Here, the compiler will throw the warning “warning: the variable “c” is unsafe as it has been set inside one of: case, cond, receive, if, and, or, &&, || …”

In my opinion, this is annoying and should be removed, because - to my understanding - the variable “c” is not unsafe. Is there any reason to keep the compiler-warning in this case?

It is unsafe, since its scope extends far beyond the if expression.

If now a is false and you use c after the if, it will never have been set, because && shortcuts.

But in this case, c is not used after the if-clause.
For example, the compiler will not throw a warning for this code:

if false, do: c = 1

It will only show a warning if c is used afterwards.

Don’t you want == instead of = ? You aren’t actually testing anything…

For example, I use this pattern to check if certain data is available and only execute logic inside an if statement if this is the case, e. g.:

if (a = Map.get(m1, :optional_key1)) && (b = Map.get(m2, :optional_key2)) do
  execute_logic(a, b)
else
  do_something_else()
end

This is perfect for a pattern match:

case {m1, m2} do
  {%{optional_key1: a}, %{optional_key2: b}} when !!a and !!b -> execute_logic(a, b)
  _ -> do_something_else()
end

Or even pull out a and b in the functions head.

PS: My version is semantically the same as yours and will do_something_else/0 when there was a falsey value (nil or false) in one of the maps. I’m not sure if that was really your intend.

1 Like

Hi NobbZ, thank you for this suggestion. This is good alternative to the code I mentioned. Another way to achieve the result is to use the with-statement, which also allows for using the matched variables as an input to other functions. However, sometimes I like the if-statement, because it is a bit shorter. Let’s extend the example and say that the map-keys don’t contain boolean values:

if (a = Map.get(m1, :optional_key1)) 
&& (b = Map.get(m2, :optional_key2))
&& (c = execute_logic1(a, b)) do
  execute_logic2(c)
else
  do_something_else()
end

When using the with-statement:

with  a when a <- Map.get(m1, :optional_key1),
      b when b <- Map.get(m2, :optional_key2),
      c when c <- execute_logic1(a, b)
do
  execute_logic2(c)
else
  _ -> do_something_else()
end

So I have to use your suggestion or the with-statement to avoid the compiler-warning, although the if-statement is also correct. This is what I don’t like about the warning.

My code does not assume boolean values at all. Your with will even fail when those values are not Bool, this is why I had the double-bang in my case (!!)!

And to be honest, the with is much more readable than the if. In the with it is clearly visible which variable is bound when and why. In your if clase it is easy to loose track about that, especially when you start to add more and more subclauses.

Thanks for the correction, I adjusted the statment to check for nil and false, to match the semantics of the if-statement:

    with  a when not(a in [nil, false]) <- Map.get(m1, :optional_key1),
          b when not(b in [nil, false]) <- Map.get(m2, :optional_key2),
          c when not(c in [nil, false]) <- execute_logic1(a, b)
    do
      execute_logic2(c)
    else
      _ -> do_something_else()
    end

The compiler said it is not possible to use the !!-operator as a guard, because the underlying macro uses a case-statment.
I now share your opinion that the with- and case-clause is more readable.

Oh… I’ve never actually used that one, it was just from the darkest areas of my mind… Perhaps some old rubyism that came up again? Sorry for that…

Just an observation:

Maybe it’s just “word choice” but it’s important to remember that if, case, and with are designed to return the value of the expression they ultimately evaluate, i.e. they are expressions themselves (if returns nil when there is no else). But even the names you choose like execute_logic and do_something_else seem to suggest a “flow of control” programming style that “operates on data in place” rather than the functional programming style of “data flowing though program logic”.

While it’s OK to occasionally ignore a returned value because you’re primarily interested in the side effect rather than the result, persisting in a “statement-based” programming style in a functional language is going to be more prone to producing “annoying warning messages”.

3 Likes

I agree with @peerreynders, and I’m no expert when it comes to functional programming (or anything in general for that matter), but I have found that the more I use elixir the less I want to use the regular imperative way of doing data flow.
I still get entangled in it sometimes, but I would say that elixir’s warnings most of the time will nudge me into a better solution.

What you’ve shown could be written as:

def execute_logic(%{optional_key1: opt_k1, optional_key2: opt_k2}), do: execute_logic2()
def execute_logic, do: something_else()

Which makes it trivial to add a new case, such as, imagine, now you want to do another thing if only optional key1 is present:
def execute_logic(%{optional_key1: opt_k1}), do: execute_for_key1(opt_k1)

Or say that you want to do something when k1 is a certain value, instead of yet another branch of logic and checks, you can:
def execute_logic(%{optional_key1: "special", optional_key2: opt_k2}), do: execute_logic3()

If you place these all together (in the correct order - funnily enough this pattern makes your code position in a file dictate the behaviour of your program):

def execute_logic(%{optional_key1: opt_k1, optional_key2: opt_k2}), do: execute_logic2(opt_k1, opt_k2)
def execute_logic(%{optional_key1: "special", optional_key2: opt_k2} = all_data), do: execute_logic3(all_data)
def execute_logic(%{optional_key1: opt_k1}), do: execute_for_key1(opt_k1)
def execute_logic(something), do: something_else(something)

You can see that not only does it take less lines (although you could reduce the ones in the sample - and also knowing the number of lines is not the ultimate measure either for simplicity or correctness), but it also takes care of 2 new other cases.

I think this is much more readable than anything you could come up with if else.
Not only that, the code is also slighty more optimized, because (if I understand correctly) erlang/elixir will be able to optimize the function calling when pattern matching on function heads (this is most likely not gonna impact the real usage of the program unless at scale, but still…).

It’s also more useful, because now you could call execute_logic from anywhere without duplicating the if-else logic on another function.
And lastly, it can be useful to create pipeable flows of data, because you now have no branching, if you design the functions correctly you can do execute_logic(something) |> process_logic_result

4 Likes

@amnu3387, @peerreynders: Thank you for your input, I did not expect to get such valuable feedback on my question about the compiler-warning :grinning: