Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

always create new bindings for for loop iteration variables #22314

Closed
JeffBezanson opened this issue Jun 9, 2017 · 12 comments
Closed

always create new bindings for for loop iteration variables #22314

JeffBezanson opened this issue Jun 9, 2017 · 12 comments
Assignees
Labels
breaking This change will break code compiler:lowering Syntax lowering (compiler front end, 2nd stage) needs decision A decision on this change is needed
Milestone

Comments

@JeffBezanson
Copy link
Member

I thought there might be an issue for this already, but if so I can't find it.

We currently have this behavior:

julia> function f()
        i = 1
        for i = 2:3
        end
        i
       end
f (generic function with 1 method)

julia> f()
3

In other words, if there is already an i in scope, then for i will reuse it. Instead, the i in the loop should probably be a new variable, equivalent to:

for temp = 2:3
    let i = temp
        # loop body
    end
end
@JeffBezanson JeffBezanson added breaking This change will break code needs decision A decision on this change is needed compiler:lowering Syntax lowering (compiler front end, 2nd stage) labels Jun 9, 2017
@JeffBezanson JeffBezanson added this to the 1.0 milestone Jun 9, 2017
@JeffBezanson JeffBezanson self-assigned this Jun 9, 2017
@yuyichao
Copy link
Contributor

yuyichao commented Jun 9, 2017

That'll be good for #14948

@JeffBezanson
Copy link
Member Author

That issue is a bit different, since my proposal here will still allow writing to variables outside the loop. This would just make for i = imply let i =, but not affect any other variables.

@stevengj
Copy link
Member

stevengj commented Jun 9, 2017

The only argument against this change that I can think of (putting aside backwards-compatibility issues) is that sometimes it is nice to be able to get the last iteration of a for loop if you break from it early. But you could always assign to another variable in this case.

@thofma
Copy link
Contributor

thofma commented Jun 9, 2017

I wonder how many hours were spent debugging bugs because of this scoping rule. On the other hand, once one knows of this "feature", one usually exploits it in the way @stevengj described it.

I am not sure if it is related, but when thinking of list comprehensions as shortcuts for for loops, then the current behavior is not consistent:

julia> function f()
         i = 1
         [ i for i in 1:2]
         println(i)
       end
f (generic function with 1 method)

julia> f()
1

@nalimilan
Copy link
Member

That behavior can also be useful for debugging at the REPL, when you want to know which iteration threw an error. Not saying we must necessarily keep it, though. I think the main goal should be language consistency to limit surprises and mistakes.

@vtjnash
Copy link
Member

vtjnash commented Jun 10, 2017

I thought there might be an issue for this already, but if so I can't find it.

Not the same, but #19324 proposed doing this (for all variables) at the top-level scope. Similarly, I think #8870 is related, since anything that makes a variable less likely to become (unnecessarily) global makes that code faster :P

I know #14948 isn't directly related, but I think I've commented somewhere that this would make it valid to mark the for loop variable as local-def, which would prevent it from getting Core.Boxed during the closure conversion (it'll be detected as known-SSA).

@benninkrs
Copy link

benninkrs commented Jun 10, 2017

The behavior I would expect is that the iterator variable in a for loop is local to the for construct, but the same for all iterations, as in (using the current semantics)

let i
   for i = 2:3
     # loop body
   end
end

To address @nalimilan's use case, perhaps for expressions could yield the value of the iterator when the loop exits. Then you could write code like

lastval = for i = 1:100
  if somecondition
    break
  end
  # do stuff
end
println("last value = ", lastval)

I believe that currently, for expressions return nothing.

@JeffBezanson
Copy link
Member Author

the iterator variable in a for loop is local to the for construct, but the same for all iterations

That's off the table; we decided quite a while ago that when loops introduce new variables they should be fresh on each iteration. Comprehensions also behave that way. One of the benefits of the change in this issue is that it makes for loops and other appearances of for behave the same.

@benninkrs
Copy link

we decided quite a while ago that when loops introduce new variables they should be fresh on each iteration

Understood. But I think the loop iterator is syntactically and conceptually different than variables introduced in the loop body.

for i = 1:10
  z = 0;
  # rest of body
end

z is clearly introduced within the loop body, hence it makes sense that z is local to the body, i.e. fresh on each iteration. But i is introduced outside the body. Furthermore, since it is the variable that determines how many times to repeat the body, it logically exists outside the body. I agree for loops and comprehensions should behave the same way. To me, this would be the least surprising behavior for both constructs.

@JeffBezanson
Copy link
Member Author

Not going to happen.

You can think of a loop either as an outer let-bound variable updated repeatedly inside, or as something more like a higher-order function that calls the body (a function of i) for each value. Neither interpretation is obviously more correct. However the second one is more parallel, and more closely matches other constructs you can implement with higher-order functions. If you want the first behavior, we also have let and while loops.

Nit pick: the variable doesn't determine how many times to repeat the body, the iterator does. The iterator does indeed exist outside the body; the variable needn't.

@benninkrs
Copy link

Ok, parallelism is a good argument for having a fresh binding each iteration. Thanks for considering my viewpoint.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jun 13, 2017

We did originally do it the way you suggest, @benninkrs and changed it for a variety of reasons. The current per-iteration-local behavior tends to produce fewer surprising bugs and work better with higher order functions. As Jeff said, in for loops either behavior is justifiable, but in comprehensions, per-iteration is clearly less surprising – and having them match is overall better.

I'm strongly in favor of this change – if you want the iteration variable to leak out of the for loop, it's much clearer coding style to explicitly assign it to another variable. If you write it that way, it's clear to anyone what's happening without needing to know the details of scoping rules, so forcing people to write it that way is a good thing. It significantly simplifies scoping behavior and removes a potentially surprising gotcha from the language.

JeffBezanson added a commit that referenced this issue Aug 22, 2017
provide the old behavior via `for outer i = ...`
JeffBezanson added a commit that referenced this issue Aug 23, 2017
provide the old behavior via `for outer i = ...`
JeffBezanson added a commit that referenced this issue Aug 23, 2017
RFC: deprecate `for` loop vars that overwrite outer vars (#22314)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code compiler:lowering Syntax lowering (compiler front end, 2nd stage) needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

8 participants