Argument Clinic: add 'self' return converter

[zware]

BPO	20410
Nosy	@larryhastings, @zware, @serhiy-storchaka, @isidentical

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2014-01-27.18:30:18.047>
labels = ['type-feature', 'expert-argument-clinic']
title = "Argument Clinic: add 'self' return converter"
updated_at = <Date 2019-08-21.19:28:52.670>
user = 'https://github.com/zware'

bugs.python.org fields:

activity = <Date 2019-08-21.19:28:52.670>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Argument Clinic']
creation = <Date 2014-01-27.18:30:18.047>
creator = 'zach.ware'
dependencies = []
files = []
hgrepos = []
issue_num = 20410
keywords = []
message_count = 3.0
messages = ['209457', '342833', '350103']
nosy_count = 4.0
nosy_names = ['larry', 'zach.ware', 'serhiy.storchaka', 'BTaskaya']
pr_nums = []
priority = 'low'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20410'
versions = ['Python 3.5']

[zware]

It would be nice to have a 'self' return converter for simple functions like winreg.HKEYType.__enter__ (which is implemented as "Py_XINCREF(self); return self;"). With the typedef and type_object specifications now required for the class directive, 'self' is passed to that function as a PyHKEYObject *, but impl is expected to return a PyObject *.

In this particular instance, I can solve the problem (which is a compiler warning) with a custom self_return_converter that casts _return_value to PyObject *. I think a generic solution would be better in the long run and useful in more places, though.

[isidentical]

Doesn't clinic have a self_converter class? https://github.com/python/cpython/blame/master/Tools/clinic/clinic.py#L3465

[serhiy-storchaka]

Batuhan, argument converters and return converters are different things.

I have no opinion about the idea itself. You can try, Zachary, and see whether it makes the code clearer.

[erlend-aasland]

Here's a proof-of-concept implementation: https://github.com/erlend-aasland/cpython/pull/new/clinic-self-return-converter

I'm not sure this is worth it, though. The convention is that a function that returns a PyObject pointer must return a new reference, so IMO, this return converter makes code harder to read and reason about.

[arhadthedev]

I'm not sure this is worth it, though. The convention is that a function that returns a PyObject pointer must return a new reference, so IMO, this return converter makes code harder to read and reason about.

👍

The new converter saves neither lines, nor abstraction layers:

/*[clinic input]
self_returner -> self
    o: object
[clinic start generated code]*/

static PyObject *
self_returner_impl(PyObject *module, PyObject *o)
/*[clinic end generated code: output=5ead17b048fc35b6 input=8c4980ce7cbf8165]*/
{
    return o;
}

vs

/*[clinic input]
self_returner -> object
    o: object
[clinic start generated code]*/

static PyObject *
self_returner_impl(PyObject *module, PyObject *o)
/*[clinic end generated code: output=5ead17b048fc35b6 input=8c4980ce7cbf8165]*/
{
    return Py_XNewRef(o);
}

Edit: I used Py_XNewRef because the proof-of-concept uses Py_XINCREF.

[erlend-aasland]

@zware, are you still interested in following up this, or should we just close it? :)

[zware]

I think the PoC is missing the point a bit: it's not about the incref, it's about functions that get self as, say, a PyHKeyObject *, but need to return a PyObject *. It's just annoying to have to cast self from PyHKEYObject * to PyObject * when clinic ought to know that PyHKEYObject * is actually fine.

That said, I'd forgotten about this completely, so feel free to ignore me here :)

[erlend-aasland]

Ah, of course, it's about the compiler warning! :) In that case, yes, I do see the benefit of this. I'll revisit this later today.

[erlend-aasland]

Looking at winreg.c, I'd say that this is a better solution¹ 😎 So, I'm still kinda -1.

diff

diff --git a/PC/clinic/winreg.c.h b/PC/clinic/winreg.c.h
index 4109c85276..d1b8416893 100644
--- a/PC/clinic/winreg.c.h
+++ b/PC/clinic/winreg.c.h
@@ -72,19 +72,13 @@ PyDoc_STRVAR(winreg_HKEYType___enter____doc__,
 #define WINREG_HKEYTYPE___ENTER___METHODDEF    \
     {"__enter__", (PyCFunction)winreg_HKEYType___enter__, METH_NOARGS, winreg_HKEYType___enter____doc__},
 
-static PyHKEYObject *
+static PyObject *
 winreg_HKEYType___enter___impl(PyHKEYObject *self);
 
 static PyObject *
 winreg_HKEYType___enter__(PyHKEYObject *self, PyObject *Py_UNUSED(ignored))
 {
-    PyObject *return_value = NULL;
-    PyHKEYObject *_return_value;
-
-    _return_value = winreg_HKEYType___enter___impl(self);
-    return_value = (PyObject *)_return_value;
-
-    return return_value;
+    return winreg_HKEYType___enter___impl(self);
 }
 
 #endif /* (defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM) || defined(MS_WINDOWS_GAMES)) */
@@ -1795,4 +1789,4 @@ exit:
 #ifndef WINREG_QUERYREFLECTIONKEY_METHODDEF
     #define WINREG_QUERYREFLECTIONKEY_METHODDEF
 #endif /* !defined(WINREG_QUERYREFLECTIONKEY_METHODDEF) */
-/*[clinic end generated code: output=15dc2e6c4d4e2ad5 input=a9049054013a1b77]*/
+/*[clinic end generated code: output=c76d3bb0f017c70b input=a9049054013a1b77]*/
diff --git a/PC/winreg.c b/PC/winreg.c
index 4884125c36..3f0cc11f35 100644
--- a/PC/winreg.c
+++ b/PC/winreg.c
@@ -239,18 +239,8 @@ class HKEY_return_converter(CReturnConverter):
         data.return_conversion.append(
             'return_value = PyHKEY_FromHKEY(_PyModule_GetState(module), _return_value);\n')
 
-# HACK: this only works for PyHKEYObjects, nothing else.
-#       Should this be generalized and enshrined in clinic.py,
-#       destroy this converter with prejudice.
-class self_return_converter(CReturnConverter):
-    type = 'PyHKEYObject *'
-
-    def render(self, function, data):
-        self.declare(data)
-        data.return_conversion.append(
-            'return_value = (PyObject *)_return_value;\n')
 [python start generated code]*/
-/*[python end generated code: output=da39a3ee5e6b4b0d input=17e645060c7b8ae1]*/
+/*[python end generated code: output=da39a3ee5e6b4b0d input=a5e887b2fe4be7c1]*/
 
 #include "clinic/winreg.c.h"
 
@@ -307,14 +297,14 @@ winreg_HKEYType_Detach_impl(PyHKEYObject *self)
 }
 
 /*[clinic input]
-winreg.HKEYType.__enter__ -> self
+winreg.HKEYType.__enter__
 [clinic start generated code]*/
 
-static PyHKEYObject *
+static PyObject *
 winreg_HKEYType___enter___impl(PyHKEYObject *self)
-/*[clinic end generated code: output=52c34986dab28990 input=c40fab1f0690a8e2]*/
+/*[clinic end generated code: output=70ec10933068a08c input=85f6abf60774c88c]*/
 {
-    return (PyHKEYObject*)Py_XNewRef(self);
+    return Py_XNewRef(self);
 }
 
 

Footnotes

1. this is also how most other "self returning" context managers are implemented in extension modules in the stdlib ↩