From 2def279845116081759b35f56a6f4bbbcdfd6428 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 11 May 2023 15:04:11 +0200 Subject: [PATCH 1/5] gh-104389: Argument Clinic now wraps unused args with Py_UNUSED Use the unused keyword param in the converter to explicitly mark an argument as unused: /*[clinic input] BaseClass.interface flag: bool(unused=True) --- Doc/howto/clinic.rst | 3 +++ Modules/_io/clinic/textio.c.h | 25 ++++++++++++++++--------- Modules/_io/textio.c | 7 ++++--- Tools/clinic/clinic.py | 20 +++++++++++++++++++- 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/Doc/howto/clinic.rst b/Doc/howto/clinic.rst index 6ebc2d9b0a71a9..a93bf814d2b1d0 100644 --- a/Doc/howto/clinic.rst +++ b/Doc/howto/clinic.rst @@ -786,6 +786,9 @@ of these arguments, along with their meanings: To accept ``None``, add ``NoneType`` to this set. + ``unused`` + Wrap the argument with :c:macro:`Py_UNUSED` in the impl function signature. + ``bitwise`` Only supported for unsigned integers. The native integer value of this Python argument will be written to the parameter without any range checking, diff --git a/Modules/_io/clinic/textio.c.h b/Modules/_io/clinic/textio.c.h index 01965013ec6abc..97588b76636f9c 100644 --- a/Modules/_io/clinic/textio.c.h +++ b/Modules/_io/clinic/textio.c.h @@ -33,7 +33,7 @@ _io__TextIOBase_detach(PyObject *self, PyTypeObject *cls, PyObject *const *args, } PyDoc_STRVAR(_io__TextIOBase_read__doc__, -"read($self, /, *args)\n" +"read($self, size=-1, /)\n" "--\n" "\n" "Read at most size characters from stream.\n" @@ -45,7 +45,8 @@ PyDoc_STRVAR(_io__TextIOBase_read__doc__, {"read", _PyCFunction_CAST(_io__TextIOBase_read), METH_METHOD|METH_FASTCALL|METH_KEYWORDS, _io__TextIOBase_read__doc__}, static PyObject * -_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, PyObject *args); +_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, + int Py_UNUSED(size)); static PyObject * _io__TextIOBase_read(PyObject *self, PyTypeObject *cls, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) @@ -57,7 +58,7 @@ _io__TextIOBase_read(PyObject *self, PyTypeObject *cls, PyObject *const *args, P # define KWTUPLE NULL #endif - static const char * const _keywords[] = { NULL}; + static const char * const _keywords[] = {"", NULL}; static _PyArg_Parser _parser = { .keywords = _keywords, .fname = "read", @@ -65,17 +66,23 @@ _io__TextIOBase_read(PyObject *self, PyTypeObject *cls, PyObject *const *args, P }; #undef KWTUPLE PyObject *argsbuf[1]; - PyObject *__clinic_args = NULL; + int size = -1; - args = _PyArg_UnpackKeywordsWithVararg(args, nargs, NULL, kwnames, &_parser, 0, 0, 0, 0, argsbuf); + args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 0, 1, 0, argsbuf); if (!args) { goto exit; } - __clinic_args = args[0]; - return_value = _io__TextIOBase_read_impl(self, cls, __clinic_args); + if (nargs < 1) { + goto skip_optional_posonly; + } + size = _PyLong_AsInt(args[0]); + if (size == -1 && PyErr_Occurred()) { + goto exit; + } +skip_optional_posonly: + return_value = _io__TextIOBase_read_impl(self, cls, size); exit: - Py_XDECREF(__clinic_args); return return_value; } @@ -934,4 +941,4 @@ _io_TextIOWrapper_close(textio *self, PyObject *Py_UNUSED(ignored)) { return _io_TextIOWrapper_close_impl(self); } -/*[clinic end generated code: output=d800e5a8a50d6720 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=e8f1fe5e26e1897c input=a9049054013a1b77]*/ diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 3cc292cc35102e..4bee4ec1652bf1 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -69,8 +69,8 @@ _io__TextIOBase_detach_impl(PyObject *self, PyTypeObject *cls) /*[clinic input] _io._TextIOBase.read cls: defining_class + size: int(unused=True) = -1 / - *args: object Read at most size characters from stream. @@ -79,8 +79,9 @@ If size is negative or omitted, read until EOF. [clinic start generated code]*/ static PyObject * -_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, PyObject *args) -/*[clinic end generated code: output=3adf28998831f461 input=cee1e84664a20de0]*/ +_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, + int Py_UNUSED(size)) +/*[clinic end generated code: output=51a5178a309ce647 input=f5e37720f9fc563f]*/ { _PyIO_State *state = IO_STATE(); return _unsupported(state, "read"); diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 2ae8a02aa6d285..19c4cd299f0bbb 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -2599,6 +2599,9 @@ class CConverter(metaclass=CConverterAutoRegister): # Every non-abstract subclass should supply a valid value. c_ignored_default = 'NULL' + # If true, wrap with Py_UNUSED. + unused = False + # The C converter *function* to be used, if any. # (If this is not None, format_unit must be 'O&'.) converter = None @@ -2651,9 +2654,22 @@ class CConverter(metaclass=CConverterAutoRegister): signature_name = None # keep in sync with self_converter.__init__! - def __init__(self, name, py_name, function, default=unspecified, *, c_default=None, py_default=None, annotation=unspecified, **kwargs): + def __init__(self, + # Positional args: + name, + py_name, + function, + default=unspecified, + *, # Keyword only args: + c_default=None, + py_default=None, + annotation=unspecified, + unused=False, + **kwargs + ): self.name = ensure_legal_c_identifier(name) self.py_name = py_name + self.unused = unused if default is not unspecified: if self.default_type and not isinstance(default, (self.default_type, Unknown)): @@ -2800,6 +2816,8 @@ def simple_declaration(self, by_reference=False, *, in_parser=False): name = self.parser_name else: name = self.name + if self.unused: + name = f"Py_UNUSED({name})" prototype.append(name) return "".join(prototype) From 90c664427abbb30b680670daaa2a17844f66ceed Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 11 May 2023 15:12:18 +0200 Subject: [PATCH 2/5] Add NEWS entry --- .../Tools-Demos/2023-05-11-15-12-11.gh-issue-104389.EiOhB3.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Tools-Demos/2023-05-11-15-12-11.gh-issue-104389.EiOhB3.rst diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-05-11-15-12-11.gh-issue-104389.EiOhB3.rst b/Misc/NEWS.d/next/Tools-Demos/2023-05-11-15-12-11.gh-issue-104389.EiOhB3.rst new file mode 100644 index 00000000000000..854e1cca967c42 --- /dev/null +++ b/Misc/NEWS.d/next/Tools-Demos/2023-05-11-15-12-11.gh-issue-104389.EiOhB3.rst @@ -0,0 +1,2 @@ +Argument Clinic C converters now accept the ``unused`` keyword, for wrapping +a parameter with :c:macro:`Py_UNUSED`. Patch by Erlend E. Aasland. From d0f2b5a3977bf0cdae53c15d8459d629c13593b5 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 11 May 2023 15:15:42 +0200 Subject: [PATCH 3/5] Fix docs --- Doc/howto/clinic.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/howto/clinic.rst b/Doc/howto/clinic.rst index a93bf814d2b1d0..4620b4617e3450 100644 --- a/Doc/howto/clinic.rst +++ b/Doc/howto/clinic.rst @@ -775,6 +775,9 @@ All Argument Clinic converters accept the following arguments: because :pep:`8` mandates that the Python library may not use annotations. + ``unused`` + Wrap the argument with :c:macro:`Py_UNUSED` in the impl function signature. + In addition, some converters accept additional arguments. Here is a list of these arguments, along with their meanings: @@ -786,9 +789,6 @@ of these arguments, along with their meanings: To accept ``None``, add ``NoneType`` to this set. - ``unused`` - Wrap the argument with :c:macro:`Py_UNUSED` in the impl function signature. - ``bitwise`` Only supported for unsigned integers. The native integer value of this Python argument will be written to the parameter without any range checking, From b03874be14ce4cb7b8a28f442280841535480d64 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 11 May 2023 19:05:21 +0200 Subject: [PATCH 4/5] Revert _io example --- Modules/_io/clinic/textio.c.h | 25 +++++++++---------------- Modules/_io/textio.c | 7 +++---- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/Modules/_io/clinic/textio.c.h b/Modules/_io/clinic/textio.c.h index 97588b76636f9c..01965013ec6abc 100644 --- a/Modules/_io/clinic/textio.c.h +++ b/Modules/_io/clinic/textio.c.h @@ -33,7 +33,7 @@ _io__TextIOBase_detach(PyObject *self, PyTypeObject *cls, PyObject *const *args, } PyDoc_STRVAR(_io__TextIOBase_read__doc__, -"read($self, size=-1, /)\n" +"read($self, /, *args)\n" "--\n" "\n" "Read at most size characters from stream.\n" @@ -45,8 +45,7 @@ PyDoc_STRVAR(_io__TextIOBase_read__doc__, {"read", _PyCFunction_CAST(_io__TextIOBase_read), METH_METHOD|METH_FASTCALL|METH_KEYWORDS, _io__TextIOBase_read__doc__}, static PyObject * -_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, - int Py_UNUSED(size)); +_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, PyObject *args); static PyObject * _io__TextIOBase_read(PyObject *self, PyTypeObject *cls, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) @@ -58,7 +57,7 @@ _io__TextIOBase_read(PyObject *self, PyTypeObject *cls, PyObject *const *args, P # define KWTUPLE NULL #endif - static const char * const _keywords[] = {"", NULL}; + static const char * const _keywords[] = { NULL}; static _PyArg_Parser _parser = { .keywords = _keywords, .fname = "read", @@ -66,23 +65,17 @@ _io__TextIOBase_read(PyObject *self, PyTypeObject *cls, PyObject *const *args, P }; #undef KWTUPLE PyObject *argsbuf[1]; - int size = -1; + PyObject *__clinic_args = NULL; - args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 0, 1, 0, argsbuf); + args = _PyArg_UnpackKeywordsWithVararg(args, nargs, NULL, kwnames, &_parser, 0, 0, 0, 0, argsbuf); if (!args) { goto exit; } - if (nargs < 1) { - goto skip_optional_posonly; - } - size = _PyLong_AsInt(args[0]); - if (size == -1 && PyErr_Occurred()) { - goto exit; - } -skip_optional_posonly: - return_value = _io__TextIOBase_read_impl(self, cls, size); + __clinic_args = args[0]; + return_value = _io__TextIOBase_read_impl(self, cls, __clinic_args); exit: + Py_XDECREF(__clinic_args); return return_value; } @@ -941,4 +934,4 @@ _io_TextIOWrapper_close(textio *self, PyObject *Py_UNUSED(ignored)) { return _io_TextIOWrapper_close_impl(self); } -/*[clinic end generated code: output=e8f1fe5e26e1897c input=a9049054013a1b77]*/ +/*[clinic end generated code: output=d800e5a8a50d6720 input=a9049054013a1b77]*/ diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 4bee4ec1652bf1..3cc292cc35102e 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -69,8 +69,8 @@ _io__TextIOBase_detach_impl(PyObject *self, PyTypeObject *cls) /*[clinic input] _io._TextIOBase.read cls: defining_class - size: int(unused=True) = -1 / + *args: object Read at most size characters from stream. @@ -79,9 +79,8 @@ If size is negative or omitted, read until EOF. [clinic start generated code]*/ static PyObject * -_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, - int Py_UNUSED(size)) -/*[clinic end generated code: output=51a5178a309ce647 input=f5e37720f9fc563f]*/ +_io__TextIOBase_read_impl(PyObject *self, PyTypeObject *cls, PyObject *args) +/*[clinic end generated code: output=3adf28998831f461 input=cee1e84664a20de0]*/ { _PyIO_State *state = IO_STATE(); return _unsupported(state, "read"); From bd4b10a7009915d923468e9ea1226f3f73d22f17 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 11 May 2023 21:48:58 +0200 Subject: [PATCH 5/5] Add test --- Lib/test/test_clinic.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 6aaf4d1ed8d560..28d9f650926427 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -774,6 +774,44 @@ def test_legacy_converters(self): module, function = block.signatures self.assertIsInstance((function.parameters['path']).converter, clinic.str_converter) + def test_unused_param(self): + block = self.parse(""" + module foo + foo.func + fn: object + k: float + i: float(unused=True) + / + * + flag: bool(unused=True) = False + """) + sig = block.signatures[1] # Function index == 1 + params = sig.parameters + conv = lambda fn: params[fn].converter + dataset = ( + {"name": "fn", "unused": False}, + {"name": "k", "unused": False}, + {"name": "i", "unused": True}, + {"name": "flag", "unused": True}, + ) + for param in dataset: + name, unused = param.values() + with self.subTest(name=name, unused=unused): + p = conv(name) + # Verify that the unused flag is parsed correctly. + self.assertEqual(unused, p.unused) + + # Now, check that we'll produce correct code. + decl = p.simple_declaration(in_parser=False) + if unused: + self.assertIn("Py_UNUSED", decl) + else: + self.assertNotIn("Py_UNUSED", decl) + + # Make sure the Py_UNUSED macro is not used in the parser body. + parser_decl = p.simple_declaration(in_parser=True) + self.assertNotIn("Py_UNUSED", parser_decl) + def parse(self, text): c = FakeClinic() parser = DSLParser(c)