Skip to content

gh-108494: Fix AC limited C API for kwargs #108516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

gh-108494: Fix AC limited C API for kwargs #108516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Lib/test/test_clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3534,6 +3534,7 @@ class LimitedCAPIFunctionalTest(unittest.TestCase):
for name in dir(_testclinic_limited) if name.startswith('test_'))

def test_my_int_func(self):
# METH_O
with self.assertRaises(TypeError):
_testclinic_limited.my_int_func()
self.assertEqual(_testclinic_limited.my_int_func(3), 3)
Expand All @@ -3543,6 +3544,7 @@ def test_my_int_func(self):
_testclinic_limited.my_int_func("xyz")

def test_my_int_sum(self):
# PyArg_ParseTuple() with "ii:my_int_sum" format
with self.assertRaises(TypeError):
_testclinic_limited.my_int_sum()
with self.assertRaises(TypeError):
Expand All @@ -3553,6 +3555,18 @@ def test_my_int_sum(self):
with self.assertRaises(TypeError):
_testclinic_limited.my_int_sum(1, "str")

def test_my_obj_func(self):
# PyArg_ParseTupleAndKeywords() with "OO:my_obj_func" format
arg1 = object()
arg2 = object()
with self.assertRaises(TypeError):
_testclinic_limited.my_obj_func()
with self.assertRaises(TypeError):
_testclinic_limited.my_obj_func(arg1)
self.assertIs(_testclinic_limited.my_obj_func(arg1, arg2), arg1)
with self.assertRaises(TypeError):
_testclinic_limited.my_obj_func(arg1, arg2, "arg3")



class PermutationTests(unittest.TestCase):
Expand Down
17 changes: 17 additions & 0 deletions Modules/_testclinic_limited.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,22 @@ my_int_func_impl(PyObject *module, int arg)
}


/*[clinic input]
my_obj_func

arg1: object
arg2: object

[clinic start generated code]*/

static PyObject *
my_obj_func_impl(PyObject *module, PyObject *arg1, PyObject *arg2)
/*[clinic end generated code: output=7f07d359d2e50436 input=c6f78c836ecab1d6]*/
{
return Py_NewRef(arg1);
}


/*[clinic input]
my_int_sum -> int

Expand All @@ -66,6 +82,7 @@ static PyMethodDef tester_methods[] = {
TEST_EMPTY_FUNCTION_METHODDEF
MY_INT_FUNC_METHODDEF
MY_INT_SUM_METHODDEF
MY_OBJ_FUNC_METHODDEF
{NULL, NULL}
};

Expand Down
30 changes: 29 additions & 1 deletion Modules/clinic/_testclinic_limited.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 26 additions & 25 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,18 +1250,32 @@ def parser_body(
parser_prototype = self.PARSER_PROTOTYPE_VARARGS
parser_definition = parser_body(parser_prototype, ' {option_group_parsing}')

elif not requires_defining_class and pos_only == len(parameters) - pseudo_args and clinic.limited_capi:
# positional-only for the limited C API
flags = "METH_VARARGS"
elif clinic.limited_capi:
if not requires_defining_class and pos_only == len(parameters) - pseudo_args:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if pseudo_args is not zero?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea. I don't know what are pseudo args.

# positional-only for the limited C API:
# PyArg_ParseTuple()
flags = "METH_VARARGS"

parser_prototype = self.PARSER_PROTOTYPE_VARARGS
parser_code = [normalize_snippet("""
if (!PyArg_ParseTuple(args, "{format_units}:{name}",
{parse_arguments}))
goto exit;
""", indent=4)]
argname_fmt = 'args[%d]'
declarations = ""
parser_prototype = self.PARSER_PROTOTYPE_VARARGS
parser_code = [normalize_snippet("""
if (!PyArg_ParseTuple(args, "{format_units}:{name}",
{parse_arguments}))
goto exit;
""", indent=4)]
declarations = ""

else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if requires_defining_class is true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't. This change is incomplete, it can be completed later. Or do you mean that an exception should be raised instead of an compilation error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it can produce invalid code which is compiled. There are two opposite approaches:

  1. Raise an exception or ensure that the compiling the generated code will get a compilation error.
  2. Try to use the limited API if possible, but fallback to private API otherwise.

I am currently trying the second approach. It allows to re-generate all clinic code using the limited API and test that at least it works (I found many cases in which the current code does not generate working code). Please hold this PR, the current code is more suitable for this experiment. After testing it will be easy to switch to the first approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A module targeting the limited C API cannot use the private API. It fails at build.

# positional-or-keyword arguments for the limited C API:
# PyArg_ParseTupleAndKeywords()
flags = "METH_VARARGS|METH_KEYWORDS"

parser_prototype = self.PARSER_PROTOTYPE_KEYWORD
parser_code = [normalize_snippet("""
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "{format_units}:{name}", _keywords,
{parse_arguments}))
goto exit;
""", indent=4)]
declarations = "char* _keywords[] = {{{keywords_c} NULL}};"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
declarations = "char* _keywords[] = {{{keywords_c} NULL}};"
declarations = "static char *_keywords[] = {{{keywords_c} NULL}};"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't reallly understand the difference between the two. The value is constant so the variable should not really exist on the stack, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference. It is an array of pointers. If it is a local variable, it should fill with initial values every time the function is called. If it is a static variable, it is only filled with initial values at the program's start (or when the function is called first time).


parser_definition = parser_body(parser_prototype, *parser_code,
declarations=declarations)
Expand Down Expand Up @@ -1384,20 +1398,7 @@ def parser_body(
)
nargs = f"Py_MIN(nargs, {max_pos})" if max_pos else "0"

if clinic.limited_capi:
# positional-or-keyword arguments
flags = "METH_VARARGS|METH_KEYWORDS"

parser_prototype = self.PARSER_PROTOTYPE_KEYWORD
parser_code = [normalize_snippet("""
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "{format_units}:{name}", _keywords,
{parse_arguments}))
goto exit;
""", indent=4)]
argname_fmt = 'args[%d]'
declarations = ""

elif not new_or_init:
if not new_or_init:
flags = "METH_FASTCALL|METH_KEYWORDS"
parser_prototype = self.PARSER_PROTOTYPE_FASTCALL_KEYWORDS
argname_fmt = 'args[%d]'
Expand Down