adding new getter for type obj #4197

Cheukting · 2024-05-21T15:58:20Z

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

newsfragments/4197.added.md

src/types/typeobject.rs

Hooksie · 2024-05-22T14:12:15Z

src/types/typeobject.rs

+                .assume_borrowed_or_err(self.py())
+        }?
+        .to_owned()
+        .downcast_into()?;


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

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

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 · 2024-05-22T14:12:59Z

src/types/typeobject.rs

+                .assume_borrowed_or_err(self.py())
+        }?
+        .to_owned()
+        .downcast_into()?;


Same as below

Cheukting · 2024-05-22T18:52:43Z

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

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

davidhewitt

This looks great to me, thanks!

davidhewitt · 2024-05-24T14:59:39Z

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 · 2024-05-24T15:06:07Z

Thanks for implementing!

Cheukting · 2024-05-24T15:09:28Z

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

Hooksie · 2024-05-24T15:56:05Z

src/types/typeobject.rs

+            .getattr(intern!(self.py(), "__mro__"))
+            .expect("Cannot get `__mro__` from object.")
+            .extract()
+            .expect("Cannot convert to Rust object.");


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 · 2024-05-24T15:57:09Z

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

src/types/typeobject.rs

Co-authored-by: Matt Hooks <me@matthooks.com>

Cheukting · 2024-05-24T16:05:18Z

Thank you @Hooksie

Cheukting added 4 commits May 21, 2024 11:55

adding new getter for type obj

cc0ca20

fixing limited api build

29edb82

fix formating ssues from clippy

f6349a4

add changelog info

560705c

davidhewitt reviewed May 21, 2024

View reviewed changes

Hooksie reviewed May 22, 2024

View reviewed changes

src/types/typeobject.rs Outdated

.assume_borrowed_or_err(self.py())

}?

.to_owned()

.downcast_into()?;

Copy link

Contributor

Hooksie May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below

Cheukting and others added 7 commits May 23, 2024 15:53

Update newsfragments/4197.added.md

9a83dad

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Update src/types/typeobject.rs

0a26207

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Update src/types/typeobject.rs

86c3fb0

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Update src/types/typeobject.rs

725bf8a

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Update src/types/typeobject.rs

5daab99

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

using uncheck downcast

ddf145a

fix formating

ebe6160

davidhewitt approved these changes May 24, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue May 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2024

Hooksie approved these changes May 24, 2024

View reviewed changes

move import

d3b9bb3

Hooksie reviewed May 24, 2024

View reviewed changes

src/types/typeobject.rs Outdated Show resolved Hide resolved

Hooksie reviewed May 24, 2024

View reviewed changes

src/types/typeobject.rs Outdated Show resolved Hide resolved

Cheukting and others added 2 commits May 24, 2024 19:04

Update src/types/typeobject.rs

7c4fde0

Co-authored-by: Matt Hooks <me@matthooks.com>

Update src/types/typeobject.rs

ccd4fef

Co-authored-by: Matt Hooks <me@matthooks.com>

davidhewitt added this pull request to the merge queue May 25, 2024

Merged via the queue into PyO3:main with commit d21045c May 25, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding new getter for type obj #4197

adding new getter for type obj #4197

Cheukting commented May 21, 2024

davidhewitt left a comment

Hooksie May 22, 2024 •

edited

Loading

davidhewitt May 22, 2024

Hooksie May 22, 2024 •

edited

Loading

Hooksie May 22, 2024

Cheukting commented May 22, 2024

davidhewitt left a comment

davidhewitt commented May 24, 2024

Hooksie commented May 24, 2024 •

edited

Loading

Cheukting commented May 24, 2024

Hooksie May 24, 2024

Hooksie commented May 24, 2024

Cheukting commented May 24, 2024

adding new getter for type obj #4197

adding new getter for type obj #4197

Conversation

Cheukting commented May 21, 2024

davidhewitt left a comment

Choose a reason for hiding this comment

Hooksie May 22, 2024 • edited Loading

Choose a reason for hiding this comment

davidhewitt May 22, 2024

Choose a reason for hiding this comment

Hooksie May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Hooksie May 22, 2024

Choose a reason for hiding this comment

Cheukting commented May 22, 2024

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt commented May 24, 2024

Hooksie commented May 24, 2024 • edited Loading

Cheukting commented May 24, 2024

Hooksie May 24, 2024

Choose a reason for hiding this comment

Hooksie commented May 24, 2024

Cheukting commented May 24, 2024

Hooksie May 22, 2024 •

edited

Loading

Hooksie May 22, 2024 •

edited

Loading

Hooksie commented May 24, 2024 •

edited

Loading