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

bpo-46433: _PyType_GetModuleByDef: handle static types in MRO #30696

Merged
merged 8 commits into from
Feb 2, 2022
Merged

bpo-46433: _PyType_GetModuleByDef: handle static types in MRO #30696

merged 8 commits into from
Feb 2, 2022
+74 −9

Conversation

encukou
Copy link
Member

@encukou encukou commented Jan 19, 2022 •
edited by miss-islington
Loading

https://bugs.python.org/issue46433

Automerge-Triggered-By: GH:encukou

@encukou encukou requested a review from markshannon as a code owner January 19, 2022 16:09
@encukou encukou requested a review from vstinner January 19, 2022 16:09
kumaraditya303
kumaraditya303 requested changes Jan 19, 2022
View reviewed changes
encukou and others added 2 commits January 19, 2022 17:27
@encukou
Regenerate Clinic
c2e855b
@encukou @kumaraditya303
Update Modules/_testmultiphase.c
2931857 
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@encukou encukou marked this pull request as draft January 20, 2022 07:45
@encukou
Copy link
Member Author

encukou commented Jan 20, 2022

I'll still improve the change itself -- or feel free to beat me to it. The tests should be useful though.

@encukou
Copy link
Member Author

encukou commented Jan 26, 2022

Ah, turns out I won't be able to optimize this further. _PyType_GetModuleByDef is currently called on static types, so the flag needs to be checked.
Removing the calls to _PyType_GetModuleByDef on static types isn't feasible IMO: it's too easy to miss the requirement in the future.

@vstinner, since you did the optimization, could you look at this code? Did I miss something performance-related?

@encukou encukou marked this pull request as ready for review January 26, 2022 14:32
vstinner
vstinner reviewed Jan 28, 2022
View reviewed changes
vstinner
vstinner approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

Lib/test/test_capi.py
@@ -1068,6 +1068,18 @@ def test_state_access(self):
with self.assertRaises(TypeError):
increment_count(1, 2, 3)

def test_get_module_bad_def(self):
Copy link
Member

Choose a reason for hiding this comment

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

You may mention "bpo-46433" in a comment, here and in test_get_module_static_in_mro().

@encukou
Copy link
Member Author

encukou commented Feb 2, 2022

Thanks for the review!

@miss-islington
Copy link
Contributor

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

@miss-islington miss-islington merged commit 0ef0853 into python:main Feb 2, 2022
@encukou encukou deleted the pytype_getmodulebydef_tests branch February 2, 2022 16:36
@encukou encukou added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Feb 4, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @encukou, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0ef08530124c5ca13a9394f4ac18bee8e6c66409 3.9

@miss-islington miss-islington self-assigned this Feb 4, 2022
@miss-islington
Copy link
Contributor

Sorry @encukou, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 0ef08530124c5ca13a9394f4ac18bee8e6c66409 3.10

encukou added a commit to encukou/cpython that referenced this pull request Feb 10, 2022
@encukou
bpo-46433: _PyType_GetModuleByDef: handle static types in MRO (python…
a74fa33 
…GH-30696)

(cherry picked from commit 0ef0853)
@encukou encukou removed the needs backport to 3.9 only security fixes label Feb 10, 2022
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 10, 2022
@bedevere-bot
Copy link

GH-31262 is a backport of this pull request to the 3.10 branch.

encukou added a commit that referenced this pull request Feb 11, 2022
@encukou
[3.10] bpo-46433: _PyType_GetModuleByDef: handle static types in MRO (G…
8b8673f 
…H-30696) (GH-31262)


(cherry picked from commit 0ef0853)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumaraditya303 kumaraditya303

@vstinner vstinner

@markshannon markshannon

Assignees

@miss-islington miss-islington

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@encukou @miss-islington @bedevere-bot @vstinner @kumaraditya303 @the-knights-who-say-ni