-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
gh-128632: fix segfault on nested __classdict__ type param #128744
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
c69a602
gh-128632: fix segfault on nested __classdict__ type param
tom-pytel c7ec3ed
📜🤖 Added by blurb_it.
blurb-it[bot] 8f813c1
requested changes
tom-pytel 285be46
disallow __classcell__ and __classdictcell__
tom-pytel 4d736e9
requested comment changes
tom-pytel 841f738
Merge branch 'main' into fix-issue-128632
tom-pytel cee5fe4
faster attr name compare maybe
tom-pytel e76604e
Merge branch 'main' into fix-issue-128632
tom-pytel 0a9bcf3
Merge branch 'main' into fix-issue-128632
tom-pytel bc1faff
Merge branch 'main' into fix-issue-128632
tom-pytel 151624a
requested changes
tom-pytel 3a7eb7a
Merge branch 'main' into fix-issue-128632
tom-pytel e2c44d7
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-11-20-11-28.gh-issu…
JelleZijlstra 3d65ad1
Merge branch 'main' into fix-issue-128632
tom-pytel 26bd8ef
Merge branch 'main' into fix-issue-128632
tom-pytel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Core_and_Builtins/2025-01-11-20-11-28.gh-issue-128632.ryhnKs.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Disallow ``__classdict__`` as the name of a type parameter. Using this | ||
name would previously crash the interpreter in some circumstances. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also test the following related cases:
__classcell__
and__classdictcell__
, which appear internally in the implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I interpret 'somewhat likely to cause problems' to mean yes disallow these names (and test them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names re-allowed and tests added. Also re-allowed
__class__
as that doesn't crash either even though it is special.