PEP 489: Multi-phase extension module initialization

Known limitations of the current implementation:

- documentation changes are incomplete
- there's a reference leak I haven't tracked down yet

The leak is most visible by running:

  ./python -m test -R3:3 test_importlib

However, you can also see it by running:

  ./python -X showrefcount

Importing the array or _testmultiphase modules, and
then deleting them from both sys.modules and the local
namespace shows significant increases in the total
number of active references each cycle. By contrast,
with _testcapi (which continues to use single-phase
initialisation) the global refcounts stabilise after
a couple of cycles.

4 files changed, 224 insertions, 32 deletions

diff --git a/Lib/imp.py b/Lib/imp.py
index 3177b28..2cd6440 100644
--- a/Lib/imp.py
+++ b/Lib/imp.py
@@ -8,15 +8,15 @@ functionality over this module.
 # (Probably) need to stay in _imp
 from _imp import (lock_held, acquire_lock, release_lock,
                   get_frozen_object, is_frozen_package,
-                  init_builtin, init_frozen, is_builtin, is_frozen,
+                  init_frozen, is_builtin, is_frozen,
                   _fix_co_filename)
 try:
-    from _imp import load_dynamic
+    from _imp import create_dynamic
 except ImportError:
     # Platform doesn't support dynamic loading.
-    load_dynamic = None
+    create_dynamic = None
 
-from importlib._bootstrap import _ERR_MSG, _exec, _load
+from importlib._bootstrap import _ERR_MSG, _exec, _load, _builtin_from_name
 from importlib._bootstrap_external import SourcelessFileLoader
 
 from importlib import machinery
@@ -312,3 +312,28 @@ def reload(module):
 
     """
     return importlib.reload(module)
+
+
+def init_builtin(name):
+    """**DEPRECATED**
+
+    Load and return a built-in module by name, or None is such module doesn't
+    exist
+    """
+    try:
+        return _builtin_from_name(name)
+    except ImportError:
+        return None
+
+
+if create_dynamic:
+    def load_dynamic(name, path, file=None):
+        """**DEPRECATED**
+
+        Load an extension module.
+        """
+        import importlib.machinery
+        loader = importlib.machinery.ExtensionFileLoader(name, path)
+        return loader.load_module()
+else:
+    load_dynamic = None
diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
index 860703c..931754e 100644
--- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -735,16 +735,17 @@ class BuiltinImporter:
         return spec.loader if spec is not None else None
 
     @classmethod
-    @_requires_builtin
-    def load_module(cls, fullname):
-        """Load a built-in module."""
-        # Once an exec_module() implementation is added we can also
-        # add a deprecation warning here.
-        with _ManageReload(fullname):
-            module = _call_with_frames_removed(_imp.init_builtin, fullname)
-        module.__loader__ = cls
-        module.__package__ = ''
-        return module
+    def create_module(self, spec):
+        """Create a built-in module"""
+        if spec.name not in sys.builtin_module_names:
+            raise ImportError('{!r} is not a built-in module'.format(spec.name),
+                              name=spec.name)
+        return _call_with_frames_removed(_imp.create_builtin, spec)
+
+    @classmethod
+    def exec_module(self, module):
+        """Exec a built-in module"""
+        _call_with_frames_removed(_imp.exec_dynamic, module)
 
     @classmethod
     @_requires_builtin
@@ -764,6 +765,8 @@ class BuiltinImporter:
         """Return False as built-in modules are never packages."""
         return False
 
+    load_module = classmethod(_load_module_shim)
+
 
 class FrozenImporter:
 
diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py
index f176961..510fa92 100644
--- a/Lib/importlib/_bootstrap_external.py
+++ b/Lib/importlib/_bootstrap_external.py
@@ -378,7 +378,8 @@ def _check_name(method):
         if name is None:
             name = self.name
         elif self.name != name:
-            raise ImportError('loader cannot handle %s' % name, name=name)
+            raise ImportError('loader for %s cannot handle %s' %
+                                (self.name, name), name=name)
         return method(self, name, *args, **kwargs)
     try:
         _wrap = _bootstrap._wrap
@@ -875,7 +876,7 @@ class SourcelessFileLoader(FileLoader, _LoaderBasics):
 EXTENSION_SUFFIXES = []
 
 
-class ExtensionFileLoader:
+class ExtensionFileLoader(FileLoader, _LoaderBasics):
 
     """Loader for extension modules.
 
@@ -894,24 +895,20 @@ class ExtensionFileLoader:
     def __hash__(self):
         return hash(self.name) ^ hash(self.path)
 
-    @_check_name
-    def load_module(self, fullname):
-        """Load an extension module."""
-        # Once an exec_module() implementation is added we can also
-        # add a deprecation warning here.
-        with _bootstrap._ManageReload(fullname):
-            module = _bootstrap._call_with_frames_removed(_imp.load_dynamic,
-                                                          fullname, self.path)
-        _verbose_message('extension module loaded from {!r}', self.path)
-        is_package = self.is_package(fullname)
-        if is_package and not hasattr(module, '__path__'):
-            module.__path__ = [_path_split(self.path)[0]]
-        module.__loader__ = self
-        module.__package__ = module.__name__
-        if not is_package:
-            module.__package__ = module.__package__.rpartition('.')[0]
+    def create_module(self, spec):
+        """Create an unitialized extension module"""
+        module = _bootstrap._call_with_frames_removed(
+            _imp.create_dynamic, spec)
+        _verbose_message('extension module {!r} loaded from {!r}',
+                         spec.name, self.path)
         return module
 
+    def exec_module(self, module):
+        """Initialize an extension module"""
+        _bootstrap._call_with_frames_removed(_imp.exec_dynamic, module)
+        _verbose_message('extension module {!r} executed from {!r}',
+                         self.name, self.path)
+
     def is_package(self, fullname):
         """Return True if the extension module is a package."""
         file_name = _path_split(self.path)[1]
diff --git a/Lib/test/test_importlib/extension/test_loader.py b/Lib/test/test_importlib/extension/test_loader.py
index aefd050..66ac2b1 100644
--- a/Lib/test/test_importlib/extension/test_loader.py
+++ b/Lib/test/test_importlib/extension/test_loader.py
@@ -7,6 +7,8 @@ import os.path
 import sys
 import types
 import unittest
+import importlib.util
+import importlib
 
 
 class LoaderTests(abc.LoaderTests):
@@ -80,6 +82,171 @@ class LoaderTests(abc.LoaderTests):
  Source_LoaderTests
  ) = util.test_both(LoaderTests, machinery=machinery)
 
+class MultiPhaseExtensionModuleTests(abc.LoaderTests):
+    """Test loading extension modules with multi-phase initialization (PEP 489)
+    """
+
+    def setUp(self):
+        self.name = '_testmultiphase'
+        finder = self.machinery.FileFinder(None)
+        self.spec = importlib.util.find_spec(self.name)
+        assert self.spec
+        self.loader = self.machinery.ExtensionFileLoader(
+            self.name, self.spec.origin)
+
+    # No extension module as __init__ available for testing.
+    test_package = None
+
+    # No extension module in a package available for testing.
+    test_lacking_parent = None
+
+    # Handling failure on reload is the up to the module.
+    test_state_after_failure = None
+
+    def test_module(self):
+        '''Test loading an extension module'''
+        with util.uncache(self.name):
+            module = self.load_module()
+            for attr, value in [('__name__', self.name),
+                                ('__file__', self.spec.origin),
+                                ('__package__', '')]:
+                self.assertEqual(getattr(module, attr), value)
+            with self.assertRaises(AttributeError):
+                module.__path__
+            self.assertIs(module, sys.modules[self.name])
+            self.assertIsInstance(module.__loader__,
+                                  self.machinery.ExtensionFileLoader)
+
+    def test_functionality(self):
+        '''Test basic functionality of stuff defined in an extension module'''
+        with util.uncache(self.name):
+            module = self.load_module()
+            self.assertIsInstance(module, types.ModuleType)
+            ex = module.Example()
+            self.assertEqual(ex.demo('abcd'), 'abcd')
+            self.assertEqual(ex.demo(), None)
+            with self.assertRaises(AttributeError):
+                ex.abc
+            ex.abc = 0
+            self.assertEqual(ex.abc, 0)
+            self.assertEqual(module.foo(9, 9), 18)
+            self.assertIsInstance(module.Str(), str)
+            self.assertEqual(module.Str(1) + '23', '123')
+            with self.assertRaises(module.error):
+                raise module.error()
+            self.assertEqual(module.int_const, 1969)
+            self.assertEqual(module.str_const, 'something different')
+
+    def test_reload(self):
+        '''Test that reload didn't re-set the module's attributes'''
+        with util.uncache(self.name):
+            module = self.load_module()
+            ex_class = module.Example
+            importlib.reload(module)
+            self.assertIs(ex_class, module.Example)
+
+    def test_try_registration(self):
+        '''Assert that the PyState_{Find,Add,Remove}Module C API doesn't work'''
+        module = self.load_module()
+        with self.subTest('PyState_FindModule'):
+            self.assertEqual(module.call_state_registration_func(0), None)
+        with self.subTest('PyState_AddModule'):
+            with self.assertRaises(SystemError):
+                module.call_state_registration_func(1)
+        with self.subTest('PyState_RemoveModule'):
+            with self.assertRaises(SystemError):
+                module.call_state_registration_func(2)
+
+    def load_module(self):
+        '''Load the module from the test extension'''
+        return self.loader.load_module(self.name)
+
+    def load_module_by_name(self, fullname):
+        '''Load a module from the test extension by name'''
+        origin = self.spec.origin
+        loader = self.machinery.ExtensionFileLoader(fullname, origin)
+        spec = importlib.util.spec_from_loader(fullname, loader)
+        module = importlib.util.module_from_spec(spec)
+        loader.exec_module(module)
+        return module
+
+    def test_load_twice(self):
+        '''Test that 2 loads result in 2 module objects'''
+        module1 = self.load_module_by_name(self.name)
+        module2 = self.load_module_by_name(self.name)
+        self.assertIsNot(module1, module2)
+
+    def test_unloadable(self):
+        '''Test nonexistent module'''
+        name = 'asdfjkl;'
+        with self.assertRaises(ImportError) as cm:
+            self.load_module_by_name(name)
+        self.assertEqual(cm.exception.name, name)
+
+    def test_unloadable_nonascii(self):
+        '''Test behavior with nonexistent module with non-ASCII name'''
+        name = 'fo\xf3'
+        with self.assertRaises(ImportError) as cm:
+            self.load_module_by_name(name)
+        self.assertEqual(cm.exception.name, name)
+
+    def test_nonmodule(self):
+        '''Test returning a non-module object from create works'''
+        name = self.name + '_nonmodule'
+        mod = self.load_module_by_name(name)
+        self.assertNotEqual(type(mod), type(unittest))
+        self.assertEqual(mod.three, 3)
+
+    def test_null_slots(self):
+        '''Test that NULL slots aren't a problem'''
+        name = self.name + '_null_slots'
+        module = self.load_module_by_name(name)
+        self.assertIsInstance(module, types.ModuleType)
+        assert module.__name__ == name
+
+    def test_bad_modules(self):
+        '''Test SystemError is raised for misbehaving extensions'''
+        for name_base in [
+                'bad_slot_large',
+                'bad_slot_negative',
+                'create_int_with_state',
+                'negative_size',
+                'export_null',
+                'export_uninitialized',
+                'export_raise',
+                'export_unreported_exception',
+                'create_null',
+                'create_raise',
+                'create_unreported_exception',
+                'nonmodule_with_exec_slots',
+                'exec_err',
+                'exec_raise',
+                'exec_unreported_exception',
+                ]:
+            with self.subTest(name_base):
+                name = self.name + '_' + name_base
+                with self.assertRaises(SystemError):
+                    self.load_module_by_name(name)
+
+    def test_nonascii(self):
+        '''Test that modules with non-ASCII names can be loaded'''
+        # punycode behaves slightly differently in some-ASCII and no-ASCII
+        # cases, so test both
+        cases = [
+            (self.name + '_zkou\u0161ka_na\u010dten\xed', 'Czech'),
+            ('\uff3f\u30a4\u30f3\u30dd\u30fc\u30c8\u30c6\u30b9\u30c8',
+             'Japanese'),
+            ]
+        for name, lang in cases:
+            with self.subTest(name):
+                module = self.load_module_by_name(name)
+                self.assertEqual(module.__name__, name)
+                self.assertEqual(module.__doc__, "Module named in %s" % lang)
+
+
+(Frozen_MultiPhaseExtensionModuleTests,
+ Source_MultiPhaseExtensionModuleTests
+ ) = util.test_both(MultiPhaseExtensionModuleTests, machinery=machinery)
 
 
 if __name__ == '__main__':