Elixir: Imperative Assignements are breaking the application in 1.7 update

Created on 13 Aug 2018  ·  14Comments  ·  Source: elixir-lang/elixir

Environment

  • Elixir & Erlang/OTP versions (elixir --version): Erlang/OTP 21 [erts-10.0] [ source] [64-bit]
    Elixir 1.7.0 (Compiled with Erlang/OTP 20)
  • Operating system: Ubuntu 16.04

Current behavior

After updated Elixir from 1.6 to 1.7 the params variable will always be equal to %{} outside the if statements

params = %{}

        if Map.has_key?(entities, "email") do
          params = Map.put_new(params, :email, get_values(entities["email"]))
        end

Expected behavior

The params variable should be correctly assigned for now and updating to 1.7 shouldn't break such a code since nothing has been mentioned in the release not of 1.7

Most helpful comment

Duplicate of #7403.

Rationale is here:
https://github.com/elixir-lang/elixir/issues/7403#issuecomment-368902698

We should have a CHANGELOG entry too. But I will be glad to clarify it.

José Valim
www.plataformatec.com.br
Skype: jv.ptec
Founder and Director of R&D

All 14 comments

I think that code will emit a warning similar to:

warning: variable "params" is unused

Note variables defined inside case, cond, fn, if and similar do not leak. If you want to conditionally override an existing variable "params", you will have to explicitly return the variable. For example:

    if some_condition? do
      atom = :one
    else
      atom = :two
    end

should be written as

    atom =
      if some_condition? do
        :one
      else
        :two
      end

Unused variable found at:
  your_file.ex:42

Actually no @fertapric since the variable is defined outside the if statement.

I can confirm the behaviour changed between 1.6 and 1.7:

~/foo% asdf local elixir 1.6.5 && elixir -e "x = 1; if(true, do: x = 2); IO.inspect x" 2> /dev/null
2
~/foo% asdf local elixir 1.7.0 && elixir -e "x = 1; if(true, do: x = 2); IO.inspect x" 2> /dev/null
1

🤔 That's odd!

Before posting, I checked the warning was there using this small example:

params = %{}

if true do
  params = Map.put_new(params, :email, 1)
end

IO.inspect params

The warning was emitted for the assignment in the line 4 :)

@fertapric I got the warning in both cases as expected, I removed it with | 2> /dev/null for brevity.

Duplicate of #7403.

Rationale is here:
https://github.com/elixir-lang/elixir/issues/7403#issuecomment-368902698

We should have a CHANGELOG entry too. But I will be glad to clarify it.

José Valim
www.plataformatec.com.br
Skype: jv.ptec
Founder and Director of R&D

Thanks for the clarification @josevalim indeed updating the changelog would have saved us (and probably others) some precious time searching for the reason behind these errors.

Julien, sorry for not being clear. I meant that this is already in the
CHANGELOG but apparently not clear enough. Could you please give it a
second pass and suggest how should we change the CHANGELOG to make this

clearer?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@josevalim I'll be glad to suggest but after I've made a CTRL+F of the following terms:

I couldn't find anything relevant.
If a line is there, can you please point me to it?

@jfayad this line:

[Kernel] Raise on unsafe variables in order to allow us to better track unused variables

I think adding “also known as imperative assignment” to that entry could be

enough. Thoughts?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

I would definitely add the key expression Imperative assignment. So adding to the end of the sentence "and stop leaking Imperative assignment variables to the upper context"

Updated, thanks everyone for the feedback!

This breaking change on the language led to this erratum on Programming Elixir 1.6 book by Dave Thomas.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

josevalim picture josevalim  ·  3Comments

GianFF picture GianFF  ·  3Comments

vothane picture vothane  ·  3Comments

cmeiklejohn picture cmeiklejohn  ·  3Comments

alexrp picture alexrp  ·  4Comments