Skip to content

gh-108494: Argument Clinic partial supports of Limited C API #108495

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

Merged
merged 2 commits into from
Aug 25, 2023
Merged

gh-108494: Argument Clinic partial supports of Limited C API #108495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1f9bed
gh-108494: Argument Clinic partial supports of Limited C API
vstinner Aug 25, 2023
c2ebc65
Fix make regen-stdlib-module-names
vstinner Aug 25, 2023
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
40 changes: 38 additions & 2 deletions Lib/test/test_clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import os.path
import re
import sys
import types
import unittest

test_tools.skip_if_missing('clinic')
Expand All @@ -21,6 +22,13 @@
from clinic import DSLParser


def default_namespace():
ns = types.SimpleNamespace()
ns.force = False
ns.limited_capi = clinic.DEFAULT_LIMITED_CAPI
return ns


def _make_clinic(*, filename='clinic_tests'):
clang = clinic.CLanguage(None)
c = clinic.Clinic(clang, filename=filename)
Expand Down Expand Up @@ -52,6 +60,11 @@ def _expect_failure(tc, parser, code, errmsg, *, filename=None, lineno=None,
return cm.exception


class MockClinic:
def __init__(self):
self.limited_capi = clinic.DEFAULT_LIMITED_CAPI


class ClinicWholeFileTest(TestCase):
maxDiff = None

Expand Down Expand Up @@ -691,8 +704,9 @@ def expect_parsing_failure(
self, *, filename, expected_error, verify=True, output=None
):
errmsg = re.escape(dedent(expected_error).strip())
ns = default_namespace()
with self.assertRaisesRegex(clinic.ClinicError, errmsg):
clinic.parse_file(filename)
clinic.parse_file(filename, ns=ns)

def test_parse_file_no_extension(self) -> None:
self.expect_parsing_failure(
Expand Down Expand Up @@ -832,8 +846,9 @@ def _test(self, input, output):

blocks = list(clinic.BlockParser(input, language))
writer = clinic.BlockPrinter(language)
mock_clinic = MockClinic()
for block in blocks:
writer.print_block(block)
writer.print_block(block, clinic=mock_clinic)
output = writer.f.getvalue()
assert output == input, "output != input!\n\noutput " + repr(output) + "\n\n input " + repr(input)

Expand Down Expand Up @@ -3508,6 +3523,27 @@ def test_depr_multi(self):
self.assertRaises(TypeError, fn, a="a", b="b", c="c", d="d", e="e", f="f", g="g")


try:
import _testclinic_limited
except ImportError:
_testclinic_limited = None

@unittest.skipIf(_testclinic_limited is None, "_testclinic_limited is missing")
class LimitedCAPIFunctionalTest(unittest.TestCase):
locals().update((name, getattr(_testclinic_limited, name))
for name in dir(_testclinic_limited) if name.startswith('test_'))

def test_my_int_func(self):
with self.assertRaises(TypeError):
_testclinic_limited.my_int_func()
self.assertEqual(_testclinic_limited.my_int_func(3), 3)
with self.assertRaises(TypeError):
_testclinic_limited.my_int_func(1.0)
with self.assertRaises(TypeError):
_testclinic_limited.my_int_func("xyz")



class PermutationTests(unittest.TestCase):
"""Test permutation support functions."""

Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Tools-Demos/2023-08-25-22-40-12.gh-issue-108494.4RbDdu.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:ref:`Argument Clinic <howto-clinic>` now has a partial support of the
:ref:`Limited API <limited-c-api>`. Patch by Victor Stinner.
1 change: 1 addition & 0 deletions Modules/Setup.stdlib.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@
@MODULE__TESTINTERNALCAPI_TRUE@_testinternalcapi _testinternalcapi.c
@MODULE__TESTCAPI_TRUE@_testcapi _testcapimodule.c _testcapi/vectorcall.c _testcapi/vectorcall_limited.c _testcapi/heaptype.c _testcapi/abstract.c _testcapi/unicode.c _testcapi/dict.c _testcapi/getargs.c _testcapi/datetime.c _testcapi/docstring.c _testcapi/mem.c _testcapi/watchers.c _testcapi/long.c _testcapi/float.c _testcapi/structmember.c _testcapi/exceptions.c _testcapi/code.c _testcapi/buffer.c _testcapi/pyos.c _testcapi/immortal.c _testcapi/heaptype_relative.c _testcapi/gc.c
@MODULE__TESTCLINIC_TRUE@_testclinic _testclinic.c
@MODULE__TESTCLINIC_TRUE@_testclinic_limited _testclinic_limited.c

# Some testing modules MUST be built as shared libraries.
*shared*
Expand Down
69 changes: 69 additions & 0 deletions Modules/_testclinic_limited.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// For now, only limited C API 3.13 is supported
#define Py_LIMITED_API 0x030d0000

/* Always enable assertions */
#undef NDEBUG

#include "Python.h"


#include "clinic/_testclinic_limited.c.h"


/*[clinic input]
module _testclinic_limited
[clinic start generated code]*/
/*[clinic end generated code: output=da39a3ee5e6b4b0d input=dd408149a4fc0dbb]*/


/*[clinic input]
test_empty_function

[clinic start generated code]*/

static PyObject *
test_empty_function_impl(PyObject *module)
/*[clinic end generated code: output=0f8aeb3ddced55cb input=0dd7048651ad4ae4]*/
{
Py_RETURN_NONE;
}


/*[clinic input]
my_int_func -> int

arg: int
/

[clinic start generated code]*/

static int
my_int_func_impl(PyObject *module, int arg)
/*[clinic end generated code: output=761cd54582f10e4f input=16eb8bba71d82740]*/
{
return arg;
}


static PyMethodDef tester_methods[] = {
TEST_EMPTY_FUNCTION_METHODDEF
MY_INT_FUNC_METHODDEF
{NULL, NULL}
};

static struct PyModuleDef _testclinic_module = {
PyModuleDef_HEAD_INIT,
.m_name = "_testclinic_limited",
.m_size = 0,
.m_methods = tester_methods,
};

PyMODINIT_FUNC
PyInit__testclinic_limited(void)
{
PyObject *m = PyModule_Create(&_testclinic_module);
if (m == NULL) {
return NULL;
}
return m;
}
53 changes: 53 additions & 0 deletions 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.

1 change: 1 addition & 0 deletions Tools/build/generate_stdlib_module_names.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
'_testbuffer',
'_testcapi',
'_testclinic',
'_testclinic_limited',
'_testconsole',
'_testimportmultiple',
'_testinternalcapi',
Expand Down
55 changes: 44 additions & 11 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@

version = '1'

DEFAULT_LIMITED_CAPI = False
NO_VARARG = "PY_SSIZE_T_MAX"
CLINIC_PREFIX = "__clinic_"
CLINIC_PREFIXED_ARGS = {
Expand Down Expand Up @@ -1360,7 +1361,21 @@ def parser_body(
vararg
)
nargs = f"Py_MIN(nargs, {max_pos})" if max_pos else "0"
if not new_or_init:

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]'
Copy link
Member

Choose a reason for hiding this comment

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

I do not think that it is correct. args is a Python tuple object here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ok, so I suppose that it can be removed.

declarations = ""

elif not new_or_init:
flags = "METH_FASTCALL|METH_KEYWORDS"
parser_prototype = self.PARSER_PROTOTYPE_FASTCALL_KEYWORDS
argname_fmt = 'args[%d]'
Expand Down Expand Up @@ -2111,7 +2126,8 @@ def print_block(
self,
block: Block,
*,
core_includes: bool = False
core_includes: bool = False,
clinic: Clinic | None = None,
) -> None:
input = block.input
output = block.output
Expand Down Expand Up @@ -2140,7 +2156,11 @@ def print_block(
write("\n")

output = ''
if core_includes:
if clinic:
limited_capi = clinic.limited_capi
else:
limited_capi = DEFAULT_LIMITED_CAPI
if core_includes and not limited_capi:
output += textwrap.dedent("""
#if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)
# include "pycore_gc.h" // PyGC_Head
Expand Down Expand Up @@ -2344,6 +2364,7 @@ def __init__(
*,
filename: str,
verify: bool = True,
limited_capi: bool = False,
) -> None:
# maps strings to Parser objects.
# (instantiated from the "parsers" global.)
Expand All @@ -2353,6 +2374,7 @@ def __init__(
fail("Custom printers are broken right now")
self.printer = printer or BlockPrinter(language)
self.verify = verify
self.limited_capi = limited_capi
self.filename = filename
self.modules: ModuleDict = {}
self.classes: ClassDict = {}
Expand Down Expand Up @@ -2450,7 +2472,7 @@ def parse(self, input: str) -> str:
self.parsers[dsl_name] = parsers[dsl_name](self)
parser = self.parsers[dsl_name]
parser.parse(block)
printer.print_block(block)
printer.print_block(block, clinic=self)

# these are destinations not buffers
for name, destination in self.destinations.items():
Expand All @@ -2465,7 +2487,7 @@ def parse(self, input: str) -> str:
block.input = "dump " + name + "\n"
warn("Destination buffer " + repr(name) + " not empty at end of file, emptying.")
printer.write("\n")
printer.print_block(block)
printer.print_block(block, clinic=self)
continue

if destination.type == 'file':
Expand All @@ -2490,7 +2512,7 @@ def parse(self, input: str) -> str:

block.input = 'preserve\n'
printer_2 = BlockPrinter(self.language)
printer_2.print_block(block, core_includes=True)
printer_2.print_block(block, core_includes=True, clinic=self)
write_file(destination.filename, printer_2.f.getvalue())
continue

Expand Down Expand Up @@ -2536,9 +2558,15 @@ def __repr__(self) -> str:
def parse_file(
filename: str,
*,
verify: bool = True,
output: str | None = None
ns: argparse.Namespace,
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'm replying here since I dislike comments lost on a commit, I don't think that many people will notice it.

I'm fine with people doing reviews after I merged changes (I like them!).

@AlexWaygood wrote:

I don't like this at all :(

Requiring that an argparse.Namespace object be passed to parse_file() feels like we're coupling the parse_file() function to the specific context in which it's currently called. A Namespace object is essentially unstructured data; we should be parsing the Namespace object in run_clinic(), and passing only the specific variables we need to parse_file(). Apart from anything else, this makes the code much less type-safe.

Free feel to change it in a follow-up PR. Your rationale makes sense and I don't have a strong preference for passing arguments here. Or I can do it if you prefer.

I'm still struggling a lot with this code base and I tried to write the minimum working change without touching too many parts of the code. The problem is that I'm not used with the AC design, so I'm not sure if it's the right place to add code...

I'd appreciate if you'd give me a day or so to review Argument Clinic PRs @vstinner — I didn't see the review request for this one until it was already merged :)

I understand your frustration. I didn't expect any review at all in fact. I'm always happy to get reviews, even if it's after I merge my change.

Today I wanted to push multiple changes to unblock my work on removing private functions: see PR #108499 which kind-of the end of my "PR serie".

I'm frequently frustrated by the fact that GitHub doesn't let me create a "patch serie" (multiple PRs depending on each others). So I have to merge my changes one by one, but I'm always bitten by a CI which dislike my change, then I have to wait 30 min for the next issue... On the AC changes, I didn't expect that make check-c-globals would fail for example.

I wanted to push the basis on AC work to add #include and to support the limited C API. For following changes, it should easier to work on multiple PRs at the same time.

I'm glad that other people are now working on AC. Adding #include and supporting the limited C API was a long awaited feature for me, I started working on that in 2020 :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up PR: #108504

Copy link
Member

Choose a reason for hiding this comment

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

I'm replying here since I dislike comments lost on a commit, I don't think that many people will notice it.

Oops, I didn't mean to add a comment on the commit — I meant to comment on the PR. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your frustration. I didn't expect any review at all in fact.

Come on, you know that both Alex and I are active on a daily basis on CPython ;)

output: str | None = None,
) -> None:
verify = not ns.force
limited_capi = ns.limited_capi
# XXX Temporary solution
if os.path.basename(filename) == '_testclinic_limited.c':
print(f"{filename} uses limited C API")
limited_capi = True
if not output:
output = filename

Expand All @@ -2560,7 +2588,10 @@ def parse_file(
return

assert isinstance(language, CLanguage)
clinic = Clinic(language, verify=verify, filename=filename)
clinic = Clinic(language,
verify=verify,
filename=filename,
limited_capi=limited_capi)
cooked = clinic.parse(raw)

write_file(output, cooked)
Expand Down Expand Up @@ -5987,6 +6018,8 @@ def create_cli() -> argparse.ArgumentParser:
cmdline.add_argument("--exclude", type=str, action="append",
help=("a file to exclude in --make mode; "
"can be given multiple times"))
cmdline.add_argument("--limited", dest="limited_capi", action='store_true',
help="use the Limited C API")
cmdline.add_argument("filename", metavar="FILE", type=str, nargs="*",
help="the list of files to process")
return cmdline
Expand Down Expand Up @@ -6077,7 +6110,7 @@ def run_clinic(parser: argparse.ArgumentParser, ns: argparse.Namespace) -> None:
continue
if ns.verbose:
print(path)
parse_file(path, verify=not ns.force)
parse_file(path, ns=ns)
return

if not ns.filename:
Expand All @@ -6089,7 +6122,7 @@ def run_clinic(parser: argparse.ArgumentParser, ns: argparse.Namespace) -> None:
for filename in ns.filename:
if ns.verbose:
print(filename)
parse_file(filename, output=ns.output, verify=not ns.force)
parse_file(filename, output=ns.output, ns=ns)


def main(argv: list[str] | None = None) -> NoReturn:
Expand Down