From 9851643ec258f1b86f7d9ac9878145cd360c7794 Mon Sep 17 00:00:00 2001 From: Narawit Rakket Date: Tue, 30 Nov 2021 02:57:25 +0700 Subject: [PATCH 1/2] refactor: add repr helper function for the collections --- vm/src/builtins/list.rs | 13 +++++------- vm/src/builtins/set.rs | 36 ++++++---------------------------- vm/src/builtins/tuple.rs | 17 +++++++--------- vm/src/stdlib/collections.rs | 34 +++++++++++++++++--------------- vm/src/utils.rs | 38 ++++++++++++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 64 deletions(-) diff --git a/vm/src/builtins/list.rs b/vm/src/builtins/list.rs index 1089648dca..94b0548da4 100644 --- a/vm/src/builtins/list.rs +++ b/vm/src/builtins/list.rs @@ -12,6 +12,7 @@ use crate::{ AsMapping, AsSequence, Comparable, Constructor, Hashable, IterNext, IterNextIterable, Iterable, PyComparisonOp, Unconstructible, Unhashable, }, + utils::collection_repr, vm::{ReprGuard, VirtualMachine}, PyClassImpl, PyComparisonValue, PyContext, PyObject, PyObjectRef, PyObjectView, PyObjectWrap, PyRef, PyResult, PyValue, TypeProtocol, @@ -229,14 +230,10 @@ impl PyList { #[pymethod(magic)] fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { - let s = if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { - let elements = zelf.borrow_vec().to_vec(); - let mut str_parts = Vec::with_capacity(elements.len()); - for elem in elements.iter() { - let s = elem.repr(vm)?; - str_parts.push(s.as_str().to_owned()); - } - format!("[{}]", str_parts.join(", ")) + let s = if zelf.len() == 0 { + "[]".to_owned() + } else if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { + collection_repr(None, "[", "]", zelf.borrow_vec().iter(), vm)? } else { "[...]".to_owned() }; diff --git a/vm/src/builtins/set.rs b/vm/src/builtins/set.rs index 2183082368..e77dd45dff 100644 --- a/vm/src/builtins/set.rs +++ b/vm/src/builtins/set.rs @@ -14,6 +14,7 @@ use crate::{ AsSequence, Comparable, Constructor, Hashable, IterNext, IterNextIterable, Iterable, PyComparisonOp, Unconstructible, Unhashable, }, + utils::collection_repr, vm::{ReprGuard, VirtualMachine}, IdProtocol, PyArithmeticValue, PyClassImpl, PyComparisonValue, PyContext, PyObject, PyObjectRef, PyRef, PyResult, PyValue, TryFromObject, TypeProtocol, @@ -225,32 +226,7 @@ impl PySetInner { } fn repr(&self, class_name: Option<&str>, vm: &VirtualMachine) -> PyResult { - let mut repr = String::new(); - if let Some(name) = class_name { - repr.push_str(name); - repr.push('('); - } - repr.push('{'); - { - let mut parts_iter = self.elements().into_iter().map(|o| o.repr(vm)); - repr.push_str( - parts_iter - .next() - .transpose()? - .expect("this is not called for empty set") - .as_str(), - ); - for part in parts_iter { - repr.push_str(", "); - repr.push_str(part?.as_str()); - } - } - repr.push('}'); - if class_name.is_some() { - repr.push(')'); - } - - Ok(repr) + collection_repr(class_name, "{", "}", self.elements().iter(), vm) } fn add(&self, item: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { @@ -549,7 +525,7 @@ impl PySet { } #[pymethod(magic)] - fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { + fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { let class = zelf.class(); let borrowed_name = class.name(); let class_name = borrowed_name.deref(); @@ -565,7 +541,7 @@ impl PySet { } else { format!("{}(...)", class_name) }; - Ok(vm.ctx.new_str(s).into()) + Ok(s) } #[pymethod] @@ -878,7 +854,7 @@ impl PyFrozenSet { } #[pymethod(magic)] - fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { + fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { let inner = &zelf.inner; let class = zelf.class(); let class_name = class.name(); @@ -889,7 +865,7 @@ impl PyFrozenSet { } else { format!("{}(...)", class_name) }; - Ok(vm.ctx.new_str(s).into()) + Ok(s) } #[pymethod(magic)] diff --git a/vm/src/builtins/tuple.rs b/vm/src/builtins/tuple.rs index 67c6fc3b1e..c6bc0fca1d 100644 --- a/vm/src/builtins/tuple.rs +++ b/vm/src/builtins/tuple.rs @@ -12,6 +12,7 @@ use crate::{ AsMapping, AsSequence, Comparable, Constructor, Hashable, IterNext, IterNextIterable, Iterable, PyComparisonOp, Unconstructible, }, + utils::collection_repr, vm::{ReprGuard, VirtualMachine}, IdProtocol, PyArithmeticValue, PyClassImpl, PyComparisonValue, PyContext, PyObject, PyObjectRef, PyRef, PyResult, PyValue, TransmuteFromObject, TryFromObject, TypeProtocol, @@ -199,17 +200,13 @@ impl PyTuple { #[pymethod(magic)] fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { - let s = if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { - let mut str_parts = Vec::with_capacity(zelf.elements.len()); - for elem in zelf.elements.iter() { - let s = elem.repr(vm)?; - str_parts.push(s.as_str().to_owned()); - } - - if str_parts.len() == 1 { - format!("({},)", str_parts[0]) + let s = if zelf.len() == 0 { + "()".to_owned() + } else if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { + if zelf.len() == 1 { + format!("({},)", zelf.elements[0].repr(vm)?) } else { - format!("({})", str_parts.join(", ")) + collection_repr(None, "(", ")", zelf.elements.iter(), vm)? } } else { "(...)".to_owned() diff --git a/vm/src/stdlib/collections.rs b/vm/src/stdlib/collections.rs index 9c952211dd..e968c6e82a 100644 --- a/vm/src/stdlib/collections.rs +++ b/vm/src/stdlib/collections.rs @@ -17,12 +17,12 @@ mod _collections { AsSequence, Comparable, Constructor, Hashable, IterNext, IterNextIterable, Iterable, PyComparisonOp, Unhashable, }, + utils::collection_repr, vm::ReprGuard, PyComparisonValue, PyObject, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol, VirtualMachine, }; use crossbeam_utils::atomic::AtomicCell; - use itertools::Itertools; use std::cmp::max; use std::collections::VecDeque; @@ -363,26 +363,28 @@ mod _collections { #[pymethod(magic)] fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { - let repr = if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { - let deque = zelf.borrow_deque().clone(); - let elements = deque - .iter() - .map(|obj| obj.repr(vm)) - .collect::, _>>()?; - let maxlen = zelf - .maxlen - .map(|maxlen| format!(", maxlen={}", maxlen)) - .unwrap_or_default(); + let class = zelf.class(); + let class_name = class.name(); + + let s = if zelf.len() == 0 { format!( - "{}([{}]{})", - zelf.class().name(), - elements.into_iter().format(", "), - maxlen + "{}([], maxlen={})", + class_name, + zelf.maxlen.unwrap_or_default() ) + } else if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { + collection_repr( + Some(&class_name), + "[", + format!("], maxlen={}", zelf.maxlen.unwrap_or_default()).as_str(), + zelf.borrow_deque().iter(), + vm, + )? } else { "[...]".to_owned() }; - Ok(repr) + + Ok(s) } #[pymethod(magic)] diff --git a/vm/src/utils.rs b/vm/src/utils.rs index d7aa50f3de..567e517fce 100644 --- a/vm/src/utils.rs +++ b/vm/src/utils.rs @@ -127,3 +127,41 @@ impl ToCString for PyStr { std::ffi::CString::new(self.as_ref()).map_err(|err| err.into_pyexception(vm)) } } + +pub(crate) fn collection_repr<'a, I>( + class_name: Option<&str>, + prefix: &str, + suffix: &str, + iter: I, + vm: &VirtualMachine, +) -> PyResult +where + I: std::iter::Iterator, +{ + let mut repr = String::new(); + if let Some(name) = class_name { + repr.push_str(name); + repr.push('('); + } + repr.push_str(prefix); + { + let mut parts_iter = iter.map(|o| o.repr(vm)); + repr.push_str( + parts_iter + .next() + .transpose()? + .expect("this is not called for empty collection") + .as_str(), + ); + for part in parts_iter { + repr.push_str(", "); + repr.push_str(part?.as_str()); + } + } + repr.push_str(suffix); + if class_name.is_some() { + repr.push(')'); + } + + Ok(repr) +} From 0f4d6cd66cd2da9b06f1fd02a9311b7f840556e0 Mon Sep 17 00:00:00 2001 From: Narawit Rakket Date: Sat, 22 Jan 2022 00:29:13 +0700 Subject: [PATCH 2/2] fix: correct deque repr result and fix hanging problem --- vm/src/stdlib/collections.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/vm/src/stdlib/collections.rs b/vm/src/stdlib/collections.rs index e968c6e82a..2f60533620 100644 --- a/vm/src/stdlib/collections.rs +++ b/vm/src/stdlib/collections.rs @@ -363,23 +363,18 @@ mod _collections { #[pymethod(magic)] fn repr(zelf: PyRef, vm: &VirtualMachine) -> PyResult { + let deque = zelf.borrow_deque().clone(); let class = zelf.class(); let class_name = class.name(); + let closing_part = zelf + .maxlen + .map(|maxlen| format!("], maxlen={}", maxlen)) + .unwrap_or_else(|| "]".to_owned()); let s = if zelf.len() == 0 { - format!( - "{}([], maxlen={})", - class_name, - zelf.maxlen.unwrap_or_default() - ) + format!("{}([{})", class_name, closing_part) } else if let Some(_guard) = ReprGuard::enter(vm, zelf.as_object()) { - collection_repr( - Some(&class_name), - "[", - format!("], maxlen={}", zelf.maxlen.unwrap_or_default()).as_str(), - zelf.borrow_deque().iter(), - vm, - )? + collection_repr(Some(&class_name), "[", &closing_part, deque.iter(), vm)? } else { "[...]".to_owned() };