From ca497fabbd970a557ed1b45acccaa714e12573af Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 3 Jul 2023 12:47:23 -0400 Subject: [PATCH] Remove some `diagnostics.extend` calls (#5483) ## Summary It's more efficient (and more idiomatic for us) to pass in the `Checker` directly. --- crates/ruff/src/checkers/ast/mod.rs | 40 ++++++------------- .../rules/hardcoded_password_default.rs | 9 ++--- .../rules/hardcoded_password_func_arg.rs | 12 +++--- .../rules/hardcoded_password_string.rs | 21 +++++----- .../rules/non_leading_receiver_decorator.rs | 28 +++++-------- .../rules/f_string_in_gettext_func_call.rs | 9 +++-- .../rules/format_in_gettext_func_call.rs | 9 +++-- .../rules/printf_in_gettext_func_call.rs | 8 ++-- .../flake8_pytest_style/rules/assertion.rs | 11 +++-- .../ruff/src/rules/flake8_return/visitor.rs | 18 ++++----- 10 files changed, 75 insertions(+), 90 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 7abc449cdb690..257dcdcf1781b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -359,11 +359,7 @@ where .. }) => { if self.enabled(Rule::DjangoNonLeadingReceiverDecorator) { - self.diagnostics - .extend(flake8_django::rules::non_leading_receiver_decorator( - decorator_list, - |expr| self.semantic.resolve_call_path(expr), - )); + flake8_django::rules::non_leading_receiver_decorator(self, decorator_list); } if self.enabled(Rule::AmbiguousFunctionName) { if let Some(diagnostic) = @@ -505,8 +501,7 @@ where } } if self.enabled(Rule::HardcodedPasswordDefault) { - self.diagnostics - .extend(flake8_bandit::rules::hardcoded_password_default(args)); + flake8_bandit::rules::hardcoded_password_default(self, args); } if self.enabled(Rule::PropertyWithParameters) { pylint::rules::property_with_parameters(self, stmt, decorator_list, args); @@ -1573,9 +1568,7 @@ where pyupgrade::rules::os_error_alias_handlers(self, handlers); } if self.enabled(Rule::PytestAssertInExcept) { - self.diagnostics.extend( - flake8_pytest_style::rules::assert_in_exception_handler(handlers), - ); + flake8_pytest_style::rules::assert_in_exception_handler(self, handlers); } if self.enabled(Rule::SuppressibleException) { flake8_simplify::rules::suppressible_exception( @@ -1616,11 +1609,7 @@ where flake8_bugbear::rules::assignment_to_os_environ(self, targets); } if self.enabled(Rule::HardcodedPasswordString) { - if let Some(diagnostic) = - flake8_bandit::rules::assign_hardcoded_password_string(value, targets) - { - self.diagnostics.push(diagnostic); - } + flake8_bandit::rules::assign_hardcoded_password_string(self, value, targets); } if self.enabled(Rule::GlobalStatement) { for target in targets.iter() { @@ -2615,8 +2604,7 @@ where flake8_bandit::rules::jinja2_autoescape_false(self, func, args, keywords); } if self.enabled(Rule::HardcodedPasswordFuncArg) { - self.diagnostics - .extend(flake8_bandit::rules::hardcoded_password_func_arg(keywords)); + flake8_bandit::rules::hardcoded_password_func_arg(self, keywords); } if self.enabled(Rule::HardcodedSQLExpression) { flake8_bandit::rules::hardcoded_sql_expression(self, expr); @@ -2871,16 +2859,13 @@ where &self.settings.flake8_gettext.functions_names, ) { if self.enabled(Rule::FStringInGetTextFuncCall) { - self.diagnostics - .extend(flake8_gettext::rules::f_string_in_gettext_func_call(args)); + flake8_gettext::rules::f_string_in_gettext_func_call(self, args); } if self.enabled(Rule::FormatInGetTextFuncCall) { - self.diagnostics - .extend(flake8_gettext::rules::format_in_gettext_func_call(args)); + flake8_gettext::rules::format_in_gettext_func_call(self, args); } if self.enabled(Rule::PrintfInGetTextFuncCall) { - self.diagnostics - .extend(flake8_gettext::rules::printf_in_gettext_func_call(args)); + flake8_gettext::rules::printf_in_gettext_func_call(self, args); } } if self.enabled(Rule::UncapitalizedEnvironmentVariables) { @@ -3221,11 +3206,10 @@ where flake8_2020::rules::compare(self, left, ops, comparators); } if self.enabled(Rule::HardcodedPasswordString) { - self.diagnostics.extend( - flake8_bandit::rules::compare_to_hardcoded_password_string( - left, - comparators, - ), + flake8_bandit::rules::compare_to_hardcoded_password_string( + self, + left, + comparators, ); } if self.enabled(Rule::ComparisonWithItself) { diff --git a/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_password_default.rs b/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_password_default.rs index 127ce31199cb6..50be800d9ea50 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_password_default.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/hardcoded_password_default.rs @@ -1,5 +1,6 @@ use rustpython_parser::ast::{Arg, ArgWithDefault, Arguments, Expr, Ranged}; +use crate::checkers::ast::Checker; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -36,9 +37,7 @@ fn check_password_kwarg(arg: &Arg, default: &Expr) -> Option { } /// S107 -pub(crate) fn hardcoded_password_default(arguments: &Arguments) -> Vec { - let mut diagnostics: Vec = Vec::new(); - +pub(crate) fn hardcoded_password_default(checker: &mut Checker, arguments: &Arguments) { for ArgWithDefault { def, default, @@ -53,9 +52,7 @@ pub(crate) fn hardcoded_password_default(arguments: &Arguments) -> Vec Vec { - keywords - .iter() - .filter_map(|keyword| { +pub(crate) fn hardcoded_password_func_arg(checker: &mut Checker, keywords: &[Keyword]) { + checker + .diagnostics + .extend(keywords.iter().filter_map(|keyword| { string_literal(&keyword.value).filter(|string| !string.is_empty())?; let arg = keyword.arg.as_ref()?; if !matches_password_name(arg) { @@ -37,6 +38,5 @@ pub(crate) fn hardcoded_password_func_arg(keywords: &[Keyword]) -> Vec Option<&str> { /// S105 pub(crate) fn compare_to_hardcoded_password_string( + checker: &mut Checker, left: &Expr, comparators: &[Expr], -) -> Vec { - comparators - .iter() - .filter_map(|comp| { +) { + checker + .diagnostics + .extend(comparators.iter().filter_map(|comp| { string_literal(comp).filter(|string| !string.is_empty())?; let Some(name) = password_target(left) else { return None; @@ -63,29 +66,29 @@ pub(crate) fn compare_to_hardcoded_password_string( }, comp.range(), )) - }) - .collect() + })); } /// S105 pub(crate) fn assign_hardcoded_password_string( + checker: &mut Checker, value: &Expr, targets: &[Expr], -) -> Option { +) { if string_literal(value) .filter(|string| !string.is_empty()) .is_some() { for target in targets { if let Some(name) = password_target(target) { - return Some(Diagnostic::new( + checker.diagnostics.push(Diagnostic::new( HardcodedPasswordString { name: name.to_string(), }, value.range(), )); + return; } } } - None } diff --git a/crates/ruff/src/rules/flake8_django/rules/non_leading_receiver_decorator.rs b/crates/ruff/src/rules/flake8_django/rules/non_leading_receiver_decorator.rs index f023431404a92..9e823858c0688 100644 --- a/crates/ruff/src/rules/flake8_django/rules/non_leading_receiver_decorator.rs +++ b/crates/ruff/src/rules/flake8_django/rules/non_leading_receiver_decorator.rs @@ -1,8 +1,9 @@ -use rustpython_parser::ast::{self, Decorator, Expr, Ranged}; +use rustpython_parser::ast::{Decorator, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::call_path::CallPath; + +use crate::checkers::ast::Checker; /// ## What it does /// Checks that Django's `@receiver` decorator is listed first, prior to @@ -48,25 +49,19 @@ impl Violation for DjangoNonLeadingReceiverDecorator { } /// DJ013 -pub(crate) fn non_leading_receiver_decorator<'a, F>( - decorator_list: &'a [Decorator], - resolve_call_path: F, -) -> Vec -where - F: Fn(&'a Expr) -> Option>, -{ - let mut diagnostics = vec![]; +pub(crate) fn non_leading_receiver_decorator(checker: &mut Checker, decorator_list: &[Decorator]) { let mut seen_receiver = false; for (i, decorator) in decorator_list.iter().enumerate() { - let is_receiver = match &decorator.expression { - Expr::Call(ast::ExprCall { func, .. }) => resolve_call_path(func) + let is_receiver = decorator.expression.as_call_expr().map_or(false, |call| { + checker + .semantic() + .resolve_call_path(&call.func) .map_or(false, |call_path| { matches!(call_path.as_slice(), ["django", "dispatch", "receiver"]) - }), - _ => false, - }; + }) + }); if i > 0 && is_receiver && !seen_receiver { - diagnostics.push(Diagnostic::new( + checker.diagnostics.push(Diagnostic::new( DjangoNonLeadingReceiverDecorator, decorator.range(), )); @@ -77,5 +72,4 @@ where seen_receiver = true; } } - diagnostics } diff --git a/crates/ruff/src/rules/flake8_gettext/rules/f_string_in_gettext_func_call.rs b/crates/ruff/src/rules/flake8_gettext/rules/f_string_in_gettext_func_call.rs index 0f6b8c6a6021f..89957a02ac3ce 100644 --- a/crates/ruff/src/rules/flake8_gettext/rules/f_string_in_gettext_func_call.rs +++ b/crates/ruff/src/rules/flake8_gettext/rules/f_string_in_gettext_func_call.rs @@ -3,6 +3,8 @@ use rustpython_parser::ast::{Expr, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use crate::checkers::ast::Checker; + #[violation] pub struct FStringInGetTextFuncCall; @@ -14,11 +16,12 @@ impl Violation for FStringInGetTextFuncCall { } /// INT001 -pub(crate) fn f_string_in_gettext_func_call(args: &[Expr]) -> Option { +pub(crate) fn f_string_in_gettext_func_call(checker: &mut Checker, args: &[Expr]) { if let Some(first) = args.first() { if first.is_joined_str_expr() { - return Some(Diagnostic::new(FStringInGetTextFuncCall {}, first.range())); + checker + .diagnostics + .push(Diagnostic::new(FStringInGetTextFuncCall {}, first.range())); } } - None } diff --git a/crates/ruff/src/rules/flake8_gettext/rules/format_in_gettext_func_call.rs b/crates/ruff/src/rules/flake8_gettext/rules/format_in_gettext_func_call.rs index ec159d03373a1..2f99369f25858 100644 --- a/crates/ruff/src/rules/flake8_gettext/rules/format_in_gettext_func_call.rs +++ b/crates/ruff/src/rules/flake8_gettext/rules/format_in_gettext_func_call.rs @@ -3,6 +3,8 @@ use rustpython_parser::ast::{self, Expr, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use crate::checkers::ast::Checker; + #[violation] pub struct FormatInGetTextFuncCall; @@ -14,15 +16,16 @@ impl Violation for FormatInGetTextFuncCall { } /// INT002 -pub(crate) fn format_in_gettext_func_call(args: &[Expr]) -> Option { +pub(crate) fn format_in_gettext_func_call(checker: &mut Checker, args: &[Expr]) { if let Some(first) = args.first() { if let Expr::Call(ast::ExprCall { func, .. }) = &first { if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() { if attr == "format" { - return Some(Diagnostic::new(FormatInGetTextFuncCall {}, first.range())); + checker + .diagnostics + .push(Diagnostic::new(FormatInGetTextFuncCall {}, first.range())); } } } } - None } diff --git a/crates/ruff/src/rules/flake8_gettext/rules/printf_in_gettext_func_call.rs b/crates/ruff/src/rules/flake8_gettext/rules/printf_in_gettext_func_call.rs index 088eaa60f83f9..eab5d74c93ef1 100644 --- a/crates/ruff/src/rules/flake8_gettext/rules/printf_in_gettext_func_call.rs +++ b/crates/ruff/src/rules/flake8_gettext/rules/printf_in_gettext_func_call.rs @@ -1,5 +1,6 @@ use rustpython_parser::ast::{self, Constant, Expr, Operator, Ranged}; +use crate::checkers::ast::Checker; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -14,7 +15,7 @@ impl Violation for PrintfInGetTextFuncCall { } /// INT003 -pub(crate) fn printf_in_gettext_func_call(args: &[Expr]) -> Option { +pub(crate) fn printf_in_gettext_func_call(checker: &mut Checker, args: &[Expr]) { if let Some(first) = args.first() { if let Expr::BinOp(ast::ExprBinOp { op: Operator::Mod { .. }, @@ -27,9 +28,10 @@ pub(crate) fn printf_in_gettext_func_call(args: &[Expr]) -> Option { .. }) = left.as_ref() { - return Some(Diagnostic::new(PrintfInGetTextFuncCall {}, first.range())); + checker + .diagnostics + .push(Diagnostic::new(PrintfInGetTextFuncCall {}, first.range())); } } } - None } diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs index b4e5aa23bee78..363b327c8c4dd 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -226,10 +226,10 @@ pub(crate) fn assert_falsy(checker: &mut Checker, stmt: &Stmt, test: &Expr) { } /// PT017 -pub(crate) fn assert_in_exception_handler(handlers: &[ExceptHandler]) -> Vec { - handlers - .iter() - .flat_map(|handler| match handler { +pub(crate) fn assert_in_exception_handler(checker: &mut Checker, handlers: &[ExceptHandler]) { + checker + .diagnostics + .extend(handlers.iter().flat_map(|handler| match handler { ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { name, body, .. }) => { @@ -239,8 +239,7 @@ pub(crate) fn assert_in_exception_handler(handlers: &[ExceptHandler]) -> Vec { +pub(super) struct Stack<'a> { /// The `return` statements in the current function. - pub(crate) returns: Vec<&'a ast::StmtReturn>, + pub(super) returns: Vec<&'a ast::StmtReturn>, /// The `else` statements in the current function. - pub(crate) elses: Vec<&'a ast::StmtIf>, + pub(super) elses: Vec<&'a ast::StmtIf>, /// The `elif` statements in the current function. - pub(crate) elifs: Vec<&'a ast::StmtIf>, + pub(super) elifs: Vec<&'a ast::StmtIf>, /// The non-local variables in the current function. - pub(crate) non_locals: FxHashSet<&'a str>, + pub(super) non_locals: FxHashSet<&'a str>, /// Whether the current function is a generator. - pub(crate) is_generator: bool, + pub(super) is_generator: bool, /// The `assignment`-to-`return` statement pairs in the current function. /// TODO(charlie): Remove the extra [`Stmt`] here, which is necessary to support statement /// removal for the `return` statement. - pub(crate) assignment_return: Vec<(&'a ast::StmtAssign, &'a ast::StmtReturn, &'a Stmt)>, + pub(super) assignment_return: Vec<(&'a ast::StmtAssign, &'a ast::StmtReturn, &'a Stmt)>, } #[derive(Default)] -pub(crate) struct ReturnVisitor<'a> { +pub(super) struct ReturnVisitor<'a> { /// The current stack of nodes. - pub(crate) stack: Stack<'a>, + pub(super) stack: Stack<'a>, /// The preceding sibling of the current node. sibling: Option<&'a Stmt>, /// The parent nodes of the current node.