Global variables must be assigned to be used by dannysepler · Pull Request #666 · PyCQA/pyflakes

dannysepler · 2021-12-24T17:47:15Z

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

asottile · 2021-12-24T21:04:22Z

pyflakes/checker.py

        self.name = name
        self.source = source
        self.used = False
+        self.isUnassigned = isUnassigned


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

asottile · 2021-12-24T21:06:12Z

pyflakes/checker.py

    """

-    def __init__(self, name, source):
+    def __init__(self, name, source, isUnassigned=False):


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

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

yeah I just merged it

asottile · 2021-12-24T21:08:28Z

pyflakes/checker.py

+        in_global_scope = value.name in self.scopeStack[-1]
+        if existing and in_global_scope and existing.isUnassigned:
+            existing.isUnassigned = False


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

sweet, added a test case for that

asottile · 2021-12-24T21:08:49Z

pyflakes/checker.py

+            except AttributeError:
+                pass


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

asottile · 2021-12-24T21:09:24Z

pyflakes/test/test_undefined_names.py

+    def test_globalIsUndefinedIfNeverGivenValue(self):
+        """
+        If a "global" is never given a value, it is undefined
+        """
+        self.flakes('''


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

asottile · 2021-12-24T21:10:02Z

pyflakes/test/test_undefined_names.py

+    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)


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

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!

asottile · 2021-12-24T21:11:39Z

pyflakes/test/test_undefined_names.py

                    a = 2
                    return a
-        ''', m.UndefinedLocal)
+        ''', m.UndefinedLocal, m.UndefinedName)


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

asottile · 2022-09-16T03:06:03Z

pyflakes/checker.py


            binding = scope.get(name, None)
+
+            if getattr(binding, 'assigned', None) is False:


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)

asottile · 2022-09-16T03:06:30Z

pyflakes/checker.py

+        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


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

dannysepler · 2022-10-06T04:45:09Z

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?

asottile reviewed Dec 24, 2021

View reviewed changes

dannysepler closed this Dec 25, 2021

dannysepler deleted the global-keyword branch December 25, 2021 02:54

dannysepler restored the global-keyword branch December 25, 2021 02:56

dannysepler reopened this Dec 25, 2021

dannysepler force-pushed the global-keyword branch 2 times, most recently from bd0a4d6 to a891547 Compare December 25, 2021 03:07

dannysepler force-pushed the global-keyword branch from a891547 to bcf8194 Compare September 16, 2022 02:42

asottile reviewed Sep 16, 2022

View reviewed changes

Global variables must be assigned to be used

5302f1f

dannysepler force-pushed the global-keyword branch from d0abe7d to 5302f1f Compare October 6, 2022 04:40


		binding = scope.get(name, None)

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

Conversation

dannysepler commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannysepler Dec 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannysepler commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

dannysepler commented Dec 24, 2021 •

edited

Loading

dannysepler Dec 25, 2021 •

edited

Loading