adding new getter for type obj

[Cheukting]

Adding new getter that works like __mro__ and __bases__ in Python.

closes #4192

[davidhewitt]

Thanks, this looks great! Just one pair of changes WRT lifetimes and some docs suggestions...

[Hooksie]

Is it necessary to do a safe downcast? As I understand, a tuple is guaranteed here from Python itself. That makes me think we can save the implicit type check and use downcast_into_unchecked

[davidhewitt]

We went back and forth at the sprints about this. It looks like it might be possible to guarantee it's ok:

$ python
Python 3.12.0 (main, Oct  8 2023, 21:31:51) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> int.__mro__ = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot set '__mro__' attribute of immutable type 'int'
>>> class Foo:
...     pass
... 
>>> Foo.__mro__ = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: attribute '__mro__' of 'type' objects is not writable

[Hooksie]

Just for completeness, I checked the CPython source directly, and yeah, can confirm this must be a tuple. Both have PyTuple_CheckExact guards on them:

static inline void
set_tp_bases(PyTypeObject *self, PyObject *bases)
{
    assert(PyTuple_CheckExact(bases));
    if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
        // XXX tp_bases can probably be statically allocated for each
        // static builtin type.
        assert(_Py_IsMainInterpreter(_PyInterpreterState_GET()));
        assert(self->tp_bases == NULL);
        if (PyTuple_GET_SIZE(bases) == 0) {
            assert(self->tp_base == NULL);
        }
        else {
            assert(PyTuple_GET_SIZE(bases) == 1);
            assert(PyTuple_GET_ITEM(bases, 0) == (PyObject *)self->tp_base);
            assert(self->tp_base->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN);
            assert(_Py_IsImmortal(self->tp_base));
        }
        _Py_SetImmortal(bases);
    }
    self->tp_bases = bases;
}

static inline void
set_tp_mro(PyTypeObject *self, PyObject *mro)
{
    assert(PyTuple_CheckExact(mro));
    if (self->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
        // XXX tp_mro can probably be statically allocated for each
        // static builtin type.
        assert(_Py_IsMainInterpreter(_PyInterpreterState_GET()));
        assert(self->tp_mro == NULL);
        /* Other checks are done via set_tp_bases. */
        _Py_SetImmortal(mro);
    }
    self->tp_mro = mro;
}

Edit: Correction, is indeed enforced as a tuple, but the real enforcement for the Python side is actually downstream from tp_setget:

static int
type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, void *context)
{
    // Check arguments
    if (!check_set_special_type_attr(type, new_bases, "__bases__")) {
        return -1;
    }
    assert(new_bases != NULL);

    if (!PyTuple_Check(new_bases)) {
        PyErr_Format(PyExc_TypeError,
             "can only assign tuple to %s.__bases__, not %s",
                 type->tp_name, Py_TYPE(new_bases)->tp_name);
        return -1;
    }
    // ...

and __mro__ has no setter defined at all

    {"__mro__", (getter)type_get_mro, NULL, NULL},

[Hooksie]

Same as below

[Cheukting]

Got it, seems we will roll back to the unsafe way to do it, right? @davidhewitt I will update this PR soon.

[davidhewitt]

This looks great to me, thanks!

[davidhewitt]

Failure looks genuine:

  error: the item `FfiPtrExt` is imported redundantly
     --> src/types/typeobject.rs:163:17
      |
  3   | use crate::ffi_ptr_ext::FfiPtrExt;
      |     ----------------------------- the item `FfiPtrExt` is already imported here
  ...
  163 |             use crate::ffi_ptr_ext::FfiPtrExt;
      |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: `-D unused-imports` implied by `-D warnings`
  
  error: could not compile `pyo3` due to previous error

... seems like some of the FfiPtrExtr imports lower down in this file can probably just be removed now that we've added an import up the top.

[Hooksie]

Thanks for implementing!

[Cheukting]

@davidhewitt thanks! Will have a look at the unused imports

[Hooksie]

very minor: The expect message here is a little awkward IMO. There is no broad "Rust object" that we're trying to convert to, instead we're converting from the PyObject down into PyTuple.

My suggestion would be something like this instead

Unexpected type in __mro__ attribute.

[Hooksie]

Adding some really small feedback on the expect message. Feel free to disregard if it's not important

[Cheukting]

Thank you @Hooksie