From 4a55fc5a9df90aa075c5756aaae0af33d9e8ca0a Mon Sep 17 00:00:00 2001 From: Larry Hastings Date: Sun, 12 Jan 2014 11:09:57 -0800 Subject: Issue #20214: Fixed a number of small issues and documentation errors in Argument Clinic (see issue for details). --- Doc/howto/clinic.rst | 140 ++++++++++++++++++++++++++++++++++++++++++------- Misc/NEWS | 3 ++ Modules/zlibmodule.c | 12 ++--- Tools/clinic/clinic.py | 118 ++++++++++++++++++++++++++++++++--------- 4 files changed, 223 insertions(+), 50 deletions(-) diff --git a/Doc/howto/clinic.rst b/Doc/howto/clinic.rst index 2324142..e9e1377 100644 --- a/Doc/howto/clinic.rst +++ b/Doc/howto/clinic.rst @@ -109,6 +109,13 @@ convert a function to work with it. Let's dive in! support all of these scenarios. But these are advanced topics--let's do something simpler for your first function. + Also, if the function has multiple calls to :c:func:`PyArg_ParseTuple` + or :c:func:`PyArg_ParseTupleAndKeywords` where it supports different + types for the same argument, or if the function uses something besides + PyArg_Parse functions to parse its arguments, it probably + isn't suitable for conversion to Argument Clinic. Argument Clinic + doesn't support generic functions or polymorphic parameters. + 3. Add the following boilerplate above the function, creating our block:: /*[clinic input] @@ -170,6 +177,11 @@ convert a function to work with it. Let's dive in! Write a pickled representation of obj to the open file. [clinic start generated code]*/ + The name of the class and module should be the same as the one + seen by Python. Check the name defined in the :c:type:`PyModuleDef` + or :c:type:`PyTypeObject` as appropriate. + + 8. Declare each of the parameters to the function. Each parameter should get its own line. All the parameter lines should be @@ -455,6 +467,28 @@ convert a function to work with it. Let's dive in! Advanced Topics =============== +Now that you've had some experience working with Argument Clinic, it's time +for some advanced topics. + + +Symbolic default values +----------------------- + +The default value you provide for a parameter can't be any arbitrary +expression. Currently the following are explicitly supported: + +* Numeric constants (integer and float) +* String constants +* ``True``, ``False``, and ``None`` +* Simple symbolic constants like ``sys.maxsize``, which must + start with the name of the module + +In case you're curious, this is implemented in ``from_builtin()`` +in ``Lib/inspect.py``. + +(In the future, this may need to get even more elaborate, +to allow full expressions like ``CONSTANT - 1``.) + Renaming the C functions generated by Argument Clinic ----------------------------------------------------- @@ -479,6 +513,29 @@ The base function would now be named ``pickler_dumper()``, and the impl function would now be named ``pickler_dumper_impl()``. +The NULL default value +---------------------- + +For string and object parameters, you can set them to ``None`` to indicate +that there is no default. However, that means the C variable will be +initialized to ``Py_None``. For convenience's sakes, there's a special +value called ``NULL`` for just this case: from Python's perspective it +behaves like a default value of ``None``, but the C variable is initialized +with ``NULL``. + + +Converting functions using PyArg_UnpackTuple +-------------------------------------------- + +To convert a function parsing its arguments with :c:func:`PyArg_UnpackTuple`, +simply write out all the arguments, specifying each as an ``object``. You +may specify the ``type`` argument to cast the type as appropriate. All +arguments should be marked positional-only (add a ``/`` on a line by itself +after the last argument). + +Currently the generated code will use :c:func:`PyArg_ParseTuple`, but this +will change soon. + Optional Groups --------------- @@ -570,8 +627,8 @@ To save time, and to minimize how much you need to learn to achieve your first port to Argument Clinic, the walkthrough above tells you to use "legacy converters". "Legacy converters" are a convenience, designed explicitly to make porting existing code to Argument Clinic -easier. And to be clear, their use is entirely acceptable when porting -code for Python 3.4. +easier. And to be clear, their use is acceptable when porting code for +Python 3.4. However, in the long term we probably want all our blocks to use Argument Clinic's real syntax for converters. Why? A couple @@ -585,8 +642,8 @@ reasons: restricted to what :c:func:`PyArg_ParseTuple` supports; this flexibility won't be available to parameters using legacy converters. -Therefore, if you don't mind a little extra effort, you should consider -using normal converters instead of legacy converters. +Therefore, if you don't mind a little extra effort, please use the normal +converters instead of legacy converters. In a nutshell, the syntax for Argument Clinic (non-legacy) converters looks like a Python function call. However, if there are no explicit @@ -597,12 +654,19 @@ the same converters. All arguments to Argument Clinic converters are keyword-only. All Argument Clinic converters accept the following arguments: -``doc_default`` - If the parameter takes a default value, normally this value is also - provided in the ``inspect.Signature`` metadata, and displayed in the - docstring. ``doc_default`` lets you override the value used in these - two places: pass in a string representing the Python value you wish - to use in these two contexts. +``py_default`` + The default value for this parameter when defined in Python. + Specifically, the value that will be used in the ``inspect.Signature`` + string. + If a default value is specified for the parameter, defaults to + ``repr(default)``, else defaults to ``None``. + Specified as a string. + +``c_default`` + The default value for this parameter when defined in C. + Specifically, this will be the initializer for the variable declared + in the "parse function". + Specified as a string. ``required`` If a parameter takes a default value, Argument Clinic infers that the @@ -612,6 +676,9 @@ All Argument Clinic converters accept the following arguments: Clinic that this parameter is not optional, even if it has a default value. + (The need for ``required`` may be obviated by ``c_default``, which is + newer but arguably a better solution.) + ``annotation`` The annotation value for this parameter. Not currently supported, because PEP 8 mandates that the Python library may not use @@ -634,10 +701,11 @@ on the right is the text you'd replace it with. ``'et'`` ``str(encoding='name_of_encoding', types='bytes bytearray str')`` ``'f'`` ``float`` ``'h'`` ``short`` -``'H'`` ``unsigned_short`` +``'H'`` ``unsigned_short(bitwise=True)`` ``'i'`` ``int`` -``'I'`` ``unsigned_int`` -``'K'`` ``unsigned_PY_LONG_LONG`` +``'I'`` ``unsigned_int(bitwise=True)`` +``'k'`` ``unsigned_long(bitwise=True)`` +``'K'`` ``unsigned_PY_LONG_LONG(bitwise=True)`` ``'L'`` ``PY_LONG_LONG`` ``'n'`` ``Py_ssize_t`` ``'O!'`` ``object(subclass_of='&PySomething_Type')`` @@ -681,6 +749,14 @@ available. For each converter it'll show you all the parameters it accepts, along with the default value for each parameter. Just run ``Tools/clinic/clinic.py --converters`` to see the full list. +Py_buffer +--------- + +When using the ``Py_buffer`` converter +(or the ``'s*'``, ``'w*'``, ``'*y'``, or ``'z*'`` legacy converters) +note that the code Argument Clinic generates for you will automatically +call :c:func:`PyBuffer_Release` on the buffer for you. + Advanced converters ------------------- @@ -745,15 +821,26 @@ encode the value you return like normal. Currently Argument Clinic supports only a few return converters:: + bool int + unsigned int long + unsigned int + size_t Py_ssize_t + float + double DecodeFSDefault None of these take parameters. For the first three, return -1 to indicate error. For ``DecodeFSDefault``, the return type is ``char *``; return a NULL pointer to indicate an error. +(There's also an experimental ``NoneType`` converter, which lets you +return ``Py_None`` on success or ``NULL`` on failure, without having +to increment the reference count on ``Py_None``. I'm not sure it adds +enough clarity to be worth using.) + To see all the return converters Argument Clinic supports, along with their parameters (if any), just run ``Tools/clinic/clinic.py --converters`` for the full list. @@ -873,13 +960,6 @@ to specify in your subclass. Here's the current list: The Python default value for this parameter, as a Python value. Or the magic value ``unspecified`` if there is no default. -``doc_default`` - ``default`` as it should appear in the documentation, - as a string. - Or ``None`` if there is no default. - This string, when run through ``eval()``, should produce - a Python value. - ``py_default`` ``default`` as it should appear in Python code, as a string. @@ -951,6 +1031,26 @@ write your own return converter, please read ``Tools/clinic/clinic.py``, specifically the implementation of ``CReturnConverter`` and all its subclasses. +METH_O and METH_NOARGS +---------------------------------------------- + +To convert a function using ``METH_O``, make sure the function's +single argument is using the ``object`` converter, and mark the +arguments as positional-only:: + + /*[clinic input] + meth_o_sample + + argument: object + / + [clinic start generated code]*/ + + +To convert a function using ``METH_NOARGS``, just don't specify +any arguments. + +You can still use a self converter, a return converter, and specify +a ``type`` argument to the object converter for ``METH_O``. Using Argument Clinic in Python files ------------------------------------- diff --git a/Misc/NEWS b/Misc/NEWS index d6be619..2b6d885 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -72,6 +72,9 @@ Tests Tools/Demos ----------- +- Issue #20214: Fixed a number of small issues and documentation errors in + Argument Clinic (see issue for details). + - Issue #20196: Fixed a bug where Argument Clinic did not generate correct parsing code for functions with positional-only parameters where all arguments are optional. diff --git a/Modules/zlibmodule.c b/Modules/zlibmodule.c index 5139888..575dbd2 100644 --- a/Modules/zlibmodule.c +++ b/Modules/zlibmodule.c @@ -198,7 +198,7 @@ static PyObject * zlib_compress(PyModuleDef *module, PyObject *args) { PyObject *return_value = NULL; - Py_buffer bytes = {NULL, NULL, 0, 0, 0, 0, NULL, NULL, NULL, NULL, NULL}; + Py_buffer bytes = {NULL, NULL}; int group_right_1 = 0; int level = 0; @@ -219,7 +219,7 @@ zlib_compress(PyModuleDef *module, PyObject *args) return_value = zlib_compress_impl(module, &bytes, group_right_1, level); /* Cleanup for bytes */ - if (bytes.buf) + if (bytes.obj) PyBuffer_Release(&bytes); return return_value; @@ -227,7 +227,7 @@ zlib_compress(PyModuleDef *module, PyObject *args) static PyObject * zlib_compress_impl(PyModuleDef *module, Py_buffer *bytes, int group_right_1, int level) -/*[clinic end generated code: checksum=9f055a396620bc1a8a13d74c3496249528b32b0d]*/ +/*[clinic end generated code: checksum=2c59af563a4595c5ecea4011701f482ae350aa5f]*/ { PyObject *ReturnVal = NULL; Byte *input, *output = NULL; @@ -789,7 +789,7 @@ static PyObject * zlib_Decompress_decompress(PyObject *self, PyObject *args) { PyObject *return_value = NULL; - Py_buffer data = {NULL, NULL, 0, 0, 0, 0, NULL, NULL, NULL, NULL, NULL}; + Py_buffer data = {NULL, NULL}; unsigned int max_length = 0; if (!PyArg_ParseTuple(args, @@ -800,7 +800,7 @@ zlib_Decompress_decompress(PyObject *self, PyObject *args) exit: /* Cleanup for data */ - if (data.buf) + if (data.obj) PyBuffer_Release(&data); return return_value; @@ -808,7 +808,7 @@ exit: static PyObject * zlib_Decompress_decompress_impl(compobject *self, Py_buffer *data, unsigned int max_length) -/*[clinic end generated code: checksum=5b1e4f9f1ef8eca55fff78356f9df0c81232ed3b]*/ +/*[clinic end generated code: checksum=e0058024c4a97b411d2e2197791b89fde175f76f]*/ { int err; unsigned int old_length, length = DEFAULTALLOC; diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index cd7c597..0da6690 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -38,6 +38,7 @@ version = '1' _empty = inspect._empty _void = inspect._void +NoneType = type(None) class Unspecified: def __repr__(self): @@ -678,8 +679,8 @@ static {impl_return_type} c.render(p, data) positional = parameters[-1].kind == inspect.Parameter.POSITIONAL_ONLY - if has_option_groups: - assert positional + if has_option_groups and (not positional): + fail("You cannot use optional groups ('[' and ']')\nunless all parameters are positional-only ('/').") # now insert our "self" (or whatever) parameters # (we deliberately don't call render on self converters) @@ -1321,6 +1322,10 @@ class CConverter(metaclass=CConverterAutoRegister): # Or the magic value "unspecified" if there is no default. default = unspecified + # If not None, default must be isinstance() of this type. + # (You can also specify a tuple of types.) + default_type = None + # "default" as it should appear in the documentation, as a string. # Or None if there is no default. doc_default = None @@ -1387,12 +1392,21 @@ class CConverter(metaclass=CConverterAutoRegister): self.name = name if default is not unspecified: + if self.default_type and not isinstance(default, self.default_type): + if isinstance(self.default_type, type): + types_str = self.default_type.__name__ + else: + types_str = ', '.join((cls.__name__ for cls in self.default_type)) + fail("{}: default value {!r} for field {} is not of type {}".format( + self.__class__.__name__, default, name, types_str)) self.default = default self.py_default = py_default if py_default is not None else py_repr(default) self.doc_default = doc_default if doc_default is not None else self.py_default self.c_default = c_default if c_default is not None else c_repr(default) - elif doc_default is not None: - fail(function.fullname + " argument " + name + " specified a 'doc_default' without having a 'default'") + else: + self.py_default = py_default + self.doc_default = doc_default + self.c_default = c_default if annotation != unspecified: fail("The 'annotation' parameter is not currently permitted.") self.required = required @@ -1538,6 +1552,7 @@ class CConverter(metaclass=CConverterAutoRegister): class bool_converter(CConverter): type = 'int' + default_type = bool format_unit = 'p' c_ignored_default = '0' @@ -1547,12 +1562,19 @@ class bool_converter(CConverter): class char_converter(CConverter): type = 'char' + default_type = str format_unit = 'c' c_ignored_default = "'\0'" + def converter_init(self): + if len(self.default) != 1: + fail("char_converter: illegal default value " + repr(self.default)) + + @add_legacy_c_converter('B', bitwise=True) class byte_converter(CConverter): type = 'byte' + default_type = int format_unit = 'b' c_ignored_default = "'\0'" @@ -1562,11 +1584,13 @@ class byte_converter(CConverter): class short_converter(CConverter): type = 'short' + default_type = int format_unit = 'h' c_ignored_default = "0" class unsigned_short_converter(CConverter): type = 'unsigned short' + default_type = int format_unit = 'H' c_ignored_default = "0" @@ -1577,6 +1601,7 @@ class unsigned_short_converter(CConverter): @add_legacy_c_converter('C', types='str') class int_converter(CConverter): type = 'int' + default_type = int format_unit = 'i' c_ignored_default = "0" @@ -1588,6 +1613,7 @@ class int_converter(CConverter): class unsigned_int_converter(CConverter): type = 'unsigned int' + default_type = int format_unit = 'I' c_ignored_default = "0" @@ -1597,11 +1623,13 @@ class unsigned_int_converter(CConverter): class long_converter(CConverter): type = 'long' + default_type = int format_unit = 'l' c_ignored_default = "0" class unsigned_long_converter(CConverter): type = 'unsigned long' + default_type = int format_unit = 'k' c_ignored_default = "0" @@ -1611,11 +1639,13 @@ class unsigned_long_converter(CConverter): class PY_LONG_LONG_converter(CConverter): type = 'PY_LONG_LONG' + default_type = int format_unit = 'L' c_ignored_default = "0" class unsigned_PY_LONG_LONG_converter(CConverter): type = 'unsigned PY_LONG_LONG' + default_type = int format_unit = 'K' c_ignored_default = "0" @@ -1625,23 +1655,27 @@ class unsigned_PY_LONG_LONG_converter(CConverter): class Py_ssize_t_converter(CConverter): type = 'Py_ssize_t' + default_type = int format_unit = 'n' c_ignored_default = "0" class float_converter(CConverter): type = 'float' + default_type = float format_unit = 'f' c_ignored_default = "0.0" class double_converter(CConverter): type = 'double' + default_type = float format_unit = 'd' c_ignored_default = "0.0" class Py_complex_converter(CConverter): type = 'Py_complex' + default_type = complex format_unit = 'D' c_ignored_default = "{0.0, 0.0}" @@ -1650,10 +1684,16 @@ class object_converter(CConverter): type = 'PyObject *' format_unit = 'O' - def converter_init(self, *, type=None, subclass_of=None): - if subclass_of: + def converter_init(self, *, converter=None, type=None, subclass_of=None): + if converter: + if subclass_of: + fail("object: Cannot pass in both 'converter' and 'subclass_of'") + self.format_unit = 'O&' + self.converter = converter + elif subclass_of: self.format_unit = 'O!' self.subclass_of = subclass_of + if type is not None: self.type = type @@ -1665,6 +1705,7 @@ class object_converter(CConverter): @add_legacy_c_converter('z#', nullable=True, length=True) class str_converter(CConverter): type = 'const char *' + default_type = (str, Null, NoneType) format_unit = 's' def converter_init(self, *, encoding=None, types="str", @@ -1731,6 +1772,7 @@ class PyByteArrayObject_converter(CConverter): class unicode_converter(CConverter): type = 'PyObject *' + default_type = (str, Null, NoneType) format_unit = 'U' @add_legacy_c_converter('u#', length=True) @@ -1738,6 +1780,7 @@ class unicode_converter(CConverter): @add_legacy_c_converter('Z#', nullable=True, length=True) class Py_UNICODE_converter(CConverter): type = 'Py_UNICODE *' + default_type = (str, Null, NoneType) format_unit = 'u' def converter_init(self, *, nullable=False, length=False): @@ -1760,11 +1803,11 @@ class Py_buffer_converter(CConverter): type = 'Py_buffer' format_unit = 'y*' impl_by_reference = True - c_ignored_default = "{NULL, NULL, 0, 0, 0, 0, NULL, NULL, NULL, NULL, NULL}" + c_ignored_default = "{NULL, NULL}" def converter_init(self, *, types='bytes bytearray buffer', nullable=False): - if self.default != unspecified: - fail("There is no legal default value for Py_buffer ") + if self.default not in (unspecified, None): + fail("The only legal default value for Py_buffer is None.") self.c_default = self.c_ignored_default types = set(types.strip().split()) bytes_type = set(('bytes',)) @@ -1783,7 +1826,7 @@ class Py_buffer_converter(CConverter): fail('Py_buffer_converter: illegal combination of arguments (nullable=True)') elif types == (bytes_bytearray_buffer_type): format_unit = 'y*' - elif types == (bytearray_type | rwuffer_type): + elif types == (bytearray_type | rwbuffer_type): format_unit = 'w*' if not format_unit: fail("Py_buffer_converter: illegal combination of arguments") @@ -1792,7 +1835,7 @@ class Py_buffer_converter(CConverter): def cleanup(self): name = ensure_legal_c_identifier(self.name) - return "".join(["if (", name, ".buf)\n PyBuffer_Release(&", name, ");\n"]) + return "".join(["if (", name, ".obj)\n PyBuffer_Release(&", name, ");\n"]) class self_converter(CConverter): @@ -1895,34 +1938,59 @@ return_value = Py_None; Py_INCREF(Py_None); '''.strip()) -class int_return_converter(CReturnConverter): +class bool_return_converter(CReturnConverter): type = 'int' def render(self, function, data): self.declare(data) self.err_occurred_if("_return_value == -1", data) - data.return_conversion.append( - 'return_value = PyLong_FromLong((long)_return_value);\n') - + data.return_conversion.append('return_value = PyBool_FromLong((long)_return_value);\n') class long_return_converter(CReturnConverter): type = 'long' + conversion_fn = 'PyLong_FromLong' + cast = '' def render(self, function, data): self.declare(data) self.err_occurred_if("_return_value == -1", data) data.return_conversion.append( - 'return_value = PyLong_FromLong(_return_value);\n') + ''.join(('return_value = ', self.conversion_fn, '(', self.cast, '_return_value);\n'))) + +class int_return_converter(long_return_converter): + type = 'int' + cast = '(long)' +class unsigned_long_return_converter(long_return_converter): + type = 'unsigned long' + conversion_fn = 'PyLong_FromUnsignedLong' + +class unsigned_int_return_converter(unsigned_long_return_converter): + type = 'unsigned int' + cast = '(unsigned long)' -class Py_ssize_t_return_converter(CReturnConverter): +class Py_ssize_t_return_converter(long_return_converter): type = 'Py_ssize_t' + conversion_fn = 'PyLong_FromSsize_t' + +class size_t_return_converter(long_return_converter): + type = 'size_t' + conversion_fn = 'PyLong_FromSize_t' + + +class double_return_converter(CReturnConverter): + type = 'double' + cast = '' def render(self, function, data): self.declare(data) - self.err_occurred_if("_return_value == -1", data) + self.err_occurred_if("_return_value == -1.0", data) data.return_conversion.append( - 'return_value = PyLong_FromSsize_t(_return_value);\n') + 'return_value = PyFloat_FromDouble(' + self.cast + '_return_value);\n') + +class float_return_converter(double_return_converter): + type = 'float' + cast = '(double)' class DecodeFSDefault_return_converter(CReturnConverter): @@ -2341,6 +2409,10 @@ class DSLParser: if isinstance(expr, ast.Name) and expr.id == 'NULL': value = NULL elif isinstance(expr, ast.Attribute): + c_default = kwargs.get("c_default") + if not (isinstance(c_default, str) and c_default): + fail("When you specify a named constant (" + repr(py_default) + ") as your default value,\nyou MUST specify a valid c_default.") + a = [] n = expr while isinstance(n, ast.Attribute): @@ -2350,11 +2422,8 @@ class DSLParser: fail("Malformed default value (looked like a Python constant)") a.append(n.id) py_default = ".".join(reversed(a)) - value = None - c_default = kwargs.get("c_default") - if not (isinstance(c_default, str) and c_default): - fail("When you specify a named constant (" + repr(py_default) + ") as your default value,\nyou MUST specify a valid c_default.") kwargs["py_default"] = py_default + value = eval(py_default) else: value = ast.literal_eval(expr) else: @@ -2388,7 +2457,8 @@ class DSLParser: if isinstance(annotation, ast.Name): return annotation.id, False, {} - assert isinstance(annotation, ast.Call) + if not isinstance(annotation, ast.Call): + fail("Annotations must be either a name, a function call, or a string.") name = annotation.func.id kwargs = {node.arg: ast.literal_eval(node.value) for node in annotation.keywords} -- cgit v0.12