From 37b581aa480cb309fa8f68f649802af96f6d25c5 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 3 Oct 2022 15:12:35 -0700 Subject: [PATCH 01/11] PoC for checking both __loader__ and __spec__.loader In _warnings.c, in the C equivalent of warnings.warn_explicit(), if the module globals are given (and not None), the warning will attempt to get the source line for the issued warning. To do this, it needs the module's loader. Previously, it would only look up `__loader__` in the module globals. In https://github.com/python/cpython/issues/86298 we want to defer to the `__spec__.loader` if available. The first step on this journey is to check that `loader` == `__spec__.loader` and issue another warning if it is not. This commit does that. Since this is a PoC, only manual testing for now. ```python import warnings import bar warnings.warn_explicit( 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__, ) ``` ```python import sys import os import pathlib ``` Then running this: `./python.exe -Wdefault /tmp/foo.py` Produces: ``` bar.py:2: RuntimeWarning: warning! import os ``` Uncomment the `__loader__ = ` line in `bar.py` and try it again: ``` sys:1: ImportWarning: Module bar; __loader__ != __spec__.loader (<_frozen_importlib_external.SourceFileLoader object at 0x109f7dfa0> != PosixPath('.')) bar.py:2: RuntimeWarning: warning! import os ``` --- Python/_warnings.c | 48 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/Python/_warnings.c b/Python/_warnings.c index 1b9e107ea30b13..e014251a2954ea 100644 --- a/Python/_warnings.c +++ b/Python/_warnings.c @@ -977,25 +977,59 @@ warnings_warn_impl(PyObject *module, PyObject *message, PyObject *category, static PyObject * get_source_line(PyInterpreterState *interp, PyObject *module_globals, int lineno) { - PyObject *loader; + PyObject *loader, *spec_loader; + PyObject *spec; PyObject *module_name; PyObject *get_source; PyObject *source; PyObject *source_list; PyObject *source_line; - /* Check/get the requisite pieces needed for the loader. */ + module_name = _PyDict_GetItemWithError(module_globals, &_Py_ID(__name__)); + if (!module_name) { + return NULL; + } + Py_INCREF(module_name); + + /* Check/get the requisite pieces needed for the loader. Get both the + __spec__.loader and __loader__ attributes and warn if they are not the + same. + */ + spec = _PyDict_GetItemWithError(module_globals, &_Py_ID(__spec__)); + if (spec == NULL) { + Py_DECREF(module_name); + return NULL; + } + Py_INCREF(spec); + + spec_loader = PyObject_GetAttrString(spec, "loader"); + Py_DECREF(spec); + if (spec_loader == NULL) { + Py_DECREF(module_name); + return NULL; + } + loader = _PyDict_GetItemWithError(module_globals, &_Py_ID(__loader__)); if (loader == NULL) { + Py_DECREF(spec_loader); + Py_DECREF(module_name); return NULL; } Py_INCREF(loader); - module_name = _PyDict_GetItemWithError(module_globals, &_Py_ID(__name__)); - if (!module_name) { - Py_DECREF(loader); - return NULL; + + if (spec_loader != loader) { + int error = PyErr_WarnFormat( + PyExc_ImportWarning, 2, + "Module %U; __loader__ != __spec__.loader (%R != %R)", + module_name, spec_loader, loader); + if (error < 0) { + Py_DECREF(module_name); + Py_DECREF(spec_loader); + Py_DECREF(loader); + return NULL; + } } - Py_INCREF(module_name); + Py_DECREF(spec_loader); /* Make sure the loader implements the optional get_source() method. */ (void)_PyObject_LookupAttr(loader, &_Py_ID(get_source), &get_source); From 317e253b6de0bf5f1c2cb95960559e692eea62fb Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 3 Oct 2022 17:01:05 -0700 Subject: [PATCH 02/11] Add a test for the __loader__ == __spec__.loader cases --- Lib/test/test_warnings/__init__.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/Lib/test/test_warnings/__init__.py b/Lib/test/test_warnings/__init__.py index 9e473e923cad03..1b3f7a483fa911 100644 --- a/Lib/test/test_warnings/__init__.py +++ b/Lib/test/test_warnings/__init__.py @@ -5,6 +5,7 @@ import re import sys import textwrap +from types import ModuleType, SimpleNamespace import unittest from test import support from test.support import import_helper @@ -885,6 +886,29 @@ def test_issue31566(self): support.swap_item(globals(), '__file__', None): self.assertRaises(UserWarning, self.module.warn, 'bar') + @support.cpython_only + def test_gh86298(self): + # warn_explicit() itself issues an ImportWarning when a module and + # module_globals are both given, and the named module has a __loader__ + # != its __spec__.loader. + bar = ModuleType('bar') + # First, make the __loader__ != the __spec__.loader. This should + # trigger the ImportWarning inside warn_explicit(). + bar.__loader__ = object() + bar.__spec__ = SimpleNamespace(loader=object()) + with original_warnings.catch_warnings(): + self.assertWarns( + ImportWarning, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + # Now make the __loader__ == __spec__.loader. This will not raise the exception. + bar.__spec__.loader = bar.__loader__ + with original_warnings.catch_warnings(): + self.module.filterwarnings('error', category=ImportWarning) + self.module.warn_explicit( + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + class WarningsDisplayTests(BaseTest): From 428d863c6d1b3f4746fb22a03588ae18c342efcb Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 4 Oct 2022 14:04:46 -0700 Subject: [PATCH 03/11] Add news --- .../2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst b/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst new file mode 100644 index 00000000000000..26d22c9d6cb468 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst @@ -0,0 +1,3 @@ +In cases where ``warnings.warn_explicit()`` consults the module's loader, an +``ImportWarning`` is issued when ``m.__loader__`` differs from +``m.__spec__.loader``. From fc50fe143ac56f2658187a8e765cd0584aaeaa42 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 4 Oct 2022 14:25:01 -0700 Subject: [PATCH 04/11] Update some documentation --- Doc/reference/import.rst | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Doc/reference/import.rst b/Doc/reference/import.rst index 507f2b3763cae4..9f39638c659d99 100644 --- a/Doc/reference/import.rst +++ b/Doc/reference/import.rst @@ -552,6 +552,11 @@ the module. for introspection, but can be used for additional loader-specific functionality, for example getting data associated with a loader. + .. versionchanged:: 3.12 + The value of ``__loader__`` is expected to be the same as + ``__spec__.loader``. If they are not, an :exc:`ImportWarning` will be + raised. + .. attribute:: __package__ The module's ``__package__`` attribute must be set. Its value must @@ -563,8 +568,7 @@ the module. details. This attribute is used instead of ``__name__`` to calculate explicit - relative imports for main modules, as defined in :pep:`366`. It is - expected to have the same value as ``__spec__.parent``. + relative imports for main modules, as defined in :pep:`366`. .. versionchanged:: 3.6 The value of ``__package__`` is expected to be the same as From 1066b9facc64accfe53b057dc639045ab38d29db Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Wed, 5 Oct 2022 17:55:04 -0700 Subject: [PATCH 05/11] Use a helper function --- Lib/importlib/_bootstrap_external.py | 55 +++++++++++++ Lib/test/test_warnings/__init__.py | 118 ++++++++++++++++++++++++--- Python/_warnings.c | 54 +++--------- 3 files changed, 175 insertions(+), 52 deletions(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index b3c31b9659d849..d354ea57449d18 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -854,6 +854,61 @@ def spec_from_file_location(name, location=None, *, loader=None, return spec +def _bless_my_loader(module): + """Helper function implementing the current module loader policy. + + In Python 3.14, the end state is to require and use the module's + __spec__.loader and ignore any __loader__ attribute on the + module. + + * If you have a __loader__ and a __spec__.loader but they are not the + same, in Python 3.12 we issue a DeprecationWarning and fall back to + __loader__ for backward compatibility. In Python 3.14, we'll flip + this case to ignoring __loader__ entirely, without error. + + * If you do not have a __spec__ or __spec__.loader, we also issue a + DeprecationWarning and fall back to __loader__. In Python 3.14, + we'll make this case fail with an AttributeError, and ignore + __loader__. + + * In Python 3.12 and beyond, if you do not have a __loader__, we don't + care as long as you still have a __spec__.loader, otherwise you get + an AttributeError, telling you to add a __spec__.loader. + + See GH#97850 for details. + """ + missing = object() + loader = getattr(module, '__loader__', None) + spec = getattr(module, '__spec__', missing) + + if loader is None and spec is missing: + raise AttributeError(f'{module!r} is missing a __spec__') + if loader is None and spec is None: + raise ValueError(f'{module!r} is missing a __spec__.loader') + + spec_loader = getattr(spec, 'loader', None) + + if spec_loader is None: + if loader is None: + raise ValueError(f'{module!r} is missing a __spec__.loader') + _warnings.warn( + f'{module!r} is missing a __spec__.loader', + DeprecationWarning) + spec_loader = loader + + assert spec_loader is not None + if loader is not None and loader != spec_loader: + _warnings.warn( + f'{module!r}; __loader__ != __spec__.loader', + DeprecationWarning) + preferred_loader = loader + if loader is None: + preferred_loader = spec_loader + + assert preferred_loader is not None + return preferred_loader + + # Loaders ##################################################################### class WindowsRegistryFinder: diff --git a/Lib/test/test_warnings/__init__.py b/Lib/test/test_warnings/__init__.py index 1b3f7a483fa911..7c6476160d40c8 100644 --- a/Lib/test/test_warnings/__init__.py +++ b/Lib/test/test_warnings/__init__.py @@ -886,26 +886,122 @@ def test_issue31566(self): support.swap_item(globals(), '__file__', None): self.assertRaises(UserWarning, self.module.warn, 'bar') + # GH#86298 is part of the migration away from module attributes and toward + # __spec__ attributes. There are several cases to test here. This will + # have to change in Python 3.14 when we actually remove/ignore __loader__ + # in favor of requiring __spec__.loader. + @support.cpython_only - def test_gh86298(self): - # warn_explicit() itself issues an ImportWarning when a module and - # module_globals are both given, and the named module has a __loader__ - # != its __spec__.loader. + def test_gh86298_no_loader_and_no_spec(self): + bar = ModuleType('bar') + with original_warnings.catch_warnings(): + self.assertRaises( + AttributeError, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_loader_is_none_and_no_spec(self): + bar = ModuleType('bar') + bar.__loader__ = None + with original_warnings.catch_warnings(): + self.assertRaises( + AttributeError, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_no_loader_and_spec_is_none(self): + bar = ModuleType('bar') + bar.__spec__ = None + with original_warnings.catch_warnings(): + self.assertRaises( + ValueError, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_loader_is_none_and_spec_is_none(self): + bar = ModuleType('bar') + bar.__loader__ = None + bar.__spec__ = None + with original_warnings.catch_warnings(): + self.assertRaises( + ValueError, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_loader_is_none_and_spec_loader_is_none(self): + bar = ModuleType('bar') + bar.__loader__ = None + bar.__spec__ = SimpleNamespace(loader=None) + with original_warnings.catch_warnings(): + self.assertRaises( + ValueError, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_no_spec(self): + bar = ModuleType('bar') + bar.__loader__ = object() + with original_warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_spec_is_none(self): + bar = ModuleType('bar') + bar.__loader__ = object() + bar.__spec__ = None + with original_warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_no_spec_loader(self): + bar = ModuleType('bar') + bar.__loader__ = object() + bar.__spec__ = SimpleNamespace() + with original_warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_loader_and_spec_loader_disagree(self): bar = ModuleType('bar') - # First, make the __loader__ != the __spec__.loader. This should - # trigger the ImportWarning inside warn_explicit(). bar.__loader__ = object() bar.__spec__ = SimpleNamespace(loader=object()) with original_warnings.catch_warnings(): self.assertWarns( - ImportWarning, self.module.warn_explicit, + DeprecationWarning, self.module.warn_explicit, + 'warning!', RuntimeWarning, + 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + + @support.cpython_only + def test_gh86298_no_loader_and_no_spec_loader(self): + bar = ModuleType('bar') + bar.__spec__ = SimpleNamespace() + with original_warnings.catch_warnings(): + self.assertRaises( + AttributeError, self.module.warn_explicit, 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - # Now make the __loader__ == __spec__.loader. This will not raise the exception. - bar.__spec__.loader = bar.__loader__ + + @support.cpython_only + def test_gh86298_no_loader_with_spec_loader_okay(self): + bar = ModuleType('bar') + bar.__spec__ = SimpleNamespace(loader=object()) with original_warnings.catch_warnings(): - self.module.filterwarnings('error', category=ImportWarning) - self.module.warn_explicit( + self.assertWarn( + RuntimeWarning, self.module.warn_explicit, 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) diff --git a/Python/_warnings.c b/Python/_warnings.c index e014251a2954ea..8a0895b0627742 100644 --- a/Python/_warnings.c +++ b/Python/_warnings.c @@ -975,61 +975,33 @@ warnings_warn_impl(PyObject *module, PyObject *message, PyObject *category, } static PyObject * -get_source_line(PyInterpreterState *interp, PyObject *module_globals, int lineno) +get_source_line(PyInterpreterState *interp, PyObject* mod, PyObject *module_globals, int lineno) { - PyObject *loader, *spec_loader; - PyObject *spec; + PyObject *external; + PyObject *loader; PyObject *module_name; PyObject *get_source; PyObject *source; PyObject *source_list; PyObject *source_line; - module_name = _PyDict_GetItemWithError(module_globals, &_Py_ID(__name__)); - if (!module_name) { + /* stolen from import.c */ + external = PyObject_GetAttrString(interp->importlib, "_bootstrap_external"); + if (external == NULL) { return NULL; } - Py_INCREF(module_name); - /* Check/get the requisite pieces needed for the loader. Get both the - __spec__.loader and __loader__ attributes and warn if they are not the - same. - */ - spec = _PyDict_GetItemWithError(module_globals, &_Py_ID(__spec__)); - if (spec == NULL) { - Py_DECREF(module_name); - return NULL; - } - Py_INCREF(spec); - - spec_loader = PyObject_GetAttrString(spec, "loader"); - Py_DECREF(spec); - if (spec_loader == NULL) { - Py_DECREF(module_name); + loader = PyObject_CallMethodObjArgs(external, "_bless_my_loader", mod, NULL); + Py_DECREF(external); + if (loader == NULL) { return NULL; } - loader = _PyDict_GetItemWithError(module_globals, &_Py_ID(__loader__)); - if (loader == NULL) { - Py_DECREF(spec_loader); - Py_DECREF(module_name); + module_name = _PyDict_GetItemWithError(module_globals, &_Py_ID(__name__)); + if (!module_name) { return NULL; } - Py_INCREF(loader); - - if (spec_loader != loader) { - int error = PyErr_WarnFormat( - PyExc_ImportWarning, 2, - "Module %U; __loader__ != __spec__.loader (%R != %R)", - module_name, spec_loader, loader); - if (error < 0) { - Py_DECREF(module_name); - Py_DECREF(spec_loader); - Py_DECREF(loader); - return NULL; - } - } - Py_DECREF(spec_loader); + Py_INCREF(module_name); /* Make sure the loader implements the optional get_source() method. */ (void)_PyObject_LookupAttr(loader, &_Py_ID(get_source), &get_source); @@ -1102,7 +1074,7 @@ warnings_warn_explicit_impl(PyObject *module, PyObject *message, return NULL; } - source_line = get_source_line(tstate->interp, module_globals, lineno); + source_line = get_source_line(tstate->interp, mod, module_globals, lineno); if (source_line == NULL && PyErr_Occurred()) { return NULL; } From 831f8322c736580e1d8f1e8ec7d3017777264570 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 6 Oct 2022 15:44:50 -0700 Subject: [PATCH 06/11] Pass warnings tests Note however that the semantics of the helper has changed, and can't be full on loader blessing... yet. --- Lib/importlib/_bootstrap_external.py | 59 ++++++++++++---------------- Lib/test/test_warnings/__init__.py | 31 ++++++++++++--- Python/_warnings.c | 6 +-- 3 files changed, 54 insertions(+), 42 deletions(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index d354ea57449d18..2405976ecb7f2d 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -854,59 +854,50 @@ def spec_from_file_location(name, location=None, *, loader=None, return spec -def _bless_my_loader(module): - """Helper function implementing the current module loader policy. - - In Python 3.14, the end state is to require and use the module's - __spec__.loader and ignore any __loader__ attribute on the - module. - - * If you have a __loader__ and a __spec__.loader but they are not the - same, in Python 3.12 we issue a DeprecationWarning and fall back to - __loader__ for backward compatibility. In Python 3.14, we'll flip - this case to ignoring __loader__ entirely, without error. - - * If you do not have a __spec__ or __spec__.loader, we also issue a - DeprecationWarning and fall back to __loader__. In Python 3.14, - we'll make this case fail with an AttributeError, and ignore - __loader__. - - * In Python 3.12 and beyond, if you do not have a __loader__, we don't - care as long as you still have a __spec__.loader, otherwise you get - an AttributeError, telling you to add a __spec__.loader. +def _bless_my_loader(module_globals): + """Helper function for _warnings.c See GH#97850 for details. """ + # 2022-10-06(warsaw): For now, this helper is only used in _warnings.c and + # that use case only has the module globals. This function could be + # extended to accept either that or a module object. However, in the + # latter case, it would be better to raise certain exceptions when looking + # at a module, which should have either a __loader__ or __spec__.loader. + # For backward compatibility, it is possible that we'll get an empty + # dictionary for the module globals, and that cannot raise an exception. + if not isinstance(module_globals, dict): + return None + missing = object() - loader = getattr(module, '__loader__', None) - spec = getattr(module, '__spec__', missing) + loader = module_globals.get('__loader__', None) + spec = module_globals.get('__spec__', missing) if loader is None and spec is missing: - raise AttributeError(f'{module!r} is missing a __spec__') + #raise AttributeError('Module globals is missing a __spec__') + return None if loader is None and spec is None: - raise ValueError(f'{module!r} is missing a __spec__.loader') + raise ValueError('Module globals is missing a __spec__.loader') - spec_loader = getattr(spec, 'loader', None) + spec_loader = getattr(spec, 'loader', missing) - if spec_loader is None: + if spec_loader in (missing, None): if loader is None: - raise ValueError(f'{module!r} is missing a __spec__.loader') + exc = AttributeError if spec_loader is missing else ValueError + raise exc('Module globals is missing a __spec__.loader') _warnings.warn( - f'{module!r} is missing a __spec__.loader', + 'Module globals is missing a __spec__.loader', DeprecationWarning) spec_loader = loader assert spec_loader is not None if loader is not None and loader != spec_loader: _warnings.warn( - f'{module!r}; __loader__ != __spec__.loader', + 'Module globals; __loader__ != __spec__.loader', DeprecationWarning) - preferred_loader = loader - if loader is None: - preferred_loader = spec_loader + return loader - assert preferred_loader is not None - return preferred_loader + return spec_loader # Loaders ##################################################################### diff --git a/Lib/test/test_warnings/__init__.py b/Lib/test/test_warnings/__init__.py index 7c6476160d40c8..972fbf57fa7c05 100644 --- a/Lib/test/test_warnings/__init__.py +++ b/Lib/test/test_warnings/__init__.py @@ -894,9 +894,18 @@ def test_issue31566(self): @support.cpython_only def test_gh86298_no_loader_and_no_spec(self): bar = ModuleType('bar') + del bar.__loader__ + del bar.__spec__ with original_warnings.catch_warnings(): - self.assertRaises( - AttributeError, self.module.warn_explicit, + # 2022-10-06(warsaw): For backward compatibility with the + # implementation in _warnings.c, this can't raise an + # AttributeError. See _bless_my_loader() in _bootstrap_external.py + ## self.assertRaises( + ## AttributeError, self.module.warn_explicit, + ## 'warning!', RuntimeWarning, + ## 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + self.assertWarns( + RuntimeWarning, self.module.warn_explicit, 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) @@ -904,15 +913,24 @@ def test_gh86298_no_loader_and_no_spec(self): def test_gh86298_loader_is_none_and_no_spec(self): bar = ModuleType('bar') bar.__loader__ = None + del bar.__spec__ with original_warnings.catch_warnings(): - self.assertRaises( - AttributeError, self.module.warn_explicit, + # 2022-10-06(warsaw): For backward compatibility with the + # implementation in _warnings.c, this can't raise an + # AttributeError. See _bless_my_loader() in _bootstrap_external.py + ## self.assertRaises( + ## AttributeError, self.module.warn_explicit, + ## 'warning!', RuntimeWarning, + ## 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) + self.assertWarns( + RuntimeWarning, self.module.warn_explicit, 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) @support.cpython_only def test_gh86298_no_loader_and_spec_is_none(self): bar = ModuleType('bar') + del bar.__loader__ bar.__spec__ = None with original_warnings.catch_warnings(): self.assertRaises( @@ -946,6 +964,7 @@ def test_gh86298_loader_is_none_and_spec_loader_is_none(self): def test_gh86298_no_spec(self): bar = ModuleType('bar') bar.__loader__ = object() + del bar.__spec__ with original_warnings.catch_warnings(): self.assertWarns( DeprecationWarning, self.module.warn_explicit, @@ -988,6 +1007,7 @@ def test_gh86298_loader_and_spec_loader_disagree(self): @support.cpython_only def test_gh86298_no_loader_and_no_spec_loader(self): bar = ModuleType('bar') + del bar.__loader__ bar.__spec__ = SimpleNamespace() with original_warnings.catch_warnings(): self.assertRaises( @@ -998,9 +1018,10 @@ def test_gh86298_no_loader_and_no_spec_loader(self): @support.cpython_only def test_gh86298_no_loader_with_spec_loader_okay(self): bar = ModuleType('bar') + del bar.__loader__ bar.__spec__ = SimpleNamespace(loader=object()) with original_warnings.catch_warnings(): - self.assertWarn( + self.assertWarns( RuntimeWarning, self.module.warn_explicit, 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) diff --git a/Python/_warnings.c b/Python/_warnings.c index 8a0895b0627742..d414ccdebe1afe 100644 --- a/Python/_warnings.c +++ b/Python/_warnings.c @@ -975,7 +975,7 @@ warnings_warn_impl(PyObject *module, PyObject *message, PyObject *category, } static PyObject * -get_source_line(PyInterpreterState *interp, PyObject* mod, PyObject *module_globals, int lineno) +get_source_line(PyInterpreterState *interp, PyObject *module_globals, int lineno) { PyObject *external; PyObject *loader; @@ -991,7 +991,7 @@ get_source_line(PyInterpreterState *interp, PyObject* mod, PyObject *module_glob return NULL; } - loader = PyObject_CallMethodObjArgs(external, "_bless_my_loader", mod, NULL); + loader = PyObject_CallMethod(external, "_bless_my_loader", "O", module_globals, NULL); Py_DECREF(external); if (loader == NULL) { return NULL; @@ -1074,7 +1074,7 @@ warnings_warn_explicit_impl(PyObject *module, PyObject *message, return NULL; } - source_line = get_source_line(tstate->interp, mod, module_globals, lineno); + source_line = get_source_line(tstate->interp, module_globals, lineno); if (source_line == NULL && PyErr_Occurred()) { return NULL; } From fe9d62eecf72beeb095cb3e8a244230f3efbc8c3 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 6 Oct 2022 16:44:53 -0700 Subject: [PATCH 07/11] Move warnings tests to helper tests --- .../test_importlib/import_/test_helpers.py | 111 ++++++++++++++ Lib/test/test_warnings/__init__.py | 141 ------------------ 2 files changed, 111 insertions(+), 141 deletions(-) diff --git a/Lib/test/test_importlib/import_/test_helpers.py b/Lib/test/test_importlib/import_/test_helpers.py index 90df56f09fe52d..75da4e7124a4e7 100644 --- a/Lib/test/test_importlib/import_/test_helpers.py +++ b/Lib/test/test_importlib/import_/test_helpers.py @@ -2,7 +2,9 @@ from importlib import _bootstrap_external, machinery import os.path +from types import ModuleType, SimpleNamespace import unittest +import warnings from .. import util @@ -67,5 +69,114 @@ def test_no_loader_no_spec_but_source(self): FrozenFixUpModuleTests, SourceFixUpModuleTests = util.test_both(FixUpModuleTests) + +class TestBlessMyLoader(unittest.TestCase): + # GH#86298 is part of the migration away from module attributes and toward + # __spec__ attributes. There are several cases to test here. This will + # have to change in Python 3.14 when we actually remove/ignore __loader__ + # in favor of requiring __spec__.loader. + + def test_gh86298_no_loader_and_no_spec(self): + bar = ModuleType('bar') + del bar.__loader__ + del bar.__spec__ + # 2022-10-06(warsaw): For backward compatibility with the + # implementation in _warnings.c, this can't raise an + # AttributeError. See _bless_my_loader() in _bootstrap_external.py + ## self.assertRaises( + ## AttributeError, _bootstrap_external._bless_my_loader, + ## bar.__dict__) + self.assertIsNone(_bootstrap_external._bless_my_loader(bar.__dict__)) + + def test_gh86298_loader_is_none_and_no_spec(self): + bar = ModuleType('bar') + bar.__loader__ = None + del bar.__spec__ + # 2022-10-06(warsaw): For backward compatibility with the + # implementation in _warnings.c, this can't raise an + # AttributeError. See _bless_my_loader() in _bootstrap_external.py + ## self.assertRaises( + ## AttributeError, _bootstrap_external._bless_my_loader, + ## bar.__dict__) + self.assertIsNone(_bootstrap_external._bless_my_loader(bar.__dict__)) + + def test_gh86298_no_loader_and_spec_is_none(self): + bar = ModuleType('bar') + del bar.__loader__ + bar.__spec__ = None + self.assertRaises( + ValueError, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_loader_is_none_and_spec_is_none(self): + bar = ModuleType('bar') + bar.__loader__ = None + bar.__spec__ = None + self.assertRaises( + ValueError, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_loader_is_none_and_spec_loader_is_none(self): + bar = ModuleType('bar') + bar.__loader__ = None + bar.__spec__ = SimpleNamespace(loader=None) + self.assertRaises( + ValueError, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_no_spec(self): + bar = ModuleType('bar') + bar.__loader__ = object() + del bar.__spec__ + with warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_spec_is_none(self): + bar = ModuleType('bar') + bar.__loader__ = object() + bar.__spec__ = None + with warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_no_spec_loader(self): + bar = ModuleType('bar') + bar.__loader__ = object() + bar.__spec__ = SimpleNamespace() + with warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_loader_and_spec_loader_disagree(self): + bar = ModuleType('bar') + bar.__loader__ = object() + bar.__spec__ = SimpleNamespace(loader=object()) + with warnings.catch_warnings(): + self.assertWarns( + DeprecationWarning, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_no_loader_and_no_spec_loader(self): + bar = ModuleType('bar') + del bar.__loader__ + bar.__spec__ = SimpleNamespace() + self.assertRaises( + AttributeError, + _bootstrap_external._bless_my_loader, bar.__dict__) + + def test_gh86298_no_loader_with_spec_loader_okay(self): + bar = ModuleType('bar') + del bar.__loader__ + loader = object() + bar.__spec__ = SimpleNamespace(loader=loader) + self.assertEqual( + _bootstrap_external._bless_my_loader(bar.__dict__), + loader) + + if __name__ == "__main__": unittest.main() diff --git a/Lib/test/test_warnings/__init__.py b/Lib/test/test_warnings/__init__.py index 972fbf57fa7c05..9e473e923cad03 100644 --- a/Lib/test/test_warnings/__init__.py +++ b/Lib/test/test_warnings/__init__.py @@ -5,7 +5,6 @@ import re import sys import textwrap -from types import ModuleType, SimpleNamespace import unittest from test import support from test.support import import_helper @@ -886,146 +885,6 @@ def test_issue31566(self): support.swap_item(globals(), '__file__', None): self.assertRaises(UserWarning, self.module.warn, 'bar') - # GH#86298 is part of the migration away from module attributes and toward - # __spec__ attributes. There are several cases to test here. This will - # have to change in Python 3.14 when we actually remove/ignore __loader__ - # in favor of requiring __spec__.loader. - - @support.cpython_only - def test_gh86298_no_loader_and_no_spec(self): - bar = ModuleType('bar') - del bar.__loader__ - del bar.__spec__ - with original_warnings.catch_warnings(): - # 2022-10-06(warsaw): For backward compatibility with the - # implementation in _warnings.c, this can't raise an - # AttributeError. See _bless_my_loader() in _bootstrap_external.py - ## self.assertRaises( - ## AttributeError, self.module.warn_explicit, - ## 'warning!', RuntimeWarning, - ## 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - self.assertWarns( - RuntimeWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_loader_is_none_and_no_spec(self): - bar = ModuleType('bar') - bar.__loader__ = None - del bar.__spec__ - with original_warnings.catch_warnings(): - # 2022-10-06(warsaw): For backward compatibility with the - # implementation in _warnings.c, this can't raise an - # AttributeError. See _bless_my_loader() in _bootstrap_external.py - ## self.assertRaises( - ## AttributeError, self.module.warn_explicit, - ## 'warning!', RuntimeWarning, - ## 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - self.assertWarns( - RuntimeWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_no_loader_and_spec_is_none(self): - bar = ModuleType('bar') - del bar.__loader__ - bar.__spec__ = None - with original_warnings.catch_warnings(): - self.assertRaises( - ValueError, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_loader_is_none_and_spec_is_none(self): - bar = ModuleType('bar') - bar.__loader__ = None - bar.__spec__ = None - with original_warnings.catch_warnings(): - self.assertRaises( - ValueError, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_loader_is_none_and_spec_loader_is_none(self): - bar = ModuleType('bar') - bar.__loader__ = None - bar.__spec__ = SimpleNamespace(loader=None) - with original_warnings.catch_warnings(): - self.assertRaises( - ValueError, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_no_spec(self): - bar = ModuleType('bar') - bar.__loader__ = object() - del bar.__spec__ - with original_warnings.catch_warnings(): - self.assertWarns( - DeprecationWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_spec_is_none(self): - bar = ModuleType('bar') - bar.__loader__ = object() - bar.__spec__ = None - with original_warnings.catch_warnings(): - self.assertWarns( - DeprecationWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_no_spec_loader(self): - bar = ModuleType('bar') - bar.__loader__ = object() - bar.__spec__ = SimpleNamespace() - with original_warnings.catch_warnings(): - self.assertWarns( - DeprecationWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_loader_and_spec_loader_disagree(self): - bar = ModuleType('bar') - bar.__loader__ = object() - bar.__spec__ = SimpleNamespace(loader=object()) - with original_warnings.catch_warnings(): - self.assertWarns( - DeprecationWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_no_loader_and_no_spec_loader(self): - bar = ModuleType('bar') - del bar.__loader__ - bar.__spec__ = SimpleNamespace() - with original_warnings.catch_warnings(): - self.assertRaises( - AttributeError, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - - @support.cpython_only - def test_gh86298_no_loader_with_spec_loader_okay(self): - bar = ModuleType('bar') - del bar.__loader__ - bar.__spec__ = SimpleNamespace(loader=object()) - with original_warnings.catch_warnings(): - self.assertWarns( - RuntimeWarning, self.module.warn_explicit, - 'warning!', RuntimeWarning, - 'bar.py', 2, module='bar knee', module_globals=bar.__dict__) - class WarningsDisplayTests(BaseTest): From 3bcdba951c4f6df2ff19ff630708d744da8a2350 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 6 Oct 2022 16:55:10 -0700 Subject: [PATCH 08/11] Clean up a few last minute things --- Doc/reference/import.rst | 4 ++-- .../2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst | 2 +- Python/_warnings.c | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Doc/reference/import.rst b/Doc/reference/import.rst index 66a1a8bb7fd1bb..136115f1edf29a 100644 --- a/Doc/reference/import.rst +++ b/Doc/reference/import.rst @@ -560,8 +560,8 @@ listed below. .. versionchanged:: 3.12 The value of ``__loader__`` is expected to be the same as - ``__spec__.loader``. If they are not, an :exc:`ImportWarning` will be - raised. + ``__spec__.loader``. The use of ``__loader__`` is deprecated and slated + for removal in Python 3.14. .. attribute:: __package__ diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst b/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst index 26d22c9d6cb468..6e349d56c99f25 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2022-10-04-14-04-40.gh-issue-86298.QVM7G1.rst @@ -1,3 +1,3 @@ In cases where ``warnings.warn_explicit()`` consults the module's loader, an -``ImportWarning`` is issued when ``m.__loader__`` differs from +``DeprecationWarning`` is issued when ``m.__loader__`` differs from ``m.__spec__.loader``. diff --git a/Python/_warnings.c b/Python/_warnings.c index d414ccdebe1afe..0d4c50f769b03c 100644 --- a/Python/_warnings.c +++ b/Python/_warnings.c @@ -999,6 +999,7 @@ get_source_line(PyInterpreterState *interp, PyObject *module_globals, int lineno module_name = _PyDict_GetItemWithError(module_globals, &_Py_ID(__name__)); if (!module_name) { + Py_DECREF(loader); return NULL; } Py_INCREF(module_name); From 5600c9c54f99034b28f40d6efff04492373b4bf6 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 6 Oct 2022 17:51:05 -0700 Subject: [PATCH 09/11] Update Lib/importlib/_bootstrap_external.py Co-authored-by: Brett Cannon --- Lib/importlib/_bootstrap_external.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 2d6950e4a1a828..84d16158a849ad 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -881,11 +881,13 @@ def _bless_my_loader(module_globals): loader = module_globals.get('__loader__', None) spec = module_globals.get('__spec__', missing) - if loader is None and spec is missing: - #raise AttributeError('Module globals is missing a __spec__') - return None - if loader is None and spec is None: - raise ValueError('Module globals is missing a __spec__.loader') + if loader is None: + if spec is missing: + # If working with a module: + # raise AttributeError('Module globals is missing a __spec__') + return None + elif spec is None: + raise ValueError('Module globals is missing a __spec__.loader') spec_loader = getattr(spec, 'loader', missing) From 8631e535a04a20915908ed5c2f3026ab431ee360 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 6 Oct 2022 17:51:54 -0700 Subject: [PATCH 10/11] Update Lib/test/test_importlib/import_/test_helpers.py Co-authored-by: Brett Cannon --- Lib/test/test_importlib/import_/test_helpers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_importlib/import_/test_helpers.py b/Lib/test/test_importlib/import_/test_helpers.py index 75da4e7124a4e7..55df85bf59eb78 100644 --- a/Lib/test/test_importlib/import_/test_helpers.py +++ b/Lib/test/test_importlib/import_/test_helpers.py @@ -83,6 +83,7 @@ def test_gh86298_no_loader_and_no_spec(self): # 2022-10-06(warsaw): For backward compatibility with the # implementation in _warnings.c, this can't raise an # AttributeError. See _bless_my_loader() in _bootstrap_external.py + # If working with a module: ## self.assertRaises( ## AttributeError, _bootstrap_external._bless_my_loader, ## bar.__dict__) From b1b0d864245cb366e762f431189465c9d955d5d5 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 6 Oct 2022 17:52:04 -0700 Subject: [PATCH 11/11] Update Lib/test/test_importlib/import_/test_helpers.py Co-authored-by: Brett Cannon --- Lib/test/test_importlib/import_/test_helpers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_importlib/import_/test_helpers.py b/Lib/test/test_importlib/import_/test_helpers.py index 55df85bf59eb78..550f88d1d7a651 100644 --- a/Lib/test/test_importlib/import_/test_helpers.py +++ b/Lib/test/test_importlib/import_/test_helpers.py @@ -96,6 +96,7 @@ def test_gh86298_loader_is_none_and_no_spec(self): # 2022-10-06(warsaw): For backward compatibility with the # implementation in _warnings.c, this can't raise an # AttributeError. See _bless_my_loader() in _bootstrap_external.py + # If working with a module: ## self.assertRaises( ## AttributeError, _bootstrap_external._bless_my_loader, ## bar.__dict__)