Implement Mapping Protocol #3121

Snowapril · 2021-09-23T07:50:04Z

This revision implements mapping protocol

get_item, set_item, del_item in the ItemProtocol will be improved by using fast-path which is provided by mapping protocol.

With mapping protocol, this new issue can be fixed with PyMapping::check
According to docs and cpython implementation mapping proxy type must created only with mapping type.

>>>>> mappingproxy = type(type.__dict__)
>>>>> mappingproxy({"ASCII": 256})
<mappingproxy object at 0x55bb16b37560>
>>>>> mappingproxy([1,2,3])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: mappingproxy() argument must be a mapping, not list

Documents

vm/src/slots.rs

vm/src/stdlib/winapi.rs

youknowone · 2021-09-25T04:44:23Z

could we merge AsMapping and PyMappingMethods

youknowone · 2021-09-25T06:17:21Z

check for update_slot in pytype.rs. it is mandatory for protocols using slots

Snowapril · 2021-09-25T13:05:30Z

@youknowone I feel a little bit confused with using update_slot currently.
with update_slot we can update one slot that have exactly same signature with the given new method defined in python-side.
But as_mapping have three sub-slot(function) and if __getitem__ is defined in python-side, I need to update as_mapping to return PyMapping struct with only subscript field modified. How can I achieve this?..

youknowone · 2021-09-25T14:15:56Z

you need only single function slots::AsMappingFunc. Maybe you will get idea from richcompare.

roughly:

"__getitem__" | "__setitem__" | "__delitem__" => {
    let func: slots::AsMappingFunc = |zelf, vm|  PyMapping {
        get: vm.get_class_attr("__getitem__").unwrap_or(getitem_fallback),
        set: vm....
        del:...
    };
}

fn getitem_fallback(zelf: PyObjectRef, key: PyObjectRef) -> PyResult {
    super().getitem(key)  // something like this. I am not sure how cpython handle it
}

Snowapril · 2021-09-25T14:39:38Z

Ah! I got it. Thanks 😊

Snowapril · 2021-09-29T06:44:50Z

Now update_slot and mappingproxy trait works as normal

>>>>> mappingproxy = type(type.__dict__)
>>>>> mappingproxy({"ASCII": 256})
<mappingproxy object at 0x55bb16b37560>
>>>>> mappingproxy([1,2,3])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: mappingproxy() argument must be a mapping, not list

but update_slot routine for slot which have multiple sub_slots is somewhat horrible now.
I think there need something like #[pyslotstruct] for a struct which have multiple #[pyslot]

Another slots in current rustpython has only one function that corresponded to its slot.
But mapping protocol, sequence protocol and number protocol have multiple slots..

youknowone · 2021-09-29T07:39:40Z

looks great!
I think mapping protocol has only one slot as_mapping but having multiple python magic methods. (probably you wanted to point this out)
Maybe this is time to check how CPython handle this.

youknowone · 2021-09-29T17:33:47Z

vm/src/slots.rs

+    #[pyslot]
+    fn tp_as_mapping(zelf: &PyObjectRef, vm: &VirtualMachine) -> PyResult<PyMapping> {
+        let zelf = zelf
+            .downcast_ref()
+            .ok_or_else(|| vm.new_type_error("unexpected payload for as_mapping".to_owned()))?;
+        Self::as_mapping(zelf, vm)
+    }


as_mapping looks like not requiring any argument. Maybe this will be ok.

Suggested change

#[pyslot]

fn tp_as_mapping(zelf: &PyObjectRef, vm: &VirtualMachine) -> PyResult<PyMapping> {

let zelf = zelf

.downcast_ref()

.ok_or_else(|| vm.new_type_error("unexpected payload for as_mapping".to_owned()))?;

Self::as_mapping(zelf, vm)

}

#[pyslot]

fn as_mapping() -> PyMapping {

PyMapping {

length: Some(Self::length),

subscript: Some(Self::subscript),

ass_subscript: Some(Self::ass_subscript),

}

}

I cannot declare empty parameter function as slot according to below error message 😢 I think function signature for pyslot must be a fn name(&PyObjectRef, &VirtualMachine)

non-primitive cast: `fn() -> std::result::Result<builtins::dict::PyMapping, pyobjectrc::PyRef<exceptions::types::PyBaseException>> {<Self as slots::AsMapping>::slot_as_mapping}` as `for<'r, 's> fn(&'r pyobjectrc::PyObjectRef, &'s vm::VirtualMachine) -> std::result::Result<builtins::dict::PyMapping, pyobjectrc::PyRef<exceptions::types::PyBaseException>>`

Although this is little bit dirty, how about this one?

#[pyslot] fn slot_as_mapping(_zelf: &PyObjectRef, _vm: &VirtualMachine) -> PyResult<PyMapping> { Self::as_mapping() } fn as_mapping() -> PyResult<PyMapping>;

youknowone · 2021-09-29T17:39:21Z

vm/src/stdlib/posix.rs

        args: crate::function::ArgIterable<PyPathLike>,
        #[pyarg(positional)]
-        env: crate::builtins::dict::PyMapping,
+        env: crate::builtins::dict::PyDictRef,


Suggested change

env: crate::builtins::dict::PyDictRef,

env: crate::protocol::PyMapping,

with only PyMapping and its fields, I cannot get its both items and keys together.
As cpython implementation, in my opinion, adding keys and values functions in PyMapping and use PyObjectRef for env is more appropriate. how do you think?

uh, it looks more complicated than I thought before. For current MyMapping Implementation, you are right.

It looks like PyMapping itself is a wrapper of PyObejctRef, how I handled PyIter in #3117. (https://github.com/RustPython/RustPython/pull/3117/files#diff-688a17d340aa55627e0e307b8cef31b99d478c8b41664cd92f5c2572b6688097R13)

And current PyMapping looks like PyMappingMethods in CPython - probably the way you originally approached.
How do you think about this?

I dont know that I understand correctly though, did you mean switching current PyMapping to PyMappingMethods and creatingPyMapping as a wrapper of PyObjectRef with its related methods right?
It seems more appropriate than I thought 😊

Yes, exactly

As cpython use `range_compute_length` naming and `length` can be conflict with mapping protocol's method

As `getitem` method in other types, get second argument as PyObjectRef.

As `getitem` and `setitem methods in other types, get second argument as PyObjectRef.

Signed-off-by: snowapril <sinjihng@gmail.com>

As in cpython implementation there exists argument type checking before create mapping proxy like below. ```c if (!PyMapping_Check(mapping) || PyList_Check(mapping) || PyTuple_Check(mapping)) // print error ... ``` Signed-off-by: snowapril <sinjihng@gmail.com>

Signed-off-by: snowapril <sinjihng@gmail.com>

youknowone · 2021-10-02T07:01:50Z

vm/src/builtins/pytype.rs

+                    Ok(PyMappingMethods {
+                        length: then_some_closure!(zelf.has_class_attr("__len__"), |zelf, vm| {
+                            vm.call_special_method(zelf, "__len__", ()).map(|obj| {
+                                obj.payload_if_subclass::<PyInt>(vm)
+                                    .map(|length_obj| {
+                                        length_obj.as_bigint().to_usize().ok_or_else(|| {
+                                            vm.new_value_error(
+                                                "__len__() should return >= 0".to_owned(),
+                                            )
+                                        })
+                                    })
+                                    .unwrap()
+                            })?
+                        }),
+                        subscript: then_some_closure!(
+                            zelf.has_class_attr("__getitem__"),
+                            |zelf: PyObjectRef, needle: PyObjectRef, vm: &VirtualMachine| {
+                                vm.call_special_method(zelf, "__getitem__", (needle,))
+                            }
+                        ),
+                        ass_subscript: then_some_closure!(
+                            zelf.has_class_attr("__setitem__") | zelf.has_class_attr("__delitem__"),
+                            |zelf, needle, value, vm| match value {
+                                Some(value) => vm
+                                    .call_special_method(zelf, "__setitem__", (needle, value),)
+                                    .map(|_| Ok(()))?,
+                                None => vm
+                                    .call_special_method(zelf, "__delitem__", (needle,))
+                                    .map(|_| Ok(()))?,
+                            }
+                        ),
+                    })


maybe we need _PyObject_NextNotImplemented later

youknowone · 2021-10-02T07:11:18Z

vm/src/pyobject.rs

+                return getitem(self.clone(), key.into_pyobject(vm), vm);
+            }
+        }
+


do we still need __getitem__ call under this line? I think PyMapping covers the whole cases

Yes. there exists extra steps (sequence protocol check & method lookup) as cpython implementation

youknowone · 2021-10-02T07:16:10Z

vm/src/stdlib/posix.rs

    #[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "macos"))]
-    fn envp_from_dict(dict: PyDictRef, vm: &VirtualMachine) -> PyResult<Vec<CString>> {
-        dict.into_iter()
+    fn envp_from_dict(env: PyMapping, vm: &VirtualMachine) -> PyResult<Vec<CString>> {


Suggested change

fn envp_from_dict(env: PyMapping, vm: &VirtualMachine) -> PyResult<Vec<CString>> {

fn envp_from_dict(env: crate::protocol::PyMapping, vm: &VirtualMachine) -> PyResult<Vec<CString>> {

to solve unused import warning

youknowone · 2021-10-02T07:24:48Z

vm/src/protocol/mapping.rs

+        } else {
+            false
+        }
+    }


impl TryFromBorrowedObject for PyMappingMethods can be placed here.

pub fn methods(&self, vm: &VirtualMachine) -> PyMappingMethods { let obj_cls = self.0.class(); ... }

Modify original `PyMapping` to `PyMappingMethods` and create `PyMapping` struct as `PyObjectRef` wrapper for implementing `PyMapping_*` functions. https://docs.python.org/3/c-api/mapping.html Signed-off-by: snowapril <sinjihng@gmail.com>

Signed-off-by: snowapril <sinjihng@gmail.com>

vm/src/builtins/mappingproxy.rs

vm/src/builtins/bytes.rs

youknowone reviewed Sep 23, 2021

View reviewed changes

vm/src/slots.rs Outdated Show resolved Hide resolved

vm/src/slots.rs Outdated Show resolved Hide resolved

vm/src/stdlib/winapi.rs Outdated Show resolved Hide resolved

Snowapril force-pushed the mapping-protocol branch 2 times, most recently from 74c8611 to b9145dd Compare September 23, 2021 08:06

Snowapril force-pushed the mapping-protocol branch from 3ae9004 to 375c86b Compare September 25, 2021 05:12

Snowapril force-pushed the mapping-protocol branch 2 times, most recently from d0651db to 7aec733 Compare September 25, 2021 13:00

Snowapril force-pushed the mapping-protocol branch from 7aec733 to b050741 Compare September 29, 2021 06:38

youknowone reviewed Sep 29, 2021

View reviewed changes

youknowone mentioned this pull request Sep 29, 2021

Remove impl TryFromObject for SequenceIndex #3167

Closed

Snowapril force-pushed the mapping-protocol branch 2 times, most recently from b5aadd8 to 203cac9 Compare September 30, 2021 14:56

youknowone mentioned this pull request Oct 1, 2021

pub(crate) for type modules #3169

Merged

Snowapril added 10 commits October 2, 2021 14:41

add slot_as_mapping slots with atomic cell

14ce6aa

add PyMapping struct for mapping protocol

77351b3

rename length to compute_length

1b6cdf5

As cpython use `range_compute_length` naming and `length` can be conflict with mapping protocol's method

add mapping protocol to proper types

fc761b4

apply mapping protocol in itemprotocol

e83b68b

switch RangeIndex to PyObjectRef

b197a17

As `getitem` method in other types, get second argument as PyObjectRef.

switch SequenceIndex to PyObjectRef

4f009ef

As `getitem` and `setitem methods in other types, get second argument as PyObjectRef.

support delitem in ass_subscript

1fbbdaa

add mapping protocol methods to update_slot

6e11b89

Signed-off-by: snowapril <sinjihng@gmail.com>

move mapping protocol to rustpython_vm::protocol

4fd0681

Signed-off-by: snowapril <sinjihng@gmail.com>

Snowapril force-pushed the mapping-protocol branch 3 times, most recently from a57c9c1 to 5ffbb13 Compare October 2, 2021 06:10

youknowone reviewed Oct 2, 2021

View reviewed changes

Snowapril added 2 commits October 2, 2021 16:45

add PyMapping as PyObjectRef wrapper

aa2a7f0

Modify original `PyMapping` to `PyMappingMethods` and create `PyMapping` struct as `PyObjectRef` wrapper for implementing `PyMapping_*` functions. https://docs.python.org/3/c-api/mapping.html Signed-off-by: snowapril <sinjihng@gmail.com>

fix posix & winapi to use proper PyMapping

a81b3f9

Signed-off-by: snowapril <sinjihng@gmail.com>

Snowapril force-pushed the mapping-protocol branch from 5ffbb13 to a81b3f9 Compare October 2, 2021 07:48

youknowone marked this pull request as ready for review October 2, 2021 08:02

youknowone changed the title ~~(WIP) Implement Mapping Protocol~~ Implement Mapping Protocol Oct 2, 2021

youknowone reviewed Oct 2, 2021

View reviewed changes

vm/src/builtins/mappingproxy.rs Outdated Show resolved Hide resolved

vm/src/builtins/bytes.rs Outdated Show resolved Hide resolved

youknowone approved these changes Oct 2, 2021

View reviewed changes

turn unreachable function to cold

7b0965c

youknowone merged commit 6b1af9a into RustPython:main Oct 2, 2021

youknowone added the z-ca-2021 Tag to track contrubution-academy 2021 label Oct 16, 2021

qingshi163 mentioned this pull request Oct 19, 2021

Reimpl Buffer Protocol and memoryview support ndarray with shape, stride and suboffset #3340

Merged

	env: crate::builtins::dict::PyDictRef,
	env: crate::protocol::PyMapping,

	fn envp_from_dict(env: PyMapping, vm: &VirtualMachine) -> PyResult<Vec<CString>> {
	fn envp_from_dict(env: crate::protocol::PyMapping, vm: &VirtualMachine) -> PyResult<Vec<CString>> {

Implement Mapping Protocol #3121

Implement Mapping Protocol #3121

Uh oh!

Conversation

Snowapril commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documents

Uh oh!

Uh oh!

Uh oh!

Uh oh!

youknowone commented Sep 25, 2021

Uh oh!

youknowone commented Sep 25, 2021

Uh oh!

Snowapril commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Snowapril commented Sep 25, 2021

Uh oh!

Snowapril commented Sep 29, 2021

Uh oh!

youknowone commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snowapril Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Snowapril commented Sep 23, 2021 •

edited

Loading

Snowapril commented Sep 25, 2021 •

edited

Loading

youknowone commented Sep 25, 2021 •

edited

Loading

Snowapril Sep 30, 2021 •

edited

Loading

youknowone Oct 2, 2021 •

edited

Loading