From 286d8c18dddefc06744be94340e89879b1a075cf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 8 Feb 2023 13:35:18 -0500 Subject: [PATCH] Remove ExprKind::Call from call path collection (#2666) --- crates/ruff/src/ast/helpers.rs | 9 +----- crates/ruff/src/checkers/ast.rs | 10 +++---- .../rules/function_call_argument_default.rs | 6 ++-- .../rules/mutable_argument_default.rs | 4 +-- .../rules/useless_contextlib_suppress.rs | 4 +-- .../rules/flake8_pytest_style/rules/fail.rs | 8 ++--- .../flake8_pytest_style/rules/helpers.rs | 29 ++++++++++++++----- .../rules/flake8_simplify/rules/ast_expr.rs | 16 +++++----- crates/ruff/src/rules/pep8_naming/helpers.rs | 17 ++++++++--- .../pyupgrade/rules/replace_stdout_stderr.rs | 4 +-- .../rules/replace_universal_newlines.rs | 4 +-- .../pyupgrade/rules/type_of_primitive.rs | 4 +-- .../tryceratops/rules/prefer_type_error.rs | 2 +- .../tryceratops/rules/raise_vanilla_class.rs | 8 +++-- 14 files changed, 73 insertions(+), 52 deletions(-) diff --git a/crates/ruff/src/ast/helpers.rs b/crates/ruff/src/ast/helpers.rs index f4ac0e5b97f35..9d64269c258bd 100644 --- a/crates/ruff/src/ast/helpers.rs +++ b/crates/ruff/src/ast/helpers.rs @@ -72,14 +72,7 @@ fn collect_call_path_inner<'a>(expr: &'a Expr, parts: &mut CallPath<'a>) -> bool /// Convert an `Expr` to its [`CallPath`] segments (like `["typing", "List"]`). pub fn collect_call_path(expr: &Expr) -> CallPath { let mut segments = smallvec![]; - collect_call_path_inner( - if let ExprKind::Call { func, .. } = &expr.node { - func - } else { - expr - }, - &mut segments, - ); + collect_call_path_inner(expr, &mut segments); segments } diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 28426da2e7b7f..1aa697a3870fd 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -2280,7 +2280,7 @@ where // pyupgrade if self.settings.rules.enabled(&Rule::TypeOfPrimitive) { - pyupgrade::rules::type_of_primitive(self, expr, args); + pyupgrade::rules::type_of_primitive(self, expr, func, args); } if self.settings.rules.enabled(&Rule::DeprecatedUnittestAlias) { pyupgrade::rules::deprecated_unittest_alias(self, func); @@ -2301,10 +2301,10 @@ where pyupgrade::rules::open_alias(self, expr, func); } if self.settings.rules.enabled(&Rule::ReplaceUniversalNewlines) { - pyupgrade::rules::replace_universal_newlines(self, expr, keywords); + pyupgrade::rules::replace_universal_newlines(self, func, keywords); } if self.settings.rules.enabled(&Rule::ReplaceStdoutStderr) { - pyupgrade::rules::replace_stdout_stderr(self, expr, keywords); + pyupgrade::rules::replace_stdout_stderr(self, expr, func, keywords); } if self.settings.rules.enabled(&Rule::OSErrorAlias) { pyupgrade::rules::os_error_alias(self, &expr); @@ -2335,7 +2335,7 @@ where .rules .enabled(&Rule::UselessContextlibSuppress) { - flake8_bugbear::rules::useless_contextlib_suppress(self, expr, args); + flake8_bugbear::rules::useless_contextlib_suppress(self, expr, func, args); } if self .settings @@ -3242,7 +3242,7 @@ where args, keywords, } => { - let callable = self.resolve_call_path(expr).and_then(|call_path| { + let callable = self.resolve_call_path(func).and_then(|call_path| { if self.match_typing_call_path(&call_path, "ForwardRef") { Some(Callable::ForwardRef) } else if self.match_typing_call_path(&call_path, "cast") { diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/function_call_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/function_call_argument_default.rs index 7d01d746d3244..bb57a7480d008 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/function_call_argument_default.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/function_call_argument_default.rs @@ -37,8 +37,8 @@ const IMMUTABLE_FUNCS: &[&[&str]] = &[ &["re", "compile"], ]; -fn is_immutable_func(checker: &Checker, expr: &Expr, extend_immutable_calls: &[CallPath]) -> bool { - checker.resolve_call_path(expr).map_or(false, |call_path| { +fn is_immutable_func(checker: &Checker, func: &Expr, extend_immutable_calls: &[CallPath]) -> bool { + checker.resolve_call_path(func).map_or(false, |call_path| { IMMUTABLE_FUNCS .iter() .any(|target| call_path.as_slice() == *target) @@ -67,7 +67,7 @@ where { self.diagnostics.push(( FunctionCallArgumentDefault { - name: compose_call_path(expr), + name: compose_call_path(func), } .into(), Range::from_located(expr), diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index 702d4ec9487bd..f1b7dadb3f811 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -66,8 +66,8 @@ const IMMUTABLE_GENERIC_TYPES: &[&[&str]] = &[ &["typing", "Tuple"], ]; -pub fn is_mutable_func(checker: &Checker, expr: &Expr) -> bool { - checker.resolve_call_path(expr).map_or(false, |call_path| { +pub fn is_mutable_func(checker: &Checker, func: &Expr) -> bool { + checker.resolve_call_path(func).map_or(false, |call_path| { MUTABLE_FUNCS .iter() .any(|target| call_path.as_slice() == *target) diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/useless_contextlib_suppress.rs b/crates/ruff/src/rules/flake8_bugbear/rules/useless_contextlib_suppress.rs index ad918bcd845f5..78ce1e049efec 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/useless_contextlib_suppress.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/useless_contextlib_suppress.rs @@ -20,9 +20,9 @@ impl Violation for UselessContextlibSuppress { } /// B005 -pub fn useless_contextlib_suppress(checker: &mut Checker, expr: &Expr, args: &[Expr]) { +pub fn useless_contextlib_suppress(checker: &mut Checker, expr: &Expr, func: &Expr, args: &[Expr]) { if args.is_empty() - && checker.resolve_call_path(expr).map_or(false, |call_path| { + && checker.resolve_call_path(func).map_or(false, |call_path| { call_path.as_slice() == ["contextlib", "suppress"] }) { 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 7e0f41d6dcd7d..046755e0790d1 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/fail.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/fail.rs @@ -18,8 +18,8 @@ impl Violation for FailWithoutMessage { } } -pub fn fail_call(checker: &mut Checker, call: &Expr, args: &[Expr], keywords: &[Keyword]) { - if is_pytest_fail(call, checker) { +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)); @@ -27,13 +27,13 @@ pub fn fail_call(checker: &mut Checker, call: &Expr, args: &[Expr], keywords: &[ if is_empty_or_null_string(msg) { checker.diagnostics.push(Diagnostic::new( FailWithoutMessage, - Range::from_located(call), + Range::from_located(func), )); } } else { checker.diagnostics.push(Diagnostic::new( FailWithoutMessage, - Range::from_located(call), + Range::from_located(func), )); } } diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs index e303125f773bf..127e8ead33df9 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs @@ -6,15 +6,26 @@ use crate::checkers::ast::Checker; const ITERABLE_INITIALIZERS: &[&str] = &["dict", "frozenset", "list", "tuple", "set"]; -pub fn get_mark_decorators(decorators: &[Expr]) -> Vec<&Expr> { +/// Given a decorators that can be used with or without explicit call syntax, return +/// the underlying callable. +fn callable_decorator(decorator: &Expr) -> &Expr { + if let ExprKind::Call { func, .. } = &decorator.node { + func + } else { + decorator + } +} + +pub fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator { decorators .iter() .filter(|decorator| is_pytest_mark(decorator)) - .collect() } pub fn get_mark_name(decorator: &Expr) -> &str { - collect_call_path(decorator).last().unwrap() + collect_call_path(callable_decorator(decorator)) + .last() + .unwrap() } pub fn is_pytest_fail(call: &Expr, checker: &Checker) -> bool { @@ -25,14 +36,18 @@ pub fn is_pytest_fail(call: &Expr, checker: &Checker) -> bool { pub fn is_pytest_fixture(decorator: &Expr, checker: &Checker) -> bool { checker - .resolve_call_path(decorator) + .resolve_call_path(if let ExprKind::Call { func, .. } = &decorator.node { + func + } else { + decorator + }) .map_or(false, |call_path| { call_path.as_slice() == ["pytest", "fixture"] }) } pub fn is_pytest_mark(decorator: &Expr) -> bool { - let segments = collect_call_path(decorator); + let segments = collect_call_path(callable_decorator(decorator)); if segments.len() > 2 { segments[0] == "pytest" && segments[1] == "mark" } else { @@ -42,7 +57,7 @@ pub fn is_pytest_mark(decorator: &Expr) -> bool { pub fn is_pytest_yield_fixture(decorator: &Expr, checker: &Checker) -> bool { checker - .resolve_call_path(decorator) + .resolve_call_path(callable_decorator(decorator)) .map_or(false, |call_path| { call_path.as_slice() == ["pytest", "yield_fixture"] }) @@ -100,7 +115,7 @@ pub fn is_falsy_constant(expr: &Expr) -> bool { pub fn is_pytest_parametrize(decorator: &Expr, checker: &Checker) -> bool { checker - .resolve_call_path(decorator) + .resolve_call_path(callable_decorator(decorator)) .map_or(false, |call_path| { call_path.as_slice() == ["pytest", "mark", "parametrize"] }) diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_expr.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_expr.rs index c9fc2c65021cc..f425535e7c712 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_expr.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_expr.rs @@ -35,14 +35,8 @@ pub fn use_capital_environment_variables(checker: &mut Checker, expr: &Expr) { return; } - // check `os.environ.get('foo')` and `os.getenv('foo')`` - if !checker.resolve_call_path(expr).map_or(false, |call_path| { - call_path.as_slice() == ["os", "environ", "get"] || call_path.as_slice() == ["os", "getenv"] - }) { - return; - } - - let ExprKind::Call { args, .. } = &expr.node else { + // check `os.environ.get('foo')` and `os.getenv('foo')`. + let ExprKind::Call { func, args, .. } = &expr.node else { return; }; let Some(arg) = args.get(0) else { @@ -51,6 +45,12 @@ pub fn use_capital_environment_variables(checker: &mut Checker, expr: &Expr) { let ExprKind::Constant { value: Constant::Str(env_var), kind } = &arg.node else { return; }; + if !checker.resolve_call_path(func).map_or(false, |call_path| { + call_path.as_slice() == ["os", "environ", "get"] || call_path.as_slice() == ["os", "getenv"] + }) { + return; + } + let capital_env_var = env_var.to_ascii_uppercase(); if &capital_env_var == env_var { return; diff --git a/crates/ruff/src/rules/pep8_naming/helpers.rs b/crates/ruff/src/rules/pep8_naming/helpers.rs index 0850bcb403c03..92204b0179fb2 100644 --- a/crates/ruff/src/rules/pep8_naming/helpers.rs +++ b/crates/ruff/src/rules/pep8_naming/helpers.rs @@ -1,6 +1,6 @@ use itertools::Itertools; use ruff_python::string::{is_lower, is_upper}; -use rustpython_parser::ast::{Stmt, StmtKind}; +use rustpython_parser::ast::{ExprKind, Stmt, StmtKind}; use crate::checkers::ast::Checker; @@ -26,7 +26,10 @@ pub fn is_namedtuple_assignment(checker: &Checker, stmt: &Stmt) -> bool { let StmtKind::Assign { value, .. } = &stmt.node else { return false; }; - checker.resolve_call_path(value).map_or(false, |call_path| { + let ExprKind::Call {func, ..} = &value.node else { + return false; + }; + checker.resolve_call_path(func).map_or(false, |call_path| { call_path.as_slice() == ["collections", "namedtuple"] || call_path.as_slice() == ["typing", "NamedTuple"] }) @@ -36,7 +39,10 @@ pub fn is_typeddict_assignment(checker: &Checker, stmt: &Stmt) -> bool { let StmtKind::Assign { value, .. } = &stmt.node else { return false; }; - checker.resolve_call_path(value).map_or(false, |call_path| { + let ExprKind::Call {func, ..} = &value.node else { + return false; + }; + checker.resolve_call_path(func).map_or(false, |call_path| { call_path.as_slice() == ["typing", "TypedDict"] }) } @@ -45,7 +51,10 @@ pub fn is_type_var_assignment(checker: &Checker, stmt: &Stmt) -> bool { let StmtKind::Assign { value, .. } = &stmt.node else { return false; }; - checker.resolve_call_path(value).map_or(false, |call_path| { + let ExprKind::Call {func, ..} = &value.node else { + return false; + }; + checker.resolve_call_path(func).map_or(false, |call_path| { call_path.as_slice() == ["typing", "TypeVar"] || call_path.as_slice() == ["typing", "NewType"] }) diff --git a/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs b/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs index f264415568c85..3e5d21bc7be5e 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs @@ -104,8 +104,8 @@ fn generate_fix( } /// UP022 -pub fn replace_stdout_stderr(checker: &mut Checker, expr: &Expr, kwargs: &[Keyword]) { - if checker.resolve_call_path(expr).map_or(false, |call_path| { +pub fn replace_stdout_stderr(checker: &mut Checker, expr: &Expr, func: &Expr, kwargs: &[Keyword]) { + if checker.resolve_call_path(func).map_or(false, |call_path| { call_path.as_slice() == ["subprocess", "run"] }) { // Find `stdout` and `stderr` kwargs. diff --git a/crates/ruff/src/rules/pyupgrade/rules/replace_universal_newlines.rs b/crates/ruff/src/rules/pyupgrade/rules/replace_universal_newlines.rs index 7d03aeb7059cf..28c6aa627db75 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/replace_universal_newlines.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/replace_universal_newlines.rs @@ -23,8 +23,8 @@ impl AlwaysAutofixableViolation for ReplaceUniversalNewlines { } /// UP021 -pub fn replace_universal_newlines(checker: &mut Checker, expr: &Expr, kwargs: &[Keyword]) { - if checker.resolve_call_path(expr).map_or(false, |call_path| { +pub fn replace_universal_newlines(checker: &mut Checker, func: &Expr, kwargs: &[Keyword]) { + if checker.resolve_call_path(func).map_or(false, |call_path| { call_path.as_slice() == ["subprocess", "run"] }) { let Some(kwarg) = find_keyword(kwargs, "universal_newlines") else { return; }; diff --git a/crates/ruff/src/rules/pyupgrade/rules/type_of_primitive.rs b/crates/ruff/src/rules/pyupgrade/rules/type_of_primitive.rs index 9d5fa867d2c43..6101973f975dd 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/type_of_primitive.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/type_of_primitive.rs @@ -27,12 +27,12 @@ impl AlwaysAutofixableViolation for TypeOfPrimitive { } /// UP003 -pub fn type_of_primitive(checker: &mut Checker, expr: &Expr, args: &[Expr]) { +pub fn type_of_primitive(checker: &mut Checker, expr: &Expr, func: &Expr, args: &[Expr]) { if args.len() != 1 { return; } if !checker - .resolve_call_path(expr) + .resolve_call_path(func) .map_or(false, |call_path| call_path.as_slice() == ["", "type"]) { return; diff --git a/crates/ruff/src/rules/tryceratops/rules/prefer_type_error.rs b/crates/ruff/src/rules/tryceratops/rules/prefer_type_error.rs index 05319cc78bd12..1f080ed7bae84 100644 --- a/crates/ruff/src/rules/tryceratops/rules/prefer_type_error.rs +++ b/crates/ruff/src/rules/tryceratops/rules/prefer_type_error.rs @@ -84,7 +84,7 @@ fn check_type_check_test(checker: &mut Checker, test: &Expr) -> bool { .iter() .all(|expr| check_type_check_test(checker, expr)), ExprKind::UnaryOp { operand, .. } => check_type_check_test(checker, operand), - ExprKind::Call { .. } => check_type_check_call(checker, test), + ExprKind::Call { func, .. } => check_type_check_call(checker, func), _ => false, } } diff --git a/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_class.rs b/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_class.rs index ed9a5befc9af0..1fb2da139dd31 100644 --- a/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_class.rs +++ b/crates/ruff/src/rules/tryceratops/rules/raise_vanilla_class.rs @@ -1,5 +1,5 @@ use ruff_macros::{define_violation, derive_message_formats}; -use rustpython_parser::ast::Expr; +use rustpython_parser::ast::{Expr, ExprKind}; use crate::ast::types::Range; use crate::checkers::ast::Checker; @@ -59,7 +59,11 @@ impl Violation for RaiseVanillaClass { /// TRY002 pub fn raise_vanilla_class(checker: &mut Checker, expr: &Expr) { if checker - .resolve_call_path(expr) + .resolve_call_path(if let ExprKind::Call { func, .. } = &expr.node { + func + } else { + expr + }) .map_or(false, |call_path| call_path.as_slice() == ["", "Exception"]) { checker.diagnostics.push(Diagnostic::new(