object.__format__ the right way

Author: youknowone

vm/src/builtins/bool.rs

@@ -116,7 +116,7 @@ impl PyBool {
 
     #[pymethod(magic)]
     fn format(obj: PyObjectRef, format_spec: PyStrRef, vm: &VirtualMachine) -> PyResult<PyStrRef> {
-        if format_spec.as_str().is_empty() {
+        if format_spec.is_empty() {
             obj.str(vm)
         } else {
             Err(vm.new_type_error("unsupported format string passed to bool.__format__".to_owned()))

vm/src/builtins/int.rs

@@ -1,10 +1,10 @@
-use super::{float, PyByteArray, PyBytes, PyStr, PyStrRef, PyType, PyTypeRef};
+use super::{float, PyByteArray, PyBytes, PyStr, PyType, PyTypeRef};
 use crate::{
     atomic_func,
+    builtins::PyStrRef,
     bytesinner::PyBytesInner,
     class::PyClassImpl,
-    common::format::FormatSpec,
-    common::hash,
+    common::{format::FormatSpec, hash},
     convert::{IntoPyException, ToPyObject, ToPyResult},
     function::{
         ArgByteOrder, ArgIntoBool, OptionalArg, OptionalOption, PyArithmeticValue,

vm/src/builtins/object.rs

@@ -243,14 +243,13 @@ impl PyBaseObject {
 
     #[pymethod(magic)]
     fn format(obj: PyObjectRef, format_spec: PyStrRef, vm: &VirtualMachine) -> PyResult<PyStrRef> {
-        if format_spec.as_str().is_empty() {
-            obj.str(vm)
-        } else {
-            Err(vm.new_type_error(format!(
+        if !format_spec.is_empty() {
+            return Err(vm.new_type_error(format!(
                 "unsupported format string passed to {}.__format__",
                 obj.class().name()
-            )))
+            )));
         }
+        obj.str(vm)
     }
 
     #[pyslot]

vm/src/builtins/str.rs

@@ -745,10 +745,20 @@ impl PyStr {
     }
 
     #[pymethod(name = "__format__")]
-    fn format_str(&self, spec: PyStrRef, vm: &VirtualMachine) -> PyResult<String> {
-        FormatSpec::parse(spec.as_str())
-            .and_then(|format_spec| format_spec.format_string(self.borrow()))
-            .map_err(|err| err.into_pyexception(vm))
+    fn __format__(zelf: PyRef<Self>, spec: PyStrRef, vm: &VirtualMachine) -> PyResult<PyStrRef> {
+        let spec = spec.as_str();
+        if spec.is_empty() {
+            return if zelf.class().is(vm.ctx.types.str_type) {
+                Ok(zelf)
+            } else {
+                zelf.as_object().str(vm)
+            };
+        }
+
+        let s = FormatSpec::parse(spec)
+            .and_then(|format_spec| format_spec.format_string(zelf.borrow()))
+            .map_err(|err| err.into_pyexception(vm))?;
+        Ok(vm.ctx.new_str(s))
     }
 
     /// Return a titlecased version of the string where words start with an

vm/src/format.rs

@@ -107,7 +107,7 @@ fn format_internal(
                 };
 
                 // FIXME: compiler can intern specs using parser tree. Then this call can be interned_str
-                pystr = vm.format(&argument, Some(vm.ctx.intern_str(format_spec).to_owned()))?;
+                pystr = vm.format(&argument, vm.ctx.new_str(format_spec))?;
                 pystr.as_ref()
             }
             FormatPart::Literal(literal) => literal,

vm/src/frame.rs

@@ -1765,7 +1765,7 @@ impl ExecutingFrame<'_> {
         };
 
         let spec = self.pop_value();
-        let formatted = vm.format(&value, Some(spec.downcast::<PyStr>().unwrap()))?;
+        let formatted = vm.format(&value, spec.downcast::<PyStr>().unwrap())?;
         self.push_value(formatted.into());
         Ok(None)
     }

vm/src/protocol/object.rs

@@ -3,7 +3,7 @@
 
 use crate::{
     builtins::{
-        pystr::IntoPyStrRef, PyBytes, PyDict, PyDictRef, PyGenericAlias, PyInt, PyStrRef,
+        pystr::IntoPyStrRef, PyBytes, PyDict, PyDictRef, PyGenericAlias, PyInt, PyStr, PyStrRef,
         PyTupleRef, PyTypeRef,
     },
     bytesinner::ByteInnerNewOptions,
@@ -330,12 +330,22 @@ impl PyObject {
 
     // Container of the virtual machine state:
     pub fn str(&self, vm: &VirtualMachine) -> PyResult<PyStrRef> {
-        if self.class().is(vm.ctx.types.str_type) {
-            Ok(self.to_owned().downcast().unwrap())
-        } else {
-            let s = vm.call_special_method(self.to_owned(), identifier!(vm, __str__), ())?;
-            s.try_into_value(vm)
-        }
+        let obj = match self.to_owned().downcast_exact::<PyStr>(vm) {
+            Ok(s) => return Ok(s.into_pyref()),
+            Err(obj) => obj,
+        };
+        // TODO: replace to obj.class().slots.str
+        let str_method = match vm.get_special_method(obj, identifier!(vm, __str__))? {
+            Ok(str_method) => str_method,
+            Err(obj) => return obj.repr(vm),
+        };
+        let s = str_method.invoke((), vm)?;
+        s.downcast::<PyStr>().map_err(|obj| {
+            vm.new_type_error(format!(
+                "__str__ returned non-string (type {})",
+                obj.class().name()
+            ))
+        })
     }
 
     // Equivalent to check_class. Masks Attribute errors (into TypeErrors) and lets everything

vm/src/stdlib/builtins.rs

@@ -316,7 +316,7 @@ mod builtins {
         format_spec: OptionalArg<PyStrRef>,
         vm: &VirtualMachine,
     ) -> PyResult<PyStrRef> {
-        vm.format(&value, format_spec.into())
+        vm.format(&value, format_spec.unwrap_or(vm.ctx.new_str("")))
     }
 
     #[pyfunction]

vm/src/vm/vm_ops.rs

@@ -495,11 +495,8 @@ impl VirtualMachine {
     }
 
     // PyObject_Format
-    pub fn format(&self, obj: &PyObject, format_spec: Option<PyStrRef>) -> PyResult<PyStrRef> {
-        let format_spec_is_empty = format_spec
-            .as_ref()
-            .map_or(true, |spec| spec.as_str().is_empty());
-        if format_spec_is_empty {
+    pub fn format(&self, obj: &PyObject, format_spec: PyStrRef) -> PyResult<PyStrRef> {
+        if format_spec.is_empty() {
             let obj = match obj.to_owned().downcast_exact::<PyStr>(self) {
                 Ok(s) => return Ok(s.into_pyref()),
                 Err(obj) => obj,