Preliminary PEP 573 (module state access) implementation#3

encukou · 2020-04-14T20:16:04Z

WIP PEP 573 implementation. Not yet ready to send as a PR to CPython.
@dormouse759 @vstinner, do you want to comment?

Look at the diff rather than individual commits. Lots of changes have been reverted/rebased. Marcel started the implementation in 2017.

There's still lots to do:

Make sure this matches all of the text in the PEP
- Argument Clinic: Check the new argument doesn not appear in __text_signature__.
Try not subclassing PyCMethod from PyCFunction
Add documentation
Move tests to a better place
Add more tests?
Convert a stdlib module or two
etc.

vstinner · 2020-04-16T13:05:54Z

Convert a stdlib module or two

Maybe reintroducing _json assertions would be a good start? See https://bugs.python.org/issue40137

Also adds PyType_FromModuleAndSpec constructor

encukou · 2020-04-21T16:38:42Z

I've rebased to make the commits show individual pieces, rather than history.
@vstinner, could you please check if this matches the newest C-API conventions?

Also adds the signatures for PyCFunction*: these were not summarized in the docs; instead and they were defined in prose with references to simpler signatures. That is hard to read.

Instead, PyCFunction_Type is made variable, so it can hold PyCMethodObject (or any future extensions) directly. (This is similar to how PyType_Type handles the need for extra storage.)

vstinner

Thanks @encukou for working on that! It's a great addition to Python 3.9 and I will ensure that it lands before beta1 :-)

As promised, here is a first review. The overall change looks good, but I didn't dig into the details yet. My main concern is about the conversion to PyObject_VAR_HEAD.

Doc/c-api/type.rst

Include/cpython/methodobject.h

Doc/c-api/type.rst

Objects/descrobject.c

Objects/methodobject.c

vstinner · 2020-04-29T18:12:16Z

Include/cpython/methodobject.h

+         ((PyCMethodObject *)func) -> mm_class : NULL)
+
+typedef struct {
+    PyObject_VAR_HEAD


Why did you convert PyCFunctionObject from PyObject_HEAD to PyObject_VAR_HEAD?

The code below suprised me:

PyCMethodObject *om = PyObject_GC_NewVar( PyCMethodObject, &PyCFunction_Type, sizeof(PyCMethodObject) - sizeof(PyCFunctionObject));

It needs space to store the new mm_class, i.e. be backed by PyCMethodObject rather than PyCFunctionObject.
What's the best way to do this?
Marcel's original work made PyCMethod a subclass of PyCFunction, which works -- but PyCFunction is not Py_TPFLAGS_BASETYPE so it can't be subclassed. It only worked because static types bypass the Py_TPFLAGS_BASETYPE check.

Maybe it would be best to add mm_class to PyCFunctionObject, i.e. to all built-in methods. But that isn't what the PEP says we'll do.

Reverted and fixed up. PyCMethod_Type (builtin_method) now inherits from PyCFunction_Type (builtin_function_or_method), as in Marcel's branch.

Objects/typeobject.c

…ython/ Add info about the new header file

Nitpicks

Move the Python part of the tests to test_capi

This reverts commit 6c39835.

Properly inherit PyCMethod_Type from PyCFunction_Type

vstinner

LGTM. I just left a few minor coding style remarks.

Objects/methodobject.c

Modules/_testmultiphase.c

Objects/typeobject.c

encukou · 2020-05-05T16:22:08Z

CPython's C code style is still foreign to me :(

Style nitpicks

encukou · 2020-05-05T17:22:21Z

This is now python#19936

pythonGH-30611) …0607)

…python#91466) Fix an uninitialized bool in exception print context. `struct exception_print_context.need_close` was uninitialized. Found by oss-fuzz in a test case running under the undefined behavior sanitizer. https://oss-fuzz.com/testcase-detail/6217746058182656 ``` Python/pythonrun.c:1241:28: runtime error: load of value 253, which is not a valid value for type 'bool' #0 0xbf2203 in print_chained cpython3/Python/pythonrun.c:1241:28 #1 0xbea4bb in print_exception_cause_and_context cpython3/Python/pythonrun.c:1320:19 #2 0xbea4bb in print_exception_recursive cpython3/Python/pythonrun.c:1470:13 #3 0xbe9e39 in _PyErr_Display cpython3/Python/pythonrun.c:1517:9 ``` Pretty obvious what the ommission was upon code inspection.

encukou changed the title ~~Preliminary PEP 573 implementation~~ Preliminary PEP 573 (module state access) implementation Apr 14, 2020

encukou and others added 7 commits April 21, 2020 16:49

Include/methodobject.h: Move Py_LIMITED_API declarations to cpython/

71ea794

Add ht_module to classes

e438e41

Also adds PyType_FromModuleAndSpec constructor

Add PyType_GetModule and PyType_GetModuleState accessors

44886c5

Add PyCMethod_* and machinery to call it

c0fe53b

Add defining_class converter to Clinic

d010680

Add NEWS blurb

95a0a61

Add tests: with clinic and without clinic

6ee77d8

encukou force-pushed the pep-573 branch from 43be64d to 6ee77d8 Compare April 21, 2020 16:35

encukou changed the base branch from master to 3.6 April 21, 2020 16:36

encukou changed the base branch from 3.6 to master April 21, 2020 16:36

encukou changed the base branch from master to 3.6 April 21, 2020 16:36

encukou changed the base branch from 3.6 to master April 21, 2020 16:36

encukou added 2 commits April 28, 2020 17:39

Add documentation for PEP 573

36a0a9f

Also adds the signatures for PyCFunction*: these were not summarized in the docs; instead and they were defined in prose with references to simpler signatures. That is hard to read.

Don't make METH_METHOD functions a separate Python type

6c39835

Instead, PyCFunction_Type is made variable, so it can hold PyCMethodObject (or any future extensions) directly. (This is similar to how PyType_Type handles the need for extra storage.)

encukou force-pushed the pep-573 branch from 66718a6 to 6c39835 Compare April 28, 2020 16:59

vstinner reviewed Apr 29, 2020

View reviewed changes

encukou added 7 commits May 5, 2020 16:00

fixup! Add documentation for PEP 573

f7033c4

fixup! Include/methodobject.h: Move Py_LIMITED_API declarations to cp…

6fc8a5e

…ython/ Add info about the new header file

fixup! Add PyType_GetModule and PyType_GetModuleState accessors

3f1623f

fixup! Add PyCMethod_* and machinery to call it

2e124a8

Nitpicks

fixup! Add tests: with clinic and without clinic

adc1daa

Move the Python part of the tests to test_capi

Revert "Don't make METH_METHOD functions a separate Python type"

7787175

This reverts commit 6c39835.

fixup! Add PyCMethod_* and machinery to call it

04a0dc8

Properly inherit PyCMethod_Type from PyCFunction_Type

vstinner approved these changes May 5, 2020

View reviewed changes

encukou added 2 commits May 5, 2020 18:26

fixup! Add PyCMethod_* and machinery to call it

1df0e6d

Style nitpicks

fixup! Add PyType_GetModule and PyType_GetModuleState accessors

b4be6d8

Style nitpicks

fixup! Add tests: with clinic and without clinic

03ed6dc

Style nitpicks

encukou closed this May 5, 2020

encukou pushed a commit that referenced this pull request Feb 2, 2022

[3.9] bpo-46383: Fix signature of zoneinfo module_free function (GH-3… (

3e7d06a

pythonGH-30611) …0607)

Conversation

encukou commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Apr 16, 2020

Uh oh!

encukou commented Apr 21, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

encukou May 5, 2020

Choose a reason for hiding this comment

Uh oh!

encukou May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

encukou commented May 5, 2020

Uh oh!

encukou commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

encukou commented Apr 14, 2020 •

edited

Loading