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

Equivalent of PyObject_TypeCheck #160

Closed
antocuni opened this issue Jan 28, 2021 · 4 comments · Fixed by #402
Closed

Equivalent of PyObject_TypeCheck #160

antocuni opened this issue Jan 28, 2021 · 4 comments · Fixed by #402

Comments

@antocuni
Copy link
Collaborator

antocuni commented Jan 28, 2021

This is the signature of PyObject_TypeCheck:

int PyObject_TypeCheck(PyObject *o, PyTypeObject *type)
// Return true if the object o is of type type or a subtype of type. Both parameters must be non-NULL.

The obvious HPy equivalent would be:

int HPy_TypeCheck(HPy o, HPy type)

But note that type is now a generic HPy instead of a specific PyTypeObject *. However, this poses some design issues:

  1. Should we do an extra check to ensure that type is actually a type object?
  2. If yes, how do we signal the failure? Returning -1 would work, but it's too easy to forget to check if people are used to the existing semantics. Moreover, if you forget to check for errors, it's very likely that you will interpret the result as true, with all kinds of funny results.
  3. Moreover, the extra check could introduce an unnecessary slowdown compared to CPython.

Note that there is also PyObject_IsInstance which has the "right" signature but has a much more complex logic.

Possible solutions:

  1. don't do the check. It's user's responsibility to ensure that what you pass is a type, similar to how they already need to ensure that you don't pass NULL. We can introduce the extra checks in debug mode, though.
  2. do the check, but don't raise an exception and use HPy_FatalError instead
  3. change the name to something else: HPy_CheckType? HPy_TryTypeCheck? HPy_TypeCheckMaybeFail?

I think my preferred solution is (1), especially considered what is the typical usage of the API. I suspect that the in the vast majority of cases it is used to define PyXXX_Check for custom types. For example, numpy does the following:

#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)

So from this POV, the probability of passing something which is not a type is very low, and a fatal error in debug mode could be enough.

Moreover, there is also an additional alternative which is a much bigger redesign, linked to this comment on #83: when you define a custom type, you almost always need helper functions such as XXX_Check, XXX_Cast, XXX_TryCast, etc.
If we go in that direction we can introduce a macro which automatically defines all theXXX_* helpers: XXX_Check will use _HPy_TypeCheck which can be made semi-private because it will not be needed anywhere else.

@hodgestar
Copy link
Contributor

hodgestar commented Jan 28, 2021

In HPy one can't do something like:

#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)

because PyArray_Type is now a heap type and thus only available long after compilation and indeed handles themselves are also only available much later.

@antocuni
Copy link
Collaborator Author

In HPy one can't do something like:

#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)

because PyArray_Type is now a heap type and thus only available long after compilation and indeed handles themselves are also only available much later.

yes I know, but at some point we need to invent a way to have per-module global variables anyway

@hodgestar
Copy link
Contributor

This might be addressed in #156 (or at least it is being discussed there).

@timfel timfel added this to the Version 0.9 milestone Nov 1, 2022
@fangerer
Copy link
Contributor

fangerer commented Feb 2, 2023

We discussed this issue in February 2023's dev call and decided to go for option 1:

don't do the check. It's user's responsibility to ensure that what you pass is a type, similar to how they already need to ensure that you don't pass NULL. We can introduce the extra checks in debug mode, though.

Since HPy_TypeCheck already exists, all that remains is to implement the check in the debug mode.

Concerning:

#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)

@hodgestar is right, one cannot do that in HPy like this. What you can do is:

static NPY_INLINE int
HPyArray_Check(HPyContext *ctx, HPy op)
{
    HPy array_type = HPyGlobal_Load(ctx, HPyArray_Type);
    int res = HPy_TypeCheck(ctx, op, array_type);
    HPy_Close(ctx, array_type);
    return res;
}

Which is exactly what we are currently using in our numpy port.

This is more expensive since we first need to load the global, do the check, and close the global.
Petr V mentioned that this problem also exists in the C API if using heap types and he already has some solution in mind: encukou/abi3#19

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

Successfully merging a pull request may close this issue.

4 participants