Issue #18070: importlib.util.module_for_loader() now sets __loader__

and __package__ unconditionally in order to do the right thing for
reloading.

Doc/library/importlib.rst

@@ -811,12 +811,11 @@ an :term:`importer`.
 
     The decorated method will take in the **name** of the module to be loaded
     as expected for a :term:`loader`. If the module is not found in
-    :data:`sys.modules` then a new one is constructed with its
-    :attr:`__name__` attribute set to **name**, :attr:`__loader__` set to
-    **self**, and :attr:`__package__` set based on what
-    :meth:`importlib.abc.InspectLoader.is_package` returns (if available). If a
-    new module is not needed then the module found in :data:`sys.modules` will
-    be passed into the method.
+    :data:`sys.modules` then a new one is constructed. Regardless of where the
+    module came from, :attr:`__loader__` set to **self** and :attr:`__package__`
+    is set based on what :meth:`importlib.abc.InspectLoader.is_package` returns
+    (if available). These attributes are set unconditionally to support
+    reloading.
 
     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
@@ -831,6 +830,10 @@ an :term:`importer`.
        :attr:`__loader__` and :attr:`__package__` are automatically set
        (when possible).
 
+    .. versionchanged:: 3.4
+       Set :attr:`__loader__` :attr:`__package__` unconditionally to support
+       reloading.
+
 .. decorator:: set_loader
 
    A :term:`decorator` for :meth:`importlib.abc.Loader.load_module`
@@ -843,12 +846,13 @@ an :term:`importer`.
    .. note::
       As this decorator sets :attr:`__loader__` after loading the module, it is
       recommended to use :func:`module_for_loader` instead when appropriate.
+      This decorator is also redundant as of Python 3.3 as import itself will
+      set these attributes post-import if necessary.
 
    .. versionchanged:: 3.4
       Set ``__loader__`` if set to ``None``, as if the attribute does not
       exist.
 
-
 .. decorator:: set_package
 
    A :term:`decorator` for :meth:`importlib.abc.Loader.load_module` to set the :attr:`__package__` attribute on the returned module. If :attr:`__package__`
@@ -856,4 +860,6 @@ an :term:`importer`.
 
    .. note::
       As this decorator sets :attr:`__package__` after loading the module, it is
-      recommended to use :func:`module_for_loader` instead when appropriate.
+      recommended to use :func:`module_for_loader` instead when appropriate. As
+      of Python 3.3 this decorator is also redundant as import will set
+      :attr:`__package__` post-import if necessary.

Doc/whatsnew/3.4.rst

@@ -250,4 +250,9 @@ that may require changes to your code.
 * The module type now initializes the :attr:`__package__` and :attr:`__loader__`
   attributes to ``None`` by default. To determine if these attributes were set
   in a backwards-compatible fashion, use e.g.
-  ``getattr(module, '__loader__', None) is not None``.
+  ``getattr(module, '__loader__', None) is not None``.
+
+* :meth:`importlib.util.module_for_loader` now sets ``__loader__`` and
+  ``__package__`` unconditionally to properly support reloading. If this is not
+  desired then you will need to set these attributes manually. You can use
+  :class:`importlib.util.ModuleManager` for module management.

Lib/importlib/_bootstrap.py

@@ -37,23 +37,13 @@ def _make_relax_case():
     return _relax_case
 
 
-# TODO: Expose from marshal
 def _w_long(x):
-    """Convert a 32-bit integer to little-endian.
-
-    XXX Temporary until marshal's long functions are exposed.
-
-    """
+    """Convert a 32-bit integer to little-endian."""
     return (int(x) & 0xFFFFFFFF).to_bytes(4, 'little')
 
 
-# TODO: Expose from marshal
 def _r_long(int_bytes):
-    """Convert 4 bytes in little-endian to an integer.
-
-    XXX Temporary until marshal's long function are exposed.
-
-    """
+    """Convert 4 bytes in little-endian to an integer."""
     return int.from_bytes(int_bytes, 'little')
 
 
@@ -569,17 +559,7 @@ def module_for_loader(fxn):
 
     """
     def module_for_loader_wrapper(self, fullname, *args, **kwargs):
-        module = sys.modules.get(fullname)
-        is_reload = module is not None
-        if not is_reload:
-            # This must be done before open() is called as the 'io' module
-            # implicitly imports 'locale' and would otherwise trigger an
-            # infinite loop.
-            module = new_module(fullname)
-            # This must be done before putting the module in sys.modules
-            # (otherwise an optimization shortcut in import.c becomes wrong)
-            module.__initializing__ = True
-            sys.modules[fullname] = module
+        with ModuleManager(fullname) as module:
             module.__loader__ = self
             try:
                 is_package = self.is_package(fullname)
@@ -590,17 +570,8 @@ def module_for_loader(fxn):
                     module.__package__ = fullname
                 else:
                     module.__package__ = fullname.rpartition('.')[0]
-        else:
-            module.__initializing__ = True
-        try:
             # If __package__ was not set above, __import__() will do it later.
             return fxn(self, module, *args, **kwargs)
-        except:
-            if not is_reload:
-                del sys.modules[fullname]
-            raise
-        finally:
-            module.__initializing__ = False
     _wrap(module_for_loader_wrapper, fxn)
     return module_for_loader_wrapper
 

Lib/test/test_importlib/test_util.py

@@ -85,12 +85,23 @@ class ModuleForLoaderTests(unittest.TestCase):
 
     def test_reload(self):
         # Test that a module is reused if already in sys.modules.
+        class FakeLoader:
+            def is_package(self, name):
+                return True
+            @util.module_for_loader
+            def load_module(self, module):
+                return module
         name = 'a.b.c'
         module = imp.new_module('a.b.c')
+        module.__loader__ = 42
+        module.__package__ = 42
         with test_util.uncache(name):
             sys.modules[name] = module
-            returned_module = self.return_module(name)
+            loader = FakeLoader()
+            returned_module = loader.load_module(name)
             self.assertIs(returned_module, sys.modules[name])
+            self.assertEqual(module.__loader__, loader)
+            self.assertEqual(module.__package__, name)
 
     def test_new_module_failure(self):
         # Test that a module is removed from sys.modules if added but an

Misc/NEWS

@@ -96,6 +96,9 @@ Core and Builtins
 Library
 -------
 
+- Issue #18070: Have importlib.util.module_for_loader() set attributes
+  unconditionally in order to properly support reloading.
+
 - Add importlib.util.ModuleManager as a context manager to provide the proper
   module object to load.