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

Infallible functions #20

Open
encukou opened this issue May 10, 2023 · 17 comments
Open

Infallible functions #20

encukou opened this issue May 10, 2023 · 17 comments
Labels

Comments

@encukou
Copy link
Contributor

encukou commented May 10, 2023

Several functions currently never fail, and don't have a way to report failure. This blocks development of features that need an error condition.

@markshannon
Copy link

If a function never fails, why does it need to report a failure?

Or do you mean functions that can fail and have to hide that failure, like PyDict_GetItem?

@JelleZijlstra
Copy link
Contributor

Here's a concrete example: PyBuffer_Release (https://docs.python.org/3/c-api/buffer.html#c.PyBuffer_Release) returns void, so there is no way for it to return an error. However, sometimes it could make sense for releasing a buffer to fail. For example, bytearray_releasebuffer (https://github.com/python/cpython/blob/a7a2dbbf72aceef61bfb50901bfa39bfb8d6d229/Objects/bytearrayobject.c#L61) could throw an exception instead of aborting if the export count drops below 0.

@markshannon
Copy link

That doesn't seem like it is infallible, that seems like the API is flawed.
Not having a means for reporting failure is not the same as infallibility.

There are infallible functions, e.g. PyTuple_Size()

@encukou
Copy link
Contributor Author

encukou commented May 10, 2023

Yes, the API is flawed. Same with PyStructSequence_SetItem, for example.

@iritkatriel iritkatriel added theme: error handling theme: implementation flaws problems with the implementation of specific APIs labels May 10, 2023
@gvanrossum
Copy link

Wouldn't PyTuple_Size fail if you pass it a non-tuple?

In a sense even (especially?!) DECREF has this flaw. There's nowhere to report errors incurred during finalization.

@markshannon
Copy link

Wouldn't PyTuple_Size fail if you pass it a non-tuple?

You're right, it does. My mistake.
PyTuple_Size takes PyObject * not PyTupleObject * so it needs to do a type check.
The inline version PyTuple_GET_SIZE doesn't fail.

@encukou
Copy link
Contributor Author

encukou commented May 17, 2023

IMO, so few API functions are infallible (in all possible implementations) that any guidelines should say there should be a way to report errors. Any exceptions need to be carefully thought out.
Like in #21.

When failure is impossible in the current implementation and the check is too expensive, inline the function and let the compiler elide the check.

@iritkatriel
Copy link
Member

PyMarshal_WriteObjectToFile can fail but it has no return value so the only way to see that it failed is to check PyErr_Occurred().

@iritkatriel
Copy link
Member

PyMarshal_WriteObjectToFile

Can we just add a return value to this function? It's void now, and it's not in the stable ABI, so it shouldn't break anything.

@encukou
Copy link
Contributor Author

encukou commented Jun 1, 2023

To 3.13, yes.
If it's void now, changing the return value to int isn't an ABI break on common/supported architectures, so in practice it should be safe to do in 3.12 too. But maybe not worth convincing the RM :)
For older releases, I'd be more careful, and only change the docs mention having to call PyErr_Occurred().

But for a function that previously didn't set an exception at all, adding such an error case would, IMO, be an API break: there should be a new function and the old one should be deprecated.

@iritkatriel
Copy link
Member

Labelling this as fixable, let's work on it in python/cpython#105184.

@vstinner
Copy link
Contributor

Concrete example: I wanted to add PyObject* PyWeakref_GetRef(PyObject *ref) which returns a strong reference to the referenced object of a weak reference. @encukou and @erlend-aasland convinced me to use the API:

int PyWeakref_GetRef(PyObject *ref, PyObject **pobj)

  • Set *pobj to a strong reference to the referenced object and return 0
  • Set *pobj to NULL and return 0 if the referenced object has been deleted
  • Raise an exception and return -1 on error (if the argument is not a weak reference)

See the issue and the PR.

@erlend-aasland
Copy link

We can work on guidelines for new APIs here: python/devguide#1122

@vstinner
Copy link
Contributor

PyFrame_FastToLocals() has to allocate a dict but its return type is void: it cannot report errors. So I added PyFrame_FastToLocalsWithError() which returns -1 on error (if an exception was set).

PyDict_GetItem() has a similar story: PyDict_GetItemWithError() was added.

It's convenient to have functions which cannot fail: it avoids to have to write boring (complex to write, easy to mess up error handling code, etc.) error code path. For example, getter functions like PyFrame_GetCode() "just" reads an attribute of a structure, so it cannot fail, right?

Well, then things become more complicated :-) Until Python 3.10, PyFrame_GetBack() just read a structure member. But in Python 3.11, if the frame has no concrete Python frame object, it has to allocate memory and the allocation can fail. On memory allocation failure, PyFrame_GetBack() returns NULL which means "no previous frame" :-(

@encukou
Copy link
Contributor Author

encukou commented Jul 12, 2023

Also, emitting a warning can cause errors (-Werror, allocation, etc.). So, if an API might ever need to raise a runtime DeprecationWarning (e.g. for a functionality change in some cases, or before removal from a stable ABI), it can't be void.

@vstinner
Copy link
Contributor

When a function evolved and then can fail but the API cannot report error, a possible workaround is to log the error with sys.unraisablehook.

@iritkatriel iritkatriel added the v label Jul 18, 2023
@iritkatriel iritkatriel removed the v label Oct 23, 2023
@encukou
Copy link
Contributor Author

encukou commented Oct 23, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants