Repository: google/importlab

Issue:
Crash when analyzing files of different Python versions
Input:
```
# file a.py
import b

# file b.py
raise 1, 2, 3  # Python 2 syntax
```

Output of -V2.7:
```
$ importlab -V2.7 -P. *.py --tree
Source tree:
+ a.py
    + b.py
```

Output of -V3.6:
```
$ importlab -V3.6 -P. *.py --tree
Traceback (most recent call last):
  File "/usr/bin/importlab", line 69, in <module>
    sys.exit(main())
  File "/usr/bin/importlab", line 53, in main
    import_graph = graph.ImportGraph.create(env, args.filenames)
  File "/usr/local/lib/python2.7/site-packages/importlab/graph.py", line 248, in create
    import_graph.add_file_recursive(os.path.abspath(filename))
  File "/usr/local/lib/python2.7/site-packages/importlab/graph.py", line 123, in add_file_recursive
    deps, broken = self.get_file_deps(filename)
  File "/usr/local/lib/python2.7/site-packages/importlab/graph.py", line 262, in get_file_deps
    for imp in parsepy.get_imports(filename, self.env.python_version):
  File "/usr/local/lib/python2.7/site-packages/importlab/parsepy.py", line 90, in get_imports
    raise Exception('parse error for ' + filename)
Exception: parse error for /tmp/imptest/b.py
```

It would be nice if importlab didn't crash here. I think a `--keep-going` option would be useful for some users.

Discussion:

Patch:
diff --git a/bin/importlab b/bin/importlab
index d263f89..bb2eb30 100755
--- a/bin/importlab
+++ b/bin/importlab
@@ -55,11 +55,13 @@ def main():
     if args.tree:
         print('Source tree:')
         output.print_tree(import_graph)
+        output.maybe_show_unreadable(import_graph)
         sys.exit(0)
 
     if args.unresolved:
         print('Unresolved dependencies:')
         output.print_unresolved_dependencies(import_graph)
+        output.maybe_show_unreadable(import_graph)
         sys.exit(0)
 
     print('Nothing to do!')
diff --git a/importlab/graph.py b/importlab/graph.py
index cc7be01..1e5c1ca 100644
--- a/importlab/graph.py
+++ b/importlab/graph.py
@@ -78,7 +78,11 @@ class DependencyGraph(object):
 
     def __init__(self):
         self.graph = nx.DiGraph()
+        # import statements that did not resolve to python files.
         self.broken_deps = collections.defaultdict(set)
+        # files that were not syntactically valid python.
+        self.unreadable_files = set()
+        # once self.final is set the graph can no longer be modified.
         self.final = False
         # sources is a set of files directly added to the graph via
         # add_file or add_file_recursive.
@@ -120,7 +124,13 @@ class DependencyGraph(object):
         while queue:
             filename = queue.popleft()
             self.graph.add_node(filename)
-            deps, broken = self.get_file_deps(filename)
+            try:
+                deps, broken = self.get_file_deps(filename)
+            except parsepy.ParseError:
+                # We have found a dependency, but python couldn't parse it.
+                self.unreadable_files.add(filename)
+                self.graph.remove_node(filename)
+                continue
             for f in broken:
                 self.broken_deps[filename].add(f)
             for f in deps:
diff --git a/importlab/output.py b/importlab/output.py
index ea23b56..b41c79b 100644
--- a/importlab/output.py
+++ b/importlab/output.py
@@ -83,3 +83,16 @@ def formatted_deps_list(import_graph):
 def print_unresolved_dependencies(import_graph):
     for imp in sorted(import_graph.get_all_unresolved()):
         print(' ', imp.name)
+
+
+def print_unreadable_files(import_graph):
+    for f in sorted(import_graph.unreadable_files):
+        print(' ', f)
+
+
+def maybe_show_unreadable(import_graph):
+    """Only print an unreadable files section if nonempty."""
+    if import_graph.unreadable_files:
+      print()
+      print('Unreadable files:')
+      print_unreadable_files(import_graph)
diff --git a/importlab/parsepy.py b/importlab/parsepy.py
index 145adf5..95ad797 100644
--- a/importlab/parsepy.py
+++ b/importlab/parsepy.py
@@ -22,6 +22,11 @@ from . import import_finder
 from . import runner
 
 
+class ParseError(Exception):
+    """Error parsing a file with python."""
+    pass
+
+
 class ImportStatement(collections.namedtuple(
         'ImportStatement',
         ['name', 'new_name', 'is_from', 'is_star', 'source'])):
@@ -87,5 +92,5 @@ def get_imports(filename, python_version):
             if sys.version_info[0] == 3:
                 stderr = stderr.decode('ascii')
             logging.info(stderr)
-            raise Exception('parse error for ' + filename)
+            raise ParseError(filename)
     return [ImportStatement(*imp) for imp in imports]


Test patch:
diff --git a/tests/test_graph.py b/tests/test_graph.py
index 5dd221f..4dc5c66 100644
--- a/tests/test_graph.py
+++ b/tests/test_graph.py
@@ -5,6 +5,7 @@ import unittest
 from importlab import environment
 from importlab import fs
 from importlab import graph
+from importlab import parsepy
 from importlab import resolve
 from importlab import utils
 
@@ -26,14 +27,20 @@ class FakeImportGraph(graph.DependencyGraph):
     Also adds ordered_foo() wrappers around output methods to help in testing.
     """
 
-    def __init__(self, deps):
+    def __init__(self, deps, unreadable=None):
         super(FakeImportGraph, self).__init__()
         self.deps = deps
+        if unreadable:
+            self.unreadable = unreadable
+        else:
+            self.unreadable = set()
 
     def get_source_file_provenance(self, filename):
         return resolve.Direct(filename, 'module.name')
 
     def get_file_deps(self, filename):
+        if filename in self.unreadable:
+            raise parsepy.ParseError()
         if filename in self.deps:
             resolved, unresolved, provenance = self.deps[filename]
             self.provenance.update(provenance)
@@ -112,6 +119,29 @@ class TestDependencyGraph(unittest.TestCase):
         self.check_order(sources, ["d.py"], ["a.py", "b.py"])
         self.check_order(sources, ["c.py"], ["a.py", "b.py"])
 
+    def test_unreadable(self):
+        g = FakeImportGraph(SIMPLE_DEPS, unreadable={"b.py"})
+        g.add_file_recursive("a.py")
+        g.build()
+        # We have pruned b from the graph because it's unreadable.
+        self.assertEqual(g.ordered_deps_list(), [
+            ("a.py", ["c.py"]),
+            ("c.py", []),
+        ])
+        sources = g.ordered_sorted_source_files()
+        self.check_order(sources, ["c.py"], ["a.py"])
+        # b.py is still in g.provenance because we resolved it to a filename.
+        self.assertEqual(sorted(g.provenance.keys()),
+                         ["a.py", "b.py", "c.py"])
+        self.assertEqual(g.unreadable_files, set(["b.py"]))
+
+    def test_unreadable_direct_source(self):
+        g = FakeImportGraph(SIMPLE_DEPS, unreadable={"a.py"})
+        g.add_file_recursive("a.py")
+        g.build()
+        # Original source file is unreadable, so return nothing.
+        self.assertEqual(g.ordered_deps_list(), [])
+
 
 FILES = {
         "foo/a.py": "from . import b",