bpo-35936: Updates to modulefinder (GH-11787)

* Properly handle SyntaxErrors in Python source files.

SyntaxErrors in the target module will rise normally, while SyntaxErrors in dependencies will be added to badmodules. This includes a new regression test.

* Fix name collision bug.

This fixes an issue where a "fromlist" import with the same name as a previously failed import would be incorrectly added to badmodules. This includes a new regression test.

* Replace mutable default values.

Bound empty lists have been replaced with the "if param is None" idiom.

* Replace deprecated imp usage.

Constants imported from imp have been moved to private module-level constants, and ModuleFinder.find_module has been refactored to use importlib. Other than an improvement on how frozen builtin imports are reported (as the frozen imports they are, rather than the stdlib modules they *may* have originated from), these changes maintain complete compatibility with past versions... including odd behavior for returning relative (below current directory, but not a C extension) vs. absolute (above current directory, or a C extension) paths.

Patch by Brandt Bucher.

Lib/modulefinder.py

@@ -8,9 +8,7 @@ import os
 import sys
 import types
 import warnings
-with warnings.catch_warnings():
+
-    warnings.simplefilter('ignore', DeprecationWarning)
-    import imp
 
 LOAD_CONST = dis.opmap['LOAD_CONST']
 IMPORT_NAME = dis.opmap['IMPORT_NAME']
@@ -19,6 +17,16 @@ STORE_GLOBAL = dis.opmap['STORE_GLOBAL']
 STORE_OPS = STORE_NAME, STORE_GLOBAL
 EXTENDED_ARG = dis.EXTENDED_ARG
 
+# Old imp constants:
+
+_SEARCH_ERROR = 0
+_PY_SOURCE = 1
+_PY_COMPILED = 2
+_C_EXTENSION = 3
+_PKG_DIRECTORY = 5
+_C_BUILTIN = 6
+_PY_FROZEN = 7
+
 # Modulefinder does a good job at simulating Python's, but it can not
 # handle __path__ modifications packages make at runtime.  Therefore there
 # is a mechanism whereby you can register extra paths in this map for a
@@ -43,6 +51,54 @@ def ReplacePackage(oldname, newname):
     replacePackageMap[oldname] = newname
 
 
+def _find_module(name, path=None):
+    """An importlib reimplementation of imp.find_module (for our purposes)."""
+
+    # It's necessary to clear the caches for our Finder first, in case any
+    # modules are being added/deleted/modified at runtime. In particular,
+    # test_modulefinder.py changes file tree contents in a cache-breaking way:
+
+    importlib.machinery.PathFinder.invalidate_caches()
+
+    spec = importlib.machinery.PathFinder.find_spec(name, path)
+
+    if spec is None:
+        raise ImportError("No module named {name!r}".format(name=name), name=name)
+
+    # Some special cases:
+
+    if spec.loader is importlib.machinery.BuiltinImporter:
+        return None, None, ("", "", _C_BUILTIN)
+
+    if spec.loader is importlib.machinery.FrozenImporter:
+        return None, None, ("", "", _PY_FROZEN)
+
+    file_path = spec.origin
+
+    if spec.loader.is_package(name):
+        return None, os.path.dirname(file_path), ("", "", _PKG_DIRECTORY)
+
+    if isinstance(spec.loader, importlib.machinery.SourceFileLoader):
+        kind = _PY_SOURCE
+        mode = "r"
+
+    elif isinstance(spec.loader, importlib.machinery.ExtensionFileLoader):
+        kind = _C_EXTENSION
+        mode = "rb"
+
+    elif isinstance(spec.loader, importlib.machinery.SourcelessFileLoader):
+        kind = _PY_COMPILED
+        mode = "rb"
+
+    else:  # Should never happen.
+        return None, None, ("", "", _SEARCH_ERROR)
+
+    file = open(file_path, mode)
+    suffix = os.path.splitext(file_path)[-1]
+
+    return file, file_path, (suffix, mode, kind)
+
+
 class Module:
 
     def __init__(self, name, file=None, path=None):
@@ -69,7 +125,7 @@ class Module:
 
 class ModuleFinder:
 
-    def __init__(self, path=None, debug=0, excludes=[], replace_paths=[]):
+    def __init__(self, path=None, debug=0, excludes=None, replace_paths=None):
         if path is None:
             path = sys.path
         self.path = path
@@ -77,8 +133,8 @@ class ModuleFinder:
         self.badmodules = {}
         self.debug = debug
         self.indent = 0
-        self.excludes = excludes
+        self.excludes = excludes if excludes is not None else []
-        self.replace_paths = replace_paths
+        self.replace_paths = replace_paths if replace_paths is not None else []
         self.processed_paths = []   # Used in debugging only
 
     def msg(self, level, str, *args):
@@ -105,14 +161,14 @@ class ModuleFinder:
     def run_script(self, pathname):
         self.msg(2, "run_script", pathname)
         with open(pathname) as fp:
-            stuff = ("", "r", imp.PY_SOURCE)
+            stuff = ("", "r", _PY_SOURCE)
             self.load_module('__main__', fp, pathname, stuff)
 
     def load_file(self, pathname):
         dir, name = os.path.split(pathname)
         name, ext = os.path.splitext(name)
         with open(pathname) as fp:
-            stuff = (ext, "r", imp.PY_SOURCE)
+            stuff = (ext, "r", _PY_SOURCE)
             self.load_module(name, fp, pathname, stuff)
 
     def import_hook(self, name, caller=None, fromlist=None, level=-1):
@@ -279,13 +335,13 @@ class ModuleFinder:
     def load_module(self, fqname, fp, pathname, file_info):
         suffix, mode, type = file_info
         self.msgin(2, "load_module", fqname, fp and "fp", pathname)
-        if type == imp.PKG_DIRECTORY:
+        if type == _PKG_DIRECTORY:
             m = self.load_package(fqname, pathname)
             self.msgout(2, "load_module ->", m)
             return m
-        if type == imp.PY_SOURCE:
+        if type == _PY_SOURCE:
             co = compile(fp.read()+'\n', pathname, 'exec')
-        elif type == imp.PY_COMPILED:
+        elif type == _PY_COMPILED:
             try:
                 data = fp.read()
                 importlib._bootstrap_external._classify_pyc(data, fqname, {})
@@ -323,17 +379,20 @@ class ModuleFinder:
         except ImportError as msg:
             self.msg(2, "ImportError:", str(msg))
             self._add_badmodule(name, caller)
+        except SyntaxError as msg:
+            self.msg(2, "SyntaxError:", str(msg))
+            self._add_badmodule(name, caller)
         else:
             if fromlist:
                 for sub in fromlist:
-                    if sub in self.badmodules:
+                    fullname = name + "." + sub
-                        self._add_badmodule(sub, caller)
+                    if fullname in self.badmodules:
+                        self._add_badmodule(fullname, caller)
                         continue
                     try:
                         self.import_hook(name, caller, [sub], level=level)
                     except ImportError as msg:
                         self.msg(2, "ImportError:", str(msg))
-                        fullname = name + "." + sub
                         self._add_badmodule(fullname, caller)
 
     def scan_opcodes(self, co):
@@ -445,10 +504,11 @@ class ModuleFinder:
 
         if path is None:
             if name in sys.builtin_module_names:
-                return (None, None, ("", "", imp.C_BUILTIN))
+                return (None, None, ("", "", _C_BUILTIN))
 
             path = self.path
-        return imp.find_module(name, path)
+
+        return _find_module(name, path)
 
     def report(self):
         """Print a report to stdout, listing the found modules with their

Lib/test/test_modulefinder.py

@@ -218,6 +218,33 @@ bytecode_test = [
     ""
 ]
 
+syntax_error_test = [
+    "a.module",
+    ["a", "a.module", "b"],
+    ["b.module"], [],
+    """\
+a/__init__.py
+a/module.py
+                                import b.module
+b/__init__.py
+b/module.py
+                                ?  # SyntaxError: invalid syntax
+"""]
+
+
+same_name_as_bad_test = [
+    "a.module",
+    ["a", "a.module", "b", "b.c"],
+    ["c"], [],
+    """\
+a/__init__.py
+a/module.py
+                                import c
+                                from b import c
+b/__init__.py
+b/c.py
+"""]
+
 
 def open_file(path):
     dirname = os.path.dirname(path)
@@ -299,6 +326,12 @@ class ModuleFinderTest(unittest.TestCase):
     def test_relative_imports_4(self):
         self._do_test(relative_import_test_4)
 
+    def test_syntax_error(self):
+        self._do_test(syntax_error_test)
+
+    def test_same_name_as_bad(self):
+        self._do_test(same_name_as_bad_test)
+
     def test_bytecode(self):
         base_path = os.path.join(TEST_DIR, 'a')
         source_path = base_path + importlib.machinery.SOURCE_SUFFIXES[0]

Misc/ACKS

@@ -222,6 +222,7 @@ Ian Bruntlett
 Floris Bruynooghe
 Matt Bryant
 Stan Bubrouski
+Brandt Bucher
 Colm Buckley
 Erik de Bueger
 Jan-Hein Bührman

Misc/NEWS.d/next/Library/2019-02-13-18-56-22.bpo-17396.oKRkrD.rst

@@ -0,0 +1,2 @@
+:mod:`modulefinder` no longer crashes when encountering syntax errors in followed imports.
+Patch by Brandt Bucher.

Misc/NEWS.d/next/Library/2019-02-13-18-56-27.bpo-35376.UFhYLj.rst

@@ -0,0 +1,2 @@
+:mod:`modulefinder` correctly handles modules that have the same name as a bad package.
+Patch by Brandt Bucher.

Misc/NEWS.d/next/Library/2019-02-16-22-19-32.bpo-35936.Ay5WtD.rst

@@ -0,0 +1,2 @@
+:mod:`modulefinder` no longer depends on the deprecated :mod:`imp` module, and the initializer for :class:`modulefinder.ModuleFinder` now has immutable default arguments.
+Patch by Brandt Bucher.