Implement the more specific PEP 302 semantics for loaders and what happens upon

load failure in relation to reloads. Also expose
importlib.util.module_for_loader to handle all of the details of this along
with making sure all current loaders behave nicely.

Doc/library/importlib.rst

@@ -151,3 +151,36 @@ find and load modules.
         searched for a finder for the path entry and, if found, is stored in
         :data:`sys.path_importer_cache` along with being queried about the
         module.
+
+
+:mod:`importlib.util` -- Utility code for importers
+---------------------------------------------------
+
+.. module:: importlib.util
+    :synopsis: Importers and path hooks
+
+This module contains the various objects that help in the construction of
+an :term:`importer`.
+
+.. function:: module_for_loader(method)
+
+    A :term:`decorator` for a :term:`loader` which handles selecting the proper
+    module object to load with. The decorated method is expected to have a call
+    signature of ``method(self, module_object)`` for which the second argument
+    will be the module object to be used (note that the decorator will not work
+    on static methods because of the assumption of two arguments).
+
+    The decorated method will take in the name of the module to be loaded as
+    normal. If the module is not found in :data:`sys.modules` then a new one is
+    constructed with its :attr:`__name__` attribute set. Otherwise the module
+    found in :data:`sys.modules` will be passed into the method. If an
+    exception is raised by the decorated method and a module was added to
+    :data:`sys.modules` it will be removed to prevent a partially initialized
+    module from being in left in :data:`sys.modules` If an exception is raised
+    by the decorated method and a module was added to :data:`sys.modules` it
+    will be removed to prevent a partially initialized module from being in
+    left in :data:`sys.modules`. If the module was already in
+    :data:`sys.modules` then it is left alone.
+
+    Use of this decorator handles all the details of what module a loader
+    should use as specified by :pep:`302`.

Lib/importlib/NOTES

@@ -1,17 +1,12 @@
 to do
 /////
 
-* Change failed loading based on PEP 302 changes.
-
 * Refactor source/bytecode finder/loader code such that bytecode support is a
   subclass of source support (makes it nicer for VMs that don't use CPython
   bytecode).
 
 * Implement PEP 302 protocol for loaders (should just be a matter of testing).
 
-    + Built-in.
-    + Frozen.
-    + Extension.
     + Source/bytecode.
 
 * Public API left to expose (w/ docs!)
@@ -27,7 +22,6 @@ to do
             * load_module
 
         - (?) Importer(Finder, Loader)
-
         - ResourceLoader(Loader)
 
             * get_data
@@ -46,7 +40,6 @@ to do
 
     + util
 
-      - get_module decorator (rename: module_for_loader)
         - set___package__ decorator
 
     + machinery
@@ -62,7 +55,12 @@ to do
             * SourceFinder
             * (?) Loader
 
-      - PathFinder
+    + test
+
+        - abc
+
+            * FinderTests [doc]
+            * LoaderTests [doc]
 
 * Make sure that there is documentation *somewhere* fully explaining the
 semantics of import that can be referenced from the package's documentation

Lib/importlib/_bootstrap.py

@@ -136,8 +136,13 @@ def load_module(cls, fullname):
         """Load a built-in module."""
         if fullname not in sys.builtin_module_names:
             raise ImportError("{0} is not a built-in module".format(fullname))
-        module = imp.init_builtin(fullname)
+        is_reload = fullname in sys.modules
-        return module
+        try:
+            return imp.init_builtin(fullname)
+        except:
+            if not is_reload and fullname in sys.modules:
+                del sys.modules[fullname]
+            raise
 
 
 class FrozenImporter:
@@ -160,8 +165,13 @@ def load_module(cls, fullname):
         """Load a frozen module."""
         if cls.find_module(fullname) is None:
             raise ImportError("{0} is not a frozen module".format(fullname))
-        module = imp.init_frozen(fullname)
+        is_reload = fullname in sys.modules
-        return module
+        try:
+            return imp.init_frozen(fullname)
+        except:
+            if not is_reload and fullname in sys.modules:
+                del sys.modules[fullname]
+            raise
 
 
 class ChainedImporter(object):
@@ -249,14 +259,13 @@ def __init__(self, name, path, is_pkg):
     @set___package__
     def load_module(self, fullname):
         """Load an extension module."""
-        assert self._name == fullname
+        is_reload = fullname in sys.modules
         try:
             module = imp.load_dynamic(fullname, self._path)
             module.__loader__ = self
             return module
         except:
-            # If an error occurred, don't leave a partially initialized module.
+            if not is_reload and fullname in sys.modules:
-            if fullname in sys.modules:
                 del sys.modules[fullname]
             raise
 
@@ -282,16 +291,17 @@ def suffix_list(suffix_type):
             if suffix[2] == suffix_type]
 
 
-# XXX Need a better name.
+def module_for_loader(fxn):
-def get_module(fxn):
+    """Decorator to handle selecting the proper module for loaders.
-    """Decorator to handle selecting the proper module for load_module
-    implementations.
 
     Decorated modules are passed the module to use instead of the module name.
     The module is either from sys.modules if it already exists (for reloading)
     or is a new module which has __name__ set. If any exception is raised by
-    the decorated method then __loader__, __name__, __file__, and __path__ are
+    the decorated method and the decorator added a module to sys.modules, then
-    all restored on the module to their original values.
+    the module is deleted from sys.modules.
+
+    The decorator assumes that the decorated method takes self/cls as a first
+    argument and the module as the second argument.
 
     """
     def decorated(self, fullname):
@@ -302,27 +312,12 @@ def decorated(self, fullname):
             # implicitly imports 'locale' and would otherwise trigger an
             # infinite loop.
             module = imp.new_module(fullname)
-            module.__name__ = fullname
             sys.modules[fullname] = module
-        else:
-            original_values = {}
-            modified_attrs = ['__loader__', '__name__', '__file__', '__path__']
-            for attr in modified_attrs:
-                try:
-                    original_values[attr] = getattr(module, attr)
-                except AttributeError:
-                    pass
         try:
             return fxn(self, module)
         except:
             if not is_reload:
                 del sys.modules[fullname]
-            else:
-                for attr in modified_attrs:
-                    if attr in original_values:
-                        setattr(module, attr, original_values[attr])
-                    elif hasattr(module, attr):
-                        delattr(module, attr)
             raise
     wrap(decorated, fxn)
     return decorated
@@ -375,7 +370,7 @@ def bytecode_path(self, fullname):
         return self._find_path(imp.PY_COMPILED)
 
     @check_name
-    @get_module
+    @module_for_loader
     def load_module(self, module):
         """Load a Python source or bytecode module."""
         name = module.__name__

Lib/importlib/test/test_util.py

@@ -0,0 +1,69 @@
+from importlib import util
+from . import util as test_util
+import imp
+import sys
+import types
+import unittest
+
+
+class ModuleForLoaderTests(unittest.TestCase):
+
+    """Tests for importlib.util.module_for_loader."""
+
+    def return_module(self, name):
+        fxn = util.module_for_loader(lambda self, module: module)
+        return fxn(self, name)
+
+    def raise_exception(self, name):
+        def to_wrap(self, module):
+            raise ImportError
+        fxn = util.module_for_loader(to_wrap)
+        try:
+            fxn(self, name)
+        except ImportError:
+            pass
+
+    def test_new_module(self):
+        # Test that when no module exists in sys.modules a new module is
+        # created.
+        module_name = 'a.b.c'
+        with test_util.uncache(module_name):
+            module = self.return_module(module_name)
+            self.assert_(module_name in sys.modules)
+        self.assert_(isinstance(module, types.ModuleType))
+        self.assertEqual(module.__name__, module_name)
+
+    def test_reload(self):
+        # Test that a module is reused if already in sys.modules.
+        name = 'a.b.c'
+        module = imp.new_module('a.b.c')
+        with test_util.uncache(name):
+            sys.modules[name] = module
+            returned_module = self.return_module(name)
+            self.assert_(sys.modules[name] is returned_module)
+
+    def test_new_module_failure(self):
+        # Test that a module is removed from sys.modules if added but an
+        # exception is raised.
+        name = 'a.b.c'
+        with test_util.uncache(name):
+            self.raise_exception(name)
+            self.assert_(name not in sys.modules)
+
+    def test_reload_failure(self):
+        # Test that a failure on reload leaves the module in-place.
+        name = 'a.b.c'
+        module = imp.new_module(name)
+        with test_util.uncache(name):
+            sys.modules[name] = module
+            self.raise_exception(name)
+            self.assert_(sys.modules[name] is module)
+
+
+def test_main():
+    from test import support
+    support.run_unittest(ModuleForLoaderTests)
+
+
+if __name__ == '__main__':
+    test_main()

Lib/importlib/util.py

@@ -0,0 +1,2 @@
+"""Utility code for constructing importers, etc."""
+from ._bootstrap import module_for_loader