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.py
import warnings

import bar

warnings.warn_explicit(
    'warning!',
    RuntimeWarning,
    'bar.py', 2,
    module='bar knee',
    module_globals=bar.__dict__,
    )

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

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 <[email protected]>

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__.loader Oct 3, 2022

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

warsaw changed the title ~~#86298: PoC for checking both __loader__ and __spec__.loader~~ gh-86298: PoC for checking both __loader__ and __spec__.loader Oct 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 resolved Hide 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

warsaw and others added 4 commits October 6, 2022 17:51

Update Lib/importlib/_bootstrap_external.py

5600c9c

Co-authored-by: Brett Cannon <[email protected]>

Update Lib/test/test_importlib/import_/test_helpers.py

8631e53

Co-authored-by: Brett Cannon <[email protected]>

Update Lib/test/test_importlib/import_/test_helpers.py

b1b0d86

Co-authored-by: Brett Cannon <[email protected]>

Merge branch 'main' into issue-86298

823400c

warsaw added the 🤖 automerge label Oct 7, 2022

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

warsaw commented Oct 3, 2022

warsaw commented Oct 4, 2022

warsaw commented Oct 4, 2022

warsaw commented Oct 4, 2022

warsaw commented Oct 7, 2022

miss-islington commented Oct 7, 2022

miss-islington commented Oct 7, 2022

miss-islington commented Oct 7, 2022

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

Conversation

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

warsaw commented Oct 3, 2022

warsaw commented Oct 4, 2022

warsaw commented Oct 4, 2022

warsaw commented Oct 4, 2022

warsaw commented Oct 7, 2022

miss-islington commented Oct 7, 2022

miss-islington commented Oct 7, 2022

miss-islington commented Oct 7, 2022

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