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

Track locals() #343

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Track locals() #343

wants to merge 2 commits into from

Conversation

jayvdb
Copy link
Member

@jayvdb jayvdb commented Jul 16, 2018

Fixes #136
Fixes #333

@jayvdb
Copy link
Member Author

jayvdb commented Jul 16, 2018

Note this also corrects handling of locals() which only sees variables preceeding it, while the current approach marks all variables as used. This is new test case test_unusedVariableAfterLocals in the patch.

if isinstance(scope, GeneratorScope):
scope = self.scopeStack[-2]
for binding in scope.values():
binding.used = (self.scope, node)
Copy link
Member Author

Choose a reason for hiding this comment

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

this could be changed to

if not binding.used:
    binding.used = (self.scope, node)

Then at the end of the scope, any bindings used only by locals() could be a different type of message.

See #333 (comment)

@asmeurer
Copy link
Contributor

IMHO locals is enough of a meta-programming technique that pyflakes should warn about it (same as import *). Otherwise locals() breaks all unused variable checks that would occur before it is called.

Having it warn but as a distinct error message that could be silenced by flake8 or whatever seems like the best solution.

@jayvdb
Copy link
Member Author

jayvdb commented Jul 18, 2018

I agree that locals() should be reported, but only if variables would otherwise be unused, like we did for * imports.
It is not meta programming when used only to capture running state , e.g. for logging. And def check_locals(): return locals()

I would prefer to tackle that new error report in a separate PR, and limit this one to only correct reporting of locals() with bug fixes where pyflakes was wrong or inconsistent.

@asmeurer
Copy link
Contributor

What exactly is the import * change you are referring to? Did that behavior change recently?

@jayvdb
Copy link
Member Author

jayvdb commented Jul 18, 2018

No, I am referring to 1.1.0 change, 0532189

@asmeurer
Copy link
Contributor

I see. I tested master and it sill warns about everything with import *, but sometimes has a better error message. Does this do the same thing for locals? The test makes it look like there are exceptions for locals, where using locals in the code makes it stop warning about unused variables. They aren't exactly the same thing because import * defines names and locals could both use or define a name (the latter being more metaprogramattic than the former).

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.

3 participants