gh-128632: fix segfault on nested __classdict__ type param#128744
gh-128632: fix segfault on nested __classdict__ type param#128744JelleZijlstra merged 15 commits intopython:mainfrom
Conversation
tom-pytel
requested review from
JelleZijlstra,
carljm,
iritkatriel and
markshannon
as code owners
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
|
||
| @support.cpython_only | ||
| def test_disallowed_type_param_names(self): | ||
| # See gh-128632 |
There was a problem hiding this comment.
Could you also test the following related cases:
- Using a generic function or type alias instead of a class with these names for the type parameters
- Using the names
__classcell__and__classdictcell__, which appear internally in the implementation.
There was a problem hiding this comment.
Added func and alias tests. Didn't add __classcell__ or __classdictcell__ tests as those are not excluded and don't cause problems currently. Do you want to add them to the forbidden list?
>>> class A:
... class B[__classcell__]: print(type(__classcell__))
...
<class 'typing.TypeVar'>
>>> class A:
... class B[__classdictcell__]: print(type(__classdictcell__))
...
<class 'typing.TypeVar'>There was a problem hiding this comment.
Thanks for adding the tests! I want the *cell* names to also be tested, because they're also somewhat likely to cause problems, even if they don't currently crash.
There was a problem hiding this comment.
So I interpret 'somewhat likely to cause problems' to mean yes disallow these names (and test them).
There was a problem hiding this comment.
Since they work fine now we should not disallow them, especially because we'd want to backport this change. However, we should have a test to make sure future changes don't introduce crashes with these names.
There was a problem hiding this comment.
Names re-allowed and tests added. Also re-allowed __class__ as that doesn't crash either even though it is special.
|
Just a reminder, according to @ZeroIntensity this needs backporting to 3.13 and 3.12. |
|
I'm going to yield to Jelle. This seems complex enough that backporting could be too risky. |
Had a look and doesn't look too bad, same functions are present to fix. |
Misc/NEWS.d/next/Core_and_Builtins/2025-01-11-20-11-28.gh-issue-128632.ryhnKs.rst
Outdated
Show resolved
Hide resolved
…e-128632.ryhnKs.rst
JelleZijlstra
added
needs backport to 3.12
|
Thanks @tom-pytel for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
|
Sorry, @tom-pytel and @JelleZijlstra, I could not cleanly backport this to |
|
Sorry, @tom-pytel and @JelleZijlstra, I could not cleanly backport this to |
|
@tom-pytel are you interested in doing the backports? You'd need to run the |
Sure |
…hon#128744) (cherry picked from commit 891c61c)
…am (pythonGH-128744) (cherry picked from commit 891c61c) Co-authored-by: Tomasz Pytel <tompytel@gmail.com>
|
GH-132085 is a backport of this pull request to the 3.13 branch. |
|
GH-132090 is a backport of this pull request to the 3.12 branch. |
|
Ping @JelleZijlstra, backports complete. |
|
Thanks! Merged the 3.13 one and left a comment on the 3.12 one. |
4 participants
Add reserved name check in
symtable.c:symtable_visit_type_param_bound_or_default()for__class__and__classdict__to prevent segfault when__classdict__is used.