FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-108494: Fix AC limited C API for kwargs by vstinner · Pull Request #108516 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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
vstinner wants to merge 1 commit into from
+86 −26
Conversation 14 Commits 1 Checks 0 Files changed 4
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 .c  (1) .h  (1) .py  (2) All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
14 changes: 14 additions & 0 deletions Lib/test/test_clinic.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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:

serhiy-storchaka Aug 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does it work if pseudo_args is not zero?

vstinner Aug 26, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

serhiy-storchaka Aug 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does it work if requires_defining_class is true?

vstinner Aug 26, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

serhiy-storchaka Aug 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

vstinner Aug 27, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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}};"

serhiy-storchaka Aug 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
declarations = "char* _keywords[] = {{{keywords_c} NULL}};"
declarations = "static char *_keywords[] = {{{keywords_c} NULL}};"

vstinner Aug 26, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

serhiy-storchaka Aug 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL