gh-86298: Ensure that loader and spec.loader agree in warnings.warn_explicit()#97803

warsaw · 2022-10-03T22:29:54Z

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 #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.

# /tmp/foo.pyimportwarningsimportbarwarnings.warn_explicit( 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__, )

# /tmp/bar.pyimportsysimportosimportpathlib# __loader__ = pathlib.Path()

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

Issue: Use specs instead of just __loader__ in C code #86298

Automerge-Triggered-By: GH:warsaw

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 python#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 ```

warsaw · 2022-10-03T22:30:10Z

Tagging @brettcannon

warsaw · 2022-10-04T00:03:29Z

Converting from draft since I now have tests.

warsaw · 2022-10-04T21:06:36Z

It's better to split these consistency fixes up into separate PRs, so this one only addresses the _warnings.c part of the issue.

warsaw · 2022-10-04T21:34:34Z

@brettcannon I think this branch is ready for review. It only changes the _warnings.c case, adds tests, and updates some documentation. We'll split any related changes outside of this file into separate PRs.

Python/_warnings.c

Note however that the semantics of the helper has changed, and can't be full on loader blessing... yet.

warsaw · 2022-10-07T00:24:56Z

@brettcannon Please take another look.

Lib/importlib/_bootstrap_external.py

Lib/test/test_importlib/import_/test_helpers.py

Python/_warnings.c

Co-authored-by: Brett Cannon <brett@python.org>

miss-islington · 2022-10-07T01:22:22Z

Status check is done, and it's a success ✅.

miss-islington · 2022-10-07T01:22:23Z

Sorry, I can't merge this PR. Reason: Head branch was modified. Review and try the merge again..

miss-islington · 2022-10-07T02:32:52Z

Status check is done, and it's a success ✅.

* main: pythongh-86298: Ensure that __loader__ and __spec__.loader agree in warnings.warn_explicit() (pythonGH-97803) pythongh-82874: Convert remaining importlib format uses to f-str. (python#98005) Docs: Fix backtick errors found by sphinx-lint (python#97998) pythongh-97850: Remove deprecated functions from `importlib.utils` (python#97898) Remove extra spaces in custom openSSL documentation. (python#93568) pythonGH-90985: Revert "Deprecate passing a message into cancel()" (python#97999)

…arnings.warn_explicit() (pythonGH-97803) 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 python#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 # /tmp/foo.py import warnings import bar warnings.warn_explicit( 'warning!', RuntimeWarning, 'bar.py', 2, module='bar knee', module_globals=bar.__dict__, ) ``` ```python # /tmp/bar.py import sys import os import pathlib # __loader__ = pathlib.Path() ``` 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 ``` Automerge-Triggered-By: GH:warsaw

bedevere-bot added the awaiting core review label Oct 3, 2022

warsaw changed the title ~~PoC for checking both __loader__ and __spec__.loader~~issue-86298: PoC for checking both __loader__ and __spec__.loaderOct 3, 2022

warsaw changed the title ~~issue-86298: PoC for checking both __loader__ and __spec__.loader~~#86298: PoC for checking both __loader__ and __spec__.loaderOct 3, 2022

warsaw changed the title ~~#86298: PoC for checking both __loader__ and __spec__.loader~~gh-86298: PoC for checking both __loader__ and __spec__.loaderOct 3, 2022

warsaw marked this pull request as draft October 3, 2022 22:33

warsaw requested a review from brettcannon October 3, 2022 22:33

warsaw self-assigned this Oct 3, 2022

warsaw added 2 commits October 3, 2022 15:34

Merge branch 'main' into issue-86298
049910b

Add a test for the __loader__ == __spec__.loader cases
317e253

warsaw marked this pull request as ready for review October 4, 2022 00:03

Add news
428d863

warsaw added 2 commits October 4, 2022 14:06

Merge branch 'main' into issue-86298
06d686d

Update some documentation
fc50fe1

warsaw mentioned this pull request Oct 4, 2022
Use specs instead of just __loader__ in C code #86298
Closed

warsaw changed the title ~~gh-86298: PoC for checking both __loader__ and __spec__.loader~~gh-86298: Ensure that __loader__ and __spec__.loader agree in warnings.warn_explicit()Oct 4, 2022

warsaw mentioned this pull request Oct 4, 2022
update the import machinery to only use __spec__ #65961
Open

warsaw added the topic-importlib label Oct 5, 2022

warsaw requested a review from ericsnowcurrently October 5, 2022 19:02

brettcannon approved these changes Oct 5, 2022
View reviewed changes

Python/_warnings.c Outdated Show resolvedHide resolved

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 5, 2022

warsaw added 4 commits October 5, 2022 17:55

Use a helper function
1066b9f

Merge branch 'main' into issue-86298
4602366

Pass warnings tests
831f832
Note however that the semantics of the helper has changed, and can't be full on loader blessing... yet.

Merge branch 'main' into issue-86298
37254cf

warsaw added 3 commits October 6, 2022 16:44

Move warnings tests to helper tests
fe9d62e

Clean up a few last minute things
3bcdba9

Merge branch 'main' into issue-86298
f5cef9c

brettcannon approved these changes Oct 7, 2022
View reviewed changes

warsawand others added 4 commits October 6, 2022 17:51

Update Lib/importlib/_bootstrap_external.py
5600c9c
Co-authored-by: Brett Cannon <brett@python.org>

Update Lib/test/test_importlib/import_/test_helpers.py
8631e53
Co-authored-by: Brett Cannon <brett@python.org>

Update Lib/test/test_importlib/import_/test_helpers.py
b1b0d86
Co-authored-by: Brett Cannon <brett@python.org>

Merge branch 'main' into issue-86298
823400c

warsaw added the 🤖 automerge label Oct 7, 2022

miss-islington merged commit 13d4489 into python:mainOct 7, 2022

bedevere-bot removed the awaiting merge label Oct 7, 2022

warsaw deleted the issue-86298 branch October 7, 2022 02:50

Uh oh!

gh-86298: Ensure that __loader__ and __spec__.loader agree in warnings.warn_explicit()#97803

gh-86298: Ensure that __loader__ and __spec__.loader agree in warnings.warn_explicit() #97803

Uh oh!

Conversation

warsaw commented Oct 3, 2022• edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warsaw commented Oct 3, 2022

Uh oh!

warsaw commented Oct 4, 2022

Uh oh!

warsaw commented Oct 4, 2022

Uh oh!

warsaw commented Oct 4, 2022

Uh oh!

Uh oh!

warsaw commented Oct 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miss-islington commented Oct 7, 2022

Uh oh!

miss-islington commented Oct 7, 2022

Uh oh!

miss-islington commented Oct 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-86298: Ensure that loader and spec.loader agree in warnings.warn_explicit()#97803

gh-86298: Ensure that loader and spec.loader agree in warnings.warn_explicit() #97803

warsaw commented Oct 3, 2022•
edited by miss-islington
Loading