Issue #20214: Fixed a number of small issues and documentation errors in

Argument Clinic (see issue for details).

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
+easier.  And to be clear, their use is acceptable when porting code for
-code for Python 3.4.
+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
+Therefore, if you don't mind a little extra effort, please use the normal
-using normal converters instead of legacy converters.
+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``
+``py_default``
-  If the parameter takes a default value, normally this value is also
+  The default value for this parameter when defined in Python.
-  provided in the ``inspect.Signature`` metadata, and displayed in the
+  Specifically, the value that will be used in the ``inspect.Signature``
-  docstring.  ``doc_default`` lets you override the value used in these
+  string.
-  two places: pass in a string representing the Python value you wish
+  If a default value is specified for the parameter, defaults to
-  to use in these two contexts.
+  ``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``
+``'I'``     ``unsigned_int(bitwise=True)``
-``'K'``     ``unsigned_PY_LONG_LONG``
+``'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
 -------------------------------------

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.

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;

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:
+            if has_option_groups and (not positional):
-                assert 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:
+        else:
-            fail(function.fullname + " argument " + name + " specified a 'doc_default' without having a 'default'")
+            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):
+    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:
+        if self.default not in (unspecified, None):
-            fail("There is no legal default value for Py_buffer ")
+            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(
+        data.return_conversion.append('return_value = PyBool_FromLong((long)_return_value);\n')
-            'return_value = PyLong_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 Py_ssize_t_return_converter(CReturnConverter):
+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(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}