Record assignments to global variables and propagate into local scopes #4

dawbarton · 2018-08-31T14:36:34Z

This PR allows recording of assignments done with blocks to allow statements like

if true
    aa = 0
end
for i = 1:10
    aa += i
end

to work even when aa has not been previously defined.

The key changes are to add any assigned variables to the globals variable whenever insertglobal is false (i.e., when in the global scope). Other assignment operators (+= and friends) are ignored as they will result in a UndefVarError if the variable isn't already a global variable.

One thing to note is that I've changed the behaviour of how let blocks were handled. Previously the let assignments (in contrast to the let body) were softscoped the inherited insertglobal value but (a) I don't understand why that is, and (b) it gives errors in cases like

softscope(Main, :(for i = 1:10; let a = i ; println(a) ; end ; end))

where a is a previously defined global.

The new way that let blocks are handled is to not softscope the assignments on the principle that it's pretty rare to try to assign to a global variable within the assignment statement of a let block, whereas the example above is not at all uncommon (particularly for programmatically defining functions).

It should be possible to do this properly but I'm not quite there yet on that one.

stevengj · 2018-08-31T14:39:18Z

src/SoftGlobalScope.jl

 elseif isexpr(ex, :block) || isexpr(ex, :if)
 return Expr(ex.head, _softscope.(ex.args, Ref(globals), insertglobal)...)
 elseif isexpr(ex, :local)
 setdiff!(globals, localvars(ex.args)) # affects globals in surrounding scope!
 return ex
 elseif insertglobal && ex.head in assignments && ex.args[1] in globals
 return Expr(:global, Expr(ex.head, ex.args[1], _softscope(ex.args[2], globals, insertglobal)))
+ elseif !insertglobal && isexpr(ex, :(=))


Won't this treat x as global even if it has been explicitly declared as local?

e.g. consider

begin local x x = 0 for i = 1:10 x += i end end

or

let x=1 x=0 for i = 1:10 x += i end end

In both cases, you don't want to transform the body of the for loop to global x += i, because x is explicitly local.

We probably need a set locals, in addition to globals, to track local variables.

No, the elseif isexpr(ex, :local) beforehand catches that one

julia> a = 1 1 julia> softscope(Main, :(begin; local a = 2; for i = 1:10; a += 1; end; end)) quote #= REPL[5]:1 =# local a = 2 #= REPL[5]:1 =# for i = 1:10 #= REPL[5]:1 =# a += 1 end end

Ah, though subsequent assignments will get caught... OK, so we might need a separate list of locals.

Sorry didn't see your extra comments - browser didn't refresh.

Your first example is problematic but the second example is fine with this PR. In the second example, the let introduces a local scope and so insertglobals is true -> the assignment is not recorded.

stevengj · 2018-08-31T15:24:45Z

src/SoftGlobalScope.jl

 if isexpr(ex, :for) || isexpr(ex, :while)
- return Expr(ex.head, ex.args[1], _softscope(ex.args[2], copy(globals), true))
+ return Expr(ex.head, ex.args[1], _softscope(ex.args[2], globals, copy(locals), true))


Needs copy(globals) as well for global x statements not to affect the surroundsing scope, no? Similarly for try and let and any other expression that introduces a new scope.

Would be good to have a test for this, e.g. in

for i = 1:10 for j = 1:3; global x += 1; end x = 3 end

the x = 3 statement should not be transformed to global x = 3 (unless x was already in the globals list).

Not quite sure what you mean. Can you give an example?

As far as I can see, once you've added a global it's there permanently for all future scopes. The only question is whether there is a local variable shadowing it or not. As such, manipulations of the locals should be sufficient. (BTW: there should be a check for global x in the code to add new globals directly but that's not in the original code either.)

Apologies, browser not refreshing again...

Actually, your test passes fine at the moment! Though I can think of other places it won't.

stevengj · 2018-08-31T16:15:42Z

src/SoftGlobalScope.jl

- return Expr(ex.head, _softscope(ex.args[1], globals, insertglobal),
- _softscope(ex.args[2], letglobals, true))
+ letlocals = union(locals, localvars(ex.args[1]))
+ return Expr(ex.head, ex.args[1], _softscope(ex.args[2], copy(globals), letlocals, true))


Note that args[1] is problematic here. e.g.

let x = (a += 1); end

needs a global a (assuming :a in globals)

Yes, I know about this - it's mentioned in my original PR text above. The original code was also broken in this regard and so this is no less bad 😄 Plus this version doesn't have the other bug that was in the original (again, see above).

stevengj · 2018-08-31T16:23:15Z

src/SoftGlobalScope.jl

+ elseif insertglobal && ex.head in assignments && ex.args[1] in globals && !(ex.args[1] in locals)
+ return Expr(:global, Expr(ex.head, ex.args[1], _softscope(ex.args[2], globals, locals, insertglobal)))
+ elseif !insertglobal && isexpr(ex, :(=)) # only assignments in the global scope need to be considered
+ union!(globals, localvars(ex))


If ex.head is a Symbol or a tuple expression, then you also want to call softscope_ on the rhs, e.g. to handle things like x = y = 3.

stevengj · 2018-08-31T16:24:55Z

src/SoftGlobalScope.jl

 return ex
- elseif insertglobal && ex.head in assignments && ex.args[1] in globals
- return Expr(:global, Expr(ex.head, ex.args[1], _softscope(ex.args[2], globals, insertglobal)))
 else
 return ex


Note that return ex here is rather conservative. e.g. it will miss things like sqrt(begin; for i=0:10; a+=1; end; a; end) that have nested blocks inside function calls, on the rhs of assignment expressions, etcetera.

True but I think that should be the subject of another PR. As it stands, this has the same behaviour as the previous code.

stevengj · 2018-08-31T16:56:07Z

src/SoftGlobalScope.jl

- return Expr(ex.head, _softscope(ex.args[1], globals, insertglobal),
- _softscope(ex.args[2], letglobals, true))
+ letlocals = union(locals, localvars(ex.args[1]))
+ return Expr(ex.head, ex.args[1], _softscope(ex.args[2], copy(globals), letlocals, true))
 elseif isexpr(ex, :block) || isexpr(ex, :if)


need || isexpr(ex, :toplevel) here too

(This was a problem in the original code too.)

stevengj · 2018-08-31T17:59:32Z

Can you file an issue for the other problems noted above?

dawbarton · 2018-08-31T19:36:36Z

Will do - sorry don't have any more time today to do any more.

Record assignments to global variables and propagate into local scopes

cd0bdb7

stevengj reviewed Aug 31, 2018

View reviewed changes

Added local set

bff6ab0

stevengj reviewed Aug 31, 2018

View reviewed changes

dawbarton added 3 commits August 31, 2018 16:54

Another test

76a8ab3

Revert back to copying globals

0a8418c

Fixed mistake in test

a47cc63

stevengj reviewed Aug 31, 2018

View reviewed changes

stevengj merged commit e4795c4 into JuliaLang:master Aug 31, 2018

This was referenced Aug 31, 2018

Added @softscope macro for ease of use #1

Merged

Treat local assignments (in for and let) correctly + calls #6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record assignments to global variables and propagate into local scopes #4

Record assignments to global variables and propagate into local scopes #4

dawbarton commented Aug 31, 2018

stevengj Aug 31, 2018 •

edited

Loading

stevengj Aug 31, 2018

dawbarton Aug 31, 2018 •

edited

Loading

dawbarton Aug 31, 2018

dawbarton Aug 31, 2018

stevengj Aug 31, 2018

stevengj Aug 31, 2018

dawbarton Aug 31, 2018

dawbarton Aug 31, 2018

dawbarton Aug 31, 2018

stevengj Aug 31, 2018

dawbarton Aug 31, 2018

stevengj Aug 31, 2018

stevengj Aug 31, 2018

dawbarton Aug 31, 2018

stevengj Aug 31, 2018

stevengj Aug 31, 2018

stevengj commented Aug 31, 2018

dawbarton commented Aug 31, 2018

Record assignments to global variables and propagate into local scopes #4

Record assignments to global variables and propagate into local scopes #4

Conversation

dawbarton commented Aug 31, 2018

stevengj Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawbarton Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Aug 31, 2018

dawbarton commented Aug 31, 2018

stevengj Aug 31, 2018 •

edited

Loading

dawbarton Aug 31, 2018 •

edited

Loading