bpo-37191: Move PEP 590 vectorcall tests

[vstinner]

• Move TestPEP590 from test_capi to test_call
• Move PEP 590 vectorcall code for tests from _testcapi to
  _testinternalcapi.

https://bugs.python.org/issue37191

[vstinner]

cc @jdemeyer

Again, this change prepares the CPython code base to move the new vectorcall declaration to the internal C API.

I also took the opportunity to move TestPEP590 from test_capi to test_call. I really hate test_capi, it's a pile of unrelated stuff. IMHO test_call is a way better place for these vector call tests.

[jdemeyer]

Again, this change prepares the CPython code base to move the new vectorcall declaration to the internal C API.

There is no plan to do that: the whole point of vectorcall is to define a public, non-internal protocol.

[vstinner]

There is no plan to do that: the whole point of vectorcall is to define a public, non-internal protocol.

Sorry, I mean private declarations. See https://bugs.python.org/issue37194 for the details and the rationale.

Public declarations are fine and stay.

[jdemeyer]

I also took the opportunity to move TestPEP590 from test_capi to test_call.

Can we please not do that right now? I have several open PRs regarding PEP 590 and this is going to cause merge conflicts.

Sorry, I mean private declarations. See https://bugs.python.org/issue37194 for the details and the rationale.

Could you say more precisely which functions you have in mind?

[vstinner]

Can we please not do that right now?

You have to fix Python 3.8: it introduced many regressions.

Could you say more precisely which functions you have in mind?

Content of pycore_abstract.h in my local branch:

#ifndef Py_INTERNAL_ABSTRACT_H
#define Py_INTERNAL_ABSTRACT_H
#ifdef __cplusplus
extern "C" {
#endif

#ifndef Py_BUILD_CORE
#  error "this header requires Py_BUILD_CORE define"
#endif

/* === Vectorcall protocol (PEP 590) ============================= */

/* Call callable using tp_call. Arguments are like _PyObject_Vectorcall()
   or _PyObject_FastCallDict() (both forms are supported),
   except that nargs is plainly the number of arguments without flags. */
PyAPI_FUNC(PyObject *) _PyObject_MakeTpCall(
    PyObject *callable,
    PyObject *const *args, Py_ssize_t nargs,
    PyObject *keywords);

static inline vectorcallfunc
_PyVectorcall_Function(PyObject *callable)
{
    PyTypeObject *tp = Py_TYPE(callable);
    if (!PyType_HasFeature(tp, _Py_TPFLAGS_HAVE_VECTORCALL)) {
        return NULL;
    }
    assert(PyCallable_Check(callable));
    Py_ssize_t offset = tp->tp_vectorcall_offset;
    assert(offset > 0);
    vectorcallfunc *ptr = (vectorcallfunc *)(((char *)callable) + offset);
    return *ptr;
}

/* Call the callable object 'callable' with the "vectorcall" calling
   convention.

   args is a C array for positional arguments.

   nargsf is the number of positional arguments plus optionally the flag
   PY_VECTORCALL_ARGUMENTS_OFFSET which means that the caller is allowed to
   modify args[-1].

   kwnames is a tuple of keyword names. The values of the keyword arguments
   are stored in "args" after the positional arguments (note that the number
   of keyword arguments does not change nargsf). kwnames can also be NULL if
   there are no keyword arguments.

   keywords must only contains str strings (no subclass), and all keys must
   be unique.

   Return the result on success. Raise an exception and return NULL on
   error. */
static inline PyObject *
_PyObject_Vectorcall(PyObject *callable, PyObject *const *args,
                     size_t nargsf, PyObject *kwnames)
{
    assert(kwnames == NULL || PyTuple_Check(kwnames));
    assert(args != NULL || PyVectorcall_NARGS(nargsf) == 0);
    vectorcallfunc func = _PyVectorcall_Function(callable);
    if (func == NULL) {
        Py_ssize_t nargs = PyVectorcall_NARGS(nargsf);
        return _PyObject_MakeTpCall(callable, args, nargs, kwnames);
    }
    PyObject *res = func(callable, args, nargsf, kwnames);
    return _Py_CheckFunctionResult(callable, res, NULL);
}

/* Same as _PyObject_Vectorcall except that keyword arguments are passed as
   dict, which may be NULL if there are no keyword arguments. */
PyAPI_FUNC(PyObject *) _PyObject_FastCallDict(
    PyObject *callable,
    PyObject *const *args,
    size_t nargsf,
    PyObject *kwargs);

/* Call "callable" (which must support vectorcall) with positional arguments
   "tuple" and keyword arguments "dict". "dict" may also be NULL */
PyAPI_FUNC(PyObject *) PyVectorcall_Call(PyObject *callable, PyObject *tuple, PyObject *dict);

/* Same as _PyObject_Vectorcall except without keyword arguments */
static inline PyObject *
_PyObject_FastCall(PyObject *func, PyObject *const *args, Py_ssize_t nargs)
{
    return _PyObject_Vectorcall(func, args, (size_t)nargs, NULL);
}

/* Call a callable without any arguments */
static inline PyObject *
_PyObject_CallNoArg(PyObject *func) {
    return _PyObject_Vectorcall(func, NULL, 0, NULL);
}

PyAPI_FUNC(PyObject *) _PyObject_Call_Prepend(
    PyObject *callable,
    PyObject *obj,
    PyObject *args,
    PyObject *kwargs);

PyAPI_FUNC(PyObject *) _PyObject_FastCall_Prepend(
    PyObject *callable,
    PyObject *obj,
    PyObject *const *args,
    Py_ssize_t nargs);

#ifdef __cplusplus
}
#endif
#endif /* !Py_INTERNAL_ABSTRACT_H */

I'm trying to prevent to add new things to the public C API. Currently, the "private" functions added in 3.8 are technically part of the public C API and they make compilation of some C extensions fail. There is no reason for these C extensions to use these private functions.

More context: http://pythoncapi.readthedocs.io/

[vstinner]

Oh my local branch is outdated: _PyObject_CallNoArg() is removed by PR #13890 :-)

[jdemeyer]

At least _PyObject_Vectorcall and _PyObject_FastCallDict are meant to become public. See also https://docs.python.org/3.8/c-api/object.html#c._PyObject_Vectorcall

[jdemeyer]

And PyVectorcall_Call is also meant to be public (it's currently undocumented but that's fixed by #13844)

[jdemeyer]

I'm trying to prevent to add new things to the public C API.

Can you explain the rationale for that? I do understand your explanations about bad C API that should be removed, but what's wrong with adding "good C API"?

My impression is that https://pythoncapi.readthedocs.io is mostly about the stable ABI (PEP 384) but we're not aiming for that with PEP 590: the plan is keep vectorcall in the "CPython C API", that is the intermediate level between Py_LIMITED_API and Py_BUILD_CORE.

[vstinner]

Can you explain the rationale for that? I do understand your explanations about bad C API that should be removed, but what's wrong with adding "good C API"?

Private functions should not land into the public C API.

At least _PyObject_Vectorcall and _PyObject_FastCallDict are meant to become public. (...) And PyVectorcall_Call is also meant to be public

According to https://bugs.python.org/issue37191 I would prefer to not add public functions declared as static inline functions, but "regular functions" instead. I'm fine with keeping static inline function variants, but in the internal C API.

[jdemeyer]

I would prefer to not add public functions declared as static inline functions

Is that only for C89 compatibility or for other reasons? If the problem is C89, then a macro would also work.

[vstinner]

me:

I would prefer to not add public functions declared as static inline functions

Jeroen:

Is that only for C89 compatibility or for other reasons? If the problem is C89, then a macro would also work.

Oh. Ignore my previous. I looked again and I already converted many macros to static inline functions in the public C API and I made this on purpose :-D I have a bad memory. Now I'm confused by https://bugs.python.org/issue37191 But as Petr wrote, we can ignore this issue, since Python 3.6 requires C99: https://www.python.org/dev/peps/pep-0007/#c-dialect

[vstinner]

I reverted the controversial part of this PR to only move the test case from test_capi to test_call.

@jdemeyer: Would you mind to review the updated PR?

[jdemeyer]

I agree in principle but I don't like that it conflicts with #13858. So I prefer to wait until that is merged.

[vstinner]

macOS job of the Azure Pipelined failed:

test omitted:
    test_subprocess

tests failed:
    test_threaded_import test_threading

These failures look to be unrelated.

[vstinner]

I agree in principle but I don't like that it conflicts with #13858. So I prefer to wait until that is merged.

I'm not sure that PR #13858 is correct: see my comments there.

So you gave me your agreement in principle, I merge this PR ;-) It should be trivial for you to rebase your PR ;-)

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[miss-islington]

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-13897 is a backport of this pull request to the 3.8 branch.