classification
Title: TODO list when PEP 573 "Module State Access from C Extension Methods" will be implemented
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: corona10, phsilva, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-04-01 15:08 by vstinner, last changed 2020-05-14 17:03 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19177 merged corona10, 2020-04-01 15:20
PR 20055 closed shihai1991, 2020-05-12 15:18
Messages (3)
msg365482 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-01 15:08
In bpo-1635741, many C extension modules are converted to PEP 489 multiphase initialization and/or modified to get a module state. Problem: the module state cannot be accessed in some functions, and so some of these changes had to workaround the fact that PEP 573 "Module State Access from C Extension Methods" is not implemented yet.

This issue tracks C extension modules which should be modified once PEP 573 will be implemented:

* _functools: Py_CLEAR(kwd_mark); is commented in _functools_free()
  See commit eacc07439591c97f69ab4a3d17391b009cd78ae2

* _abc: abc_invalidation_counter is shared by all module instances. abc_data_new() requires access to abc_invalidation_counter but it doesn't have access to the module state. abc_invalidation_counter should be moved to the module state.
msg365486 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-01 15:19
Another minor issue: "assert(PyScanner_Check(self));" and "assert(PyEncoder_Check(self));" were removed by commit 33f15a16d40cb8010a8c758952cbf88d7912ee2d when _json module got a module state.

scanner_traverse(), scanner_clear(), encoder_traverse() and encoder_clear() cannot get the module state of a module using PEP 489 multiphase initialization.

Similar issue in scanner_call() and encoder_call().

I'm not sure that the PEP 573 provide a solution for this exact issue, since the intent of the assertion is to check the type of the argument. The PEP 573 gives access to the module state through types. But here we are not sure that the object type is the expected type... :-)

Again, it's a minor issue, it can be ignored. That's why I accepted to merge the commit. Technically, I don't see how we could get the wrong type in practice.
msg368852 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-14 17:03
I consider that all things that could be done have already been done, so I close the issue. Thanks for Hai and Dong-hee who helped ;-)

> * _functools: Py_CLEAR(kwd_mark); is commented in _functools_free()

Sadly, PEP 573 implementation is not complete enough to use it in _functools: see PR 20055.

> * _abc: abc_invalidation_counter

Fixed in bpo-40566 by:

commit 77c614624b6bf2145bef69830d0f499d8b55ec0c
Author: Dong-hee Na <donghee.na92@gmail.com>
Date:   Sat May 9 17:31:40 2020 +0900

    bpo-40566: Apply PEP 573 to abc module (GH-20005)


> scanner_traverse(), scanner_clear(), encoder_traverse() and encoder_clear()

tp_clear slot cannot get the defining type (PEP 573 may be extended later to allow that).

Using PyType_GetModule(Py_TYPE(self)) to access types stored in the module state and then compare Py_TYPE(self) to these types... looks badly broken :-) If we get the wrong type, PyType_GetModule(Py_TYPE(self)) will return the wrong module and so we cannot get the json state from the wrong module... It's a chicken-and-egg issue :-)

I think that it's not worth it to attempt to add back these assertions. It's very unlikely to get the wrong types in practice.
History
Date User Action Args
2020-05-14 17:03:45vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg368852

stage: patch review -> resolved
2020-05-12 15:18:42shihai1991setpull_requests: + pull_request19364
2020-04-01 16:17:55shihai1991setnosy: + shihai1991
2020-04-01 15:20:42corona10setkeywords: + patch
nosy: + corona10

pull_requests: + pull_request18636
stage: patch review
2020-04-01 15:19:56vstinnersetmessages: + msg365486
2020-04-01 15:14:45phsilvasetnosy: + phsilva
2020-04-01 15:08:23vstinnercreate