From f84dd2e84f2e679da7556c493ebb7dbca7fd6860 Mon Sep 17 00:00:00 2001
From: yt2b <reinventing.the.wheels@gmail.com>
Date: Thu, 5 Jan 2023 21:54:32 +0900
Subject: [PATCH 1/3] Implement format validation

---
 common/src/format.rs   | 14 +++++++++++---
 vm/src/builtins/str.rs |  8 +++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/common/src/format.rs b/common/src/format.rs
index 6f6fc661e..5ebbe107d 100644
--- a/common/src/format.rs
+++ b/common/src/format.rs
@@ -608,7 +608,8 @@ impl FormatSpec {
         .map_err(|msg| msg.to_owned())
     }
 
-    pub fn format_string(&self, s: &BorrowedStr) -> Result<String, &'static str> {
+    pub fn format_string(&self, s: &BorrowedStr) -> Result<String, String> {
+        self.validate_format(FormatType::String)?;
         match self.format_type {
             Some(FormatType::String) | None => self
                 .format_sign_and_align(s, "", FormatAlign::Left)
@@ -617,8 +618,15 @@ impl FormatSpec {
                         value.truncate(precision);
                     }
                     value
-                }),
-            _ => Err("Unknown format code for object of type 'str'"),
+                })
+                .map_err(|msg| msg.to_owned()),
+            _ => {
+                let ch = char::from(self.format_type.as_ref().unwrap());
+                Err(format!(
+                    "Unknown format code '{}' for object of type 'str'",
+                    ch
+                ))
+            }
         }
     }
 
diff --git a/vm/src/builtins/str.rs b/vm/src/builtins/str.rs
index 443f61502..83e2581d5 100644
--- a/vm/src/builtins/str.rs
+++ b/vm/src/builtins/str.rs
@@ -730,9 +730,11 @@ 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| vm.new_value_error(err.to_string()))
+        let format_spec =
+            FormatSpec::parse(spec.as_str()).map_err(|err| vm.new_value_error(err.to_string()))?;
+        format_spec
+            .format_string(self.borrow())
+            .map_err(|msg| vm.new_value_error(msg))
     }
 
     /// Return a titlecased version of the string where words start with an

From 1534112cfe3fb2d44ca38b85c940fcb7024f153a Mon Sep 17 00:00:00 2001
From: yt2b <reinventing.the.wheels@gmail.com>
Date: Thu, 5 Jan 2023 21:55:02 +0900
Subject: [PATCH 2/3] Add test

---
 extra_tests/snippets/builtin_format.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/extra_tests/snippets/builtin_format.py b/extra_tests/snippets/builtin_format.py
index a4b14c559..dca207104 100644
--- a/extra_tests/snippets/builtin_format.py
+++ b/extra_tests/snippets/builtin_format.py
@@ -72,3 +72,5 @@ assert_raises(ValueError, "{:,o}".format, 1, _msg="ValueError: Cannot specify ',
 assert_raises(ValueError, "{:_n}".format, 1, _msg="ValueError: Cannot specify '_' with 'n'.")
 assert_raises(ValueError, "{:,o}".format, 1.0, _msg="ValueError: Cannot specify ',' with 'o'.")
 assert_raises(ValueError, "{:_n}".format, 1.0, _msg="ValueError: Cannot specify '_' with 'n'.")
+assert_raises(ValueError, "{:,}".format, "abc", _msg="ValueError: Cannot specify ',' with 's'.")
+assert_raises(ValueError, "{:,x}".format, "abc", _msg="ValueError: Cannot specify ',' with 'x'.")

From e60a264dc82b1f01a8399704d7acb7c5783b66af Mon Sep 17 00:00:00 2001
From: yt2b <reinventing.the.wheels@gmail.com>
Date: Thu, 5 Jan 2023 22:09:53 +0900
Subject: [PATCH 3/3] Remove map_err

---
 common/src/format.rs | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/common/src/format.rs b/common/src/format.rs
index 5ebbe107d..901566741 100644
--- a/common/src/format.rs
+++ b/common/src/format.rs
@@ -522,7 +522,6 @@ impl FormatSpec {
             sign_str,
             FormatAlign::Right,
         )
-        .map_err(|msg| msg.to_owned())
     }
 
     #[inline]
@@ -605,7 +604,6 @@ impl FormatSpec {
             &sign_prefix,
             FormatAlign::Right,
         )
-        .map_err(|msg| msg.to_owned())
     }
 
     pub fn format_string(&self, s: &BorrowedStr) -> Result<String, String> {
@@ -618,8 +616,7 @@ impl FormatSpec {
                         value.truncate(precision);
                     }
                     value
-                })
-                .map_err(|msg| msg.to_owned()),
+                }),
             _ => {
                 let ch = char::from(self.format_type.as_ref().unwrap());
                 Err(format!(
@@ -635,7 +632,7 @@ impl FormatSpec {
         magnitude_str: &BorrowedStr,
         sign_str: &str,
         default_align: FormatAlign,
-    ) -> Result<String, &'static str> {
+    ) -> Result<String, String> {
         let align = self.align.unwrap_or(default_align);
 
         let num_chars = magnitude_str.char_len();