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.

Sign up for GitHub

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

Always report imports shadowed by another import #45

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

Always report imports shadowed by another import #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pyflakes/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,14 @@ def addBinding(self, node, value):
self.report(messages.RedefinedWhileUnused,
node, value.name, existing.source)

elif isinstance(existing, Importation) and value.redefines(existing):
existing.redefined.append(node)
elif isinstance(existing, Importation):
if isinstance(node, (ast.Import, ast.ImportFrom)):
if not isinstance(self.scope, DoctestScope):
self.report(messages.ImportShadowedByImport,
node, value.name, existing.source)

elif value.redefines(existing):
existing.redefined.append(node)

if value.name in self.scope:
# then assume the rebound name is used as a global or within a loop
Expand Down
8 changes: 8 additions & 0 deletions pyflakes/messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ def __init__(self, filename, loc, name, orig_loc):
self.message_args = (name, orig_loc.lineno)


class ImportShadowedByImport(Message):
message = 'import %r from line %r shadowed by import'

def __init__(self, filename, loc, name, orig_loc):
Message.__init__(self, filename, loc)
self.message_args = (name, orig_loc.lineno)


class ImportShadowedByLoopVar(Message):
message = 'import %r from line %r shadowed by loop variable'

Expand Down
5 changes: 2 additions & 3 deletions pyflakes/test/test_doctests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from pyflakes.test.test_other import Test as TestOther
from pyflakes.test.test_imports import Test as TestImports
from pyflakes.test.test_undefined_names import Test as TestUndefinedNames
from pyflakes.test.harness import TestCase, skip
from pyflakes.test.harness import TestCase


class _DoctestMixin(object):
Expand Down Expand Up @@ -193,7 +193,6 @@ def doctest_stuff():
'''
""")

@skip("todo")
def test_importBeforeAndInDoctest(self):
self.flakes('''
import foo
Expand All @@ -205,7 +204,7 @@ def doctest_stuff():
"""

foo
''', m.RedefinedWhileUnused)
''')

def test_importInDoctestAndAfter(self):
self.flakes('''
Expand Down
14 changes: 11 additions & 3 deletions pyflakes/test/test_imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,8 @@ def bar():
def baz():
def fu():
pass
''',
m.RedefinedWhileUnused, m.RedefinedWhileUnused,
m.UnusedImport, m.UnusedImport)
''', *(m.ImportShadowedByImport, m.RedefinedWhileUnused,
Copy link
Member

Choose a reason for hiding this comment

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

What's the use of the *(...) construction here?

Copy link
Member Author

Choose a reason for hiding this comment

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

To simplify neater indentation.

Copy link
Member

Choose a reason for hiding this comment

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

Eh, this isn't exactly clear or obvious. I understand why, but I'm not sure I agree with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries; I can rework it when I create a narrower patch after #54 has been approved.

m.UnusedImport, m.UnusedImport))

def test_redefinedButUsedLater(self):
"""
Expand Down Expand Up @@ -564,6 +563,15 @@ def bar():
fu
''', m.UnusedImport, m.UndefinedName)

def test_shadowedInNestedScope(self):
self.flakes('''
import fu
def bar():
import fu
fu
fu
''', m.ImportShadowedByImport)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree this is a desirable behavior. I know there's also an ImportShadowedByLoopVar ... I'm not sure I agree with that either. Citing the design principles:

Pyflakes makes a simple promise: it will never complain about style, and it will try very, very hard to never emit false positives.

There's nothing inherently erroneous about the code here. Can we be sure that no one will ever have a legitimate reason to do this? What if we change the example a bit:

from StringIO import StringIO

def bar():
    from cStringIO import StringIO
    StringIO

StringIO

Copy link
Member Author

Choose a reason for hiding this comment

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

Unused imports is also style, not an error.

I could exlude ast.ImportFrom from this new rule to avoid some scenarios where redefining an imported name is less problematic, such as your example.

Copy link
Member

Choose a reason for hiding this comment

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

That could work. Though, #44 makes me think of another case:

import foo

class bar:
    import foo

foo

Probably there should be no errors here, but right now it looks like the 2nd import foo yields "test.py:4: import 'foo' from line 1 shadowed by import"

Copy link
Member Author

Choose a reason for hiding this comment

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

Differentiating between types of imports will be much cleaner and simpler after #54

Copy link
Contributor

Choose a reason for hiding this comment

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

But this isn't even bad style. It's no different from

var = 1

def func():
    var = 2
    ...

Redefining variables in inner scopes is a perfectly legitimate thing to do, whether that be via imports, loops, as constructs, or assignments. Python's scoping rules mostly protect you from this doing anything bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO it isnt the same, as imports are an external resource, and should be consistently named through the module. I can appreciate if others think that is too much a style issue for pyflakes to take on.

However, the case I am most interested in is

import foo

def blah():
     import foo
     foo

foo

Currently that isnt caught as a redefinition because foo is used. However the import in the function is superfluous, and should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noting that in the case of the class attribute, the same can be achieved without shadowing using

import foo

class bar:
    foo = foo

But as said earlier, I am happy to allow shadowing for class attributes, and other instances if there is consensus one way or the other.


def test_methodsDontUseClassScope(self):
self.flakes('''
class bar:
Expand Down