GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-86298: Ensure that __loader__ and __spec__.loader agree in warnings.warn_explicit() by warsaw · Pull Request #97803 · python/cpython

Conversation

@warsaw

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@warsaw
PoC for checking both __loader__ and __spec__.loader
37b581a
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
```
@bedevere-bot bedevere-bot added the awaiting core review label Oct 3, 2022
@warsaw

warsaw commented Oct 3, 2022

Copy link
Copy Markdown
Member Author

Tagging @brettcannon

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@warsaw 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 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 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
warsaw marked this pull request as draft October 3, 2022 22:33
@warsaw
warsaw requested a review from brettcannon October 3, 2022 22:33
@warsaw warsaw self-assigned this Oct 3, 2022
warsaw added 2 commits October 3, 2022 15:34
@warsaw
Merge branch 'main' into issue-86298
049910b
@warsaw
Add a test for the __loader__ == __spec__.loader cases
317e253
@warsaw
warsaw marked this pull request as ready for review October 4, 2022 00:03
@warsaw

warsaw commented Oct 4, 2022

Copy link
Copy Markdown
Member Author

Converting from draft since I now have tests.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@warsaw
Add news
428d863
@warsaw

warsaw commented Oct 4, 2022

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

warsaw added 2 commits October 4, 2022 14:06
@warsaw
Merge branch 'main' into issue-86298
06d686d
@warsaw
Update some documentation
fc50fe1
@warsaw

warsaw commented Oct 4, 2022

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@warsaw warsaw mentioned this pull request Oct 4, 2022
Use specs instead of just __loader__ in C code #86298
Closed
@warsaw 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 warsaw mentioned this pull request Oct 4, 2022
update the import machinery to only use __spec__ #65961
Open
@warsaw warsaw added the topic-importlib label Oct 5, 2022
@warsaw
warsaw requested a review from ericsnowcurrently October 5, 2022 19:02
brettcannon
brettcannon approved these changes Oct 5, 2022
View reviewed changes
Comment thread Python/_warnings.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Oct 5, 2022
warsaw added 4 commits October 5, 2022 17:55
@warsaw
Use a helper function
1066b9f
@warsaw
Merge branch 'main' into issue-86298
4602366
@warsaw
Pass warnings tests
831f832
Note however that the semantics of the helper has changed, and can't be full
on loader blessing... yet.
@warsaw
Merge branch 'main' into issue-86298
37254cf
warsaw added 3 commits October 6, 2022 16:44
@warsaw
Move warnings tests to helper tests
fe9d62e
@warsaw
Clean up a few last minute things
3bcdba9
@warsaw
Merge branch 'main' into issue-86298
f5cef9c
@warsaw

warsaw commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

@brettcannon Please take another look.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon
brettcannon approved these changes Oct 7, 2022
View reviewed changes
Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_importlib/import_/test_helpers.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_importlib/import_/test_helpers.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/_warnings.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

warsaw and others added 4 commits October 6, 2022 17:51
@warsaw @brettcannon
Update Lib/importlib/_bootstrap_external.py
5600c9c
Co-authored-by: Brett Cannon <[email protected]>
@warsaw @brettcannon
Update Lib/test/test_importlib/import_/test_helpers.py
8631e53
Co-authored-by: Brett Cannon <[email protected]>
@warsaw @brettcannon
Update Lib/test/test_importlib/import_/test_helpers.py
b1b0d86
Co-authored-by: Brett Cannon <[email protected]>
@warsaw
Merge branch 'main' into issue-86298
823400c
@warsaw warsaw added the 🤖 automerge label Oct 7, 2022
@miss-islington

miss-islington commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington
miss-islington merged commit 13d4489 into python:main Oct 7, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 7, 2022
@warsaw
warsaw deleted the issue-86298 branch October 7, 2022 02:50
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
0de5aff
* 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)
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@warsaw @mpage
pythongh-86298: Ensure that __loader__ and __spec__.loader agree in w…
2781fef
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@warsaw warsaw

Labels

topic-importlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@warsaw @miss-islington @brettcannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.