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

Catch usage of undefined name with global statement #324

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

Conversation

seeeturtle
Copy link
Contributor

@seeeturtle seeeturtle commented Jan 17, 2018

Related to #249

@seeeturtle seeeturtle changed the title Closes https://github.com/PyCQA/pyflakes/issues/249 Closes issues:249 Jan 17, 2018
@seeeturtle seeeturtle force-pushed the issue-249 branch 2 times, most recently from a9bdb17 to 869e6c0 Compare January 17, 2018 18:10
@seeeturtle seeeturtle changed the title Closes issues:249 Catch usage of undefined name with global statement Jan 17, 2018
@seeeturtle
Copy link
Contributor Author

@sigmavirus24 it seems the CI is failing not because of the test.
Can you look at it?

@seeeturtle
Copy link
Contributor Author

seeeturtle commented Jan 17, 2018

ah. sorry, I didn't know that it's error of flake8
please ignore the above comment

@seeeturtle
Copy link
Contributor Author

wait... all flake8 error isn't related with my PR.
it should be fixed.

Copy link

@adhikasp adhikasp left a comment

Choose a reason for hiding this comment

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

Also please add the test from the original issue to make sure the problem is corrected.

def test(x):
    global x

elif isinstance(node.ctx, ast.Del):
self.handleNodeDelete(node)
if (isinstance(self.scope, FunctionScope) and
node.id in self.scope.global_names):
if not node.id in global_scope:

Choose a reason for hiding this comment

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

This line causing the CI to fail. E713, should be written if x not in y instead of if not x in y.

else:
name = alias.asname or alias.name
importation = Importation(name, node, alias.name)
importation = Importation(name, node, name)

Choose a reason for hiding this comment

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

Are you sure about this? The 3rd arg of Importation originally always pass alias.name (the fullname of import) and now it could have value of alias.asname. The meaning of code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sorry. it's my mistake

@@ -430,6 +431,13 @@ def unusedAssignments(self):
and isinstance(binding, Assignment)):
yield name, binding

def usedAssignments(self):

Choose a reason for hiding this comment

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

Why is this declared but never used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry. my mistake

@@ -1331,6 +1364,13 @@ def IMPORTFROM(self, node):
module, alias.name)
self.addBinding(node, importation)

if isinstance(self.scope, FunctionScope):
Copy link
Member

Choose a reason for hiding this comment

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

This code block is the same as the block above. It should probably be factored out into a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

@seeeturtle
Copy link
Contributor Author

seeeturtle commented Jan 18, 2018

Also please add the test from the original issue to make sure the problem is corrected.

@adhikasp, I've already added. check the commit message.

@adhikasp
Copy link

I've already added. check the commit message.

You mean this one? fdf2353#diff-44d7abf892e78127bd9438c51e1cfd49R340

But this is different compared to the snippet in the original issue. There should be another test when a function have args with same name as a global variable declared inside it. The function in your test above doesn't receive any argument.

@seeeturtle
Copy link
Contributor Author

Sorry. I thought it was same code.

@seeeturtle
Copy link
Contributor Author

@adhikasp By the way, What error should this test catch? UndefinedName?

@seeeturtle
Copy link
Contributor Author

The issue is actually different with the given task at gci...

@seeeturtle seeeturtle force-pushed the issue-249 branch 2 times, most recently from 1bda547 to 4b00b8d Compare January 18, 2018 13:56
@myint
Copy link
Member

myint commented Jan 18, 2018

Yeah, the GCI task I specified is different to #249. The one I specified matches the test case @seeeturtle created.

def main():
    global x
    print(x)

main()

So I think @adhikasp's comment about the test doesn't apply.

(Actually, #249 would have been easier. I probably should have specified that case for the GCI task.)

@seeeturtle
Copy link
Contributor Author

@myint Can you approve the task too?

@seeeturtle
Copy link
Contributor Author

Hooray! I didn't thought I can do this task well, but I did :)

@adhikasp
Copy link

Oh I see, my bad then 😄

@seeeturtle
Copy link
Contributor Author

Just waiting for February now :) See you then

@seeeturtle
Copy link
Contributor Author

seeeturtle commented Jul 15, 2018

Hello, @myint, I just found that this PR hasn't merged for a while.
Is there any problem?

@myint
Copy link
Member

myint commented Jul 15, 2018

@seeeturtle One of the other PyCQA members needs to approve this before I can merge it. This is the case for several of the open pull requests.

@seeeturtle
Copy link
Contributor Author

seeeturtle commented Jul 15, 2018 via email

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

One fix required, and a few restructures.

Also please add a test

def f():
    global m
    print(m)

def g():
    global m
    m = 1

g()
f()

That should have no errors, and your patch handles this correctly. The added test will help prevent it breaking in the future, when other changes are made to pyflakes.

else:
name = alias.asname or alias.name
importation = Importation(name, node, alias.name)
self.addBinding(node, importation)

self.store_global_scope(name, alias)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be self.store_global_scope(importation.name, alias) , otherwise the following will fail:

        self.flakes('''
            def f(): global foo; import foo.bar
            def g(): foo.is_used()
        ''')

You can add that test into test_assignedToGlobal.

Importing foo.bar is a bit strange -- it imports and binds the module foo as that name, and then imports foo.bar but there is no additional binding in the scope for foo.bar.

The determination of the binding name is done within SubmoduleImportation.

Note it would be much better if you did the self.store_global_scope(...) inside addBinding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for responding too late, @jayvdb.
I was reading the addBinding and there is something I can't understand.
Where does scope comes from? I can't find it...

Anyway, Thanks for reviewing :)

Copy link
Contributor Author

@seeeturtle seeeturtle Aug 4, 2018

Choose a reason for hiding this comment

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

self-answering...

the variable in for loop (for controlling the loop) can be used at outside of it.

if '.' in alias.name and not alias.asname:
importation = SubmoduleImportation(alias.name, node)
importation = SubmoduleImportation(name, node)
Copy link
Member

Choose a reason for hiding this comment

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

this change can be reverted.

elif isinstance(node.ctx, ast.Del):
self.handleNodeDelete(node)
if (isinstance(self.scope, FunctionScope) and
Copy link
Member

Choose a reason for hiding this comment

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

this should go in handleNodeDelete, where there is already logic regarding globals

elif isinstance(node.ctx, (ast.Store, ast.AugStore)):
self.handleNodeStore(node)
if (isinstance(self.scope, FunctionScope) and
Copy link
Member

Choose a reason for hiding this comment

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

this should go in handleNodeStore, where there is already logic regarding globals

# Locate the name in locals / function / globals scopes.
if isinstance(node.ctx, (ast.Load, ast.AugLoad)):
self.handleNodeLoad(node)
if (node.id == 'locals' and isinstance(self.scope, FunctionScope)
and isinstance(node.parent, ast.Call)):
# we are doing locals() call in current scope
self.scope.usesLocals = True
if (isinstance(self.scope, FunctionScope) and
Copy link
Member

Choose a reason for hiding this comment

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

this should probably go in handleNodeLoad

When we use undefined name declared by global statement,
pyflakes should catch the error but it doesn't.
This will fix pyflakes to catch this error.
Note test_undefined_global for example of this.
@seeeturtle
Copy link
Contributor Author

done @jayvdb

@retnikt
Copy link

retnikt commented Apr 28, 2020

Will this ever get merged? It's been two years!

@janosh
Copy link

janosh commented Oct 14, 2021

Anything I can do to help push this over the finish line?

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.

7 participants