Remove CallArguments abstraction (#6279)

## Summary

This PR removes a now-unnecessary abstraction from `helper.rs`
(`CallArguments`), in favor of adding methods to `Arguments` directly,
which helps with discoverability.
This commit is contained in:
Charlie Marsh 2023-08-02 13:25:43 -04:00 committed by GitHub
parent 8a0f844642
commit 041946fb64
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 148 additions and 209 deletions

View file

@ -528,19 +528,19 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_bandit::rules::exec_used(checker, func);
}
if checker.enabled(Rule::BadFilePermissions) {
flake8_bandit::rules::bad_file_permissions(checker, func, args, keywords);
flake8_bandit::rules::bad_file_permissions(checker, call);
}
if checker.enabled(Rule::RequestWithNoCertValidation) {
flake8_bandit::rules::request_with_no_cert_validation(checker, func, keywords);
}
if checker.enabled(Rule::UnsafeYAMLLoad) {
flake8_bandit::rules::unsafe_yaml_load(checker, func, args, keywords);
flake8_bandit::rules::unsafe_yaml_load(checker, call);
}
if checker.enabled(Rule::SnmpInsecureVersion) {
flake8_bandit::rules::snmp_insecure_version(checker, func, keywords);
}
if checker.enabled(Rule::SnmpWeakCryptography) {
flake8_bandit::rules::snmp_weak_cryptography(checker, func, args, keywords);
flake8_bandit::rules::snmp_weak_cryptography(checker, call);
}
if checker.enabled(Rule::Jinja2AutoescapeFalse) {
flake8_bandit::rules::jinja2_autoescape_false(checker, func, keywords);
@ -552,9 +552,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_bandit::rules::hardcoded_sql_expression(checker, expr);
}
if checker.enabled(Rule::HashlibInsecureHashFunction) {
flake8_bandit::rules::hashlib_insecure_hash_functions(
checker, func, args, keywords,
);
flake8_bandit::rules::hashlib_insecure_hash_functions(checker, call);
}
if checker.enabled(Rule::RequestWithoutTimeout) {
flake8_bandit::rules::request_without_timeout(checker, func, keywords);
@ -765,9 +763,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
pylint::rules::nested_min_max(checker, expr, func, args, keywords);
}
if checker.enabled(Rule::PytestPatchWithLambda) {
if let Some(diagnostic) =
flake8_pytest_style::rules::patch_with_lambda(func, args, keywords)
{
if let Some(diagnostic) = flake8_pytest_style::rules::patch_with_lambda(call) {
checker.diagnostics.push(diagnostic);
}
}
@ -788,7 +784,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_pytest_style::rules::raises_call(checker, func, args, keywords);
}
if checker.enabled(Rule::PytestFailWithoutMessage) {
flake8_pytest_style::rules::fail_call(checker, func, args, keywords);
flake8_pytest_style::rules::fail_call(checker, call);
}
if checker.enabled(Rule::PairwiseOverZipped) {
if checker.settings.target_version >= PythonVersion::Py310 {
@ -874,10 +870,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
Rule::LoggingExcInfo,
Rule::LoggingRedundantExcInfo,
]) {
flake8_logging_format::rules::logging_call(checker, func, args, keywords);
flake8_logging_format::rules::logging_call(checker, call);
}
if checker.any_enabled(&[Rule::LoggingTooFewArgs, Rule::LoggingTooManyArgs]) {
pylint::rules::logging_call(checker, func, args, keywords);
pylint::rules::logging_call(checker, call);
}
if checker.enabled(Rule::DjangoLocalsInRenderFunction) {
flake8_django::rules::locals_in_render_function(checker, func, args, keywords);

View file

@ -1,10 +1,9 @@
use num_traits::ToPrimitive;
use ruff_python_ast::{self as ast, Constant, Expr, Keyword, Operator, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::CallPath;
use ruff_python_ast::helpers::CallArguments;
use ruff_python_ast::{self as ast, Constant, Expr, Operator, Ranged};
use ruff_python_semantic::SemanticModel;
use crate::checkers::ast::Checker;
@ -48,19 +47,13 @@ impl Violation for BadFilePermissions {
}
/// S103
pub(crate) fn bad_file_permissions(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
pub(crate) fn bad_file_permissions(checker: &mut Checker, call: &ast::ExprCall) {
if checker
.semantic()
.resolve_call_path(func)
.resolve_call_path(&call.func)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["os", "chmod"]))
{
let call_args = CallArguments::new(args, keywords);
if let Some(mode_arg) = call_args.argument("mode", 1) {
if let Some(mode_arg) = call.arguments.find_argument("mode", 1) {
if let Some(int_value) = int_value(mode_arg, checker.semantic()) {
if (int_value & WRITE_WORLD > 0) || (int_value & EXECUTE_GROUP > 0) {
checker.diagnostics.push(Diagnostic::new(

View file

@ -1,8 +1,7 @@
use ruff_python_ast::{Expr, Keyword, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::{find_keyword, is_const_false, CallArguments};
use ruff_python_ast::helpers::is_const_false;
use ruff_python_ast::{self as ast, Arguments, Ranged};
use crate::checkers::ast::Checker;
@ -60,15 +59,11 @@ impl Violation for HashlibInsecureHashFunction {
}
/// S324
pub(crate) fn hashlib_insecure_hash_functions(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
if let Some(hashlib_call) = checker
pub(crate) fn hashlib_insecure_hash_functions(checker: &mut Checker, call: &ast::ExprCall) {
if let Some(hashlib_call) =
checker
.semantic()
.resolve_call_path(func)
.resolve_call_path(&call.func)
.and_then(|call_path| match call_path.as_slice() {
["hashlib", "new"] => Some(HashlibCall::New),
["hashlib", "md4"] => Some(HashlibCall::WeakHash("md4")),
@ -78,12 +73,12 @@ pub(crate) fn hashlib_insecure_hash_functions(
_ => None,
})
{
if !is_used_for_security(keywords) {
if !is_used_for_security(&call.arguments) {
return;
}
match hashlib_call {
HashlibCall::New => {
if let Some(name_arg) = CallArguments::new(args, keywords).argument("name", 0) {
if let Some(name_arg) = call.arguments.find_argument("name", 0) {
if let Some(hash_func_name) = string_literal(name_arg) {
// `hashlib.new` accepts both lowercase and uppercase names for hash
// functions.
@ -106,15 +101,16 @@ pub(crate) fn hashlib_insecure_hash_functions(
HashlibInsecureHashFunction {
string: (*func_name).to_string(),
},
func.range(),
call.func.range(),
));
}
}
}
}
fn is_used_for_security(keywords: &[Keyword]) -> bool {
find_keyword(keywords, "usedforsecurity")
fn is_used_for_security(arguments: &Arguments) -> bool {
arguments
.find_keyword("usedforsecurity")
.map_or(true, |keyword| !is_const_false(&keyword.value))
}

View file

@ -1,8 +1,6 @@
use ruff_python_ast::{Expr, Keyword, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::CallArguments;
use ruff_python_ast::{self as ast, Ranged};
use crate::checkers::ast::Checker;
@ -42,21 +40,18 @@ impl Violation for SnmpWeakCryptography {
}
/// S509
pub(crate) fn snmp_weak_cryptography(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
pub(crate) fn snmp_weak_cryptography(checker: &mut Checker, call: &ast::ExprCall) {
if call.arguments.len() < 3 {
if checker
.semantic()
.resolve_call_path(func)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["pysnmp", "hlapi", "UsmUserData"]))
.resolve_call_path(&call.func)
.is_some_and(|call_path| {
matches!(call_path.as_slice(), ["pysnmp", "hlapi", "UsmUserData"])
})
{
if CallArguments::new(args, keywords).len() < 3 {
checker
.diagnostics
.push(Diagnostic::new(SnmpWeakCryptography, func.range()));
.push(Diagnostic::new(SnmpWeakCryptography, call.func.range()));
}
}
}

View file

@ -1,8 +1,6 @@
use ruff_python_ast::{self as ast, Expr, Keyword, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::CallArguments;
use ruff_python_ast::{self as ast, Expr, Ranged};
use crate::checkers::ast::Checker;
@ -60,19 +58,13 @@ impl Violation for UnsafeYAMLLoad {
}
/// S506
pub(crate) fn unsafe_yaml_load(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
pub(crate) fn unsafe_yaml_load(checker: &mut Checker, call: &ast::ExprCall) {
if checker
.semantic()
.resolve_call_path(func)
.resolve_call_path(&call.func)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["yaml", "load"]))
{
let call_args = CallArguments::new(args, keywords);
if let Some(loader_arg) = call_args.argument("Loader", 1) {
if let Some(loader_arg) = call.arguments.find_argument("Loader", 1) {
if !checker
.semantic()
.resolve_call_path(loader_arg)
@ -93,7 +85,7 @@ pub(crate) fn unsafe_yaml_load(
} else {
checker.diagnostics.push(Diagnostic::new(
UnsafeYAMLLoad { loader: None },
func.range(),
call.func.range(),
));
}
}

View file

@ -1,7 +1,5 @@
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Keyword, Operator, Ranged};
use ruff_diagnostics::{Diagnostic, Edit, Fix};
use ruff_python_ast::helpers::{find_keyword, CallArguments};
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Keyword, Operator, Ranged};
use ruff_python_semantic::analyze::logging;
use ruff_python_stdlib::logging::LoggingLevel;
@ -153,13 +151,8 @@ impl LoggingCallType {
}
/// Check logging calls for violations.
pub(crate) fn logging_call(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = func else {
pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) {
let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = call.func.as_ref() else {
return;
};
@ -167,13 +160,17 @@ pub(crate) fn logging_call(
return;
};
if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
if !logging::is_logger_candidate(
&call.func,
checker.semantic(),
&checker.settings.logger_objects,
) {
return;
}
// G001 - G004
let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall));
if let Some(format_arg) = CallArguments::new(args, keywords).argument("msg", msg_pos) {
if let Some(format_arg) = call.arguments.find_argument("msg", msg_pos) {
check_msg(checker, format_arg);
}
@ -196,7 +193,7 @@ pub(crate) fn logging_call(
// G101
if checker.enabled(Rule::LoggingExtraAttrClash) {
if let Some(extra) = find_keyword(keywords, "extra") {
if let Some(extra) = call.arguments.find_keyword("extra") {
check_log_record_attr_clash(checker, extra);
}
}
@ -206,7 +203,7 @@ pub(crate) fn logging_call(
if !checker.semantic().in_exception_handler() {
return;
}
let Some(exc_info) = logging::exc_info(keywords, checker.semantic()) else {
let Some(exc_info) = logging::exc_info(&call.arguments, checker.semantic()) else {
return;
};
if let LoggingCallType::LevelCall(logging_level) = logging_call_type {

View file

@ -1,8 +1,6 @@
use ruff_python_ast::{Expr, Keyword, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::CallArguments;
use ruff_python_ast::{self as ast, Ranged};
use crate::checkers::ast::Checker;
@ -56,26 +54,25 @@ impl Violation for PytestFailWithoutMessage {
}
}
pub(crate) fn fail_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) {
if is_pytest_fail(func, checker.semantic()) {
let call_args = CallArguments::new(args, keywords);
pub(crate) fn fail_call(checker: &mut Checker, call: &ast::ExprCall) {
if is_pytest_fail(&call.func, checker.semantic()) {
// Allow either `pytest.fail(reason="...")` (introduced in pytest 7.0) or
// `pytest.fail(msg="...")` (deprecated in pytest 7.0)
let msg = call_args
.argument("reason", 0)
.or_else(|| call_args.argument("msg", 0));
let msg = call
.arguments
.find_argument("reason", 0)
.or_else(|| call.arguments.find_argument("msg", 0));
if let Some(msg) = msg {
if is_empty_or_null_string(msg) {
checker
.diagnostics
.push(Diagnostic::new(PytestFailWithoutMessage, func.range()));
.push(Diagnostic::new(PytestFailWithoutMessage, call.func.range()));
}
} else {
checker
.diagnostics
.push(Diagnostic::new(PytestFailWithoutMessage, func.range()));
.push(Diagnostic::new(PytestFailWithoutMessage, call.func.range()));
}
}
}

View file

@ -1,11 +1,10 @@
use ruff_python_ast::{self as ast, Expr, Keyword, Parameters, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::collect_call_path;
use ruff_python_ast::helpers::{find_keyword, includes_arg_name, CallArguments};
use ruff_python_ast::helpers::includes_arg_name;
use ruff_python_ast::visitor;
use ruff_python_ast::visitor::Visitor;
use ruff_python_ast::{self as ast, Expr, Parameters, Ranged};
#[violation]
pub struct PytestPatchWithLambda;
@ -44,13 +43,8 @@ where
}
}
fn check_patch_call(
call: &Expr,
args: &[Expr],
keywords: &[Keyword],
new_arg_number: usize,
) -> Option<Diagnostic> {
if find_keyword(keywords, "return_value").is_some() {
fn check_patch_call(call: &ast::ExprCall, index: usize) -> Option<Diagnostic> {
if call.arguments.find_keyword("return_value").is_some() {
return None;
}
@ -58,8 +52,9 @@ fn check_patch_call(
parameters,
body,
range: _,
} = CallArguments::new(args, keywords)
.argument("new", new_arg_number)?
} = call
.arguments
.find_argument("new", index)?
.as_lambda_expr()?;
// Walk the lambda body.
@ -72,16 +67,13 @@ fn check_patch_call(
if visitor.uses_args {
None
} else {
Some(Diagnostic::new(PytestPatchWithLambda, call.range()))
Some(Diagnostic::new(PytestPatchWithLambda, call.func.range()))
}
}
pub(crate) fn patch_with_lambda(
call: &Expr,
args: &[Expr],
keywords: &[Keyword],
) -> Option<Diagnostic> {
let call_path = collect_call_path(call)?;
/// PT008
pub(crate) fn patch_with_lambda(call: &ast::ExprCall) -> Option<Diagnostic> {
let call_path = collect_call_path(&call.func)?;
if matches!(
call_path.as_slice(),
@ -95,7 +87,7 @@ pub(crate) fn patch_with_lambda(
"patch"
] | ["unittest", "mock", "patch"]
) {
check_patch_call(call, args, keywords, 1)
check_patch_call(call, 1)
} else if matches!(
call_path.as_slice(),
[
@ -109,7 +101,7 @@ pub(crate) fn patch_with_lambda(
"object"
] | ["unittest", "mock", "patch", "object"]
) {
check_patch_call(call, args, keywords, 2)
check_patch_call(call, 2)
} else {
None
}

View file

@ -1,8 +1,6 @@
use ruff_python_ast::{self as ast, Constant, Expr, Keyword, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::CallArguments;
use ruff_python_ast::{self as ast, Constant, Expr, Ranged};
use ruff_python_semantic::analyze::logging;
use ruff_python_stdlib::logging::LoggingLevel;
@ -86,23 +84,23 @@ impl Violation for LoggingTooManyArgs {
/// PLE1205
/// PLE1206
pub(crate) fn logging_call(
checker: &mut Checker,
func: &Expr,
args: &[Expr],
keywords: &[Keyword],
) {
pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) {
// If there are any starred arguments, abort.
if args.iter().any(Expr::is_starred_expr) {
if call.arguments.args.iter().any(Expr::is_starred_expr) {
return;
}
// If there are any starred keyword arguments, abort.
if keywords.iter().any(|keyword| keyword.arg.is_none()) {
if call
.arguments
.keywords
.iter()
.any(|keyword| keyword.arg.is_none())
{
return;
}
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func else {
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = call.func.as_ref() else {
return;
};
@ -110,16 +108,19 @@ pub(crate) fn logging_call(
return;
}
let call_args = CallArguments::new(args, keywords);
let Some(Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
..
})) = call_args.argument("msg", 0)
})) = call.arguments.find_argument("msg", 0)
else {
return;
};
if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
if !logging::is_logger_candidate(
&call.func,
checker.semantic(),
&checker.settings.logger_objects,
) {
return;
}
@ -135,22 +136,22 @@ pub(crate) fn logging_call(
return;
}
let message_args = call_args.num_args() - 1;
let num_message_args = call.arguments.args.len() - 1;
let num_keywords = call.arguments.keywords.len();
if checker.enabled(Rule::LoggingTooManyArgs) {
if summary.num_positional < message_args {
if summary.num_positional < num_message_args {
checker
.diagnostics
.push(Diagnostic::new(LoggingTooManyArgs, func.range()));
.push(Diagnostic::new(LoggingTooManyArgs, call.func.range()));
}
}
if checker.enabled(Rule::LoggingTooFewArgs) {
if message_args > 0 && call_args.num_kwargs() == 0 && summary.num_positional > message_args
{
if num_message_args > 0 && num_keywords == 0 && summary.num_positional > num_message_args {
checker
.diagnostics
.push(Diagnostic::new(LoggingTooFewArgs, func.range()));
.push(Diagnostic::new(LoggingTooFewArgs, call.func.range()));
}
}
}

View file

@ -66,7 +66,7 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
for expr in calls {
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() {
if attr == "error" {
if exc_info(&expr.arguments.keywords, checker.semantic()).is_none() {
if exc_info(&expr.arguments, checker.semantic()).is_none() {
checker
.diagnostics
.push(Diagnostic::new(ErrorInsteadOfException, expr.range()));

View file

@ -1034,64 +1034,6 @@ pub fn is_docstring_stmt(stmt: &Stmt) -> bool {
}
}
/// A representation of a function call's positional and keyword arguments that ignores
/// starred expressions.
#[derive(Default)]
pub struct CallArguments<'a> {
args: &'a [Expr],
keywords: &'a [Keyword],
}
impl<'a> CallArguments<'a> {
pub fn new(args: &'a [Expr], keywords: &'a [Keyword]) -> Self {
Self { args, keywords }
}
/// Get the argument with the given name or position, or `None` if no such
/// argument exists.
pub fn argument(&self, name: &str, position: usize) -> Option<&'a Expr> {
self.keywords
.iter()
.find(|keyword| {
let Keyword { arg, .. } = keyword;
arg.as_ref().is_some_and(|arg| arg == name)
})
.map(|keyword| &keyword.value)
.or_else(|| {
self.args
.iter()
.take_while(|expr| !expr.is_starred_expr())
.nth(position)
})
}
/// Return the number of arguments.
pub fn len(&self) -> usize {
self.args.len() + self.keywords.len()
}
/// Return `true` if there are no arguments.
pub fn is_empty(&self) -> bool {
self.len() == 0
}
/// Return the number of positional arguments.
pub fn num_args(&self) -> usize {
self.args
.iter()
.take_while(|expr| !expr.is_starred_expr())
.count()
}
/// Return the number of keyword arguments.
pub fn num_kwargs(&self) -> usize {
self.keywords
.iter()
.filter(|keyword| keyword.arg.is_some())
.count()
}
}
/// Check if a node is part of a conditional branch.
pub fn on_conditional_branch<'a>(parents: &mut impl Iterator<Item = &'a Stmt>) -> bool {
parents.any(|parent| {

View file

@ -2121,6 +2121,45 @@ pub struct Arguments {
pub keywords: Vec<Keyword>,
}
impl Arguments {
/// Return the number of positional and keyword arguments.
pub fn len(&self) -> usize {
self.args.len() + self.keywords.len()
}
/// Return `true` if there are no positional or keyword arguments.
pub fn is_empty(&self) -> bool {
self.len() == 0
}
/// Return the [`Keyword`] with the given name, or `None` if no such [`Keyword`] exists.
pub fn find_keyword(&self, keyword_name: &str) -> Option<&Keyword> {
self.keywords.iter().find(|keyword| {
let Keyword { arg, .. } = keyword;
arg.as_ref().is_some_and(|arg| arg == keyword_name)
})
}
/// Return the argument with the given name or at the given position, or `None` if no such
/// argument exists. Used to retrieve arguments that can be provided _either_ as keyword or
/// positional arguments.
pub fn find_argument(&self, name: &str, position: usize) -> Option<&Expr> {
self.keywords
.iter()
.find(|keyword| {
let Keyword { arg, .. } = keyword;
arg.as_ref().is_some_and(|arg| arg == name)
})
.map(|keyword| &keyword.value)
.or_else(|| {
self.args
.iter()
.take_while(|expr| !expr.is_starred_expr())
.nth(position)
})
}
}
/// An AST node used to represent a sequence of type parameters.
///
/// For example, given:

View file

@ -1,7 +1,6 @@
use ruff_python_ast::{self as ast, Expr, Keyword};
use ruff_python_ast::call_path::{collect_call_path, from_qualified_name};
use ruff_python_ast::helpers::{find_keyword, is_const_true};
use ruff_python_ast::helpers::is_const_true;
use ruff_python_ast::{self as ast, Arguments, Expr, Keyword};
use crate::model::SemanticModel;
@ -61,8 +60,8 @@ pub fn is_logger_candidate(
/// If the keywords to a logging call contain `exc_info=True` or `exc_info=sys.exc_info()`,
/// return the `Keyword` for `exc_info`.
pub fn exc_info<'a>(keywords: &'a [Keyword], semantic: &SemanticModel) -> Option<&'a Keyword> {
let exc_info = find_keyword(keywords, "exc_info")?;
pub fn exc_info<'a>(arguments: &'a Arguments, semantic: &SemanticModel) -> Option<&'a Keyword> {
let exc_info = arguments.find_keyword("exc_info")?;
// Ex) `logging.error("...", exc_info=True)`
if is_const_true(&exc_info.value) {