Skip to content

Conversation

@ericsnowcurrently
Copy link
Member

@ericsnowcurrentlyericsnowcurrently commented May 30, 2023

In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process.

Copy link
Member

@gpsheadgpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a meaningful place to put a regression test to assert that tp_bases and tp_mro are never NULL in the situations user code was encountering that in 3.12beta1?

PyTypeObject*type;
intreadying;
intready;
// XXX tp_dict, tp_bases, and tp_mro can probably be statically
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this comment (added by the original PR) be updated now that bases and mro don't exist here?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@ericsnowcurrently
Copy link
MemberAuthor

Do we have a meaningful place to put a regression test to assert that tp_bases and tp_mro are never NULL in the situations user code was encountering that in 3.12beta1?

I'll add a test in test_capi.

@ericsnowcurrently
Copy link
MemberAuthor

I've added a test. Thanks for the suggestion.

Copy link
Member

@gpsheadgpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

I do wonder if anything is going to be tripped up by the tp_dict as well, but that seems like a separate category of change given I believe it could've been NULL in the past - if it comes up it can be addressed separately.

@gpshead
Copy link
Member

you might want a NEWS entry fwiw... as people do read the changelog between beta1 and beta2.

@ericsnowcurrentlyericsnowcurrently enabled auto-merge (squash) May 30, 2023 23:46
@ericsnowcurrentlyericsnowcurrently merged commit 7be667d into python:mainMay 31, 2023
@miss-islington
Copy link
Contributor

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @ericsnowcurrently, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 7be667dfafa2465df6342d72dca9c1f82dd830d0 3.12

@miss-islington
Copy link
Contributor

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 31, 2023
…ll Static Builtin Types (pythongh-105115) In pythongh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process. (cherry picked from commit 7be667d) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@bedevere-bot
Copy link

GH-105124 is a backport of this pull request to the 3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12 only security fixes label May 31, 2023
@ericsnowcurrentlyericsnowcurrently deleted the fix-tp-bases branch May 31, 2023 00:23
ericsnowcurrently pushed a commit that referenced this pull request Jun 1, 2023
…All Static Builtin Types (gh-105115) (gh-105124) In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process. (cherry picked from commit 7be667d) Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ericsnowcurrently@gpshead@miss-islington@bedevere-bot