From 49b5d379055eed98d62ad3fabadb9b0b9d36a302 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 11 Nov 2020 16:27:02 -0600 Subject: [PATCH 1/9] Fix min test case by doing mroe validation --- Lib/test/test_builtin.py | 2 -- vm/src/builtins/make_module.rs | 27 +++++++++++++++++++++------ vm/src/function.rs | 17 ++++++++++++++--- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index 59b5c7227..a46f10b88 100644 --- a/Lib/test/test_builtin.py +++ b/Lib/test/test_builtin.py @@ -927,8 +927,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..2dcf2bcc7 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -508,23 +508,38 @@ mod decl { } #[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])?, + fn min(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult { + let (candidates, default_allowed) = match args.args.len().cmp(&1) { + std::cmp::Ordering::Greater => (args.args.clone(), false), + std::cmp::Ordering::Equal => (vm.extract_elements(&args.args[0])?, true), std::cmp::Ordering::Less => { // zero arguments means type error: return Err(vm.new_type_error("Expected 1 or more arguments".to_owned())); } }; + let default = args.take_keyword("default"); + let mut key_func = args.take_keyword("key"); + + match args.check_kwargs_empty(vm) { + Some(err) => return Err(err), + None => {}, + } + + if !default_allowed && default.is_some() { + return Err(vm.new_type_error("Specifying default not allowed with more than 1 argument".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"); + if let Some(ref obj) = key_func { + if vm.is_none(obj) { + key_func = None + } + } let mut candidates_iter = candidates.into_iter(); let mut x = candidates_iter.next().unwrap(); diff --git a/vm/src/function.rs b/vm/src/function.rs index 8153d182f..49fb4389b 100644 --- a/vm/src/function.rs +++ b/vm/src/function.rs @@ -196,12 +196,23 @@ 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 { + } else { + match self.check_kwargs_empty(vm) { + Some(err) => return Err(err), + None => {}, + } 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 From 1aa603ac6ae1b8e7239908af8af48a6ed91a7b2a Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 11 Nov 2020 16:40:42 -0600 Subject: [PATCH 2/9] Format --- vm/src/builtins/make_module.rs | 6 ++++-- vm/src/function.rs | 5 ++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index 2dcf2bcc7..c1933a899 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -523,11 +523,13 @@ mod decl { match args.check_kwargs_empty(vm) { Some(err) => return Err(err), - None => {}, + None => {} } if !default_allowed && default.is_some() { - return Err(vm.new_type_error("Specifying default not allowed with more than 1 argument".to_owned())); + return Err(vm.new_type_error( + "Specifying default not allowed with more than 1 argument".to_owned(), + )); } if candidates.is_empty() { diff --git a/vm/src/function.rs b/vm/src/function.rs index 49fb4389b..6e6d59e30 100644 --- a/vm/src/function.rs +++ b/vm/src/function.rs @@ -196,10 +196,10 @@ impl FuncArgs { T::arity().end(), given_args, ))) - } else { + } else { match self.check_kwargs_empty(vm) { Some(err) => return Err(err), - None => {}, + None => {} } Ok(bound) } @@ -212,7 +212,6 @@ impl FuncArgs { None } } - } /// An error encountered while binding arguments to the parameters of a Python From 3a45839e01d52ae5272f5915fb9420d9f83f225a Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 11 Nov 2020 19:17:39 -0600 Subject: [PATCH 3/9] Supress clippy lint that doesn't work --- vm/src/builtins/make_module.rs | 10 ++++------ vm/src/function.rs | 5 ++--- vm/src/lib.rs | 2 ++ 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index c1933a899..00e9face5 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -521,20 +521,18 @@ mod decl { let default = args.take_keyword("default"); let mut key_func = args.take_keyword("key"); - match args.check_kwargs_empty(vm) { - Some(err) => return Err(err), - None => {} + if let Some(err) = args.check_kwargs_empty(vm) { + return Err(err); } - if !default_allowed && default.is_some() { + if default.is_some() && !default_allowed { return Err(vm.new_type_error( "Specifying default not allowed with more than 1 argument".to_owned(), )); } if candidates.is_empty() { - return default - .ok_or_else(|| vm.new_value_error("min() arg is an empty sequence".to_owned())); + return default.ok_or_else(|| vm.new_value_error("min() arg is an empty sequence".to_owned())); } if let Some(ref obj) = key_func { diff --git a/vm/src/function.rs b/vm/src/function.rs index 6e6d59e30..902920d9b 100644 --- a/vm/src/function.rs +++ b/vm/src/function.rs @@ -197,9 +197,8 @@ impl FuncArgs { given_args, ))) } else { - match self.check_kwargs_empty(vm) { - Some(err) => return Err(err), - None => {} + if let Some(err) = self.check_kwargs_empty(vm) { + return Err(err); } Ok(bound) } diff --git a/vm/src/lib.rs b/vm/src/lib.rs index bcd00bb68..9204867a5 100644 --- a/vm/src/lib.rs +++ b/vm/src/lib.rs @@ -7,6 +7,8 @@ // for methods like vm.to_str(), not the typical use of 'to' as a method prefix #![allow(clippy::wrong_self_convention, clippy::implicit_hasher)] +// clippy warns on this for macro generated code rust-clippy/issues/6249 +#![allow(clippy::suspicious_else_formatting)] // to allow `mod foo {}` in foo.rs; clippy thinks this is a mistake/misunderstanding of // how `mod` works, but we want this sometimes for pymodule declarations #![allow(clippy::module_inception)] From b66ed56099be7395aa5d7b2e4f7bf3c70d51cc5b Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 11 Nov 2020 20:12:04 -0600 Subject: [PATCH 4/9] Format --- vm/src/builtins/make_module.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index 00e9face5..d100c66e1 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -532,7 +532,8 @@ mod decl { } if candidates.is_empty() { - return default.ok_or_else(|| vm.new_value_error("min() arg is an empty sequence".to_owned())); + return default + .ok_or_else(|| vm.new_value_error("min() arg is an empty sequence".to_owned())); } if let Some(ref obj) = key_func { From 792aed37855a9c7bbc67abd2b9bb57a18dc3919f Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Fri, 13 Nov 2020 14:58:31 -0600 Subject: [PATCH 5/9] Refactor into `min_or_max` --- vm/src/builtins/make_module.rs | 88 +++++++--------------------------- 1 file changed, 17 insertions(+), 71 deletions(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index d100c66e1..8fb95c4c3 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -449,23 +449,19 @@ 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 - ))); + let mut key_func = args.take_keyword("key"); + + 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(), + format!("Cannot specify a default for {} with multiple positional arguments", func_name), )); } args.args @@ -482,7 +478,7 @@ mod decl { Some(x) => x, None => { return default - .ok_or_else(|| vm.new_value_error("max() arg is an empty sequence".to_owned())) + .ok_or_else(|| vm.new_value_error(format!("{} arg is an empty sequence", func_name))) } }; @@ -491,14 +487,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,65 +503,15 @@ mod decl { Ok(x) } + + #[pyfunction] + fn max(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult { + min_or_max(args, vm, "max()", PyComparisonOp::Gt) + } + #[pyfunction] fn min(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult { - let (candidates, default_allowed) = match args.args.len().cmp(&1) { - std::cmp::Ordering::Greater => (args.args.clone(), false), - std::cmp::Ordering::Equal => (vm.extract_elements(&args.args[0])?, true), - std::cmp::Ordering::Less => { - // zero arguments means type error: - return Err(vm.new_type_error("Expected 1 or more arguments".to_owned())); - } - }; - - let default = args.take_keyword("default"); - let mut key_func = args.take_keyword("key"); - - if let Some(err) = args.check_kwargs_empty(vm) { - return Err(err); - } - - if default.is_some() && !default_allowed { - return Err(vm.new_type_error( - "Specifying default not allowed with more than 1 argument".to_owned(), - )); - } - - if candidates.is_empty() { - return default - .ok_or_else(|| vm.new_value_error("min() arg is an empty sequence".to_owned())); - } - - if let Some(ref obj) = key_func { - if vm.is_none(obj) { - key_func = None - } - } - - 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] From 55332276adc27889e1397987dc10c8a3868fb8d2 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Fri, 13 Nov 2020 15:02:39 -0600 Subject: [PATCH 6/9] Format --- vm/src/builtins/make_module.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index 8fb95c4c3..ddea2d109 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -449,7 +449,12 @@ mod decl { locals.copy().into_ref(vm) } - fn min_or_max(mut args: FuncArgs, vm: &VirtualMachine, func_name: &str, op: PyComparisonOp) -> PyResult { + fn min_or_max( + mut args: FuncArgs, + vm: &VirtualMachine, + func_name: &str, + op: PyComparisonOp, + ) -> PyResult { let default = args.take_keyword("default"); let mut key_func = args.take_keyword("key"); @@ -460,9 +465,10 @@ mod decl { let candidates = match args.args.len().cmp(&1) { std::cmp::Ordering::Greater => { if default.is_some() { - return Err(vm.new_type_error( - format!("Cannot specify a default for {} with multiple positional arguments", func_name), - )); + return Err(vm.new_type_error(format!( + "Cannot specify a default for {} with multiple positional arguments", + func_name + ))); } args.args } @@ -477,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(format!("{} arg is an empty sequence", func_name))) + return default.ok_or_else(|| { + vm.new_value_error(format!("{} arg is an empty sequence", func_name)) + }) } }; @@ -503,7 +510,6 @@ mod decl { Ok(x) } - #[pyfunction] fn max(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult { min_or_max(args, vm, "max()", PyComparisonOp::Gt) From 135c0e56efbe9394c6a8fa24a24626d7bae5fde2 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Fri, 13 Nov 2020 15:20:29 -0600 Subject: [PATCH 7/9] Delete unnecessary mut --- vm/src/builtins/make_module.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index ddea2d109..71efa7e5e 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -456,7 +456,7 @@ mod decl { op: PyComparisonOp, ) -> PyResult { let default = args.take_keyword("default"); - let mut key_func = args.take_keyword("key"); + let key_func = args.take_keyword("key"); if let Some(err) = args.check_kwargs_empty(vm) { return Err(err); From a6f7bb88455a516a3718c5c15cc451dfbe00a05f Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Fri, 13 Nov 2020 19:52:59 -0600 Subject: [PATCH 8/9] Remove mut --- vm/src/builtins/make_module.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vm/src/builtins/make_module.rs b/vm/src/builtins/make_module.rs index 71efa7e5e..e093f076d 100644 --- a/vm/src/builtins/make_module.rs +++ b/vm/src/builtins/make_module.rs @@ -511,12 +511,12 @@ mod decl { } #[pyfunction] - fn max(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult { + fn max(args: FuncArgs, vm: &VirtualMachine) -> PyResult { min_or_max(args, vm, "max()", PyComparisonOp::Gt) } #[pyfunction] - fn min(mut args: FuncArgs, vm: &VirtualMachine) -> PyResult { + fn min(args: FuncArgs, vm: &VirtualMachine) -> PyResult { min_or_max(args, vm, "min()", PyComparisonOp::Lt) } From 6498d7922ddd15beddb32e61b017bc910c9c3df5 Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Sat, 14 Nov 2020 13:11:47 -0600 Subject: [PATCH 9/9] Fix clippy maybe? --- vm/src/function.rs | 5 ++--- vm/src/lib.rs | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/vm/src/function.rs b/vm/src/function.rs index 902920d9b..ab4d35fc7 100644 --- a/vm/src/function.rs +++ b/vm/src/function.rs @@ -196,10 +196,9 @@ impl FuncArgs { T::arity().end(), given_args, ))) + } else if let Some(err) = self.check_kwargs_empty(vm) { + Err(err) } else { - if let Some(err) = self.check_kwargs_empty(vm) { - return Err(err); - } Ok(bound) } } diff --git a/vm/src/lib.rs b/vm/src/lib.rs index 9204867a5..bcd00bb68 100644 --- a/vm/src/lib.rs +++ b/vm/src/lib.rs @@ -7,8 +7,6 @@ // for methods like vm.to_str(), not the typical use of 'to' as a method prefix #![allow(clippy::wrong_self_convention, clippy::implicit_hasher)] -// clippy warns on this for macro generated code rust-clippy/issues/6249 -#![allow(clippy::suspicious_else_formatting)] // to allow `mod foo {}` in foo.rs; clippy thinks this is a mistake/misunderstanding of // how `mod` works, but we want this sometimes for pymodule declarations #![allow(clippy::module_inception)]