Merge pull request #2328 from carbotaniuman/fix-min

Fix min test case by doing mroe validation

Lib/test/test_builtin.py

@@ -933,8 +933,6 @@ class BuiltinTest(unittest.TestCase):
         self.assertEqual(max(data, key=f),
                          sorted(reversed(data), key=f)[-1])
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_min(self):
         self.assertEqual(min('123123'), '1')
         self.assertEqual(min(1, 2, 3), 1)

vm/src/builtins/make_module.rs

@@ -449,24 +449,26 @@ mod decl {
         locals.copy().into_ref(vm)
     }
 
-    #[pyfunction]
-    fn max(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult {
+    fn min_or_max(
+        mut args: FuncArgs,
+        vm: &VirtualMachine,
+        func_name: &str,
+        op: PyComparisonOp,
+    ) -> PyResult {
         let default = args.take_keyword("default");
         let key_func = args.take_keyword("key");
-        if !args.kwargs.is_empty() {
-            let invalid_keyword = args.kwargs.get_index(0).unwrap();
-            return Err(vm.new_type_error(format!(
-                "'{}' is an invalid keyword argument for max()",
-                invalid_keyword.0
-            )));
+
+        if let Some(err) = args.check_kwargs_empty(vm) {
+            return Err(err);
         }
+
         let candidates = match args.args.len().cmp(&1) {
             std::cmp::Ordering::Greater => {
                 if default.is_some() {
-                    return Err(vm.new_type_error(
-                        "Cannot specify a default for max() with multiple positional arguments"
-                            .to_owned(),
-                    ));
+                    return Err(vm.new_type_error(format!(
+                        "Cannot specify a default for {} with multiple positional arguments",
+                        func_name
+                    )));
                 }
                 args.args
             }
@@ -481,8 +483,9 @@ mod decl {
         let mut x = match candidates_iter.next() {
             Some(x) => x,
             None => {
-                return default
-                    .ok_or_else(|| vm.new_value_error("max() arg is an empty sequence".to_owned()))
+                return default.ok_or_else(|| {
+                    vm.new_value_error(format!("{} arg is an empty sequence", func_name))
+                })
             }
         };
 
@@ -491,14 +494,14 @@ mod decl {
             let mut x_key = vm.invoke(key_func, (x.clone(),))?;
             for y in candidates_iter {
                 let y_key = vm.invoke(key_func, (y.clone(),))?;
-                if vm.bool_cmp(&y_key, &x_key, PyComparisonOp::Gt)? {
+                if vm.bool_cmp(&y_key, &x_key, op)? {
                     x = y;
                     x_key = y_key;
                 }
             }
         } else {
             for y in candidates_iter {
-                if vm.bool_cmp(&y, &x, PyComparisonOp::Gt)? {
+                if vm.bool_cmp(&y, &x, op)? {
                     x = y;
                 }
             }
@@ -507,49 +510,14 @@ mod decl {
         Ok(x)
     }
 
+    #[pyfunction]
+    fn max(args: FuncArgs, vm: &VirtualMachine) -> PyResult {
+        min_or_max(args, vm, "max()", PyComparisonOp::Gt)
+    }
+
     #[pyfunction]
     fn min(args: FuncArgs, vm: &VirtualMachine) -> PyResult {
-        let candidates = match args.args.len().cmp(&1) {
-            std::cmp::Ordering::Greater => args.args.clone(),
-            std::cmp::Ordering::Equal => vm.extract_elements(&args.args[0])?,
-            std::cmp::Ordering::Less => {
-                // zero arguments means type error:
-                return Err(vm.new_type_error("Expected 1 or more arguments".to_owned()));
-            }
-        };
-
-        if candidates.is_empty() {
-            let default = args.get_optional_kwarg("default");
-            return default
-                .ok_or_else(|| vm.new_value_error("min() arg is an empty sequence".to_owned()));
-        }
-
-        let key_func = args.get_optional_kwarg("key");
-
-        let mut candidates_iter = candidates.into_iter();
-        let mut x = candidates_iter.next().unwrap();
-        // TODO: this key function looks pretty duplicate. Maybe we can create
-        // a local function?
-        let mut x_key = if let Some(ref f) = &key_func {
-            vm.invoke(f, (x.clone(),))?
-        } else {
-            x.clone()
-        };
-
-        for y in candidates_iter {
-            let y_key = if let Some(ref f) = &key_func {
-                vm.invoke(f, (y.clone(),))?
-            } else {
-                y.clone()
-            };
-
-            if vm.bool_cmp(&x_key, &y_key, PyComparisonOp::Gt)? {
-                x = y.clone();
-                x_key = y_key;
-            }
-        }
-
-        Ok(x)
+        min_or_max(args, vm, "min()", PyComparisonOp::Lt)
     }
 
     #[pyfunction]

vm/src/function.rs

@@ -196,12 +196,20 @@ impl FuncArgs {
                 T::arity().end(),
                 given_args,
             )))
-        } else if let Some(k) = self.kwargs.keys().next() {
-            Err(vm.new_type_error(format!("Unexpected keyword argument {}", k)))
+        } else if let Some(err) = self.check_kwargs_empty(vm) {
+            Err(err)
         } else {
             Ok(bound)
         }
     }
+
+    pub fn check_kwargs_empty(&self, vm: &VirtualMachine) -> Option<PyBaseExceptionRef> {
+        if let Some(k) = self.kwargs.keys().next() {
+            Some(vm.new_type_error(format!("Unexpected keyword argument {}", k)))
+        } else {
+            None
+        }
+    }
 }
 
 /// An error encountered while binding arguments to the parameters of a Python