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

Use specs instead of just __loader__ in C code #86298

Closed
brettcannon opened this issue Oct 23, 2020 · 7 comments
Closed

Use specs instead of just __loader__ in C code #86298

brettcannon opened this issue Oct 23, 2020 · 7 comments
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) sprint topic-importlib type-bug An unexpected behavior, bug, or error

Comments

@brettcannon
Copy link
Member

brettcannon commented Oct 23, 2020 •
edited by bedevere-app bot
Loading

BPO 42132
Nosy @brettcannon, @corona10

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-10-23.22:16:01.448>
labels = ['interpreter-core', 'type-bug', '3.10']
title = 'Use specs instead of just __loader__ in C code'
updated_at = <Date 2020-10-24.16:28:23.690>
user = 'https://github.com/brettcannon'

bugs.python.org fields:

activity = <Date 2020-10-24.16:28:23.690>
actor = 'corona10'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2020-10-23.22:16:01.448>
creator = 'brett.cannon'
dependencies = []
files = []
hgrepos = []
issue_num = 42132
keywords = []
message_count = 1.0
messages = ['379483']
nosy_count = 2.0
nosy_names = ['brett.cannon', 'corona10']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42132'
versions = ['Python 3.10']

Linked PRs
@brettcannon
Copy link
Member Author

_warnings.c, pylifecycle.c, and pythonrun.c all either use or set __loader__ but without also falling back on __spec__.

@brettcannon brettcannon added 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Oct 23, 2020
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@warsaw warsaw added the sprint label Oct 3, 2022
@warsaw
Copy link
Member

warsaw commented Oct 3, 2022

I'm going to take a look at these module attribute issues during the 2022 core sprint.

@warsaw warsaw mentioned this issue Oct 3, 2022
Merged
@ezio-melotti ezio-melotti moved this to Todo in Sprint 2024 Oct 3, 2022
@warsaw
Copy link
Member

warsaw commented Oct 4, 2022

I have _warnings.c implemented and tested.

@warsaw
Copy link
Member

warsaw commented Oct 4, 2022

I think there's nothing to do for pythonrun.c. These set __loader__ but not __spec__ directly, and the documentation outlines the expected semantics. If the documented semantics are incorrect, then there should be a separate tracking bug for that case.

Looking at pylifecycle.c next.

@warsaw
Copy link
Member

warsaw commented Oct 4, 2022

I think pylifecycle.c is in the same boat. So landing #97803 should close this ticket.

@warsaw warsaw mentioned this issue Oct 4, 2022
Open
@warsaw
Copy link
Member

warsaw commented Oct 7, 2022

Done!

@warsaw warsaw closed this as completed Oct 7, 2022
Repository owner moved this from Todo to Done in Sprint 2024 Oct 7, 2022
miss-islington pushed a commit that referenced this issue Oct 7, 2022
@warsaw
gh-86298: Ensure that __loader__ and __spec__.loader agree in warning…
13d4489 
…s.warn_explicit() (GH-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 #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
carljm added a commit to carljm/cpython that referenced this issue 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 issue 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
jessecomeau87 pushed a commit to jessecomeau87/Python that referenced this issue May 20, 2024
@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/cpython#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
```
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 24, 2024
@serhiy-storchaka
pythongh-122191: Fix test_warnings failure if run with -Werror
cdbc98d 
__spec__.loader is now required in the module globals (see pythongh-86298).
@serhiy-storchaka serhiy-storchaka mentioned this issue Jul 24, 2024
Merged
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 24, 2024
@serhiy-storchaka
pythongh-86298: Add black box tests in test_warnings
680eaef 
They are similar to white box tests in test_importlib.
@bedevere-app bedevere-app bot mentioned this issue Jul 24, 2024
Merged
@serhiy-storchaka serhiy-storchaka mentioned this issue Jul 25, 2024
Open
@serhiy-storchaka
Copy link
Member

These warnings are only emitted for the C implementation. See #122255.

serhiy-storchaka added a commit that referenced this issue Jul 25, 2024
@serhiy-storchaka
gh-122191: Fix test_warnings failure if run with -Werror (GH-122222)
9b4fe9b 
__spec__.loader is now required in the module globals (see gh-86298).
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 25, 2024
@serhiy-storchaka @miss-islington
pythongh-122191: Fix test_warnings failure if run with -Werror (pytho…
5c7c8f4 
…nGH-122222)

__spec__.loader is now required in the module globals (see pythongh-86298).
(cherry picked from commit 9b4fe9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington miss-islington mentioned this issue Jul 25, 2024
Merged
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 25, 2024
@serhiy-storchaka
[3.12] pythongh-122191: Fix test_warnings failure if run with -Werror (…
8377c48 
…pythonGH-122222)

__spec__.loader is now required in the module globals (see pythongh-86298).
(cherry picked from commit 9b4fe9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
@serhiy-storchaka serhiy-storchaka mentioned this issue Jul 25, 2024
Merged
serhiy-storchaka added a commit that referenced this issue Jul 25, 2024
@serhiy-storchaka
[3.12] gh-122191: Fix test_warnings failure if run with -Werror (GH-1…
be5e229 
…22222) (GH-122257)

__spec__.loader is now required in the module globals (see gh-86298).
(cherry picked from commit 9b4fe9b)
serhiy-storchaka added a commit that referenced this issue Jul 25, 2024
@miss-islington @serhiy-storchaka
[3.13] gh-122191: Fix test_warnings failure if run with -Werror (GH-1…
94db4cc 
…22222) (GH-122256)

__spec__.loader is now required in the module globals (see gh-86298).
(cherry picked from commit 9b4fe9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this issue Aug 8, 2024
@serhiy-storchaka
gh-122255: Add black box tests in test_warnings (GH-122227)
fe13c9b 
They are similar to white box tests for gh-86298 in test_importlib.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 8, 2024
@serhiy-storchaka @miss-islington
pythongh-122255: Add black box tests in test_warnings (pythonGH-122227)
6126b60 
They are similar to white box tests for pythongh-86298 in test_importlib.
(cherry picked from commit fe13c9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington miss-islington mentioned this issue Aug 8, 2024
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 8, 2024
@serhiy-storchaka @miss-islington
pythongh-122255: Add black box tests in test_warnings (pythonGH-122227)
2ed027a 
They are similar to white box tests for pythongh-86298 in test_importlib.
(cherry picked from commit fe13c9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington miss-islington mentioned this issue Aug 8, 2024
Merged
gpshead pushed a commit that referenced this issue Aug 9, 2024
@miss-islington @serhiy-storchaka
[3.13] gh-122255: Add black box tests in test_warnings (GH-122227) (G…
46f973f 
…H-122818)

gh-122255: Add black box tests in test_warnings (GH-122227)

They are similar to white box tests for gh-86298 in test_importlib.
(cherry picked from commit fe13c9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
@serhiy-storchaka serhiy-storchaka mentioned this issue Aug 14, 2024
Open
serhiy-storchaka added a commit that referenced this issue Aug 14, 2024
@miss-islington @serhiy-storchaka
[3.12] gh-122255: Add black box tests in test_warnings (GH-122227) (G…
c3ee1a7 
…H-122819)

They are similar to white box tests for gh-86298 in test_importlib.
(cherry picked from commit fe13c9b)

Co-authored-by: Serhiy Storchaka <[email protected]>
blhsing pushed a commit to blhsing/cpython that referenced this issue Aug 22, 2024
@serhiy-storchaka @blhsing
pythongh-122255: Add black box tests in test_warnings (pythonGH-122227)
d591fcd 
They are similar to white box tests for pythongh-86298 in test_importlib.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) sprint topic-importlib type-bug An unexpected behavior, bug, or error
Projects
Sprint 2024
Archived in project
Milestone
No milestone
Development

No branches or pull requests
3 participants
@brettcannon @warsaw @serhiy-storchaka