Merge pull request RustPython#4421 from yt2b/implement_string_format_validation

youknowone · web-flow · commit f1ca3596c07c · 2023-01-06T01:43:04.000+09:00
Implement string format validation
diff --git 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,10 +604,10 @@ impl FormatSpec {
             &sign_prefix,
             FormatAlign::Right,
         )
-        .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)
@@ -618,7 +617,13 @@ impl FormatSpec {
                     }
                     value
                 }),
-            _ => Err("Unknown format code for object of type 'str'"),
+            _ => {
+                let ch = char::from(self.format_type.as_ref().unwrap());
+                Err(format!(
+                    "Unknown format code '{}' for object of type 'str'",
+                    ch
+                ))
+            }
         }
     }
 
@@ -627,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();
diff --git a/extra_tests/snippets/builtin_format.py b/extra_tests/snippets/builtin_format.py
@@ -72,3 +72,5 @@ def test_zero_padding():
 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'.")
diff --git 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

Original file line number	Diff line number	Diff line change
`@@ -522,7 +522,6 @@ impl FormatSpec {`
`522`	`522`	`sign_str,`
`523`	`523`	`FormatAlign::Right,`
`524`	`524`	`)`
`525`		`- .map_err(\|msg\| msg.to_owned())`
`526`	`525`	`}`
`527`	`526`
`528`	`527`	`#[inline]`
`@@ -605,10 +604,10 @@ impl FormatSpec {`
`605`	`604`	`&sign_prefix,`
`606`	`605`	`FormatAlign::Right,`
`607`	`606`	`)`
`608`		`- .map_err(\|msg\| msg.to_owned())`
`609`	`607`	`}`
`610`	`608`
`611`		`- pub fn format_string(&self, s: &BorrowedStr) -> Result<String, &'static str> {`
	`609`	`+ pub fn format_string(&self, s: &BorrowedStr) -> Result<String, String> {`
	`610`	`+ self.validate_format(FormatType::String)?;`
`612`	`611`	`match self.format_type {`
`613`	`612`	`Some(FormatType::String) \| None => self`
`614`	`613`	`.format_sign_and_align(s, "", FormatAlign::Left)`
`@@ -618,7 +617,13 @@ impl FormatSpec {`
`618`	`617`	`}`
`619`	`618`	`value`
`620`	`619`	`}),`
`621`		`- _ => Err("Unknown format code for object of type 'str'"),`
	`620`	`+ _ => {`
	`621`	`+ let ch = char::from(self.format_type.as_ref().unwrap());`
	`622`	`+ Err(format!(`
	`623`	`+ "Unknown format code '{}' for object of type 'str'",`
	`624`	`+ ch`
	`625`	`+ ))`
	`626`	`+ }`
`622`	`627`	`}`
`623`	`628`	`}`
`624`	`629`
`@@ -627,7 +632,7 @@ impl FormatSpec {`
`627`	`632`	`magnitude_str: &BorrowedStr,`
`628`	`633`	`sign_str: &str,`
`629`	`634`	`default_align: FormatAlign,`
`630`		`- ) -> Result<String, &'static str> {`
	`635`	`+ ) -> Result<String, String> {`
`631`	`636`	`let align = self.align.unwrap_or(default_align);`
`632`	`637`
`633`	`638`	`let num_chars = magnitude_str.char_len();`