gh-96002: Add functional test for Argument Clinic

[colorfulappl]

A draft implementation of Argument Clinic functional test.

The test do:

• Add a module with AC-declared functions, each function corresponds to one type of function signatures
• Setup a venv, build the module and install it
• Call the functions in the module to check if all the arguments are processed correctly (gathered to a tuple and returned)

Currently it's a draft and only two PoC of https://github.com/python/cpython/pull/32092 is added, so the test crashes.

cc. @erlend-aasland @kumaraditya303

• Issue: gh-96002

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[erlend-aasland]

Thanks for taking this on!

I suggest you take a look at how the test_capi module is laid out; the C code lives in the Modules/ dir, and the test code lives in Lib/test/test_capi.py. The tests there can be split in two patterns:

1. Tests that are fully implemented in C, for example test_from_spec_metatype_inheritance:

https://github.com/python/cpython/blob/129998bd7b133defa37c7529bfad9052c0022b5c/Modules/_testcapi/heaptype.c#L54-L106

2. Tests that are set up in C and tested in Python, for example test_instancemethod:

https://github.com/python/cpython/blob/129998bd7b133defa37c7529bfad9052c0022b5c/Lib/test/test_capi.py#L50-L65 https://github.com/python/cpython/blob/129998bd7b133defa37c7529bfad9052c0022b5c/Modules/_testcapimodule.c#L6518-L6519

You'll also need to modify configure.ac, Makefile.pre.in, and Modules/Setup.stdlib for the new module; you can grep -i for _testcapi in those files to see what needs to be done.

[colorfulappl]

You'll also need to modify configure.ac, Makefile.pre.in, and Modules/Setup.stdlib for the new module; you can grep -i for _testcapi in those files to see what needs to be done.

Thanks for your advise. I have rewritten this commit and now it is able to run testcases written both in C or Python. Could you take a look again?

Once the framework is ready, I will add more case into it.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[erlend-aasland]

@colorfulappl, are you planning to pursue this PR? If not, would you mind me taking over and driving it forward. I'd really like to have this merged; it is good work.

[colorfulappl]

Sorry, I have been busy with other stuff these days so this PR is delayed. :(

I am planning to finish this in this week. Here I made some changes at your suggestions and am going to add more test cases.

[colorfulappl]

I have added some test cases. Their "clinic input" are mostly copied from Lib/test/clinic.test and modified a bit.

A few cases crash the interpreter unexpectedly. One is newly found, when _PyArg_ParseStack fails, the parsed arguments are double-freed. The others are the same as https://github.com/python/cpython/pull/32092.

The crashes happen during autogenerated "argument parse" procedure so I think it is not proper to add assertions before the crash to raise AssertionErrors to the interpreter.

[erlend-aasland]

@colorfulappl are you ready for another round of review?

[colorfulappl]

@erlend-aasland Yes, I think it's ready for review though the test still crashes due to problems I mentioned above.

[erlend-aasland]

@erlend-aasland Yes, I think it's ready for review though the test still crashes due to problems I mentioned above.

We can skip the tests for gh-32092, or remove those specific tests from this PR and add them to gh-32092 after this has been merged. I'm fine with either.

@kumaraditya303?

[kumaraditya303]

We can skip the tests for https://github.com/python/cpython/pull/32092, or remove those specific tests from this PR and add them to https://github.com/python/cpython/pull/32092 after this has been merged. I'm fine with either.

I am also fine either way, I'll review this soon.

[colorfulappl]

First pass, you can check for refleaks with ./python -m test -R 3:3 test_foo

Thanks, I don't know pytest has such a feature before.

Now the refleaks are fixed, and there is a new bug of Argument Clinic found. Should I fix the two newly found bugs in this patch?

[erlend-aasland]

Should I fix the two newly found bugs in this patch?

I prefer to add just the test framework and one or two "proof-of-concept" tests in this PR.

Then for each bug, file an issue (unless there is one already) and open a PR that fixes the bug and adds the functional test for that specific bug.

[colorfulappl]

for each bug, file an issue (unless there is one already) and open a PR that fixes the bug and adds the functional test for that specific bug.

I deleted all the cases which can fail this test. After this PR is merged, I will add them to the corresponding bug-fix PRs.

[erlend-aasland]

I deleted all the cases which can fail this test. After this PR is merged, I will add them to the corresponding bug-fix PRs.

Thank you so much! I've started a final review. At first sight, there are some minor PEP 7 and PEP 8 style nits (I can take care of those before merging; let me know what you prefer), and more importantly, there are some error handling that must be more explicit.

Regarding the latter:

• A function that sets an exception must return NULL; this means if you set an exception using the PyErr_* APIs, you must return NULL immediately.
• For Python C APIs that return PyObject *, we must check their return value immediately and bail if needed. For example, as in char_converter_impl, calling multiple PyLong_FromUnsignedLong without checking their return value may result in exception objects leaking. See Mark's comment on a recent PR: https://github.com/python/cpython/pull/98587#discussion_r1005557715

[colorfulappl]

Thank you so much! I've started a final review. At first sight, there are some minor PEP 7 and PEP 8 style nits (I can take care of those before merging; let me know what you prefer), and more importantly, there are some error handling that must be more explicit.

Thanks for the advice. I will recheck these problems tomorrow and endeavor to fix them. :)

[colorfulappl]

• For Python C APIs that return PyObject *, we must check their return value immediately and bail if needed. For example, as in char_converter_impl, calling multiple PyLong_FromUnsignedLong without checking their return value may result in exception objects leaking.

Now function pack_arguments checks each arguments and return NULL if any error occurs.

And I also made some subtle style changes.

[erlend-aasland]

I'm now getting reference leaks with this PR:

$ ./python.exe -m test -R : test_clinic 
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 1.21 Run tests sequentially
0:00:00 load avg: 1.21 [1/1] test_clinic
beginning 9 repetitions
123456789
.........
test_clinic leaked [520, 520, 520, 520] references, sum=2080
test_clinic leaked [14, 14, 14, 14] memory blocks, sum=56
test_clinic failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_clinic

Total duration: 2.4 sec
Tests result: FAILURE

[colorfulappl]

Sorry, seems that the RETURN_PACKED_ARGS macro does not function as I expected.

For example, in int_converter_impl, the macro

RETURN_PACKED_ARGS(3,
                   PyLong_FromLong(a),
                   PyLong_FromLong(b),
                   PyLong_FromLong(c));

is expanded to

do {
    assert(!PyErr_Occurred());
    PyObject *out[3] = {NULL,};
    for (int _i = 0; _i < 3; _i++) {
        out[_i] = ((PyObject *[]) {PyLong_FromLong(a),
                                   PyLong_FromLong(b),
                                   PyLong_FromLong(c)})[_i];
        assert(out[_i] || PyErr_Occurred());
        if (!out[_i]) {
            for (int _j = 0; _j < _i; _j++) {
                Py_DECREF(out[_j]);
            }
            return NULL;
        }
    }
    PyObject *tuple = PyTuple_New(3);
    if (!tuple) {
        for (int _i = 0; _i < 3; _i++) {
            Py_DECREF(out[_i]);
        }
        return NULL;
    }
    for (int _i = 0; _i < 3; _i++) {
        PyTuple_SET_ITEM(tuple, _i, out[_i]);
    }
    return tuple;
} while (0);

. So the PyLong_FromLong function is invoked multiple times to cause the reference leak.

Maybe I have to write all the test cases manually rather than use a function or macro. Anyhow, I will give it a try.

[kumaraditya303]

The following patch fixes the refleaks:

diff --git a/Modules/_testclinic.c b/Modules/_testclinic.c
index 0d58bee7bb..77b9d2671d 100644
--- a/Modules/_testclinic.c
+++ b/Modules/_testclinic.c
@@ -50,9 +50,8 @@ pack_arguments_newref(int argc, ...)
  * Do not accept NULL arguments unless error occurs. */
 #define RETURN_PACKED_ARGS(argc, ...) do { \
         assert(!PyErr_Occurred()); \
-        PyObject *out[argc] = {NULL,}; \
+        PyObject *out[argc] = {__VA_ARGS__}; \
         for (int _i = 0; _i < argc; _i++) { \
-            out[_i] = ((PyObject *[]){__VA_ARGS__})[_i]; \
             assert(out[_i] || PyErr_Occurred()); \
             if (!out[_i]) { \
                 for (int _j = 0; _j < _i; _j++) { \

@kumaraditya303 ➜ /workspaces/cpython (test_clinic_functionality ✗) $ ./python -m test -R 3:3 test_clinic
0:00:00 load avg: 2.72 Run tests sequentially
0:00:00 load avg: 2.72 [1/1] test_clinic
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 986 ms
Tests result: SUCCESS

[colorfulappl]

@kumaraditya303 Thank you very much for this revision. This patch does resolve the reference leak problem.

After this patch,

RETURN_PACKED_ARGS(3,
                   PyLong_FromLong(a),
                   PyLong_FromLong(b),
                   PyLong_FromLong(c));

is expanded to

do {
    assert(!PyErr_Occurred());
    PyObject *out[3] = {PyLong_FromUnsignedLong(a),
                        PyLong_FromUnsignedLong(b),
                        PyLong_FromUnsignedLong(c)};
    for (int _i = 0; _i < 3; _i++) {
        assert(out[_i] || PyErr_Occurred());
        if (!out[_i]) {
            for (int _j = 0; _j < _i; _j++) {
                Py_DECREF(out[_j]);
            }
            return NULL;
        }
    }
    PyObject *tuple = PyTuple_New(3);
    if (!tuple) {
        for (int _i = 0; _i < 3; _i++) {
            Py_DECREF(out[_i]);
        }
        return NULL;
    }
    for (int _i = 0; _i < 3; _i++) {
        PyTuple_SET_ITEM(tuple, _i, out[_i]);
    }
    return tuple;
} while (0);

.

But since all the PyLong_FromUnsignedLong functions are invoked at the initialization of out, there is still the object leak problem which erlend-aasland mentioned:

• For Python C APIs that return PyObject *, we must check their return value immediately and bail if needed. For example, as in char_converter_impl, calling multiple PyLong_FromUnsignedLong without checking their return value may result in exception objects leaking.

There should be some way can we delay the evaluation of the arguments to where we actually use them. Any good ideas?

[colorfulappl]

Now RETURN_PACKED_ARGS macro accepts a wrapper function and invoke it inside a for loop. This indeed handles the arguments one-by-one, though not in an elegant manner.

[erlend-aasland]

Also, I'll wait for @kumaraditya303's thumbs up before landing this.

@isidentical, do you want to have a look?

[erlend-aasland]

BTW, @colorfulappl, please resolve the conflicts in Modules/Setup.stdlib.in (it's because of my recent PRs for the _testcapi module).

[colorfulappl]

Sure. It's done. :)

[erlend-aasland]

Note to self: Like with _testcapi, we should consider adding a short README (or C comment) regarding how to add new tests. We can do this in a follow-up PR.

[erlend-aasland]

I'm going to give Batuhan a chance to chime in before merging. If I haven't heard anything until Friday, I'll land this. Thanks for doing this, and thanks for your patience with our nitpicking, @colorfulappl :)

[colorfulappl]

@erlend-aasland @kumaraditya303 Thank you very much for your patient guidance! 🎉

[erlend-aasland]

@colorfulappl: please go ahead with the various Argument Clinic bugfixes and their accompanying tests :)