hpy icon indicating copy to clipboard operation
hpy copied to clipboard

Return an error state from HPyField_Store

Open mattip opened this issue 1 year ago • 4 comments

Currently HPyField_Store has a void return value. On PyPy, the function can fail if the type has no tp_traverse.

mattip avatar Nov 30 '23 21:11 mattip

It's about the type of the "owner" or the field value or both?

For the owner: it must be defined in the extension that calls HPyField_Store, so the user should know and it can be viewed as a bug from the same category as casting to a different struct or such, it feels to me that one could justify making it just a debug mode check instead. But that does not hold for the value, it can be of an arbitrary type, even coming from another extension...

steve-s avatar Dec 01 '23 10:12 steve-s

I remember that we discussed if HPyField_Store should be an operation that may fail. Unfortunately, I cannot find any notes about that neither in the dev call minutes nor in the issues/PRs. However, as far as I remember, we agreed that if HPyField_Store would fail, this would be a fatal error like a segmentation fault.

I'm curious: why does it fail on PyPy if tp_traverse is not there? Is it due to a sanity chech? Anyway, I suggest that you do something like HPy_FatalError.

fangerer avatar Dec 01 '23 11:12 fangerer

Yes, it is a sanity check. The Visit code in PyPy marks the memory as "still used" so it will not be collected. If there is no tp_traverse, we will get the problems that led to this comment and the subsequent discussion. While I can do a HPy_FatalError, I would prefer to give a clean exception instead by returning an error value from the function.

mattip avatar Dec 01 '23 11:12 mattip

Cython takes this approach: rather than segfault it will fail to import a c-extension that does not match its expectations. Accepting a segfault as a proper way to notify users about errors seems to be a last-result technique that should be avoided if possible.

mattip avatar Dec 01 '23 11:12 mattip