diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index 2f449aa90..47f08658a 100644 --- a/Lib/test/test_builtin.py +++ b/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) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index 0a2f54886..e093f076d 100644 --- a/vm/src/builtins/make_module.rs +++ b/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] diff --git a/vm/src/function.rs b/vm/src/function.rs index 8153d182f..ab4d35fc7 100644 --- a/vm/src/function.rs +++ b/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 { + 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