Skip to content
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.

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

gh-114329: Add PyList_GetItemRef function #114504

Merged
merged 6 commits into from
Feb 2, 2024
Merged

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Jan 23, 2024

The new PyList_GetItemRef is similar to PyList_GetItem, but returns a strong reference instead of a borrowed reference. Additionally, if the passed "list" object is not a list, the function sets a TypeError instead of calling PyErr_BadInternalCall().


📚 Documentation preview 📚: https://cpython-previews--114504.org.readthedocs.build/

The new `PyList_GetItemRef` is similar to `PyList_GetItem`, but returns
a strong reference instead of a borrowed reference. Additionally, if the
passed "list" object is not a list, the function sets a `TypeError`
instead of calling `PyErr_BadInternalCall()`.
@vstinner
Copy link
Member

Additionally, if the passed "list" object is not a list, the function sets a TypeError instead of calling PyErr_BadInternalCall().

When I proposed such change, @serhiy-storchaka asked me to stick to RuntimeError, but I don't recall why :-D @serhiy-storchaka, do you prefer RuntimeError or TypeError?

:exc:`TypeError` exception.

This behaves like :c:func:`PyList_GetItem`, but returns a
:term:`strong reference` instead of a :term:`borrowed reference`.
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to make this function the new reference, and document PyList_GetItem() as: "Similar to PyList_GetItemRef(), but return a borrowed reference".

@@ -112,6 +112,15 @@ def test_list_get_item(self):
# CRASHES get_item(21, 2)
# CRASHES get_item(NULL, 1)

def test_list_get_item_ref(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you share most code between test_list_get_item() and test_list_get_item_ref()?

Objects/listobject.c Show resolved Hide resolved
Comment on lines 908 to 909
[function.PyList_GetItemRef]
added = '3.13'
Copy link
Member

Choose a reason for hiding this comment

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

Move it to the end of the file.

@serhiy-storchaka
Copy link
Member

When I proposed such change, @serhiy-storchaka asked me to stick to RuntimeError, but I don't recall why :-D @serhiy-storchaka, do you prefer RuntimeError or TypeError?

Is it RuntimeError or SystemError?

The only discussed difference was returning a strong reference instead of a weak reference. If we want to change also the exception type, it should be a separate discussion.

Most concrete type C API raises SystemError if they are called with wrong type. It is considered a programming error, not a data error.

@vstinner
Copy link
Member

Most concrete type C API raises SystemError if they are called with wrong type. It is considered a programming error, not a data error.

I suggest to document that the function "parameter must be list" , without mentioning the exact exception type.

@colesbury
Copy link
Contributor Author

Most concrete type C API raises SystemError if they are called with wrong type. It is considered a programming error, not a data error.

The TypeError behavior is explicitly stated in the C API WG discussion of this API. See also capi-workgroup/api-evolution#29 (comment) and PyWeakref_GetRef().

@colesbury
Copy link
Contributor Author

I've removed the sentence mentioning TypeError from the PyList_GetItemRef documentation. I don't think saying that "parameter must be list" without describing the error return is helpful. That merely states the obvious and risks being confusing: it raises the question of why, out of all the functions that must take a list, only this one says "parameter must be list".

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

The TypeError behavior is explicitly stated in the C API WG capi-workgroup/decisions#9 (comment). See also capi-workgroup/api-evolution#29 (comment) and PyWeakref_GetRef().

Oh ok, I didn't notice that TypeError was explicitly mentioned.

Objects/listobject.c Show resolved Hide resolved
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM too.

I will merge the PR if @serhiy-storchaka approve.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

If the WG does discuss the exception type, it'll take some time.
Let's merge this, the type can be changed later

Doc/c-api/list.rst Outdated Show resolved Hide resolved
Doc/c-api/list.rst Outdated Show resolved Hide resolved
Co-authored-by: Erlend E. Aasland <[email protected]>
@encukou encukou merged commit d0f1307 into python:main Feb 2, 2024
34 checks passed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
The new `PyList_GetItemRef` is similar to `PyList_GetItem`, but returns
a strong reference instead of a borrowed reference. Additionally, if the
passed "list" object is not a list, the function sets a `TypeError`
instead of calling `PyErr_BadInternalCall()`.
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
The new `PyList_GetItemRef` is similar to `PyList_GetItem`, but returns
a strong reference instead of a borrowed reference. Additionally, if the
passed "list" object is not a list, the function sets a `TypeError`
instead of calling `PyErr_BadInternalCall()`.
@colesbury colesbury deleted the gh-114329-list branch April 8, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants