-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: deprecate for
loop vars that overwrite outer vars (#22314)
#22659
Conversation
test/read.jl
Outdated
@@ -176,7 +176,8 @@ for (name, f) in l | |||
old_text = text | |||
cleanup() | |||
|
|||
for text in [ | |||
for text_ in [ | |||
text = text_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't look right...
I'm a bit confused by this. Capturing the value in the last iteration is a pretty common task. How would one have to write that after this deprecation? |
You would have to explicitly assign a separate variable to the loop variable, e.g.
|
Thanks for the explanation. That seems fine to me, if a bit not-entirely-obvious. I suppose it provides a clearer scoping rule for this case. What happens at the end of the deprecation period? Does reusing a variable name in this way then become an error? It appears @vtjnash isn't a fan, based on his use of reactions. Jameson: What are your concerns with this approach? |
The idea of this is to make |
Ref #22314 |
147fae2
to
bfa4ef6
Compare
Update: the syntax |
+1 for
I don't think so. There aren't many places where declaring something as |
The more obvious thing would be not to change |
I think you've hit the nail on the head in your own comment, @martinholters – requiring |
I think we'd only need it if we changed the default scoping behavior. Of course it's possible that even now somebody might want "access the outer |
I wasn't worried about incompatibility, but rather about staking out the |
Related is also #19324. To introduce |
I like what was proposed here:
as an alternative to |
This isn't what the hard/soft scope distinction means. If you ignore global scope and only look inside functions, all scope constructs behave the same and use the same default of inheriting parent scope variables. |
I have to say, this is pretty appealing. No new word required, and much purer (in that you only need to return a value, and not repeatedly update a variable). |
I guess the only downside – if it even is one – is that for loops return their last iterator value/tuple. I'm wondering if that would be annoying in interactive usage or if it might even be useful? I guess the only way to find out is to try it. |
What is returned when the iterator is empty? |
Ah, yes, that's is a problem. I suppose it would have to be a default value like |
In the case where the iterator can be empty, instead of writing something like this: i = 0
for outer i = 1:n
# do something
end you would have to do this instead: i = for i = 1:n
# do something
end
i == nothing && (i = 0) which is pretty awkward. |
It's a bit ad-hoc, but I'd actually prefer to only provide this when the |
Not that I think it's a specially good idea, but one possibility is to have |
That would put us in the same situation as Stefan mentioned above, except instead of i == nothing && (i = 0) you have i = get(i, 0) Marginally less awkward but still awkward. |
I know, it's a bit weird. But there are other cases where an apparent assignment doesn't happen, like
or
Anyway, I'm ok with |
The cases where an apparent assignment don't happen can all be explained by the RHS expression being evaluated first and the value of the RHS being bound to a local name as a separate step: if the expression never returns, the binding step doesn't happen. In this case, you would have an apparent assignment from a RHS expression that does evaluate normally and return but depending on how it evaluates, the assignment might not happen. We don't have anything else like that in the language and I think it would be radically more confusing and unexpected. The |
bfa4ef6
to
ff12967
Compare
Implemented |
ff12967
to
9433618
Compare
Can you elaborate on the places where |
This is a great change, I recently got bugged by an outer loop variable being altered by an inner loop variable of the same name (inserted by a splicing macro, so not easy to spot). I don't love the name |
|
Outer in the parent scope or outer to their respective scopes? If the former, it's no longer equivalent to for outer i = x
for outer j = y
...
end
end since in that case |
The short form is exactly equivalent to the form with nested |
Okay, so the |
The outer |
Ah, okay. Thanks for the explanation, much appreciated. Would it be worth adding some of that to the documentation? |
Sure it would, I was just kind of hoping people wouldn't use this feature :) |
Necessary due to JuliaLang/julia#22659, but simpler anyway.
Sorry if I miss something obvious, but the following errors out:
It would be convenient to support, shall I open an issue? |
#23511 |
Oh sorry to not have found this. The "break-with-value-and-else" for-loop PR would also be satisfying for my case. |
This fixes conflicts introduced by JuliaLang#24075 and JuliaLang#22659.
Connoisseurs of elaborate deprecations will appreciate this.
This scoping change basically has two cases. The first, simpler and more common case, is where the loop variable overwrites an existing variable in a way that doesn't affect program behavior:
The second, more breaking, case is where the last value of the loop variable is actually used:
So I implemented two deprecation modes: one warns for all cases, the other tries to identify only the second case. Warning in all cases was pretty annoying for Base, though good for identifying potentially confusing variable name collisions. So the limited mode is more useful, but very approximate, as getting it right would require full def-use analysis. It does seem to catch almost all cases that occur in practice though. The commit here has limited mode enabled. I have not looked at the test suite yet.