Commits

larry  committed 3eee396

Issue #20214: Fixed a number of small issues and documentation errors in
Argument Clinic (see issue for details).

  • Participants
  • Parent commits 250b481

Comments (0)

Files changed (4)

File Doc/howto/clinic.rst

    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]
     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
 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
 -----------------------------------------------------
 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
 ---------------
 
 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
   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
 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
   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
 ``'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')``
 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
 -------------------
 
 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.
     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.
 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
 -------------------------------------
 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.

File Modules/zlibmodule.c

 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;
 
     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;
 
 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;
 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,
 
 exit:
     /* Cleanup for data */
-    if (data.buf)
+    if (data.obj)
        PyBuffer_Release(&data);
 
     return return_value;
 
 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;

File Tools/clinic/clinic.py

 _empty = inspect._empty
 _void = inspect._void
 
+NoneType = type(None)
 
 class Unspecified:
     def __repr__(self):
                 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)
     # 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
         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
 
 class bool_converter(CConverter):
     type = 'int'
+    default_type = bool
     format_unit = 'p'
     c_ignored_default = '0'
 
 
 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'"
 
 
 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"
 
 @add_legacy_c_converter('C', types='str')
 class int_converter(CConverter):
     type = 'int'
+    default_type = int
     format_unit = 'i'
     c_ignored_default = "0"
 
 
 class unsigned_int_converter(CConverter):
     type = 'unsigned int'
+    default_type = int
     format_unit = 'I'
     c_ignored_default = "0"
 
 
 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"
 
 
 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"
 
 
 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}"
 
     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
 
 @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",
 
 class unicode_converter(CConverter):
     type = 'PyObject *'
+    default_type = (str, Null, NoneType)
     format_unit = 'U'
 
 @add_legacy_c_converter('u#', length=True)
 @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):
     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',))
                 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")
 
     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):
 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 = 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((long)_return_value);\n')
-
-
-class long_return_converter(CReturnConverter):
-    type = 'long'
+            ''.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(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_FromLong(_return_value);\n')
-
-
-class Py_ssize_t_return_converter(CReturnConverter):
-    type = 'Py_ssize_t'
-
-    def render(self, function, data):
-        self.declare(data)
-        self.err_occurred_if("_return_value == -1", 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):
             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):
                     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:
         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}