From: Martin Panter <vadmium+py@gmail.com>
Date: Thu, 3 Dec 2015 01:23:10 +0000 (+0000)
Subject: Issue #14285: Do not catch ImportError from __init__.py in runpy
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=7e59ce8b07127fade6400ea5d66a12ef4ae68475;p=python

Issue #14285: Do not catch ImportError from __init__.py in runpy

Initialize package before calling get_loader() for __main__, so that we do
not incorrectly handle ImportError from __init__.py. When runpy is used from
the Python CLI, use an internal exception rather than ImportError, to avoid
catching an unexpected ImportError.

Also simplify message formatting: str() is redundant with %s.

Also fix test_dash_m_error_code_is_one() in test_cmd_line_script, which was
failing because the test package was not in the current directlry, rather
the desired ValueError.
---

diff --git a/Lib/runpy.py b/Lib/runpy.py
index c4d7cc26a2..ad4d077a45 100644
--- a/Lib/runpy.py
+++ b/Lib/runpy.py
@@ -97,27 +97,35 @@ def _get_filename(loader, mod_name):
     return None
 
 # Helper to get the loader, code and filename for a module
-def _get_module_details(mod_name):
-    loader = get_loader(mod_name)
-    if loader is None:
-        raise ImportError("No module named %s" % mod_name)
-    if loader.is_package(mod_name):
+def _get_module_details(mod_name, error=ImportError):
+    try:
+        loader = get_loader(mod_name)
+        if loader is None:
+            raise error("No module named %s" % mod_name)
+        ispkg = loader.is_package(mod_name)
+    except ImportError as e:
+        raise error(format(e))
+    if ispkg:
         if mod_name == "__main__" or mod_name.endswith(".__main__"):
-            raise ImportError("Cannot use package as __main__ module")
+            raise error("Cannot use package as __main__ module")
+        __import__(mod_name)  # Do not catch exceptions initializing package
         try:
             pkg_main_name = mod_name + ".__main__"
             return _get_module_details(pkg_main_name)
         except ImportError, e:
-            raise ImportError(("%s; %r is a package and cannot " +
+            raise error(("%s; %r is a package and cannot " +
                                "be directly executed") %(e, mod_name))
-    code = loader.get_code(mod_name)
+    try:
+        code = loader.get_code(mod_name)
+    except ImportError as e:
+        raise error(format(e))
     if code is None:
-        raise ImportError("No code object available for %s" % mod_name)
+        raise error("No code object available for %s" % mod_name)
     filename = _get_filename(loader, mod_name)
     return mod_name, loader, code, filename
 
 
-def _get_main_module_details():
+def _get_main_module_details(error=ImportError):
     # Helper that gives a nicer error message when attempting to
     # execute a zipfile or directory by invoking __main__.py
     main_name = "__main__"
@@ -125,10 +133,13 @@ def _get_main_module_details():
         return _get_module_details(main_name)
     except ImportError as exc:
         if main_name in str(exc):
-            raise ImportError("can't find %r module in %r" %
+            raise error("can't find %r module in %r" %
                               (main_name, sys.path[0]))
         raise
 
+class _Error(Exception):
+    """Error that _run_module_as_main() should report without a traceback"""
+
 # This function is the actual implementation of the -m switch and direct
 # execution of zipfiles and directories and is deliberately kept private.
 # This avoids a repeat of the situation where run_module() no longer met the
@@ -148,11 +159,12 @@ def _run_module_as_main(mod_name, alter_argv=True):
     """
     try:
         if alter_argv or mod_name != "__main__": # i.e. -m switch
-            mod_name, loader, code, fname = _get_module_details(mod_name)
+            mod_name, loader, code, fname = _get_module_details(
+                mod_name, _Error)
         else:          # i.e. directory or zipfile execution
-            mod_name, loader, code, fname = _get_main_module_details()
-    except ImportError as exc:
-        msg = "%s: %s" % (sys.executable, str(exc))
+            mod_name, loader, code, fname = _get_main_module_details(_Error)
+    except _Error as exc:
+        msg = "%s: %s" % (sys.executable, exc)
         sys.exit(msg)
     pkg_name = mod_name.rpartition('.')[0]
     main_globals = sys.modules["__main__"].__dict__
diff --git a/Lib/test/script_helper.py b/Lib/test/script_helper.py
index 7f7c70ea06..6be47bd4ff 100644
--- a/Lib/test/script_helper.py
+++ b/Lib/test/script_helper.py
@@ -134,9 +134,9 @@ def make_zip_script(zip_dir, zip_basename, script_name, name_in_zip=None):
     #    zip_file.close()
     return zip_name, os.path.join(zip_name, name_in_zip)
 
-def make_pkg(pkg_dir):
+def make_pkg(pkg_dir, init_source=''):
     os.mkdir(pkg_dir)
-    make_script(pkg_dir, '__init__', '')
+    make_script(pkg_dir, '__init__', init_source)
 
 def make_zip_pkg(zip_dir, zip_basename, pkg_name, script_basename,
                  source, depth=1, compiled=False):
diff --git a/Lib/test/test_cmd_line_script.py b/Lib/test/test_cmd_line_script.py
index 8b05227b8b..cefa1e9900 100644
--- a/Lib/test/test_cmd_line_script.py
+++ b/Lib/test/test_cmd_line_script.py
@@ -1,5 +1,6 @@
 # Tests command line execution of scripts
 
+import contextlib
 import unittest
 import os
 import os.path
@@ -207,18 +208,69 @@ class CmdLineTest(unittest.TestCase):
             launch_name = _make_launch_script(script_dir, 'launch', 'test_pkg')
             self._check_import_error(launch_name, msg)
 
+    @contextlib.contextmanager
+    def setup_test_pkg(self, *args):
+        with temp_dir() as script_dir, \
+                test.test_support.change_cwd(script_dir):
+            pkg_dir = os.path.join(script_dir, 'test_pkg')
+            make_pkg(pkg_dir, *args)
+            yield pkg_dir
+
+    def check_dash_m_failure(self, *args):
+        rc, out, err = assert_python_failure('-m', *args)
+        if verbose > 1:
+            print(out)
+        self.assertEqual(rc, 1)
+        return err
+
     def test_dash_m_error_code_is_one(self):
         # If a module is invoked with the -m command line flag
         # and results in an error that the return code to the
         # shell is '1'
-        with temp_dir() as script_dir:
-            pkg_dir = os.path.join(script_dir, 'test_pkg')
-            make_pkg(pkg_dir)
+        with self.setup_test_pkg() as pkg_dir:
             script_name = _make_test_script(pkg_dir, 'other', "if __name__ == '__main__': raise ValueError")
-            rc, out, err = assert_python_failure('-m', 'test_pkg.other', *example_args)
-            if verbose > 1:
-                print(out)
+            err = self.check_dash_m_failure('test_pkg.other', *example_args)
+            self.assertIn(b'ValueError', err)
+
+    def test_dash_m_errors(self):
+        # Exercise error reporting for various invalid package executions
+        tests = (
+            ('__builtin__', br'No code object available'),
+            ('__builtin__.x', br'No module named'),
+            ('__builtin__.x.y', br'No module named'),
+            ('os.path', br'Loader.*cannot handle'),
+            ('importlib', br'No module named.*'
+                br'is a package and cannot be directly executed'),
+            ('importlib.nonexistant', br'No module named'),
+        )
+        for name, regex in tests:
+            rc, _, err = assert_python_failure('-m', name)
             self.assertEqual(rc, 1)
+            self.assertRegexpMatches(err, regex)
+            self.assertNotIn(b'Traceback', err)
+
+    def test_dash_m_init_traceback(self):
+        # These were wrapped in an ImportError and tracebacks were
+        # suppressed; see Issue 14285
+        exceptions = (ImportError, AttributeError, TypeError, ValueError)
+        for exception in exceptions:
+            exception = exception.__name__
+            init = "raise {0}('Exception in __init__.py')".format(exception)
+            with self.setup_test_pkg(init) as pkg_dir:
+                err = self.check_dash_m_failure('test_pkg')
+                self.assertIn(exception.encode('ascii'), err)
+                self.assertIn(b'Exception in __init__.py', err)
+                self.assertIn(b'Traceback', err)
+
+    def test_dash_m_main_traceback(self):
+        # Ensure that an ImportError's traceback is reported
+        with self.setup_test_pkg() as pkg_dir:
+            main = "raise ImportError('Exception in __main__ module')"
+            _make_test_script(pkg_dir, '__main__', main)
+            err = self.check_dash_m_failure('test_pkg')
+            self.assertIn(b'ImportError', err)
+            self.assertIn(b'Exception in __main__ module', err)
+            self.assertIn(b'Traceback', err)
 
 
 def test_main():
diff --git a/Lib/test/test_runpy.py b/Lib/test/test_runpy.py
index 76858d53a2..7f9fefaaaa 100644
--- a/Lib/test/test_runpy.py
+++ b/Lib/test/test_runpy.py
@@ -270,6 +270,30 @@ from ..uncle.cousin import nephew
             if verbose: print "Testing package depth:", depth
             self._check_package(depth)
 
+    def test_run_package_init_exceptions(self):
+        # These were previously wrapped in an ImportError; see Issue 14285
+        exceptions = (ImportError, AttributeError, TypeError, ValueError)
+        for exception in exceptions:
+            name = exception.__name__
+            source = "raise {0}('{0} in __init__.py.')".format(name)
+
+            result = self._make_pkg("", 1, "__main__")
+            pkg_dir, _, mod_name = result
+            mod_name = mod_name.replace(".__main__", "")
+            try:
+                init = os.path.join(pkg_dir, "__runpy_pkg__", "__init__.py")
+                with open(init, "wt") as mod_file:
+                    mod_file.write(source)
+                try:
+                    run_module(mod_name)
+                except exception as err:
+                    msg = "cannot be directly executed"
+                    self.assertNotIn(msg, format(err))
+                else:
+                    self.fail("Nothing raised; expected {}".format(name))
+            finally:
+                self._del_pkg(pkg_dir, 1, mod_name)
+
     def test_explicit_relative_import(self):
         for depth in range(2, 5):
             if verbose: print "Testing relative imports at depth:", depth
diff --git a/Misc/NEWS b/Misc/NEWS
index b073ec653d..0b98f21e25 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -22,6 +22,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #14285: When executing a package with the "python -m package" option,
+  and package initialization raises ImportError, a proper traceback is now
+  reported.
+
 - Issue #6478: _strptime's regexp cache now is reset after changing timezone
   with time.tzset().