Merge lp:~dobey/divmod.org/pyflakes-less-redef into lp:divmod.org

[dobey]

Add an ExceptHandler for dealing with ImportError cases of RedefinedWhileUnused.

[lp:~dobey/divmod.org/pyflakes-less-redef]

2700. By dobey on 2012-10-17

Add copyright notice for change

[Jean-Paul Calderone (exarkun)]

Hiya. This is an error that a lot of people come across. It's been reported at least three times, even. :) It's somewhat tricky to resolve, though. For example, the change proposed here makes pyflakes stop reporting the error in this code:

    try:
        pass
    except ImportError:
        import foo
        import bar as foo
    print foo

Some amount of care is needed in implementing this to ensure that at least existing functionality is preserved. Beyond that, it's probably also necessary to handle quite a variety of variations on this error. The later can be addressed incrementally, at least, but it's really necessary to keep the former in mind at every stage.

review: Needs Fixing

[lp:~dobey/divmod.org/pyflakes-less-redef]

2701. By dobey on 2012-10-19

Link the bug

[dobey (dobey)]

The only place where this will create a potential regression issue, is the example you provide here. However, it appears extremely unlikely that anyone will write such code; and if they do, will experience other issues as a result anyway. While I agree that it would be ideal to not have this happen, simply browsing Python code from various projects shows that

try:
   from new_module_name import foo
except:
   from old_module_name import foo

will occur significantly often. Similar examples even occur directly in the pyflakes code itself (which I presume does not consistently have pyflakes run on it; though it should). I think the pragmatic solution here is to fix the 99.999999+% cases first, and fix the more specific and much less likely to occur cases later, when the code can be refactored to handle them better; as it appears that fixing this "properly" will require such refactoring.

I'm happy to file a bug about this error case, and provide a "TODO" test case that references it, and even help fix it. But I don't think we should continue forcing people to write ugly workarounds to get around this error in pyflakes, as they currently must do, for the extremely common case.

[Glyph Lefkowitz (glyph)]

Personally, after some consideration, I'd prefer that this be merged despite the regression. But, I don't feel like I'm involved enough with recent pyflakes development to make that call alone.

I am concerned, though, that the reason that this is at issue is that Dobey didn't want to take the time to fix the regression in order to get this branch done quickly, yet in the intervening month in a half, nobody has made a fix to the issue that was described in a branch off of this one. If we simply had a fix there would be no need to discuss whether the regression was an acceptable price, we could just land it :).

[Marcin Cieślak (saperski)]

Hello everyone,

I took another approach to fix this bug (it is done against kevinw's fork but I hope those projects will soon merge).

The change:
https://github.com/saper/pyflakes/commit/09c52fcfab742b4f67c9d21d18ae389c300b4ea6

Updated tests:
https://github.com/saper/pyflakes/commit/5d2b7563cb63b080c99f6df24661dd9dcec188e1

The code can be easily cloned from github with

git clone git://github.com/saper/pyflakes.git

My approach, while not perfect and still to be fine-tuned, is different: whenever a RedefinedWhileUnused exception is to be raised, additional check is made whether "existing" (already detected existing name binding) and "loc" (current "redefinition") do not reside on
different "forks" of if: else: or try: except: else: blocks.

First, AST tree is searched upwards for both of them to find a common ancestor. If this is "IF"
or "TRYEXCEPT", there is a good chance that they might be under different blocks (like "if:"
or "else:"). Appropriate lists are checked, and exception is raised in case of:

try:
  import os
  import os
except:
  pass

but not

try:
  import os
except:
  import os

it does not matter what exception is raised/checked, of course conditions are not evaluated
(as this is not a dynamic analysis), but it seems to work fine for most typical Python
idioms when importing modules using if: or try:

I'd love to hear your feedback on this approach!

[Jean-Paul Calderone (exarkun)]

As pyflakes has moved out of divmod.org, it no longer makes sense to merge this code here. Please see <https://github.com/pyflakes/pyflakes>.

[Glyph Lefkowitz (glyph)]

I think that Florent is actually continuing development on Launchpad: <https://launchpad.net/pyflakes>.

He mentioned in his announcement mailing that he still accepts merge proposals on LP: <https://lists.launchpad.net/divmod-dev/msg00398.html>

[Florent (florent.x)]

I confirmed that this merge proposal is obsolete.
There are some recent proposals which encompass more conditional usages.

review: Disapprove

Unmerged revisions

2701. By dobey on 2012-10-19

Link the bug

2700. By dobey on 2012-10-17

Add copyright notice for change

2699. By dobey on 2012-10-16

Add an ExceptHandler for dealing with ImportError cases of RedefinedWhileUnused

=== modified file 'Pyflakes/pyflakes/checker.py'
--- Pyflakes/pyflakes/checker.py	2012-01-10 19:09:22 +0000
+++ Pyflakes/pyflakes/checker.py	2012-10-19 22:04:19 +0000
@@ -1,5 +1,6 @@
 # -*- test-case-name: pyflakes -*-
 # (c) 2005-2010 Divmod, Inc.
+# Copyright 2012 Canonical, Ltd.
 # See LICENSE file for details
 
 import __builtin__
@@ -194,6 +195,7 @@
 
     nodeDepth = 0
     traceTree = False
+    inImportError = False
 
     def __init__(self, tree, filename='(none)'):
         self._deferredFunctions = []
@@ -350,7 +352,7 @@
     EQ = NOTEQ = LT = LTE = GT = GTE = IS = ISNOT = IN = NOTIN = ignore
 
     # additional node types
-    COMPREHENSION = EXCEPTHANDLER = KEYWORD = handleChildren
+    COMPREHENSION = KEYWORD = handleChildren
 
     def addBinding(self, lineno, value, reportRedef=True):
         '''Called when a binding is altered.
@@ -367,7 +369,7 @@
                 if (isinstance(existing, Importation)
                         and not existing.used
                         and (not isinstance(value, Importation) or value.fullName == existing.fullName)
-                        and reportRedef):
+                        and reportRedef and not self.inImportError):
 
                     self.report(messages.RedefinedWhileUnused,
                                 lineno, value.name, scope[value.name].source.lineno)
@@ -632,3 +634,21 @@
             if node.module == '__future__':
                 importation.used = (self.scope, node.lineno)
             self.addBinding(node.lineno, importation)
+
+    def EXCEPTHANDLER(self, node):
+        """
+        We need to override this in order to note when we are inside
+        an ImportError exception handler, and avoid a RedefinedWhileUnused
+        message for the valid code:
+
+        try:
+            import foo
+        except ImportError:
+            import old_foo as foo
+        """
+        if isinstance(node.type, _ast.Name) and node.type.id == 'ImportError':
+            self.inImportError = True
+        try:
+            self.handleChildren(node)
+        finally:
+            self.inImportError = False

=== modified file 'Pyflakes/pyflakes/test/test_imports.py'
--- Pyflakes/pyflakes/test/test_imports.py	2011-10-31 15:19:35 +0000
+++ Pyflakes/pyflakes/test/test_imports.py	2012-10-19 22:04:19 +0000
@@ -24,6 +24,20 @@
         self.flakes('import fu; fu, bar = 3', m.RedefinedWhileUnused)
         self.flakes('import fu; [fu, bar] = 3', m.RedefinedWhileUnused)
 
+    def test_notRedefined(self):
+        self.flakes('''
+        try:
+            import fu
+        except ImportError:
+            import kung as fu
+        ''', m.UnusedImport);
+        self.flakes('''
+        try:
+            import fu
+        except ImportError:
+            fu = None
+        ''');
+
     def test_redefinedByFunction(self):
         self.flakes('''
         import fu