Bring shadowed variables warnings in functions back #84

Closed
josevalim opened this Issue Jan 1, 2012 · 14 comments

Comments

Projects
None yet
3 participants
@josevalim
Owner

josevalim commented Jan 1, 2012

No description provided.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Jan 5, 2012

Owner

Today, if you do:

x = 1
fn(x) { x = 2 }

Elixir will simply work fine, but it should raise a warning the x variable is being shadowed.

Owner

josevalim commented Jan 5, 2012

Today, if you do:

x = 1
fn(x) { x = 2 }

Elixir will simply work fine, but it should raise a warning the x variable is being shadowed.

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Jun 5, 2012

Contributor

+1

Contributor

yrashk commented Jun 5, 2012

+1

@alco

This comment has been minimized.

Show comment Hide comment
@alco

alco Jun 13, 2012

Member

Sorry for a slight off-topic, but there can be another issue here.

x = 1
y = fn(x) -> x = 2 end
y.(1)  #=> 2
y.(2)  #=> 2
y.(3)  #=> 2

This is a side-effect of the ability to reassign a variable, however is it a desired one? It's a perfectly correct piece of code, but it can be unclear at first what is going on. Maybe we could benefit from another warning about reassigning function params?

Member

alco commented Jun 13, 2012

Sorry for a slight off-topic, but there can be another issue here.

x = 1
y = fn(x) -> x = 2 end
y.(1)  #=> 2
y.(2)  #=> 2
y.(3)  #=> 2

This is a side-effect of the ability to reassign a variable, however is it a desired one? It's a perfectly correct piece of code, but it can be unclear at first what is going on. Maybe we could benefit from another warning about reassigning function params?

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Oct 14, 2012

Contributor

so, what's the status of this. should we do it?

Contributor

yrashk commented Oct 14, 2012

so, what's the status of this. should we do it?

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Oct 14, 2012

Owner

@alco That's what this pull request is about. You are shadowing the variable x, we should warn.

@yrashk yes.

Owner

josevalim commented Oct 14, 2012

@alco That's what this pull request is about. You are shadowing the variable x, we should warn.

@yrashk yes.

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Oct 27, 2012

Contributor

what will it take to implement this? where to start?

Contributor

yrashk commented Oct 27, 2012

what will it take to implement this? where to start?

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Nov 3, 2012

Contributor

Looks like this issue is getting old. @josevalim can you give me pointers as to where to start on this and I might be able to take a shot at this?

Contributor

yrashk commented Nov 3, 2012

Looks like this issue is getting old. @josevalim can you give me pointers as to where to start on this and I might be able to take a shot at this?

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 3, 2012

Owner

A good place to start is here:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_translator.erl#L458

Notice we simply consider a function clause to be a normal assignment, which, by Elixir standards, will reassign a new variable. So when you do:

a = 1
fn(a) -> a end

Elixir transforms it to:

a = 1
fn(a@1) -> a@1 end

We need a way to tell Elixir to not generate a new variable... it could simply reuse the previous one. This case is handled by this clause:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_scope.erl#L30

Basically it says that, if we have already a version of a, create a new one. We basically need to have a new #elixir_scope key that tells it to not create a new one in case one exists, that reusing the previous one will be fine and set it from translate_fn.

Owner

josevalim commented Nov 3, 2012

A good place to start is here:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_translator.erl#L458

Notice we simply consider a function clause to be a normal assignment, which, by Elixir standards, will reassign a new variable. So when you do:

a = 1
fn(a) -> a end

Elixir transforms it to:

a = 1
fn(a@1) -> a@1 end

We need a way to tell Elixir to not generate a new variable... it could simply reuse the previous one. This case is handled by this clause:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_scope.erl#L30

Basically it says that, if we have already a version of a, create a new one. We basically need to have a new #elixir_scope key that tells it to not create a new one in case one exists, that reusing the previous one will be fine and set it from translate_fn.

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Nov 3, 2012

Contributor

A good question here is whether we should change this. May be it is good
that we allow shadowing? Or would you rather match Erlang's semantics there?

On Fri, Nov 2, 2012 at 5:25 PM, José Valim notifications@github.com wrote:

A good place to start is here:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_translator.erl#L458

Notice we simply consider a function clause to be a normal assignment,
which, by Elixir standards, will reassign a new variable. So when you do:

a = 1
fn(a) -> a end

Elixir transforms it to:

a = 1
fn(a@1) -> a@1 end

We need a way to tell Elixir to not generate a new variable... it could
simply reuse the previous one. This case is handled by this clause:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_scope.erl#L30

Basically it says that, if we have already a version of a, create a new
one. We basically need to have a new #elixir_scope key that tells it to *
not* create a new one in case one exists, that reusing the previous one
will be fine and set it from translate_fn.


Reply to this email directly or view it on GitHubhttps://github.com/elixir-lang/elixir/issues/84#issuecomment-10034324.

Contributor

yrashk commented Nov 3, 2012

A good question here is whether we should change this. May be it is good
that we allow shadowing? Or would you rather match Erlang's semantics there?

On Fri, Nov 2, 2012 at 5:25 PM, José Valim notifications@github.com wrote:

A good place to start is here:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_translator.erl#L458

Notice we simply consider a function clause to be a normal assignment,
which, by Elixir standards, will reassign a new variable. So when you do:

a = 1
fn(a) -> a end

Elixir transforms it to:

a = 1
fn(a@1) -> a@1 end

We need a way to tell Elixir to not generate a new variable... it could
simply reuse the previous one. This case is handled by this clause:

https://github.com/elixir-lang/elixir/blob/master/lib/elixir/src/elixir_scope.erl#L30

Basically it says that, if we have already a version of a, create a new
one. We basically need to have a new #elixir_scope key that tells it to *
not* create a new one in case one exists, that reusing the previous one
will be fine and set it from translate_fn.


Reply to this email directly or view it on GitHubhttps://github.com/elixir-lang/elixir/issues/84#issuecomment-10034324.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 3, 2012

Owner

Maybe we are fine with the current behavior. I believe Erlang warns you because you may think it is a match. For example:

A = 1,
fun(A) -> A + 1 end.

You could mistakenly think that you are matching by A in the function head (and therefore the function would only work when A is equal to one) but that is not actually what is happening. So it is a way of erlang to let you know that you are shadowing a variable.

In our case, since the default is to re-assign the variable anyway, we could argue that it is actually ok. In fact, we could trigger the erlang warning today simply by adding ^a:

a = 1
fn(^a) -> a + 1 end

However, other languages have semantics similar to Elixir but they still warn you.

Owner

josevalim commented Nov 3, 2012

Maybe we are fine with the current behavior. I believe Erlang warns you because you may think it is a match. For example:

A = 1,
fun(A) -> A + 1 end.

You could mistakenly think that you are matching by A in the function head (and therefore the function would only work when A is equal to one) but that is not actually what is happening. So it is a way of erlang to let you know that you are shadowing a variable.

In our case, since the default is to re-assign the variable anyway, we could argue that it is actually ok. In fact, we could trigger the erlang warning today simply by adding ^a:

a = 1
fn(^a) -> a + 1 end

However, other languages have semantics similar to Elixir but they still warn you.

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Nov 3, 2012

Contributor

I like that you can do

fn(^a) -> a + 1 end

except that it dodnt work for me :)

iex(1)> a=1
1
iex(2)> fn(^a) -> a + 1 end
#Fun<erl_eval.6.82930912>

On Fri, Nov 2, 2012 at 5:58 PM, José Valim notifications@github.com wrote:

Maybe we are fine with the current behavior. I believe Erlang warns you
because you may think it is a match. For example:

A = 1,
fun(A) -> A + 1 end.

You could mistakenly think that you are matching by A in the function head
(and therefore the function would only work when A is equal to one) but
that is not actually what is happening. So it is a way of erlang to let you
know that you are shadowing a variable.

In our case, since the default is to re-assign the variable anyway, we
could argue that it is actually ok. In fact, we could trigger the erlang
warning today simply by adding ^a:

a = 1
fn(^a) -> a + 1 end

However, other languages have semantics similar to Elixir but they still
warn you.


Reply to this email directly or view it on GitHubhttps://github.com/elixir-lang/elixir/issues/84#issuecomment-10034781.

Contributor

yrashk commented Nov 3, 2012

I like that you can do

fn(^a) -> a + 1 end

except that it dodnt work for me :)

iex(1)> a=1
1
iex(2)> fn(^a) -> a + 1 end
#Fun<erl_eval.6.82930912>

On Fri, Nov 2, 2012 at 5:58 PM, José Valim notifications@github.com wrote:

Maybe we are fine with the current behavior. I believe Erlang warns you
because you may think it is a match. For example:

A = 1,
fun(A) -> A + 1 end.

You could mistakenly think that you are matching by A in the function head
(and therefore the function would only work when A is equal to one) but
that is not actually what is happening. So it is a way of erlang to let you
know that you are shadowing a variable.

In our case, since the default is to re-assign the variable anyway, we
could argue that it is actually ok. In fact, we could trigger the erlang
warning today simply by adding ^a:

a = 1
fn(^a) -> a + 1 end

However, other languages have semantics similar to Elixir but they still
warn you.


Reply to this email directly or view it on GitHubhttps://github.com/elixir-lang/elixir/issues/84#issuecomment-10034781.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 3, 2012

Owner

Yup, it doesn't work at all. It will give you a warning about shadowing
(except in iex, because we are in eval mode, but put that into a file and
you will see it). :)

Owner

josevalim commented Nov 3, 2012

Yup, it doesn't work at all. It will give you a warning about shadowing
(except in iex, because we are in eval mode, but put that into a file and
you will see it). :)

@yrashk

This comment has been minimized.

Show comment Hide comment
@yrashk

yrashk Nov 3, 2012

Contributor

So, what should we do, close the issue or enable the warning?

On Fri, Nov 2, 2012 at 6:02 PM, José Valim notifications@github.com wrote:

Yup, it doesn't work at all. It will give you a warning about shadowing
(except in iex, because we are in eval mode, but put that into a file and
you will see it). :)


Reply to this email directly or view it on GitHubhttps://github.com/elixir-lang/elixir/issues/84#issuecomment-10034830.

Contributor

yrashk commented Nov 3, 2012

So, what should we do, close the issue or enable the warning?

On Fri, Nov 2, 2012 at 6:02 PM, José Valim notifications@github.com wrote:

Yup, it doesn't work at all. It will give you a warning about shadowing
(except in iex, because we are in eval mode, but put that into a file and
you will see it). :)


Reply to this email directly or view it on GitHubhttps://github.com/elixir-lang/elixir/issues/84#issuecomment-10034830.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 3, 2012

Owner

I have decided to close the issue. Reassigning the variable is the expected behavior and the scope does not leak to outside. The warning is still triggered today if you mistakenly use ^a in functions and comprehensions.

Owner

josevalim commented Nov 3, 2012

I have decided to close the issue. Reassigning the variable is the expected behavior and the scope does not leak to outside. The warning is still triggered today if you mistakenly use ^a in functions and comprehensions.

@josevalim josevalim closed this Nov 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment