GNOME Bugzilla – Bug 659334
Global variable mutation in functions
Last modified: 2011-10-23 18:59:20 UTC
Originally filed by jwienke@techfak.uni-bielefeld.de as http://www.reinteract.org/trac/ticket/29 - I've noticed a problem concerning the (re-)evaluation of global variables. Reinteract seems to be unaware of the global variables. For example begin with the following code: globals = {'count': 0} def doIt(): global globals globals['count'] = globals['count'] + 1 doIt() globals['count'] The output of the last line is 1, which is correct. Now you erase the line with the call to doIt(). Reinteract only marks the last line with pink. If you reevaluate the code the output from the last line then is still 1, which is obviously wrong.
Reinteract now fails this with: The global statement is not supported but if you remove 'global globals', this program still has the same meaning, and causes the same behavior. The level 0 thing we could do is to do analysis of function definitions and apply the Python scope binding rules and detect implicit use of global variables without an explicit 'global' keyword.
Created attachment 199149 [details] [review] Do a better job at understanding variable binding Mostly accurately compute the binding of names to the correct lexical scope so we know what variables are global variables and what are not. This allows us to do two things: - Don't emit copies in the global scope when the variable being mutated is actually a local variable. - Catch mutations of global variables inside functions even when the function doesn't explicitly use the 'global' keyword.
Created attachment 199162 [details] [review] Do a better job at understanding variable binding Add test cases
After a bit of testing, the only problem that I see is: If you mistype a variable name in a mutating statement in a function definition, you get an error about mutating a global variable, rather than a NameError. Unless we hold off on the former error until the function is called, I don't see a way to avoid this. But the error could be made more useful by highlighting the line in question and/or including the variable name in the error message.
Created attachment 199605 [details] [review] Make UnsupportedSyntaxErrors more useful Lines causing UnsupportedSyntaxErrors are now highlighted. Additionally, we include the name of the global variable in errors about mutations, to make it more clear when such errors arise because of typos, for example.
Review of attachment 199605 [details] [review]: Generally, this looks good to me - just see one problem. ::: lib/reinteract/rewrite.py @@ +192,3 @@ # only actually bad if the function is used again at a later point. if binding == NAME_GLOBAL and self.in_function: + raise UnsupportedSyntaxError("Assigning to global variable '%s' inside a function is not supported"%target.id, target.lineno) Looks like you missed testing this line - we have a string 'name' here rather than a node 'target' - I think you need to add another parameter to this function, a 'location' node, since not all callers have a ast.Name node with an id property.
That line works fine for me. Perhaps we've suffered some divergence in our trees somehow? In my version of handle_assign_targets, the argument is 'targets', which gets iterated through as 'target'. I don't see a string 'name' anywhere.
(In reply to comment #7) > That line works fine for me. Perhaps we've suffered some divergence in our > trees somehow? In my version of handle_assign_targets, the argument is > 'targets', which gets iterated through as 'target'. I don't see a string > 'name' anywhere. Ah, I see, I changed that in the patch in bug 662054 .... I'll merge things up and push.
Pushed both patches. Thanks for the improvement! Attachment 199162 [details] pushed as 522cee1 - Do a better job at understanding variable binding Attachment 199605 [details] pushed as 8484b1d - Make UnsupportedSyntaxErrors more useful