Title: Cleanup extension functions using _PyObject_LookupSpecial
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: eelizondo, josh.r, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2018-12-07 23:25 by eelizondo, last changed 2018-12-10 21:31 by josh.r. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11082 closed eelizondo, 2018-12-10 16:17
Messages (8)
msg331364 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2018-12-07 23:25
Three extension modules: _testcapimodule.c, posixmodule.c, and mathmodule.c are using `_PyObject_LookupSpecial` which is not API.

These should instead use `PyObject_GetAttrString`, `PyType_GetSlot`.
msg331372 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2018-12-08 01:50
Correction, this is not as trivial as just using `PyObject_GetAttrString`. Will investigate the correct behavior.
msg331373 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2018-12-08 03:01
Batteries-included extension modules aren't limited to the public and/or limited API; they use tons of undocumented internal APIs (everything to do with Py_IDENTIFIERs being an obvious and frequently used non-public API).

_PyObject_LookupSpecial is necessary to lookup special methods on the class of an instance (bypassing the instance itself) when no C level slot is associated with the special method (e.g. the math module using it to look up __ceil__ to implement math.ceil). Sure, each of these modules could reimplement it from scratch, but I'm not seeing the point in doing so.
msg331522 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-10 16:18
> Three extension modules: _testcapimodule.c, posixmodule.c, and mathmodule.c are using `_PyObject_LookupSpecial` which is not API.

I don't understand the issue that you are trying to solve. Yes, Python builtin extensions use private functions of the C API.
msg331523 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2018-12-10 17:08
@vstinner: Sorry for not being clear - The intention of this change is two-fold:
1) Simplify the implementation of these functions.
2) Reduce the surface area of the C-API.

Given that the same functionality can be achieved with public functions of the C-API. This is a nice cleanup over the current approach.
msg331524 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2018-12-10 17:11
I also fixed the title to properly reflect what this is trying to achieve.
msg331532 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-10 19:25
There is nothing wrong with using private C API in the implementation of standard CPython extensions. This API was designed for this.

In contrary, there are problems with your code:

* It is less efficient. String objects are created and destroyed twice per every function call, in PyObject_HasAttrString() and in PyObject_CallMethod(). Format string is parsed in PyObject_CallMethod(). Other temporary objects are created and destroyed.

* It uses inherently broken PyObject_HasAttrString(). PyObject_HasAttrString() swallows exceptions (for example a MemoryError raised when create a temporary string object) and can return an incorrect result.

* It is not equivalent with the existing code. For example it does not work properly if the dunder method is a static method.
msg331537 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2018-12-10 21:31
Agreed with everything in Serhiy's comments. This patch disregards why _PyObject_LookupSpecial and the various _Py_IDENTIFIER related stuff was created in the first place (to handle a non-trivial task efficiently/correctly) in favor of trying to avoid C-APIs that are explicitly okay to use for the CPython standard extensions. The goal is a mistake in the first place; no patch fix will make the goal correct.

Closing as not a bug.
Date User Action Args
2018-12-10 21:31:29josh.rsetstatus: open -> closed
resolution: not a bug
messages: + msg331537

stage: patch review -> resolved
2018-12-10 19:25:08serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg331532
2018-12-10 17:11:35eelizondosetmessages: + msg331524
2018-12-10 17:11:10eelizondosettitle: Extension modules using non-API functions -> Cleanup extension functions using _PyObject_LookupSpecial
2018-12-10 17:08:42eelizondosetmessages: + msg331523
2018-12-10 16:18:46vstinnersetnosy: + vstinner
messages: + msg331522
2018-12-10 16:17:11eelizondosetkeywords: + patch
stage: patch review
pull_requests: + pull_request10316
2018-12-08 03:01:12josh.rsetnosy: + josh.r
messages: + msg331373
2018-12-08 01:50:03eelizondosetmessages: + msg331372
2018-12-07 23:25:59eelizondocreate