bpo-24792: Fix zipimporter masking the cause of import errors (GH-22204)

zipimport's _unmarshal_code swallows import errors and then _get_module_code doesn't know the cause of the error, and returns the generic, and sometimes incorrect, 'could not find...'.

Automerge-Triggered-By: GH:brettcannon

Doc/library/zipimport.rst

@@ -121,7 +121,7 @@ zipimporter Objects
    .. method:: get_code(fullname)
 
       Return the code object for the specified module. Raise
-      :exc:`ZipImportError` if the module couldn't be found.
+      :exc:`ZipImportError` if the module couldn't be imported.
 
 
    .. method:: get_data(pathname)
@@ -137,7 +137,7 @@ zipimporter Objects
 
       Return the value ``__file__`` would be set to if the specified module
       was imported. Raise :exc:`ZipImportError` if the module couldn't be
-      found.
+      imported.
 
       .. versionadded:: 3.1
 
@@ -159,14 +159,13 @@ zipimporter Objects
    .. method:: load_module(fullname)
 
       Load the module specified by *fullname*. *fullname* must be the fully
-      qualified (dotted) module name. It returns the imported module, or raises
+      qualified (dotted) module name. Returns the imported module on success,
-      :exc:`ZipImportError` if it wasn't found.
+      raises :exc:`ZipImportError` on failure.
 
       .. deprecated:: 3.10
 
          Use :meth:`exec_module` instead.
 
-
    .. attribute:: archive
 
       The file name of the importer's associated ZIP file, without a possible

Lib/test/test_zipimport.py

@@ -242,10 +242,10 @@ class UncompressedZipImportTestCase(ImportHooksBaseTestCase):
         files = {TESTMOD + pyc_ext: (NOW, badmagic_pyc)}
         try:
             self.doTest(".py", files, TESTMOD)
-        except ImportError:
+            self.fail("This should not be reached")
-            pass
+        except zipimport.ZipImportError as exc:
-        else:
+            self.assertIsInstance(exc.__cause__, ImportError)
-            self.fail("expected ImportError; import from bad pyc")
+            self.assertIn("magic number", exc.__cause__.msg)
 
     def testBadMTime(self):
         badtime_pyc = bytearray(test_pyc)

Lib/zipimport.py

@@ -185,7 +185,7 @@ class zipimporter(_bootstrap_external._LoaderBasics):
         """get_code(fullname) -> code object.
 
         Return the code object for the specified module. Raise ZipImportError
-        if the module couldn't be found.
+        if the module couldn't be imported.
         """
         code, ispackage, modpath = _get_module_code(self, fullname)
         return code
@@ -215,7 +215,8 @@ class zipimporter(_bootstrap_external._LoaderBasics):
     def get_filename(self, fullname):
         """get_filename(fullname) -> filename string.
 
-        Return the filename for the specified module.
+        Return the filename for the specified module or raise ZipImportError
+        if it couldn't be imported.
         """
         # Deciding the filename requires working out where the code
         # would come from if the module was actually loaded
@@ -267,7 +268,7 @@ class zipimporter(_bootstrap_external._LoaderBasics):
 
         Load the module specified by 'fullname'. 'fullname' must be the
         fully qualified (dotted) module name. It returns the imported
-        module, or raises ZipImportError if it wasn't found.
+        module, or raises ZipImportError if it could not be imported.
 
         Deprecated since Python 3.10. Use exec_module() instead.
         """
@@ -613,20 +614,15 @@ def _eq_mtime(t1, t2):
 
 
 # Given the contents of a .py[co] file, unmarshal the data
-# and return the code object. Return None if it the magic word doesn't
+# and return the code object. Raises ImportError it the magic word doesn't
-# match, or if the recorded .py[co] metadata does not match the source,
+# match, or if the recorded .py[co] metadata does not match the source.
-# (we do this instead of raising an exception as we fall back
-# to .py if available and we don't want to mask other errors).
 def _unmarshal_code(self, pathname, fullpath, fullname, data):
     exc_details = {
         'name': fullname,
         'path': fullpath,
     }
 
-    try:
+    flags = _bootstrap_external._classify_pyc(data, fullname, exc_details)
-        flags = _bootstrap_external._classify_pyc(data, fullname, exc_details)
-    except ImportError:
-        return None
 
     hash_based = flags & 0b1 != 0
     if hash_based:
@@ -640,11 +636,8 @@ def _unmarshal_code(self, pathname, fullpath, fullname, data):
                     source_bytes,
                 )
 
-                try:
+                _bootstrap_external._validate_hash_pyc(
-                    _bootstrap_external._validate_hash_pyc(
+                    data, source_hash, fullname, exc_details)
-                        data, source_hash, fullname, exc_details)
-                except ImportError:
-                    return None
     else:
         source_mtime, source_size = \
             _get_mtime_and_size_of_source(self, fullpath)
@@ -730,6 +723,7 @@ def _get_pyc_source(self, path):
 # 'fullname'.
 def _get_module_code(self, fullname):
     path = _get_module_path(self, fullname)
+    import_error = None
     for suffix, isbytecode, ispackage in _zip_searchorder:
         fullpath = path + suffix
         _bootstrap._verbose_message('trying {}{}{}', self.archive, path_sep, fullpath, verbosity=2)
@@ -740,8 +734,12 @@ def _get_module_code(self, fullname):
         else:
             modpath = toc_entry[0]
             data = _get_data(self.archive, toc_entry)
+            code = None
             if isbytecode:
-                code = _unmarshal_code(self, modpath, fullpath, fullname, data)
+                try:
+                    code = _unmarshal_code(self, modpath, fullpath, fullname, data)
+                except ImportError as exc:
+                    import_error = exc
             else:
                 code = _compile_source(modpath, data)
             if code is None:
@@ -751,4 +749,8 @@ def _get_module_code(self, fullname):
             modpath = toc_entry[0]
             return code, ispackage, modpath
     else:
-        raise ZipImportError(f"can't find module {fullname!r}", name=fullname)
+        if import_error:
+            msg = f"module load failed: {import_error}"
+            raise ZipImportError(msg, name=fullname) from import_error
+        else:
+            raise ZipImportError(f"can't find module {fullname!r}", name=fullname)

Misc/NEWS.d/next/Library/2020-09-11-16-07-00.bpo-24792.Z-ARra.rst

@@ -0,0 +1 @@
+Fixed bug where :mod:`zipimporter` sometimes reports an incorrect cause of import errors.