Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-108512: Add and use new replacements for PySys_GetObject()#111035

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
serhiy-storchaka wants to merge24 commits intopython:main
base:main
Choose a base branch
Loading
fromserhiy-storchaka:capi-PySys_GetAttr

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchakaserhiy-storchaka commentedOct 18, 2023•
edited by github-actionsbot
Loading

Add functions PySys_GetAttr(), PySys_GetAttrString(), PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().

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

Add functions PySys_GetAttr(), PySys_GetAttrString(),PySys_GetOptionalAttr() and PySys_GetOptionalAttrString().
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

Would it be possible to leave changes to use these changes aside, in a separated PR, so we can focus only on the API, doc and tests on the new functions?

The function is called "GetAttr" but in the doc, you wrote "if the object exists". IMO "if the attribute exists" is more appropriate.

Is it required to add these new functions to the stable ABI in Python 3.13? Can we wait one Python release to see how it does, before add them to the stable ABI?

Would it be possible to add tests?

Comment on lines 251 to 256
If the object exists, set *\*result* to a new :term:`strong reference`
to the object and return ``1``.
If the object does not exist, set *\*result* to ``NULL`` and return ``0``,
without setting an exception.
If other error occurred, set an exception, set *\*result* to ``NULL`` and
return ``-1``.
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 use a list and start with the return value, so it's easier to see the 3 cases:

Suggested change
If the object exists, set *\*result* to a new:term:`strong reference`
to the objectand return ``1``.
If the object does not exist,set *\*result* to ``NULL`` and return ``0``,
without setting an exception.
If other error occurred, set an exception,set *\*result* to ``NULL`` and
return ``-1``.
* Return ``1`` and set *\*result* to a new:term:`strong reference`
to the objectif the attribute exists.
* Return ``0`` without setting an exception andset *\*result* to ``NULL``
if the attribute does not exist.
* Return ``-1``, set an exception andset *\*result* to ``NULL``
if an error occurred.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

"Return and then set exception and variable" looks like a wrong sequence to me. It cannot do anything after returning.

Copy link
Member

Choose a reason for hiding this comment

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

Just say it in the opposite order in this case:

Set an exception, set *\*result* to ``NULL``, and return ``-1``, if an error occurred.

serhiy-storchaka reacted with thumbs up emoji
Co-authored-by: Victor Stinner <vstinner@python.org>
with support.swap_attr(sys, '\U0001f40d', 42):
self.assertEqual(sys_getattr('\U0001f40d'), 42)

with self.assertRaisesRegex(RuntimeError, r'lost sys\.nonexisting'):
Copy link
Member

Choose a reason for hiding this comment

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

This error message is surprising. sys has no attribute "nonexisting". It's not "lost", it simply doesn't exist.

I would prefer to always raise AttributeError with a message like "module 'sys' has no attribute 'x'", similar than in Python:

>>> import sys>>> sys.xAttributeError: module 'sys' has no attribute 'x'

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It leaves no use cases forPySys_GetAttr(). They all can be replaced withPySys_GetOptionalAttr() followed byPyErr_SetString(PyExc_RuntimeError,).

If you leavePySys_GetAttr(), you will always use it withPyErr_ExceptionMatches(PyExc_AttributeError) followed byPyErr_SetString(PyExc_RuntimeError,).

# CRASHES sys_getattr(NULL)

def test_sys_getoptionalattr(self):
sys_getattr = _testcapi.sys_getoptionalattr
Copy link
Member

Choose a reason for hiding this comment

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

It's surprising that in all tests, the function is called "sys_getattr", whereas here you test PySys_GetOptionalAttr(), not PySys_GetAttr().

I suggest to rename the variable t o"sys_getoptionalattr", or even PySys_GetOptionalAttr() since this is a C API test. It would be more explicit to use the name of the C API.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Actually, it was exactly what I wrote initially, but in last minute I replaced all names with the same name to make reading easy. But if you think that it does not help, I'll restore previous names.

self.assertEqual(sys_getattr('\U0001f40d'.encode()), 42)

self.assertIs(sys_getattr(b'nonexisting'), AttributeError)
self.assertRaises(UnicodeDecodeError, sys_getattr, b'\xff')
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 add a UnicodeDecodeError to test_sys_getattr() as well?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

No,PySys_GetAttr() does not raise UnicodeDecodeError. The wrapper does, but we do not testPyArg_Parse() here.


switch (PySys_GetOptionalAttr(name, &value)) {
case -1:
assert(value == NULL);
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 adding:assert(PyErr_Occurred());. Same remark in sys_getoptionalattrstring().

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Isn't there a check that each function that returns NULL should also set an exception? I relied on this in all other tests.

return value;
default:
Py_FatalError("PySys_GetOptionalAttr() returned invalid code");
Py_UNREACHABLE();
Copy link
Member

Choose a reason for hiding this comment

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

It should not be needed, Py_FatalError() is annotated with _Py_NO_RETURN. Same remark in sys_getoptionalattrstring().

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Well, removing.

}
if (result == NULL) {
result = PyExc_AttributeError;
Py_INCREF(PyExc_AttributeError);
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this code path. The function must return NULLand raise an exception if the attribute does not exist. This code path must never be reached according to the API doc.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I added it exactly to test that it never happens. But perhaps it can be removed.

@serhiy-storchaka
Copy link
MemberAuthor

Would it be possible to leave changes to use these changes aside, in a separated PR, so we can focus only on the API, doc and tests on the new functions?

It is easy to see the effect of these changes if they are in a single PR. Also, it replaces old private functions with new public API that is impossible if they are still used. If you prefer, I will split this PR into two parts after the development of the new API is complete, immediately before merging.

The function is called "GetAttr" but in the doc, you wrote "if the object exists". IMO "if the attribute exists" is more appropriate.

These are the words used in the description of the existing functionPySys_GetObject(). Module attributes are rarely referred to as "attributes". They are more often referred as module variables, module constants, module globals. Sphynx has a special role:data: for this instead of more general:attr:.

Is it required to add these new functions to the stable ABI in Python 3.13? Can we wait one Python release to see how it does, before add them to the stable ABI?

What exactly do you propose? Isn't it a point that we can suggest them as replacements forPySys_GetObject() and deprecate the latter in distant future? Should not all new public API be in the stable ABI or not be public at all?

Would it be possible to add tests?

Good point, I forgot about this. Done.

@vstinner
Copy link
Member

Module attributes are rarely referred to as "attributes".

Well, trying to getsys.x raises anAttributeError, not aNameError :-)

@vstinner
Copy link
Member

Should not all new public API be in the stable ABI or not be public at all?

I'm asking if we should only add the API to Include/cpython/ in Python 3.13, and wait for Python 3.14 to add it to the limited API. Just in case if something goes wrong, if the API changes when we notice issues. If it lands directly in the stable ABI, we can no longer change it, it's too late.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

About the exception, I see two options:

  • We consider that we get an object from anamespace, similar to LOAD_GLOBAL, and soNameError should be raised. IMO the function should be calledPySys_GetVar() in this case. Example:https://docs.python.org/dev/c-api/frame.html#c.PyFrame_GetVar raises NameError.

  • Or we consider that we are getting an attribute,PySys_GetAttr() name is good, andAttributeError should be raised in this case.

For me, RuntimeError is just meaningless and it should be avoided. RuntimeError means everything and nothing: "something failed". Thanks Python...

In Python, usually I consider that the sys module is an object and I get sys attributes withgetattr(sys, "stdout") which raisesAttributeError.

In Python, I'm not even sure how to treat sys as a namespace.sys.__dict__['stdout'] raises KeyError, not NameError.

@serhiy-storchaka
Copy link
MemberAuthor

Many of existing uses ofPySys_GetObject() and_PySys_GetAttr() raise RuntimeError (if they raise an exception at all). None raises AttributeError. If you want an AttributeError, usePyObject_GetAttr(). I do not think there are many cases for this.

@serhiy-storchakaserhiy-storchaka marked this pull request as draftMay 4, 2025 16:22
Comment on lines 251 to 256
If the object exists, set *\*result* to a new :term:`strong reference`
to the object and return ``1``.
If the object does not exist, set *\*result* to ``NULL`` and return ``0``,
without setting an exception.
If other error occurred, set an exception, set *\*result* to ``NULL`` and
return ``-1``.
Copy link
Member

Choose a reason for hiding this comment

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

Just say it in the opposite order in this case:

Set an exception, set *\*result* to ``NULL``, and return ``-1``, if an error occurred.

serhiy-storchaka reacted with thumbs up emoji
.. c:function:: PyObject *PySys_GetAttr(PyObject *name)

Get the attribute *name* of the :mod:`sys` module. Return a :term:`strong reference`.
Raise :exc:`RuntimeError` and return ``NULL`` if it does not exist.
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
Raise:exc:`RuntimeError` and return ``NULL`` if it does not exist.
Raise:exc:`RuntimeError` and return ``NULL`` if it does not exist or if the:mod:`sys` module cannot be found.

serhiy-storchaka reacted with thumbs up emoji
@@ -0,0 +1,23 @@
#ifndef Py_CPYTHON_SYSMODULE_H
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related to the 4 new functions?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

No, it is a merging error.

Copy link
MemberAuthor

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Thank you for review@vstinner.

@@ -0,0 +1,23 @@
#ifndef Py_CPYTHON_SYSMODULE_H
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

No, it is a merging error.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner approved these changes

@iritkatrieliritkatrielAwaiting requested review from iritkatriel

@encukouencukouAwaiting requested review from encukouencukou is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@FFY00FFY00Awaiting requested review from FFY00FFY00 is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 will be requested when the pull request is marked ready for reviewkumaraditya303 is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@serhiy-storchaka@vstinner
[8]ページ先頭
©2009-2025 Movatter.jp