bpo-32303 - Consistency fixes for namespace loaders (python#5481)

warsaw · web-flow · commit bbbcf8693b87 · 2018-02-02T15:15:58.000-05:00
* Make sure ``__spec__.loader`` matches ``__loader__`` for namespace packages. * Make sure ``__spec__.origin` matches ``__file__`` for namespace packages. https://bugs.python.org/issue32303 https://bugs.python.org/issue32305
diff --git a/Doc/library/importlib.rst b/Doc/library/importlib.rst
@@ -1291,7 +1291,7 @@ find and load modules.
    Name of the place from which the module is loaded, e.g. "builtin" for
    built-in modules and the filename for modules loaded from source.
    Normally "origin" should be set, but it may be ``None`` (the default)
-   which indicates it is unspecified.
+   which indicates it is unspecified (e.g. for namespace packages).
 
    .. attribute:: submodule_search_locations
 
diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
@@ -522,6 +522,18 @@ def _init_module_attrs(spec, module, *, override=False):
 
                 loader = _NamespaceLoader.__new__(_NamespaceLoader)
                 loader._path = spec.submodule_search_locations
+                spec.loader = loader
+                # While the docs say that module.__file__ is not set for
+                # built-in modules, and the code below will avoid setting it if
+                # spec.has_location is false, this is incorrect for namespace
+                # packages.  Namespace packages have no location, but their
+                # __spec__.origin is None, and thus their module.__file__
+                # should also be None for consistency.  While a bit of a hack,
+                # this is the best place to ensure this consistency.
+                #
+                # See # https://docs.python.org/3/library/importlib.html#importlib.abc.Loader.load_module
+                # and bpo-32305
+                module.__file__ = None
         try:
             module.__loader__ = loader
         except AttributeError:
diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py
@@ -1276,9 +1276,9 @@ def find_spec(cls, fullname, path=None, target=None):
         elif spec.loader is None:
             namespace_path = spec.submodule_search_locations
             if namespace_path:
-                # We found at least one namespace path.  Return a
-                #  spec which can create the namespace package.
-                spec.origin = 'namespace'
+                # We found at least one namespace path.  Return a spec which
+                # can create the namespace package.
+                spec.origin = None
                 spec.submodule_search_locations = _NamespacePath(fullname, namespace_path, cls._get_spec)
                 return spec
             else:
diff --git a/Lib/importlib/resources.py b/Lib/importlib/resources.py
@@ -66,13 +66,19 @@ def _get_resource_reader(
     return None
 
 
+def _check_location(package):
+    if package.__spec__.origin is None or not package.__spec__.has_location:
+        raise FileNotFoundError(f'Package has no location {package!r}')
+
+
 def open_binary(package: Package, resource: Resource) -> BinaryIO:
     """Return a file-like object opened for binary reading of the resource."""
     resource = _normalize_path(resource)
     package = _get_package(package)
     reader = _get_resource_reader(package)
     if reader is not None:
         return reader.open_resource(resource)
+    _check_location(package)
     absolute_package_path = os.path.abspath(package.__spec__.origin)
     package_path = os.path.dirname(absolute_package_path)
     full_path = os.path.join(package_path, resource)
@@ -106,6 +112,7 @@ def open_text(package: Package,
     reader = _get_resource_reader(package)
     if reader is not None:
         return TextIOWrapper(reader.open_resource(resource), encoding, errors)
+    _check_location(package)
     absolute_package_path = os.path.abspath(package.__spec__.origin)
     package_path = os.path.dirname(absolute_package_path)
     full_path = os.path.join(package_path, resource)
@@ -172,6 +179,8 @@ def path(package: Package, resource: Resource) -> Iterator[Path]:
             return
         except FileNotFoundError:
             pass
+    else:
+        _check_location(package)
     # Fall-through for both the lack of resource_path() *and* if
     # resource_path() raises FileNotFoundError.
     package_directory = Path(package.__spec__.origin).parent
@@ -232,9 +241,9 @@ def contents(package: Package) -> Iterator[str]:
         yield from reader.contents()
         return
     # Is the package a namespace package?  By definition, namespace packages
-    # cannot have resources.
-    if (package.__spec__.origin == 'namespace' and
-            not package.__spec__.has_location):
+    # cannot have resources.  We could use _check_location() and catch the
+    # exception, but that's extra work, so just inline the check.
+    if package.__spec__.origin is None or not package.__spec__.has_location:
         return []
     package_directory = Path(package.__spec__.origin).parent
     yield from os.listdir(str(package_directory))
diff --git a/Lib/test/test_importlib/test_api.py b/Lib/test/test_importlib/test_api.py
@@ -305,6 +305,7 @@ def test_reload_namespace_changed(self):
                     expected = {'__name__': name,
                                 '__package__': name,
                                 '__doc__': None,
+                                '__file__': None,
                                 }
                     os.mkdir(name)
                     with open(bad_path, 'w') as init_file:
@@ -316,8 +317,9 @@ def test_reload_namespace_changed(self):
                     spec = ns.pop('__spec__')
                     ns.pop('__builtins__', None)  # An implementation detail.
                     self.assertEqual(spec.name, name)
-                    self.assertIs(spec.loader, None)
-                    self.assertIsNot(loader, None)
+                    self.assertIsNotNone(spec.loader)
+                    self.assertIsNotNone(loader)
+                    self.assertEqual(spec.loader, loader)
                     self.assertEqual(set(path),
                                      set([os.path.dirname(bad_path)]))
                     with self.assertRaises(AttributeError):
diff --git a/Lib/test/test_importlib/test_namespace_pkgs.py b/Lib/test/test_importlib/test_namespace_pkgs.py
@@ -317,5 +317,21 @@ def test_dynamic_path(self):
         self.assertEqual(foo.two.attr, 'portion2 foo two')
 
 
+class LoaderTests(NamespacePackageTest):
+    paths = ['portion1']
+
+    def test_namespace_loader_consistency(self):
+        # bpo-32303
+        import foo
+        self.assertEqual(foo.__loader__, foo.__spec__.loader)
+        self.assertIsNotNone(foo.__loader__)
+
+    def test_namespace_origin_consistency(self):
+        # bpo-32305
+        import foo
+        self.assertIsNone(foo.__spec__.origin)
+        self.assertIsNone(foo.__file__)
+
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-02-01-10-16-28.bpo-32303.VsvhSl.rst b/Misc/NEWS.d/next/Core and Builtins/2018-02-01-10-16-28.bpo-32303.VsvhSl.rst
@@ -0,0 +1 @@
+Make sure ``__spec__.loader`` matches ``__loader__`` for namespace packages.
diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-02-01-10-56-41.bpo-32305.dkU9Qa.rst b/Misc/NEWS.d/next/Core and Builtins/2018-02-01-10-56-41.bpo-32305.dkU9Qa.rst
@@ -0,0 +1,2 @@
+For namespace packages, ensure that both ``__file__`` and
+``__spec__.origin`` are set to None.
diff --git a/Python/importlib.h b/Python/importlib.h
diff --git a/Python/importlib_external.h b/Python/importlib_external.h

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Make sure ``__spec__.loader`` matches ``__loader__`` for namespace packages.
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+For namespace packages, ensure that both ``__file__`` and
	`2`	+``__spec__.origin`` are set to None.