Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 17 commits into from
Oct 7, 2022
Merged

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

merged 17 commits into from
Oct 7, 2022

Conversation

warsaw
Copy link
Member

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

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

Automerge-Triggered-By: GH:warsaw

@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
```
@warsaw
Copy link
Member Author

warsaw commented Oct 3, 2022

Tagging @brettcannon

@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 self-assigned this Oct 3, 2022
@warsaw warsaw marked this pull request as ready for review October 4, 2022 00:03
@warsaw
Copy link
Member Author

warsaw commented Oct 4, 2022

Converting from draft since I now have tests.

@warsaw
Copy link
Member Author

warsaw commented Oct 4, 2022

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
Copy link
Member Author

warsaw commented Oct 4, 2022

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

@warsaw warsaw mentioned this pull request Oct 4, 2022
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
Open
brettcannon
brettcannon approved these changes Oct 5, 2022
View reviewed changes
Python/_warnings.c Outdated Show resolved Hide resolved
@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
Copy link
Member Author

warsaw commented Oct 7, 2022

@brettcannon Please take another look.

brettcannon
brettcannon approved these changes Oct 7, 2022
View reviewed changes
Lib/importlib/_bootstrap_external.py Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Show resolved Hide resolved
Lib/test/test_importlib/import_/test_helpers.py Show resolved Hide resolved
Lib/test/test_importlib/import_/test_helpers.py Show resolved Hide resolved
Python/_warnings.c Show resolved Hide resolved
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington miss-islington merged commit 13d4489 into python:main 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

Assignees

@warsaw warsaw

Labels
topic-importlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@warsaw @miss-islington @brettcannon @bedevere-bot