Merge pull request #644 from sebpuetz/fix-mapping-protocol

Remove contains and iter from PyMappingProtocol.

CHANGELOG.md

@@ -15,8 +15,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 * Fix handling of invalid utf-8 sequences in `PyString::as_bytes` [#639](https://github.com/PyO3/pyo3/pull/639)
 and `PyString::to_string_lossy` [#642](https://github.com/PyO3/pyo3/pull/642).
+* Remove `__contains__` and `__iter__` from PyMappingProtocol. [#644](https://github.com/PyO3/pyo3/pull/644)
 * Fix proc-macro definition of PySetAttrProtocol. [#645](https://github.com/PyO3/pyo3/pull/645)
 
+
 ## [0.8.1]
 
 ### Added

pyo3-derive-backend/src/defs.rs

@@ -277,37 +277,16 @@ pub const MAPPING: Proto = Proto {
             pyres: false,
             proto: "pyo3::class::mapping::PyMappingDelItemProtocol",
         },
-        MethodProto::Binary {
-            name: "__contains__",
-            arg: "Value",
-            pyres: false,
-            proto: "pyo3::class::mapping::PyMappingContainsProtocol",
-        },
         MethodProto::Unary {
             name: "__reversed__",
             pyres: true,
             proto: "pyo3::class::mapping::PyMappingReversedProtocol",
         },
-        MethodProto::Unary {
-            name: "__iter__",
-            pyres: true,
-            proto: "pyo3::class::mapping::PyMappingIterProtocol",
-        },
-    ],
-    py_methods: &[
-        PyMethod {
-            name: "__iter__",
-            proto: "pyo3::class::mapping::PyMappingIterProtocolImpl",
-        },
-        PyMethod {
-            name: "__contains__",
-            proto: "pyo3::class::mapping::PyMappingContainsProtocolImpl",
-        },
-        PyMethod {
-            name: "__reversed__",
-            proto: "pyo3::class::mapping::PyMappingReversedProtocolImpl",
-        },
     ],
+    py_methods: &[PyMethod {
+        name: "__reversed__",
+        proto: "pyo3::class::mapping::PyMappingReversedProtocolImpl",
+    }],
 };
 
 pub const SEQ: Proto = Proto {

src/class/mapping.rs

@@ -43,20 +43,6 @@ pub trait PyMappingProtocol<'p>: PyTypeInfo {
         unimplemented!()
     }
 
-    fn __iter__(&'p self, py: Python<'p>) -> Self::Result
-    where
-        Self: PyMappingIterProtocol<'p>,
-    {
-        unimplemented!()
-    }
-
-    fn __contains__(&'p self, value: Self::Value) -> Self::Result
-    where
-        Self: PyMappingContainsProtocol<'p>,
-    {
-        unimplemented!()
-    }
-
     fn __reversed__(&'p self) -> Self::Result
     where
         Self: PyMappingReversedProtocol<'p>,
@@ -89,16 +75,6 @@ pub trait PyMappingDelItemProtocol<'p>: PyMappingProtocol<'p> {
     type Result: Into<PyResult<()>>;
 }
 
-pub trait PyMappingIterProtocol<'p>: PyMappingProtocol<'p> {
-    type Success: IntoPy<PyObject>;
-    type Result: Into<PyResult<Self::Success>>;
-}
-
-pub trait PyMappingContainsProtocol<'p>: PyMappingProtocol<'p> {
-    type Value: FromPyObject<'p>;
-    type Result: Into<PyResult<bool>>;
-}
-
 pub trait PyMappingReversedProtocol<'p>: PyMappingProtocol<'p> {
     type Success: IntoPy<PyObject>;
     type Result: Into<PyResult<Self::Success>>;
@@ -142,12 +118,6 @@ where
     fn methods() -> Vec<PyMethodDef> {
         let mut methods = Vec::new();
 
-        if let Some(def) = <Self as PyMappingIterProtocolImpl>::__iter__() {
-            methods.push(def)
-        }
-        if let Some(def) = <Self as PyMappingContainsProtocolImpl>::__contains__() {
-            methods.push(def)
-        }
         if let Some(def) = <Self as PyMappingReversedProtocolImpl>::__reversed__() {
             methods.push(def)
         }
@@ -283,20 +253,6 @@ where
     }
 }
 
-#[doc(hidden)]
-pub trait PyMappingContainsProtocolImpl {
-    fn __contains__() -> Option<PyMethodDef>;
-}
-
-impl<'p, T> PyMappingContainsProtocolImpl for T
-where
-    T: PyMappingProtocol<'p>,
-{
-    default fn __contains__() -> Option<PyMethodDef> {
-        None
-    }
-}
-
 #[doc(hidden)]
 pub trait PyMappingReversedProtocolImpl {
     fn __reversed__() -> Option<PyMethodDef>;
@@ -310,17 +266,3 @@ where
         None
     }
 }
-
-#[doc(hidden)]
-pub trait PyMappingIterProtocolImpl {
-    fn __iter__() -> Option<PyMethodDef>;
-}
-
-impl<'p, T> PyMappingIterProtocolImpl for T
-where
-    T: PyMappingProtocol<'p>,
-{
-    default fn __iter__() -> Option<PyMethodDef> {
-        None
-    }
-}

tests/test_mapping.rs

@@ -0,0 +1,130 @@
+#![feature(specialization)]
+use std::collections::HashMap;
+
+use pyo3::exceptions::KeyError;
+use pyo3::prelude::*;
+use pyo3::types::IntoPyDict;
+use pyo3::types::PyList;
+use pyo3::PyMappingProtocol;
+
+#[pyclass]
+struct Mapping {
+    index: HashMap<String, usize>,
+}
+
+#[pymethods]
+impl Mapping {
+    #[new]
+    fn new(obj: &PyRawObject, elements: Option<&PyList>) -> PyResult<()> {
+        if let Some(pylist) = elements {
+            let mut elems = HashMap::with_capacity(pylist.len());
+            for (i, pyelem) in pylist.into_iter().enumerate() {
+                let elem = String::extract(pyelem)?;
+                elems.insert(elem, i);
+            }
+            obj.init(Self { index: elems });
+        } else {
+            obj.init(Self {
+                index: HashMap::new(),
+            });
+        }
+        Ok(())
+    }
+}
+
+#[pyproto]
+impl PyMappingProtocol for Mapping {
+    fn __len__(&self) -> PyResult<usize> {
+        Ok(self.index.len())
+    }
+
+    fn __getitem__(&self, query: String) -> PyResult<usize> {
+        self.index
+            .get(&query)
+            .copied()
+            .ok_or_else(|| KeyError::py_err("unknown key"))
+    }
+
+    fn __setitem__(&mut self, key: String, value: usize) -> PyResult<()> {
+        self.index.insert(key, value);
+        Ok(())
+    }
+
+    fn __delitem__(&mut self, key: String) -> PyResult<()> {
+        if self.index.remove(&key).is_none() {
+            KeyError::py_err("unknown key").into()
+        } else {
+            Ok(())
+        }
+    }
+
+    /// not an actual reversed implementation, just to demonstrate that the method is callable.
+    fn __reversed__(&self) -> PyResult<PyObject> {
+        let gil = Python::acquire_gil();
+        Ok(self
+            .index
+            .keys()
+            .cloned()
+            .collect::<Vec<String>>()
+            .into_py(gil.python()))
+    }
+}
+
+#[test]
+fn test_getitem() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let d = [("Mapping", py.get_type::<Mapping>())].into_py_dict(py);
+
+    let run = |code| py.run(code, None, Some(d)).unwrap();
+    let err = |code| py.run(code, None, Some(d)).unwrap_err();
+
+    run("m = Mapping(['1', '2', '3']); assert m['1'] == 0");
+    run("m = Mapping(['1', '2', '3']); assert m['2'] == 1");
+    run("m = Mapping(['1', '2', '3']); assert m['3'] == 2");
+    err("m = Mapping(['1', '2', '3']); print(m['4'])");
+}
+
+#[test]
+fn test_setitem() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let d = [("Mapping", py.get_type::<Mapping>())].into_py_dict(py);
+
+    let run = |code| py.run(code, None, Some(d)).unwrap();
+    let err = |code| py.run(code, None, Some(d)).unwrap_err();
+
+    run("m = Mapping(['1', '2', '3']); m['1'] = 4; assert m['1'] == 4");
+    run("m = Mapping(['1', '2', '3']); m['0'] = 0; assert m['0'] == 0");
+    run("m = Mapping(['1', '2', '3']); len(m) == 4");
+    err("m = Mapping(['1', '2', '3']); m[0] = 'hello'");
+    err("m = Mapping(['1', '2', '3']); m[0] = -1");
+}
+
+#[test]
+fn test_delitem() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+
+    let d = [("Mapping", py.get_type::<Mapping>())].into_py_dict(py);
+    let run = |code| py.run(code, None, Some(d)).unwrap();
+    let err = |code| py.run(code, None, Some(d)).unwrap_err();
+
+    run(
+        "m = Mapping(['1', '2', '3']); del m['1']; assert len(m) == 2; \
+         assert m['2'] == 1; assert m['3'] == 2",
+    );
+    err("m = Mapping(['1', '2', '3']); del m[-1]");
+    err("m = Mapping(['1', '2', '3']); del m['4']");
+}
+
+#[test]
+fn test_reversed() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+
+    let d = [("Mapping", py.get_type::<Mapping>())].into_py_dict(py);
+    let run = |code| py.run(code, None, Some(d)).unwrap();
+
+    run("m = Mapping(['1', '2']); assert set(reversed(m)) == {'1', '2'}");
+}