hpyproject / hpy

HPy: a better API for Python
https://hpyproject.org
MIT License
1.02k stars 52 forks source link

Return an error state from HPyField_Store #460

Open mattip opened 7 months ago

mattip commented 7 months ago

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

steve-s commented 7 months ago

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...

fangerer commented 7 months ago

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.

mattip commented 7 months ago

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 commented 7 months ago

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.