Skip to content

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

Merged
miss-islington merged 8 commits intopython:mainfrom
encukou:pytype_getmodulebydef_tests
Feb 2, 2022
Merged

bpo-46433: _PyType_GetModuleByDef: handle static types in MRO#30696
miss-islington merged 8 commits intopython:mainfrom
encukou:pytype_getmodulebydef_tests

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
with self.assertRaises(TypeError):
increment_count(1, 2, 3)

def test_get_module_bad_def(self):
Copy link
Member

@vstinner vstinner Feb 2, 2022

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

miss-islington commented Feb 2, 2022

@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
@miss-islington
Copy link
Contributor

miss-islington commented Feb 4, 2022

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

@miss-islington
Copy link
Contributor

miss-islington commented Feb 4, 2022

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

@miss-islington
Copy link
Contributor

miss-islington commented Feb 4, 2022

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

miss-islington commented Feb 4, 2022

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)
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 10, 2022
@bedevere-bot
Copy link

bedevere-bot commented Feb 10, 2022

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 kumaraditya303 requested changes

@vstinner vstinner vstinner approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

@miss-islington miss-islington

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@encukou @miss-islington @bedevere-bot @vstinner @kumaraditya303 @the-knights-who-say-ni