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

Global variables must be assigned to be used #666

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dannysepler
Copy link
Contributor

@dannysepler dannysepler commented Dec 24, 2021

Closes #590. This also adds a new NoBindingForNonlocal error

I'm pretty new to pyflakes so just let me know if I messed something up

self.name = name
self.source = source
self.used = False
self.isUnassigned = isUnassigned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new code should use snake_case -- we're slowly migrating over the legacy code which uses the old casing

@@ -322,10 +322,11 @@ class Binding(object):
the node that this binding was last used.
"""

def __init__(self, name, source):
def __init__(self, name, source, isUnassigned=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit of a double-negative and a boolean trap -- it would probably be best to change this to assigned=True and make it keyword-only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! should i wait for #660 to be merged before introducing a keyword-only arg?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I just merged it

Comment on lines 1133 to 1135
in_global_scope = value.name in self.scopeStack[-1]
if existing and in_global_scope and existing.isUnassigned:
existing.isUnassigned = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct -- for instance a closure-scoped variable can shadow a global variable (and is a separate scope)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, added a test case for that

Comment on lines 1218 to 1219
except AttributeError:
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't be blindly catching AttributeError -- this is a programming error

Comment on lines 338 to 305
def test_globalIsUndefinedIfNeverGivenValue(self):
"""
If a "global" is never given a value, it is undefined
"""
self.flakes('''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessarily true, a function later in the module could modify that variable and give it a value

Comment on lines 384 to 397
def test_unusedGlobalAcrossFunctions(self):
"""
An unused "global" statement in another function does not define the name.
"""
self.flakes('''
def f1():
m
global m

def f2():
global m
m
''', m.UndefinedName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test shouldn't have been altered -- it was demonstrating a behaviour that should happen

Copy link
Contributor Author

@dannysepler dannysepler Dec 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the moment, this is the one part of the diff i'm still iffy about. you're right, this should eventually just be un-skipped. pyflakes at the moment only errors when:

def f1():
    m

def f2():
    global m

and it does not error if the global is defined in f1

for now i've just gone back to skipping it and reverted my changes, not sure if this is some bigger thing with pyflakes or if it's a simple fix i can do in here!

@@ -499,7 +515,7 @@ def fun2():
a
a = 2
return a
''', m.UndefinedLocal)
''', m.UndefinedLocal, m.UndefinedName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this new error here is correct -- it seems to be correct on the main branch

@dannysepler dannysepler deleted the global-keyword branch December 25, 2021 02:54
@dannysepler dannysepler restored the global-keyword branch December 25, 2021 02:56
@dannysepler dannysepler reopened this Dec 25, 2021
@dannysepler dannysepler force-pushed the global-keyword branch 2 times, most recently from bd0a4d6 to a891547 Compare December 25, 2021 03:07
@@ -1155,6 +1162,10 @@ def handleNodeLoad(self, node):
continue

binding = scope.get(name, None)

if getattr(binding, 'assigned', None) is False:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah -- my point on this was more that this shouldn't be something we're guessing at -- it should always have this attribute or never have this attribute (it should be set unconditionally on construction)

Comment on lines +1077 to +1015
global_scope = self.scopeStack[-1]
if (existing and global_scope.get(value.name) == existing and
not existing.assigned):
# make sure the variable is in the global scope before setting as assigned
existing.assigned = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyflakes running on itself is showing this is probably not quite right -- or at least it's not happy about nonlocal

@dannysepler
Copy link
Contributor Author

pyflakes is treating nonlocal the same as globals at the moment, this is starting to fork the two! it doesn't solve #249 or #461 yet, but at least it opens us up to solving it later?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

global keyword removes undefined variable warning
2 participants