From 685c2427612ef504e5c3e83b46a2c1b51b6dc14d Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 13 Mar 2023 18:18:25 +0100 Subject: [PATCH] refactor(ruff_python_ast): Split `get_argument` (#3478) --- .../rules/bad_file_permissions.rs | 2 +- .../rules/hashlib_insecure_hash_functions.rs | 4 +- .../rules/jinja2_autoescape_false.rs | 2 +- .../rules/logging_config_insecure_listen.rs | 2 +- .../rules/request_with_no_cert_validation.rs | 2 +- .../rules/request_without_timeout.rs | 2 +- .../rules/snmp_insecure_version.rs | 2 +- .../flake8_bandit/rules/unsafe_yaml_load.rs | 2 +- .../src/rules/flake8_logging_format/rules.rs | 2 +- .../rules/flake8_pytest_style/rules/fail.rs | 2 +- .../rules/flake8_pytest_style/rules/patch.rs | 4 +- crates/ruff/src/rules/pylint/rules/logging.rs | 2 +- crates/ruff_python_ast/src/helpers.rs | 53 +++++++++---------- 13 files changed, 39 insertions(+), 42 deletions(-) diff --git a/crates/ruff/src/rules/flake8_bandit/rules/bad_file_permissions.rs b/crates/ruff/src/rules/flake8_bandit/rules/bad_file_permissions.rs index 9544fd2aab..e529f25c15 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/bad_file_permissions.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/bad_file_permissions.rs @@ -108,7 +108,7 @@ pub fn bad_file_permissions( .map_or(false, |call_path| call_path.as_slice() == ["os", "chmod"]) { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(mode_arg) = call_args.get_argument("mode", Some(1)) { + if let Some(mode_arg) = call_args.argument("mode", 1) { if let Some(int_value) = get_int_value(mode_arg) { if (int_value & WRITE_WORLD > 0) || (int_value & EXECUTE_GROUP > 0) { checker.diagnostics.push(Diagnostic::new( diff --git a/crates/ruff/src/rules/flake8_bandit/rules/hashlib_insecure_hash_functions.rs b/crates/ruff/src/rules/flake8_bandit/rules/hashlib_insecure_hash_functions.rs index 8133ba54c4..63ae4a8e18 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/hashlib_insecure_hash_functions.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/hashlib_insecure_hash_functions.rs @@ -25,7 +25,7 @@ impl Violation for HashlibInsecureHashFunction { const WEAK_HASHES: [&str; 4] = ["md4", "md5", "sha", "sha1"]; fn is_used_for_security(call_args: &SimpleCallArgs) -> bool { - match call_args.get_argument("usedforsecurity", None) { + match call_args.keyword_argument("usedforsecurity") { Some(expr) => !matches!( &expr.node, ExprKind::Constant { @@ -67,7 +67,7 @@ pub fn hashlib_insecure_hash_functions( return; } - if let Some(name_arg) = call_args.get_argument("name", Some(0)) { + if let Some(name_arg) = call_args.argument("name", 0) { if let Some(hash_func_name) = string_literal(name_arg) { if WEAK_HASHES.contains(&hash_func_name.to_lowercase().as_str()) { checker.diagnostics.push(Diagnostic::new( diff --git a/crates/ruff/src/rules/flake8_bandit/rules/jinja2_autoescape_false.rs b/crates/ruff/src/rules/flake8_bandit/rules/jinja2_autoescape_false.rs index d192bafc15..3670759b81 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/jinja2_autoescape_false.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/jinja2_autoescape_false.rs @@ -46,7 +46,7 @@ pub fn jinja2_autoescape_false( { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(autoescape_arg) = call_args.get_argument("autoescape", None) { + if let Some(autoescape_arg) = call_args.keyword_argument("autoescape") { match &autoescape_arg.node { ExprKind::Constant { value: Constant::Bool(true), diff --git a/crates/ruff/src/rules/flake8_bandit/rules/logging_config_insecure_listen.rs b/crates/ruff/src/rules/flake8_bandit/rules/logging_config_insecure_listen.rs index 1617a559d2..e216732c60 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/logging_config_insecure_listen.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/logging_config_insecure_listen.rs @@ -33,7 +33,7 @@ pub fn logging_config_insecure_listen( { let call_args = SimpleCallArgs::new(args, keywords); - if call_args.get_argument("verify", None).is_none() { + if call_args.keyword_argument("verify").is_none() { checker.diagnostics.push(Diagnostic::new( LoggingConfigInsecureListen, Range::from(func), diff --git a/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs b/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs index 2a954699b7..bf2f2138e1 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/request_with_no_cert_validation.rs @@ -56,7 +56,7 @@ pub fn request_with_no_cert_validation( None }) { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(verify_arg) = call_args.get_argument("verify", None) { + if let Some(verify_arg) = call_args.keyword_argument("verify") { if let ExprKind::Constant { value: Constant::Bool(false), .. diff --git a/crates/ruff/src/rules/flake8_bandit/rules/request_without_timeout.rs b/crates/ruff/src/rules/flake8_bandit/rules/request_without_timeout.rs index 00926f0307..18d4c6cee8 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/request_without_timeout.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/request_without_timeout.rs @@ -44,7 +44,7 @@ pub fn request_without_timeout( }) { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(timeout_arg) = call_args.get_argument("timeout", None) { + if let Some(timeout_arg) = call_args.keyword_argument("timeout") { if let Some(timeout) = match &timeout_arg.node { ExprKind::Constant { value: value @ Constant::None, diff --git a/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs b/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs index af57325743..9525c78d0e 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/snmp_insecure_version.rs @@ -33,7 +33,7 @@ pub fn snmp_insecure_version( }) { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(mp_model_arg) = call_args.get_argument("mpModel", None) { + if let Some(mp_model_arg) = call_args.keyword_argument("mpModel") { if let ExprKind::Constant { value: Constant::Int(value), .. diff --git a/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs b/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs index e748078621..9ec7019c8c 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/unsafe_yaml_load.rs @@ -39,7 +39,7 @@ pub fn unsafe_yaml_load(checker: &mut Checker, func: &Expr, args: &[Expr], keywo .map_or(false, |call_path| call_path.as_slice() == ["yaml", "load"]) { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(loader_arg) = call_args.get_argument("Loader", Some(1)) { + if let Some(loader_arg) = call_args.argument("Loader", 1) { if !checker .ctx .resolve_call_path(loader_arg) diff --git a/crates/ruff/src/rules/flake8_logging_format/rules.rs b/crates/ruff/src/rules/flake8_logging_format/rules.rs index 45797a2e66..3e6b8f7ff8 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules.rs +++ b/crates/ruff/src/rules/flake8_logging_format/rules.rs @@ -146,7 +146,7 @@ pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: ); // G001 - G004 - if let Some(format_arg) = call_args.get_argument("msg", Some(0)) { + if let Some(format_arg) = call_args.argument("msg", 0) { check_msg(checker, format_arg); } diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/fail.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/fail.rs index a43c5ce1f4..d9f94bd052 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/fail.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/fail.rs @@ -22,7 +22,7 @@ impl Violation for FailWithoutMessage { pub fn fail_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { if is_pytest_fail(func, checker) { let call_args = SimpleCallArgs::new(args, keywords); - let msg = call_args.get_argument("msg", Some(0)); + let msg = call_args.argument("msg", 0); if let Some(msg) = msg { if is_empty_or_null_string(msg) { diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs index 6c4b81754d..b5491e28a7 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/patch.rs @@ -68,11 +68,11 @@ fn check_patch_call( new_arg_number: usize, ) -> Option { let simple_args = SimpleCallArgs::new(args, keywords); - if simple_args.get_argument("return_value", None).is_some() { + if simple_args.keyword_argument("return_value").is_some() { return None; } - if let Some(new_arg) = simple_args.get_argument("new", Some(new_arg_number)) { + if let Some(new_arg) = simple_args.argument("new", new_arg_number) { if let ExprKind::Lambda { args, body } = &new_arg.node { // Walk the lambda body. let mut visitor = LambdaBodyVisitor { diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs index db390314dd..6966b24a2e 100644 --- a/crates/ruff/src/rules/pylint/rules/logging.rs +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -107,7 +107,7 @@ pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: if let ExprKind::Attribute { attr, .. } = &func.node { if LoggingLevel::from_attribute(attr.as_str()).is_some() { let call_args = SimpleCallArgs::new(args, keywords); - if let Some(msg) = call_args.get_argument("msg", Some(0)) { + if let Some(msg) = call_args.argument("msg", 0) { if let ExprKind::Constant { value: Constant::Str(value), .. diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 039d31cd8f..9070609d82 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1213,42 +1213,39 @@ pub struct SimpleCallArgs<'a> { } impl<'a> SimpleCallArgs<'a> { - pub fn new(args: &'a [Expr], keywords: &'a [Keyword]) -> Self { - let mut result = SimpleCallArgs::default(); + pub fn new( + args: impl IntoIterator, + keywords: impl IntoIterator, + ) -> Self { + let args = args + .into_iter() + .take_while(|arg| !matches!(arg.node, ExprKind::Starred { .. })) + .collect(); - for arg in args { - match &arg.node { - ExprKind::Starred { .. } => { - break; - } - _ => { - result.args.push(arg); - } - } - } + let kwargs = keywords + .into_iter() + .filter_map(|keyword| { + let node = &keyword.node; + node.arg.as_ref().map(|arg| (arg.as_ref(), &node.value)) + }) + .collect(); - for keyword in keywords { - if let Some(arg) = &keyword.node.arg { - result.kwargs.insert(arg, &keyword.node.value); - } - } + SimpleCallArgs { args, kwargs } + } - result + /// Get the argument with the given name. + /// If the argument is not found by name, return + /// `None`. + pub fn keyword_argument(&self, name: &str) -> Option<&'a Expr> { + self.kwargs.get(name).copied() } /// Get the argument with the given name or position. /// If the argument is not found with either name or position, return /// `None`. - pub fn get_argument(&self, name: &'a str, position: Option) -> Option<&'a Expr> { - if let Some(kwarg) = self.kwargs.get(name) { - return Some(kwarg); - } - if let Some(position) = position { - if position < self.args.len() { - return Some(self.args[position]); - } - } - None + pub fn argument(&self, name: &str, position: usize) -> Option<&'a Expr> { + self.keyword_argument(name) + .or_else(|| self.args.get(position).copied()) } /// Return the number of positional and keyword arguments.