feat: Adds the Bound<'_, PyMappingProxy> type #4644

KLMatlock · 2024-10-24T15:24:44Z

Continuation of #2435 and #3521. Implement the PyMappingProxy type (called PyDictProxy in CPython).

Resolves #2108.

Adds in the MappingProxy type.

davidhewitt · 2024-10-25T09:10:05Z

Thanks for the PR! This is in draft status; is there more you want to add before review?

KLMatlock · 2024-10-25T15:35:30Z

Thanks for the PR! This is in draft status; is there more you want to add before review?

Nope! Just got busy yesterday and wanted to do a final look through. Please review when ready.

davidhewitt

Thanks for picking this up; overall this looks close to merge. While I have added a bunch of comments, they're primarily small tweaks where I think the design is relatively clear 😄

davidhewitt · 2024-10-26T06:33:45Z

pyo3-ffi/src/descrobject.rs

+#[inline]
+pub unsafe fn PyDictProxy_Check(op: *mut PyObject) -> c_int {
+    PyObject_TypeCheck(op, std::ptr::addr_of_mut!(PyDictProxy_Type))
+}


This function doesn't exist in https://github.com/python/cpython/blob/3.13/Include/descrobject.h (indeed I couldn't find it anywhere). So we shouldn't add this.

Suggested change

#[inline]

pub unsafe fn PyDictProxy_Check(op: *mut PyObject) -> c_int {

PyObject_TypeCheck(op, std::ptr::addr_of_mut!(PyDictProxy_Type))

}

davidhewitt · 2024-10-26T06:37:45Z

src/types/dict.rs

@@ -780,7 +780,7 @@ where
 }

 /// Represents a tuple which can be used as a PyDict item.
-trait PyDictItem<'py> {
+pub trait PyDictItem<'py> {


This looks a possibly unintentional change

Suggested change

pub trait PyDictItem<'py> {

trait PyDictItem<'py> {

Removed! For some reason I thought I needed this for one of the tests.

davidhewitt · 2024-10-26T06:39:12Z

src/types/mappingproxy.rs

+    #checkfunction=ffi::PyDictProxy_Check
+);


I think rather than the ffi function (as discussed, it doesn't seem to exist in CPython), we should just have fn PyDictProxy_Check defined in this file.

Suggested change

#checkfunction=ffi::PyDictProxy_Check

);

#checkfunction=PyDictProxy_Check

);

#[inline]

unsafe fn PyDictProxy_Check(op: *mut PyObject) -> c_int {

ffi::PyObject_TypeCheck(op, std::ptr::addr_of_mut!(PyDictProxy_Type))

}

Also, it looks like subclassing mappingproxy is not allowed so we can probably just do Py_IS_TYPE here.

>>> class F(types.MappingProxyType): ... pass ... Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: type 'mappingproxy' is not an acceptable base type

Done, I changed the check function to the following:

#[inline] unsafe fn dict_proxy_check(op: *mut ffi::PyObject) -> c_int { ffi::Py_IS_TYPE(op, std::ptr::addr_of_mut!(ffi::PyDictProxy_Type)) }

davidhewitt · 2024-10-26T06:43:24Z

src/types/mappingproxy.rs

+    fn copy(&self) -> PyResult<Bound<'_, PyMappingProxy>> {
+        let res = self.call_method0("copy")?;
+        unsafe { Ok(res.downcast_into_unchecked::<PyMappingProxy>()) }
+    }


I think the type here is incorrect, it looks like .copy() returns a copy of the underlying mapping, perhaps?

>>> type(types.MappingProxyType({}).copy()) <class 'dict'>

I changed this to return PyMapping instead.

davidhewitt · 2024-10-26T06:46:16Z

src/types/mappingproxy.rs

+    #[inline]
+    fn keys(&self) -> PyResult<Bound<'py, PySequence>> {
+        unsafe {
+            Ok(ffi::PyMapping_Keys(self.as_ptr())
+                .assume_owned_or_err(self.py())?
+                .downcast_into_unchecked())
+        }
+    }
+
+    #[inline]
+    fn values(&self) -> PyResult<Bound<'py, PySequence>> {
+        unsafe {
+            Ok(ffi::PyMapping_Values(self.as_ptr())
+                .assume_owned_or_err(self.py())?
+                .downcast_into_unchecked())
+        }
+    }
+
+    #[inline]
+    fn items(&self) -> PyResult<Bound<'py, PySequence>> {
+        unsafe {
+            Ok(ffi::PyMapping_Items(self.as_ptr())
+                .assume_owned_or_err(self.py())?
+                .downcast_into_unchecked())
+        }
+    }


It looks like since 3.7 (our minimum supported version) these FFI functions are now guaranteed to return lists: https://docs.python.org/3/c-api/mapping.html#c.PyMapping_Keys

So I think we can use PyList as the return type. It would allow for performance gains versus going through the sequence abstraction.

Similarly, would you be willing to update PyMappingMethods in the same way (maybe as a separate PR)?

Done! Made the changes and opened #4661

davidhewitt · 2024-10-26T06:51:28Z

src/conversions/std/map.rs

I'm open to considering the conversion changes in this file, but these changes might break existing users' code silently. I'm unsure both if it's worth the churn and also what the performance cost might be.

If we move ahead with these we should also include the corresponding changes to the index map and hash brown conversions.

At a minimum I would at like to see these changes split off into a separate PR, although I will be honest that there is a chance we might decide not to have these. Was it critical for your use case?

Went ahead and removed the conversion changes. It isn't necessary for me, I just had it in there because of the previous PR.

* Addressing more comments * Remove extract_bound. * Remove extract methods.

davidhewitt

This looks great to me, thanks very much for helping get this one finally over the line!

KLMatlock and others added 9 commits October 21, 2024 19:00

Mappingproxy (#1)

b2c4d76

Adds in the MappingProxy type.

Move over from iter to try_iter.

8ae75b4

Added lifetime to try_iter, preventing need to clone when iterating.

4bad990

Remove unneccessary borrow.

a7126d0

Merge remote-tracking branch 'source/main' into feature/mappingproxy

b155ecd

Merge remote-tracking branch 'source/main' into feature/mappingproxy

fdee81a

Add newsfragment

878ed16

Newline to newsfragment.

de9743f

Remove explicit lifetime,

b209d58

Merge remote-tracking branch 'source/main' into feature/mappingproxy

55de052

KLMatlock marked this pull request as ready for review October 25, 2024 15:35

Merge remote-tracking branch 'source/main' into feature/mappingproxy

14aa10c

davidhewitt reviewed Oct 26, 2024

View reviewed changes

davidhewitt mentioned this pull request Oct 26, 2024

feat: Support for mappingproxy objects #3521

Closed

KLMatlock and others added 3 commits October 26, 2024 14:46

Review comments (#2)

24304f9

* Addressing more comments * Remove extract_bound. * Remove extract methods.

Merge remote-tracking branch 'source/main' into feature/mappingproxy

e9a33ae

Update comments for list return type.

14a3751

KLMatlock mentioned this pull request Oct 26, 2024

Mapping returns PyList for keys, values, and items #4661

Merged

davidhewitt approved these changes Oct 29, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Oct 29, 2024

Merged via the queue into PyO3:main with commit 0aa13c8 Oct 29, 2024
44 checks passed

messense mentioned this pull request Dec 6, 2024

Allow frozenset in attributes parameter of clean function messense/nh3#29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds the Bound<'_, PyMappingProxy> type #4644

feat: Adds the Bound<'_, PyMappingProxy> type #4644

KLMatlock commented Oct 24, 2024

davidhewitt commented Oct 25, 2024

KLMatlock commented Oct 25, 2024 •

edited

Loading

davidhewitt left a comment

davidhewitt Oct 26, 2024

KLMatlock Oct 26, 2024

davidhewitt Oct 26, 2024

KLMatlock Oct 26, 2024

davidhewitt Oct 26, 2024

davidhewitt Oct 26, 2024

KLMatlock Oct 26, 2024

davidhewitt Oct 26, 2024

KLMatlock Oct 26, 2024

davidhewitt Oct 26, 2024

KLMatlock Oct 26, 2024

davidhewitt Oct 26, 2024

KLMatlock Oct 26, 2024

davidhewitt left a comment

feat: Adds the Bound<'_, PyMappingProxy> type #4644

feat: Adds the Bound<'_, PyMappingProxy> type #4644

Conversation

KLMatlock commented Oct 24, 2024

davidhewitt commented Oct 25, 2024

KLMatlock commented Oct 25, 2024 • edited Loading

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

KLMatlock commented Oct 25, 2024 •

edited

Loading