Merge pull request RustPython#4412 from yt2b/implement_format_validation

Implement integer format validation

Author: youknowone

Lib/test/test_types.py

@@ -223,8 +223,6 @@ def test_type_function(self):
         self.assertRaises(TypeError, type, 1, 2)
         self.assertRaises(TypeError, type, 1, 2, 3, 4)
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_int__format__(self):
         def test(i, format_spec, result):
             # just make sure we have the unified type for integers
@@ -576,8 +574,6 @@ def test(f, format_spec, result):
         test(12345.6, "1=20", '111111111111112345.6')
         test(12345.6, "*=20", '*************12345.6')
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_format_spec_errors(self):
         # int, float, and string all share the same format spec
         # mini-language parser.

common/src/format.rs

@@ -137,6 +137,28 @@ pub enum FormatType {
     Percentage,
 }
 
+impl From<&FormatType> for char {
+    fn from(from: &FormatType) -> char {
+        match from {
+            FormatType::String => 's',
+            FormatType::Binary => 'b',
+            FormatType::Character => 'c',
+            FormatType::Decimal => 'd',
+            FormatType::Octal => 'o',
+            FormatType::HexLower => 'x',
+            FormatType::HexUpper => 'X',
+            FormatType::Number => 'n',
+            FormatType::ExponentLower => 'e',
+            FormatType::ExponentUpper => 'E',
+            FormatType::GeneralFormatLower => 'g',
+            FormatType::GeneralFormatUpper => 'G',
+            FormatType::FixedPointLower => 'f',
+            FormatType::FixedPointUpper => 'F',
+            FormatType::Percentage => '%',
+        }
+    }
+}
+
 impl FormatParse for FormatType {
     fn parse(text: &str) -> (Option<Self>, &str) {
         let mut chars = text.chars();
@@ -337,6 +359,33 @@ impl FormatSpec {
         magnitude_str
     }
 
+    fn validate_format(&self, default_format_type: FormatType) -> Result<(), String> {
+        let format_type = self.format_type.as_ref().unwrap_or(&default_format_type);
+        match (&self.grouping_option, format_type) {
+            (
+                Some(FormatGrouping::Comma),
+                FormatType::String
+                | FormatType::Character
+                | FormatType::Binary
+                | FormatType::Octal
+                | FormatType::HexLower
+                | FormatType::HexUpper
+                | FormatType::Number,
+            ) => {
+                let ch = char::from(format_type);
+                Err(format!("Cannot specify ',' with '{}'.", ch))
+            }
+            (
+                Some(FormatGrouping::Underscore),
+                FormatType::String | FormatType::Character | FormatType::Number,
+            ) => {
+                let ch = char::from(format_type);
+                Err(format!("Cannot specify '_' with '{}'.", ch))
+            }
+            _ => Ok(()),
+        }
+    }
+
     fn get_separator_interval(&self) -> usize {
         match self.format_type {
             Some(FormatType::Binary) => 4,
@@ -467,14 +516,15 @@ impl FormatSpec {
     }
 
     #[inline]
-    fn format_int_radix(&self, magnitude: BigInt, radix: u32) -> Result<String, &'static str> {
+    fn format_int_radix(&self, magnitude: BigInt, radix: u32) -> Result<String, String> {
         match self.precision {
-            Some(_) => Err("Precision not allowed in integer format specifier"),
+            Some(_) => Err("Precision not allowed in integer format specifier".to_owned()),
             None => Ok(magnitude.to_str_radix(radix)),
         }
     }
 
-    pub fn format_int(&self, num: &BigInt) -> Result<String, &'static str> {
+    pub fn format_int(&self, num: &BigInt) -> Result<String, String> {
+        self.validate_format(FormatType::Decimal)?;
         let magnitude = num.abs();
         let prefix = if self.alternate_form {
             match self.format_type {
@@ -487,30 +537,34 @@ impl FormatSpec {
         } else {
             ""
         };
-        let raw_magnitude_str: Result<String, &'static str> = match self.format_type {
+        let raw_magnitude_str: Result<String, String> = match self.format_type {
             Some(FormatType::Binary) => self.format_int_radix(magnitude, 2),
             Some(FormatType::Decimal) => self.format_int_radix(magnitude, 10),
             Some(FormatType::Octal) => self.format_int_radix(magnitude, 8),
             Some(FormatType::HexLower) => self.format_int_radix(magnitude, 16),
             Some(FormatType::HexUpper) => match self.precision {
-                Some(_) => Err("Precision not allowed in integer format specifier"),
+                Some(_) => Err("Precision not allowed in integer format specifier".to_owned()),
                 None => {
                     let mut result = magnitude.to_str_radix(16);
                     result.make_ascii_uppercase();
                     Ok(result)
                 }
             },
             Some(FormatType::Number) => self.format_int_radix(magnitude, 10),
-            Some(FormatType::String) => Err("Unknown format code 's' for object of type 'int'"),
+            Some(FormatType::String) => {
+                Err("Unknown format code 's' for object of type 'int'".to_owned())
+            }
             Some(FormatType::Character) => match (self.sign, self.alternate_form) {
-                (Some(_), _) => Err("Sign not allowed with integer format specifier 'c'"),
-                (_, true) => {
-                    Err("Alternate form (#) not allowed with integer format specifier 'c'")
+                (Some(_), _) => {
+                    Err("Sign not allowed with integer format specifier 'c'".to_owned())
                 }
+                (_, true) => Err(
+                    "Alternate form (#) not allowed with integer format specifier 'c'".to_owned(),
+                ),
                 (_, _) => match num.to_u32() {
                     Some(n) if n <= 0x10ffff => Ok(std::char::from_u32(n).unwrap().to_string()),
                     // TODO: raise OverflowError
-                    Some(_) | None => Err("%c arg not in range(0x110000)"),
+                    Some(_) | None => Err("%c arg not in range(0x110000)".to_owned()),
                 },
             },
             Some(FormatType::GeneralFormatUpper)
@@ -520,8 +574,8 @@ impl FormatSpec {
             | Some(FormatType::ExponentUpper)
             | Some(FormatType::ExponentLower)
             | Some(FormatType::Percentage) => match num.to_f64() {
-                Some(float) => return self.format_float(float),
-                _ => Err("Unable to convert int to float"),
+                Some(float) => return self.format_float(float).map_err(|msg| msg.to_owned()),
+                _ => Err("Unable to convert int to float".to_owned()),
             },
             None => self.format_int_radix(magnitude, 10),
         };
@@ -537,6 +591,7 @@ impl FormatSpec {
         let sign_prefix = format!("{}{}", sign_str, prefix);
         let magnitude_str = self.add_magnitude_separators(raw_magnitude_str?, &sign_prefix);
         self.format_sign_and_align(&magnitude_str, &sign_prefix, FormatAlign::Right)
+            .map_err(|msg| msg.to_owned())
     }
 
     pub fn format_string(&self, s: &str) -> Result<String, &'static str> {

extra_tests/snippets/builtin_format.py

@@ -66,3 +66,5 @@ def test_zero_padding():
 assert f"{123.456:+011,}" == "+00,123.456"
 assert f"{1234:.3g}" == "1.23e+03"
 assert f"{1234567:.6G}" == "1.23457E+06"
+assert_raises(ValueError, "{:,o}".format, 1, _msg="ValueError: Cannot specify ',' with 'o'.")
+assert_raises(ValueError, "{:_n}".format, 1, _msg="ValueError: Cannot specify '_' with 'n'.")

vm/src/builtins/int.rs

@@ -568,9 +568,11 @@ impl PyInt {
 
     #[pymethod(magic)]
     fn format(&self, spec: PyStrRef, vm: &VirtualMachine) -> PyResult<String> {
-        FormatSpec::parse(spec.as_str())
-            .and_then(|format_spec| format_spec.format_int(&self.value))
-            .map_err(|msg| vm.new_value_error(msg.to_owned()))
+        let format_spec =
+            FormatSpec::parse(spec.as_str()).map_err(|msg| vm.new_value_error(msg.to_owned()))?;
+        format_spec
+            .format_int(&self.value)
+            .map_err(|msg| vm.new_value_error(msg))
     }
 
     #[pymethod(magic)]