Close #15486: Simplify the mechanism used to remove importlib frames from...

Close #15486: Simplify the mechanism used to remove importlib frames from tracebacks when they just introduce irrelevant noise

Lib/importlib/_bootstrap.py

@@ -297,8 +297,20 @@ def _lock_unlock_module(name):
     else:
         lock.release()
 
+# Frame stripping magic ###############################################
 
-# Finder/loader utility code ##################################################
+def _call_with_frames_removed(f, *args, **kwds):
+    """remove_importlib_frames in import.c will always remove sequences
+    of importlib frames that end with a call to this function
+
+    Use it instead of a normal call in places where including the importlib
+    frames introduces unwanted noise into the traceback (e.g. when executing
+    module code)
+    """
+    return f(*args, **kwds)
+
+
+# Finder/loader utility code ###############################################
 
 """Magic word to reject .pyc files generated by other Python versions.
 It should change for each incompatible change to the bytecode.
@@ -629,19 +641,12 @@ class BuiltinImporter:
         """Load a built-in module."""
         is_reload = fullname in sys.modules
         try:
-            return cls._exec_module(fullname)
+            return _call_with_frames_removed(_imp.init_builtin, fullname)
         except:
             if not is_reload and fullname in sys.modules:
                 del sys.modules[fullname]
             raise
 
-    @classmethod
-    def _exec_module(cls, fullname):
-        """Helper for load_module, allowing to isolate easily (when
-        looking at a traceback) whether an error comes from executing
-        an imported module's code."""
-        return _imp.init_builtin(fullname)
-
     @classmethod
     @_requires_builtin
     def get_code(cls, fullname):
@@ -687,7 +692,7 @@ class FrozenImporter:
         """Load a frozen module."""
         is_reload = fullname in sys.modules
         try:
-            m = cls._exec_module(fullname)
+            m = _call_with_frames_removed(_imp.init_frozen, fullname)
             # Let our own module_repr() method produce a suitable repr.
             del m.__file__
             return m
@@ -714,13 +719,6 @@ class FrozenImporter:
         """Return if the frozen module is a package."""
         return _imp.is_frozen_package(fullname)
 
-    @classmethod
-    def _exec_module(cls, fullname):
-        """Helper for load_module, allowing to isolate easily (when
-        looking at a traceback) whether an error comes from executing
-        an imported module's code."""
-        return _imp.init_frozen(fullname)
-
 
 class WindowsRegistryImporter:
 
@@ -850,15 +848,9 @@ class _LoaderBasics:
         else:
             module.__package__ = module.__package__.rpartition('.')[0]
         module.__loader__ = self
-        self._exec_module(code_object, module.__dict__)
+        _call_with_frames_removed(exec, code_object, module.__dict__)
         return module
 
-    def _exec_module(self, code_object, module_dict):
-        """Helper for _load_module, allowing to isolate easily (when
-        looking at a traceback) whether an error comes from executing
-        an imported module's code."""
-        exec(code_object, module_dict)
-
 
 class SourceLoader(_LoaderBasics):
 
@@ -956,7 +948,8 @@ class SourceLoader(_LoaderBasics):
                             raise ImportError(msg.format(bytecode_path),
                                               name=fullname, path=bytecode_path)
         source_bytes = self.get_data(source_path)
-        code_object = compile(source_bytes, source_path, 'exec',
+        code_object = _call_with_frames_removed(compile,
+                          source_bytes, source_path, 'exec',
                           dont_inherit=True)
         _verbose_message('code object from {}', source_path)
         if (not sys.dont_write_bytecode and bytecode_path is not None and
@@ -1093,7 +1086,8 @@ class ExtensionFileLoader:
         """Load an extension module."""
         is_reload = fullname in sys.modules
         try:
-            module = self._exec_module(fullname, self.path)
+            module = _call_with_frames_removed(_imp.load_dynamic,
+                                               fullname, self.path)
             _verbose_message('extension module loaded from {!r}', self.path)
             return module
         except:
@@ -1113,12 +1107,6 @@ class ExtensionFileLoader:
         """Return None as extension modules have no source code."""
         return None
 
-    def _exec_module(self, fullname, path):
-        """Helper for load_module, allowing to isolate easily (when
-        looking at a traceback) whether an error comes from executing
-        an imported module's code."""
-        return _imp.load_dynamic(fullname, path)
-
 
 class _NamespacePath:
     """Represents a namespace package's path.  It uses the module name
@@ -1472,7 +1460,7 @@ def _find_and_load_unlocked(name, import_):
     parent = name.rpartition('.')[0]
     if parent:
         if parent not in sys.modules:
-            _recursive_import(import_, parent)
+            _call_with_frames_removed(import_, parent)
         # Crazy side-effects!
         if name in sys.modules:
             return sys.modules[name]
@@ -1550,13 +1538,6 @@ def _gcd_import(name, package=None, level=0):
     _lock_unlock_module(name)
     return module
 
-def _recursive_import(import_, name):
-    """Common exit point for recursive calls to the import machinery
-
-    This simplifies the process of stripping importlib from tracebacks
-    """
-    return import_(name)
-
 def _handle_fromlist(module, fromlist, import_):
     """Figure out what __import__ should return.
 
@@ -1575,7 +1556,7 @@ def _handle_fromlist(module, fromlist, import_):
                 fromlist.extend(module.__all__)
         for x in fromlist:
             if not hasattr(module, x):
-                _recursive_import(import_,
+                _call_with_frames_removed(import_,
                                   '{}.{}'.format(module.__name__, x))
     return module
 

Lib/test/test_import.py

@@ -785,11 +785,13 @@ class ImportTracebackTests(unittest.TestCase):
         sys.path[:] = self.old_path
         rmtree(TESTFN)
 
-    def create_module(self, mod, contents):
-        with open(os.path.join(TESTFN, mod + ".py"), "w") as f:
+    def create_module(self, mod, contents, ext=".py"):
+        fname = os.path.join(TESTFN, mod + ext)
+        with open(fname, "w") as f:
             f.write(contents)
         self.addCleanup(unload, mod)
         importlib.invalidate_caches()
+        return fname
 
     def assert_traceback(self, tb, files):
         deduped_files = []

Python/import.c

@@ -1153,9 +1153,7 @@ static void
 remove_importlib_frames(void)
 {
     const char *importlib_filename = "<frozen importlib._bootstrap>";
-    const char *exec_funcname = "_exec_module";
-    const char *get_code_funcname = "get_code";
-    const char *recursive_import = "_recursive_import";
+    const char *remove_frames = "_call_with_frames_removed";
     int always_trim = 0;
     int trim_get_code = 0;
     int in_importlib = 0;
@@ -1163,18 +1161,8 @@ remove_importlib_frames(void)
     PyObject **prev_link, **outer_link = NULL;
 
     /* Synopsis: if it's an ImportError, we trim all importlib chunks
-       from the traceback.  If it's a SyntaxError, we trim any chunks that
-       end with a call to "get_code", We always trim chunks
-       which end with a call to "_exec_module". */
-
-    /* Thanks to issue 15425, we also strip any chunk ending with
-     * _recursive_import. This is used when making a recursive call to the
-     * full import machinery which means the inner stack gets stripped early
-     * and the normal heuristics won't fire properly for outer frames. A
-     * more elegant mechanism would be nice, as this one can misfire if
-     * builtins.__import__ has been replaced with a custom implementation.
-     * However, the current approach at least gets the job done.
-     */
+       from the traceback. We always trim chunks
+       which end with a call to "_call_with_frames_removed". */
 
     PyErr_Fetch(&exception, &value, &base_tb);
     if (!exception || Py_VerboseFlag)
@@ -1207,14 +1195,8 @@ remove_importlib_frames(void)
 
         if (in_importlib &&
             (always_trim ||
-             (PyUnicode_CompareWithASCIIString(code->co_name,
-                                              exec_funcname) == 0) ||
-             (PyUnicode_CompareWithASCIIString(code->co_name,
-                                              recursive_import) == 0) ||
-             (trim_get_code &&
              PyUnicode_CompareWithASCIIString(code->co_name,
-                                              get_code_funcname) == 0)
-           )) {
+                                              remove_frames) == 0)) {
             PyObject *tmp = *outer_link;
             *outer_link = next;
             Py_XINCREF(next);