Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 4 additions & 0 deletions Include/internal/pycore_abstract.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ extern int _PyObject_RealIsSubclass(PyObject *derived, PyObject *cls);
// Export for '_bisect' shared extension.
PyAPI_FUNC(int) _Py_convert_optional_to_ssize_t(PyObject *, void *);

// Convert Python int to Py_ssize_t. Do nothing if the argument is None.
// Raises ValueError if argument is negative
PyAPI_FUNC(int) _Py_convert_optional_to_non_negative_ssize_t(PyObject *, void *);

// Same as PyNumber_Index() but can return an instance of a subclass of int.
// Export for 'math' shared extension.
PyAPI_FUNC(PyObject*) _PyNumber_Index(PyObject *o);
Expand Down
42 changes: 36 additions & 6 deletions Lib/test/clinic.test.c
Original file line number Diff line number Diff line change
Expand Up @@ -1612,12 +1612,14 @@ test_Py_ssize_t_converter
a: Py_ssize_t = 12
b: Py_ssize_t(accept={int}) = 34
c: Py_ssize_t(accept={int, NoneType}) = 56
d: Py_ssize_t(accept={int}, allow_negative=False) = 78
e: Py_ssize_t(accept={int, NoneType}, allow_negative=False) = 90
/

[clinic start generated code]*/

PyDoc_STRVAR(test_Py_ssize_t_converter__doc__,
"test_Py_ssize_t_converter($module, a=12, b=34, c=56, /)\n"
"test_Py_ssize_t_converter($module, a=12, b=34, c=56, d=78, e=90, /)\n"
"--\n"
"\n");

Expand All @@ -1626,7 +1628,7 @@ PyDoc_STRVAR(test_Py_ssize_t_converter__doc__,

static PyObject *
test_Py_ssize_t_converter_impl(PyObject *module, Py_ssize_t a, Py_ssize_t b,
Py_ssize_t c);
Py_ssize_t c, Py_ssize_t d, Py_ssize_t e);

static PyObject *
test_Py_ssize_t_converter(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
Expand All @@ -1635,8 +1637,10 @@ test_Py_ssize_t_converter(PyObject *module, PyObject *const *args, Py_ssize_t na
Py_ssize_t a = 12;
Py_ssize_t b = 34;
Py_ssize_t c = 56;
Py_ssize_t d = 78;
Py_ssize_t e = 90;

if (!_PyArg_CheckPositional("test_Py_ssize_t_converter", nargs, 0, 3)) {
if (!_PyArg_CheckPositional("test_Py_ssize_t_converter", nargs, 0, 5)) {
goto exit;
}
if (nargs < 1) {
Expand Down Expand Up @@ -1675,17 +1679,43 @@ test_Py_ssize_t_converter(PyObject *module, PyObject *const *args, Py_ssize_t na
if (!_Py_convert_optional_to_ssize_t(args[2], &c)) {
goto exit;
}
if (nargs < 4) {
goto skip_optional;
}
{
Py_ssize_t ival = -1;
PyObject *iobj = _PyNumber_Index(args[3]);
if (iobj != NULL) {
ival = PyLong_AsSsize_t(iobj);
Py_DECREF(iobj);
}
if (ival == -1 && PyErr_Occurred()) {
goto exit;
}
d = ival;
if (d < 0) {
PyErr_SetString(PyExc_ValueError,
"d must not be negative");
goto exit;
}
}
if (nargs < 5) {
goto skip_optional;
}
if (!_Py_convert_optional_to_non_negative_ssize_t(args[4], &e)) {
goto exit;
}
skip_optional:
return_value = test_Py_ssize_t_converter_impl(module, a, b, c);
return_value = test_Py_ssize_t_converter_impl(module, a, b, c, d, e);

exit:
return return_value;
}

static PyObject *
test_Py_ssize_t_converter_impl(PyObject *module, Py_ssize_t a, Py_ssize_t b,
Py_ssize_t c)
/*[clinic end generated code: output=48214bc3d01f4dd7 input=3855f184bb3f299d]*/
Py_ssize_t c, Py_ssize_t d, Py_ssize_t e)
/*[clinic end generated code: output=df3873c05e53e497 input=5c693ea198fa3dd5]*/


/*[clinic input]
Expand Down
8 changes: 6 additions & 2 deletions Lib/test/test_clinic.py
Original file line number Diff line number Diff line change
Expand Up @@ -3194,8 +3194,12 @@ def test_py_ssize_t_converter(self):
ac_tester.py_ssize_t_converter(PY_SSIZE_T_MAX + 1)
with self.assertRaises(TypeError):
ac_tester.py_ssize_t_converter([])
self.assertEqual(ac_tester.py_ssize_t_converter(), (12, 34, 56))
self.assertEqual(ac_tester.py_ssize_t_converter(1, 2, None), (1, 2, 56))
with self.assertRaises(ValueError):
ac_tester.py_ssize_t_converter(12, 34, 56, -1)
with self.assertRaises(ValueError):
ac_tester.py_ssize_t_converter(12, 34, 56, 78, -1)
self.assertEqual(ac_tester.py_ssize_t_converter(), (12, 34, 56, 78, 90))
self.assertEqual(ac_tester.py_ssize_t_converter(1, 2, None, 3, None), (1, 2, 56, 3, 90))

def test_slice_index_converter(self):
from _testcapi import PY_SSIZE_T_MIN, PY_SSIZE_T_MAX
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add ``allow_negative`` flag to ``Py_ssize_t`` Argument Clinic converter, to
Copy link
Member

Choose a reason for hiding this comment

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

AC is unfortunately an internal tool so I don't think we should document it. However, what could be interesting is whether we want to expose a new parsing character for positive Py_ssize_t values. For now, https://docs.python.org/3/c-api/arg.html#numbers only documents n for Py_ssize_t but maybe having something for Py_ssize_t >= 0 would make sense (I don't know, so for now, don't change anything).

Copy link
Contributor Author

@wiomoc wiomoc Aug 31, 2025

Choose a reason for hiding this comment

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

AC is unfortunately an internal tool so I don't think we should document it.

There are already entries regarding arg clinic in the changelog 1, therefore I think it's plausible to keep the entry. I moved it to tools/demos however - should be a better fit.

Whether we want to expose a new parsing character for positive Py_ssize_t values.

I wonder why not providing a format unit for conversion to size_t directly.
The only advantage is see with Py_ssize_t(allow_negative) in comparison to size_t is the fact that you could provide a negative value e.g. -1 default value as undefined / None sentinel.

Copy link
Member

Choose a reason for hiding this comment

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

I agree no NEWS entry is needed, but @wiomoc could create a PR for the devguide after this is merged, which is where we document AC.

Copy link
Member

@picnixz picnixz Aug 31, 2025

Choose a reason for hiding this comment

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

I wonder why not providing a format unit for conversion to size_t directly.

Because we use ssize_t for most internal sizes in Python as we need to indicate when a conversion failed with value = -1 && PyErr_Occurred() (I think).

The only advantage is see with Py_ssize_t(allow_negative) in comparison to size_t is the fact that you could provide a negative value e.g. -1 default value as undefined / None sentinel.

Unfortunately, internally, most structures use ssize_t for sizes and not size_t, so we could have overflows here (that is, we pass a value that fits on a size_t but not on a ssize_t). See Py_buffer for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. Now I can make sense of it. I'll create an issue for adding N as the format unit for Py_ssize_t >= 0.

enable generation of bound checking
8 changes: 5 additions & 3 deletions Modules/_testclinic.c
Original file line number Diff line number Diff line change
Expand Up @@ -443,16 +443,18 @@ py_ssize_t_converter
a: Py_ssize_t = 12
b: Py_ssize_t(accept={int}) = 34
c: Py_ssize_t(accept={int, NoneType}) = 56
d: Py_ssize_t(accept={int}, allow_negative=False) = 78
e: Py_ssize_t(accept={int, NoneType}, allow_negative=False) = 90
/

[clinic start generated code]*/

static PyObject *
py_ssize_t_converter_impl(PyObject *module, Py_ssize_t a, Py_ssize_t b,
Py_ssize_t c)
/*[clinic end generated code: output=ce252143e0ed0372 input=76d0f342e9317a1f]*/
Py_ssize_t c, Py_ssize_t d, Py_ssize_t e)
/*[clinic end generated code: output=32c03be1ad1b7c97 input=121f2d9ed09ae210]*/
{
RETURN_PACKED_ARGS(3, PyLong_FromSsize_t, Py_ssize_t, a, b, c);
RETURN_PACKED_ARGS(5, PyLong_FromSsize_t, Py_ssize_t, a, b, c, d, e);
}


Expand Down
38 changes: 33 additions & 5 deletions Modules/clinic/_testclinic.c.h

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

17 changes: 17 additions & 0 deletions Python/modsupport.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,23 @@ _Py_convert_optional_to_ssize_t(PyObject *obj, void *result)
return 1;
}

int
_Py_convert_optional_to_non_negative_ssize_t(PyObject *obj, void *result)
{
if (_Py_convert_optional_to_ssize_t(obj, result)) {
if(obj == Py_None || *((Py_ssize_t *)result) >= 0) {
return 1;
}
else {
PyErr_SetString(PyExc_ValueError,
"argument must not be negative");
return 0;
}
}
else {
return 0;
}
}

/* Helper for mkvalue() to scan the length of a format */

Expand Down
29 changes: 24 additions & 5 deletions Tools/clinic/libclinic/converters.py
Original file line number Diff line number Diff line change
Expand Up @@ -420,21 +420,38 @@ class Py_ssize_t_converter(CConverter):
type = 'Py_ssize_t'
c_ignored_default = "0"

def converter_init(self, *, accept: TypeSet = {int}) -> None:
def converter_init(self, *, accept: TypeSet = {int}, allow_negative: bool = True) -> None:
self.allow_negative = allow_negative
if accept == {int}:
self.format_unit = 'n'
self.default_type = int
elif accept == {int, NoneType}:
self.converter = '_Py_convert_optional_to_ssize_t'
if self.allow_negative:
self.converter = '_Py_convert_optional_to_ssize_t'
else:
self.converter = '_Py_convert_optional_to_non_negative_ssize_t'
else:
fail(f"Py_ssize_t_converter: illegal 'accept' argument {accept!r}")

def use_converter(self) -> None:
if self.converter == '_Py_convert_optional_to_ssize_t':
self.add_include('pycore_abstract.h',
'_Py_convert_optional_to_ssize_t()')
elif self.converter == '_Py_convert_optional_to_non_negative_ssize_t':
self.add_include('pycore_abstract.h',
'_Py_convert_optional_to_non_negative_ssize_t()')

def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> str | None:
if self.allow_negative:
non_negative_check = ""
else:
non_negative_check = self.format_code("""
if ({paramname} < 0) {{{{
PyErr_SetString(PyExc_ValueError,
"{paramname} must not be negative");
goto exit;
}}}}""",
argname=argname)
if self.format_unit == 'n':
if limited_capi:
PyNumber_Index = 'PyNumber_Index'
Expand All @@ -452,11 +469,12 @@ def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> st
if (ival == -1 && PyErr_Occurred()) {{{{
goto exit;
}}}}
{paramname} = ival;
{paramname} = ival;{non_negative_check}
}}}}
""",
argname=argname,
PyNumber_Index=PyNumber_Index)
PyNumber_Index=PyNumber_Index,
non_negative_check=non_negative_check)
if not limited_capi:
return super().parse_arg(argname, displayname, limited_capi=limited_capi)
return self.format_code("""
Expand All @@ -465,7 +483,7 @@ def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> st
{paramname} = PyNumber_AsSsize_t({argname}, PyExc_OverflowError);
if ({paramname} == -1 && PyErr_Occurred()) {{{{
goto exit;
}}}}
}}}}{non_negative_check}
}}}}
else {{{{
{bad_argument}
Expand All @@ -475,6 +493,7 @@ def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> st
""",
argname=argname,
bad_argument=self.bad_argument(displayname, 'integer or None', limited_capi=limited_capi),
non_negative_check=non_negative_check
)


Expand Down
Loading