Do not report redefinition of import in a local scope if the global name is used elsewhere in the module; fixes lp:1297161

3 files changed, 80 insertions, 42 deletions

diff --git a/NEWS.txt b/NEWS.txt
index 776fb6f..2d507b7 100644
--- a/NEWS.txt
+++ b/NEWS.txt
@@ -1,5 +1,7 @@
 UNRELEASED:
   - Detect the declared encoding in Python 3.
+  - Do not report redefinition of import in a local scope, if the
+    global name is used elsewhere in the module.
 
 0.8.0 (2014-03-22):
   - Adapt for the AST in Python 3.4.
diff --git a/pyflakes/checker.py b/pyflakes/checker.py
index 6e1f1d1..91661d4 100644
--- a/pyflakes/checker.py
+++ b/pyflakes/checker.py
@@ -82,8 +82,17 @@ class Binding(object):
                                                         self.source.lineno,
                                                         id(self))
 
+    def redefines(self, other):
+        return isinstance(other, Definition) and self.name == other.name
 
-class Importation(Binding):
+
+class Definition(Binding):
+    """
+    A binding that defines a function or a class.
+    """
+
+
+class Importation(Definition):
     """
     A binding created by an import statement.
 
@@ -93,9 +102,15 @@ class Importation(Binding):
     """
     def __init__(self, name, source):
         self.fullName = name
+        self.redefined = []
         name = name.split('.')[0]
         super(Importation, self).__init__(name, source)
 
+    def redefines(self, other):
+        if isinstance(other, Importation):
+            return self.fullName == other.fullName
+        return isinstance(other, Definition) and self.name == other.name
+
 
 class Argument(Binding):
     """
@@ -103,12 +118,6 @@ class Argument(Binding):
     """
 
 
-class Definition(Binding):
-    """
-    A binding that defines a function or a class.
-    """
-
-
 class Assignment(Binding):
     """
     Represents binding a name with an explicit assignment.
@@ -323,6 +332,9 @@ class Checker(object):
                 if (isinstance(importation, Importation) and
                         not importation.used and
                         importation.name not in all_names):
+                    for node in importation.redefined:
+                        self.report(messages.RedefinedWhileUnused,
+                                    node, importation.name, importation.source)
                     self.report(messages.UnusedImport,
                                 importation.source, importation.name)
 
@@ -381,45 +393,33 @@ class Checker(object):
                 return True
         return False
 
-    def addBinding(self, node, value, reportRedef=True):
+    def addBinding(self, node, value):
         """
         Called when a binding is altered.
 
         - `node` is the statement responsible for the change
-        - `value` is the optional new value, a Binding instance, associated
-          with the binding; if None, the binding is deleted if it exists.
-        - if `reportRedef` is True (default), rebinding while unused will be
-          reported.
+        - `value` is the new value, a Binding instance
         """
-        redefinedWhileUnused = False
-        if not isinstance(self.scope, ClassScope):
-            for scope in self.scopeStack[::-1]:
-                existing = scope.get(value.name)
-                if (isinstance(existing, Importation)
-                        and not existing.used
-                        and (not isinstance(value, Importation) or
-                             value.fullName == existing.fullName)
-                        and reportRedef
-                        and not self.differentForks(node, existing.source)):
-                    redefinedWhileUnused = True
+        for scope in self.scopeStack[::-1]:
+            if value.name in scope:
+                break
+        existing = scope.get(value.name)
+
+        if existing and not self.differentForks(node, existing.source):
+
+            if scope is self.scope:
+                if (self.hasParent(value.source, ast.ListComp) and
+                        not self.hasParent(existing.source, (ast.For, ast.ListComp))):
+                    self.report(messages.RedefinedInListComp,
+                                node, value.name, existing.source)
+                elif not existing.used and value.redefines(existing):
                     self.report(messages.RedefinedWhileUnused,
                                 node, value.name, existing.source)
 
-        existing = self.scope.get(value.name)
-        if not redefinedWhileUnused and self.hasParent(value.source, ast.ListComp):
-            if (existing and reportRedef
-                    and not self.hasParent(existing.source, (ast.For, ast.ListComp))
-                    and not self.differentForks(node, existing.source)):
-                self.report(messages.RedefinedInListComp,
-                            node, value.name, existing.source)
-
-        if (isinstance(existing, Definition)
-                and not existing.used
-                and not self.differentForks(node, existing.source)):
-            self.report(messages.RedefinedWhileUnused,
-                        node, value.name, existing.source)
-        else:
-            self.scope[value.name] = value
+            elif isinstance(existing, Importation) and value.redefines(existing):
+                existing.redefined.append(node)
+
+        self.scope[value.name] = value
 
     def getNodeHandler(self, node_class):
         try:
@@ -766,7 +766,7 @@ class Checker(object):
 
             self.pushScope()
             for name in args:
-                self.addBinding(node, Argument(name, node), reportRedef=False)
+                self.addBinding(node, Argument(name, node))
             if isinstance(node.body, list):
                 # case for FunctionDefs
                 for stmt in node.body:
diff --git a/pyflakes/test/test_imports.py b/pyflakes/test/test_imports.py
index 76853c7..515def7 100644
--- a/pyflakes/test/test_imports.py
+++ b/pyflakes/test/test_imports.py
@@ -172,6 +172,39 @@ class Test(TestCase):
                     pass
         ''', m.RedefinedWhileUnused, m.UnusedImport)
 
+    def test_redefinedInNestedFunctionTwice(self):
+        """
+        Test that shadowing a global name with a nested function definition
+        generates a warning.
+        """
+        self.flakes('''
+        import fu
+        def bar():
+            import fu
+            def baz():
+                def fu():
+                    pass
+        ''', m.RedefinedWhileUnused, m.RedefinedWhileUnused,
+             m.UnusedImport, m.UnusedImport)
+
+    def test_redefinedButUsedLater(self):
+        """
+        Test that a global import which is redefined locally,
+        but used later in another scope does not generate a warning.
+        """
+        self.flakes('''
+        import unittest, transport
+
+        class GetTransportTestCase(unittest.TestCase):
+            def test_get_transport(self):
+                transport = 'transport'
+                self.assertIsNotNone(transport)
+
+        class TestTransportMethodArgs(unittest.TestCase):
+            def test_send_defaults(self):
+                transport.Transport()
+        ''')
+
     def test_redefinedByClass(self):
         self.flakes('''
         import fu
@@ -214,7 +247,7 @@ class Test(TestCase):
         import fu
         def fun(fu):
             print(fu)
-        ''', m.UnusedImport)
+        ''', m.UnusedImport, m.RedefinedWhileUnused)
 
         self.flakes('''
         import fu
@@ -433,7 +466,8 @@ class Test(TestCase):
         self.flakes('import fu; [1 for _ in range(1) if fu]')
 
     def test_redefinedByListComp(self):
-        self.flakes('import fu; [1 for fu in range(1)]', m.RedefinedWhileUnused)
+        self.flakes('import fu; [1 for fu in range(1)]',
+                    m.RedefinedInListComp)
 
     def test_usedInTryFinally(self):
         self.flakes('''
@@ -481,7 +515,9 @@ class Test(TestCase):
         self.flakes('import fu; lambda: fu')
 
     def test_shadowedByLambda(self):
-        self.flakes('import fu; lambda fu: fu', m.UnusedImport)
+        self.flakes('import fu; lambda fu: fu',
+                    m.UnusedImport, m.RedefinedWhileUnused)
+        self.flakes('import fu; lambda fu: fu\nfu()')
 
     def test_usedInSliceObj(self):
         self.flakes('import fu; "meow"[::fu]')