Merge pull request RustPython#786 from RustPython/dict_cleanup

windelbouwman · web-flow · commit 8535ba9a56a9 · 2019-04-05T20:56:58.000+02:00
Dict cleanup
diff --git a/vm/src/frame.rs b/vm/src/frame.rs
@@ -12,7 +12,7 @@ use crate::function::PyFuncArgs;
 use crate::obj::objbool;
 use crate::obj::objbuiltinfunc::PyBuiltinFunction;
 use crate::obj::objcode::PyCodeRef;
-use crate::obj::objdict::{self, PyDictRef};
+use crate::obj::objdict::PyDictRef;
 use crate::obj::objint::PyInt;
 use crate::obj::objiter;
 use crate::obj::objlist;
@@ -21,8 +21,8 @@ use crate::obj::objstr;
 use crate::obj::objtype;
 use crate::obj::objtype::PyClassRef;
 use crate::pyobject::{
-    DictProtocol, IdProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject,
-    TypeProtocol,
+    DictProtocol, IdProtocol, ItemProtocol, PyContext, PyObjectRef, PyRef, PyResult, PyValue,
+    TryFromObject, TypeProtocol,
 };
 use crate::vm::VirtualMachine;
 
@@ -390,7 +390,9 @@ impl Frame {
                     let obj = self.pop_value();
                     if *unpack {
                         // Take all key-value pairs from the dict:
-                        let dict_elements = objdict::get_key_value_pairs(&obj);
+                        let dict: PyDictRef =
+                            obj.downcast().expect("Need a dictionary to build a map.");
+                        let dict_elements = dict.get_key_value_pairs();
                         for (key, value) in dict_elements.iter() {
                             map_obj.set_item(key.clone(), value.clone(), vm);
                         }
@@ -612,8 +614,9 @@ impl Frame {
                     }
                     bytecode::CallType::Ex(has_kwargs) => {
                         let kwargs = if *has_kwargs {
-                            let kw_dict = self.pop_value();
-                            let dict_elements = objdict::get_key_value_pairs(&kw_dict).clone();
+                            let kw_dict: PyDictRef =
+                                self.pop_value().downcast().expect("Kwargs must be a dict.");
+                            let dict_elements = kw_dict.get_key_value_pairs();
                             dict_elements
                                 .into_iter()
                                 .map(|elem| (objstr::get_value(&elem.0), elem.1))
@@ -987,22 +990,18 @@ impl Frame {
         }
     }
 
-    fn subscript(&self, vm: &VirtualMachine, a: PyObjectRef, b: PyObjectRef) -> PyResult {
-        vm.call_method(&a, "__getitem__", vec![b])
-    }
-
     fn execute_store_subscript(&self, vm: &VirtualMachine) -> FrameResult {
         let idx = self.pop_value();
         let obj = self.pop_value();
         let value = self.pop_value();
-        vm.call_method(&obj, "__setitem__", vec![idx, value])?;
+        obj.set_item(idx, value, vm)?;
         Ok(None)
     }
 
     fn execute_delete_subscript(&self, vm: &VirtualMachine) -> FrameResult {
         let idx = self.pop_value();
         let obj = self.pop_value();
-        vm.call_method(&obj, "__delitem__", vec![idx])?;
+        obj.del_item(idx, vm)?;
         Ok(None)
     }
 
@@ -1037,7 +1036,7 @@ impl Frame {
             bytecode::BinaryOperator::FloorDivide => vm._floordiv(a_ref, b_ref),
             // TODO: Subscript should probably have its own op
             bytecode::BinaryOperator::Subscript if inplace => unreachable!(),
-            bytecode::BinaryOperator::Subscript => self.subscript(vm, a_ref, b_ref),
+            bytecode::BinaryOperator::Subscript => a_ref.get_item(b_ref, vm),
             bytecode::BinaryOperator::Modulo if inplace => vm._imod(a_ref, b_ref),
             bytecode::BinaryOperator::Modulo => vm._mod(a_ref, b_ref),
             bytecode::BinaryOperator::Lshift if inplace => vm._ilshift(a_ref, b_ref),
diff --git a/vm/src/obj/objdict.rs b/vm/src/obj/objdict.rs
@@ -13,7 +13,6 @@ use crate::dictdatatype;
 use super::objiter;
 use super::objlist::PyListIterator;
 use super::objstr;
-use super::objtype;
 use crate::obj::objtype::PyClassRef;
 
 pub type DictContentType = dictdatatype::Dict;
@@ -38,14 +37,6 @@ impl PyValue for PyDict {
     }
 }
 
-pub fn get_key_value_pairs(dict: &PyObjectRef) -> Vec<(PyObjectRef, PyObjectRef)> {
-    dict.payload::<PyDict>()
-        .unwrap()
-        .entries
-        .borrow()
-        .get_items()
-}
-
 // Python dict methods:
 impl PyDictRef {
     fn new(
@@ -56,9 +47,10 @@ impl PyDictRef {
     ) -> PyResult<PyDictRef> {
         let dict = vm.ctx.new_dict();
         if let OptionalArg::Present(dict_obj) = dict_obj {
-            if objtype::isinstance(&dict_obj, &vm.ctx.dict_type()) {
-                for (needle, value) in get_key_value_pairs(&dict_obj) {
-                    dict.set_item(needle, value, vm);
+            let dicted: PyResult<PyDictRef> = dict_obj.clone().downcast();
+            if let Ok(dict_obj) = dicted {
+                for (key, value) in dict_obj.get_key_value_pairs() {
+                    dict.set_item(key, value, vm);
                 }
             } else {
                 let iter = objiter::get_iter(vm, &dict_obj)?;
@@ -71,18 +63,17 @@ impl PyDictRef {
                         None => break,
                     };
                     let elem_iter = objiter::get_iter(vm, &element)?;
-                    let needle =
-                        objiter::get_next_object(vm, &elem_iter)?.ok_or_else(|| err(vm))?;
+                    let key = objiter::get_next_object(vm, &elem_iter)?.ok_or_else(|| err(vm))?;
                     let value = objiter::get_next_object(vm, &elem_iter)?.ok_or_else(|| err(vm))?;
                     if objiter::get_next_object(vm, &elem_iter)?.is_some() {
                         return Err(err(vm));
                     }
-                    dict.set_item(needle, value, vm);
+                    dict.set_item(key, value, vm);
                 }
             }
         }
-        for (needle, value) in kwargs.into_iter() {
-            dict.set_item(vm.new_str(needle), value, vm);
+        for (key, value) in kwargs.into_iter() {
+            dict.set_item(vm.new_str(key), value, vm);
         }
         Ok(dict)
     }
@@ -97,9 +88,8 @@ impl PyDictRef {
 
     fn repr(self, vm: &VirtualMachine) -> PyResult {
         let s = if let Some(_guard) = ReprGuard::enter(self.as_object()) {
-            let elements = get_key_value_pairs(self.as_object());
             let mut str_parts = vec![];
-            for (key, value) in elements {
+            for (key, value) in self.get_key_value_pairs() {
                 let key_repr = vm.to_repr(&key)?;
                 let value_repr = vm.to_repr(&value)?;
                 str_parts.push(format!("{}: {}", key_repr.value, value_repr.value));
@@ -176,8 +166,12 @@ impl PyDictRef {
         }
     }
 
-    fn setitem(self, needle: PyObjectRef, value: PyObjectRef, vm: &VirtualMachine) {
-        self.set_item(needle, value, vm)
+    pub fn get_key_value_pairs(&self) -> Vec<(PyObjectRef, PyObjectRef)> {
+        self.entries.borrow().get_items()
+    }
+
+    fn setitem(self, key: PyObjectRef, value: PyObjectRef, vm: &VirtualMachine) {
+        self.set_item(key, value, vm)
     }
 
     fn getitem(self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult {
@@ -212,23 +206,19 @@ impl PyDictRef {
     fn hash(self, vm: &VirtualMachine) -> PyResult {
         Err(vm.new_type_error("unhashable type".to_string()))
     }
-}
 
-impl DictProtocol for PyDictRef {
-    fn contains_key<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> bool {
+    pub fn contains_key<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> bool {
         let key = key.into_pyobject(vm).unwrap();
         self.entries.borrow().contains(vm, &key).unwrap()
     }
+}
 
+impl DictProtocol for PyDictRef {
     fn get_item<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> Option<PyObjectRef> {
         let key = key.into_pyobject(vm).unwrap();
         self.entries.borrow().get(vm, &key).ok()
     }
 
-    fn get_key_value_pairs(&self) -> Vec<(PyObjectRef, PyObjectRef)> {
-        get_key_value_pairs(self.as_object())
-    }
-
     // Item set/get:
     fn set_item<T: IntoPyObject>(&self, key: T, value: PyObjectRef, vm: &VirtualMachine) {
         let key = key.into_pyobject(vm).unwrap();
diff --git a/vm/src/obj/objmodule.rs b/vm/src/obj/objmodule.rs
@@ -1,5 +1,5 @@
 use crate::obj::objtype::PyClassRef;
-use crate::pyobject::{DictProtocol, PyContext, PyRef, PyResult, PyValue};
+use crate::pyobject::{PyContext, PyRef, PyResult, PyValue};
 use crate::vm::VirtualMachine;
 
 #[derive(Debug)]
diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs
@@ -913,9 +913,7 @@ impl<T> TypeProtocol for PyRef<T> {
 }
 
 pub trait DictProtocol {
-    fn contains_key<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> bool;
     fn get_item<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> Option<PyObjectRef>;
-    fn get_key_value_pairs(&self) -> Vec<(PyObjectRef, PyObjectRef)>;
     fn set_item<T: IntoPyObject>(&self, key: T, value: PyObjectRef, vm: &VirtualMachine);
     fn del_item<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine);
 }
diff --git a/vm/src/stdlib/json.rs b/vm/src/stdlib/json.rs
@@ -7,7 +7,9 @@ use serde_json;
 
 use crate::function::PyFuncArgs;
 use crate::obj::{
-    objbool, objdict, objfloat, objint, objsequence,
+    objbool,
+    objdict::PyDictRef,
+    objfloat, objint, objsequence,
     objstr::{self, PyString},
     objtype,
 };
@@ -63,7 +65,8 @@ impl<'s> serde::Serialize for PyObjectSerializer<'s> {
             let elements = objsequence::get_elements(self.pyobject);
             serialize_seq_elements(serializer, &elements)
         } else if objtype::isinstance(self.pyobject, &self.vm.ctx.dict_type()) {
-            let pairs = objdict::get_key_value_pairs(self.pyobject);
+            let dict: PyDictRef = self.pyobject.clone().downcast().unwrap();
+            let pairs = dict.get_key_value_pairs();
             let mut map = serializer.serialize_map(Some(pairs.len()))?;
             for (key, e) in pairs.iter() {
                 map.serialize_entry(&self.clone_with_object(key), &self.clone_with_object(&e))?;
diff --git a/wasm/lib/src/browser_module.rs b/wasm/lib/src/browser_module.rs
@@ -7,6 +7,7 @@ use wasm_bindgen_futures::{future_to_promise, JsFuture};
 
 use rustpython_vm::function::{OptionalArg, PyFuncArgs};
 use rustpython_vm::obj::{
+    objdict::PyDictRef,
     objfunction::PyFunction,
     objint,
     objstr::{self, PyStringRef},
@@ -72,7 +73,8 @@ fn browser_fetch(vm: &VirtualMachine, args: PyFuncArgs) -> PyResult {
 
     if let Some(headers) = headers {
         let h = request.headers();
-        for (key, value) in rustpython_vm::obj::objdict::get_key_value_pairs(&headers) {
+        let headers: PyDictRef = headers.downcast().unwrap();
+        for (key, value) in headers.get_key_value_pairs() {
             let key = &vm.to_str(&key)?.value;
             let value = &vm.to_str(&value)?.value;
             h.set(key, value)

Original file line number	Diff line number	Diff line change
`@@ -913,9 +913,7 @@ impl<T> TypeProtocol for PyRef<T> {`
`913`	`913`	`}`
`914`	`914`
`915`	`915`	`pub trait DictProtocol {`
`916`		`- fn contains_key<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> bool;`
`917`	`916`	`fn get_item<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine) -> Option<PyObjectRef>;`
`918`		`- fn get_key_value_pairs(&self) -> Vec<(PyObjectRef, PyObjectRef)>;`
`919`	`917`	`fn set_item<T: IntoPyObject>(&self, key: T, value: PyObjectRef, vm: &VirtualMachine);`
`920`	`918`	`fn del_item<T: IntoPyObject>(&self, key: T, vm: &VirtualMachine);`
`921`	`919`	`}`