Rewrite a variety of .contains() calls as matches! statements (#5432)

## Summary

These have the potential to be much more efficient, as we've seen in the
past.
This commit is contained in:
Charlie Marsh 2023-06-28 22:42:27 -04:00 committed by GitHub
parent aa887d5a1d
commit a973019358
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 183 additions and 169 deletions

View file

@ -4,50 +4,57 @@ use ruff_diagnostics::{Diagnostic, DiagnosticKind};
use crate::checkers::ast::Checker;
pub(super) const FUNC_CALL_NAME_ALLOWLIST: &[&str] = &[
"append",
"assertEqual",
"assertEquals",
"assertNotEqual",
"assertNotEquals",
"bool",
"bytes",
"count",
"failIfEqual",
"failUnlessEqual",
"float",
"fromkeys",
"get",
"getattr",
"getboolean",
"getfloat",
"getint",
"index",
"insert",
"int",
"param",
"pop",
"remove",
"set_blocking",
"set_enabled",
"setattr",
"__setattr__",
"setdefault",
"str",
];
/// Returns `true` if a function call is allowed to use a boolean trap.
pub(super) fn is_allowed_func_call(name: &str) -> bool {
matches!(
name,
"append"
| "assertEqual"
| "assertEquals"
| "assertNotEqual"
| "assertNotEquals"
| "bool"
| "bytes"
| "count"
| "failIfEqual"
| "failUnlessEqual"
| "float"
| "fromkeys"
| "get"
| "getattr"
| "getboolean"
| "getfloat"
| "getint"
| "index"
| "insert"
| "int"
| "param"
| "pop"
| "remove"
| "set_blocking"
| "set_enabled"
| "setattr"
| "__setattr__"
| "setdefault"
| "str"
)
}
pub(super) const FUNC_DEF_NAME_ALLOWLIST: &[&str] = &["__setitem__"];
/// Returns `true` if a function definition is allowed to use a boolean trap.
pub(super) fn is_allowed_func_def(name: &str) -> bool {
matches!(name, "__setitem__")
}
/// Returns `true` if an argument is allowed to use a boolean trap. To return
/// `true`, the function name must be explicitly allowed, and the argument must
/// be either the first or second argument in the call.
pub(super) fn allow_boolean_trap(func: &Expr) -> bool {
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func {
return FUNC_CALL_NAME_ALLOWLIST.contains(&attr.as_ref());
return is_allowed_func_call(attr);
}
if let Expr::Name(ast::ExprName { id, .. }) = func {
return FUNC_CALL_NAME_ALLOWLIST.contains(&id.as_ref());
return is_allowed_func_call(id);
}
false

View file

@ -1,14 +1,11 @@
use rustpython_parser::ast::{ArgWithDefault, Arguments, Decorator};
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::collect_call_path;
use crate::checkers::ast::Checker;
use crate::rules::flake8_boolean_trap::helpers::add_if_boolean;
use super::super::helpers::FUNC_DEF_NAME_ALLOWLIST;
use crate::rules::flake8_boolean_trap::helpers::{add_if_boolean, is_allowed_func_def};
/// ## What it does
/// Checks for the use of booleans as default values in function definitions.
@ -64,7 +61,7 @@ pub(crate) fn check_boolean_default_value_in_function_definition(
decorator_list: &[Decorator],
arguments: &Arguments,
) {
if FUNC_DEF_NAME_ALLOWLIST.contains(&name) {
if is_allowed_func_def(name) {
return;
}

View file

@ -6,7 +6,7 @@ use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::collect_call_path;
use crate::checkers::ast::Checker;
use crate::rules::flake8_boolean_trap::helpers::FUNC_DEF_NAME_ALLOWLIST;
use crate::rules::flake8_boolean_trap::helpers::is_allowed_func_def;
/// ## What it does
/// Checks for boolean positional arguments in function definitions.
@ -82,7 +82,7 @@ pub(crate) fn check_positional_boolean_in_def(
decorator_list: &[Decorator],
arguments: &Arguments,
) {
if FUNC_DEF_NAME_ALLOWLIST.contains(&name) {
if is_allowed_func_def(name) {
return;
}

View file

@ -155,7 +155,7 @@ pub(crate) fn non_self_return_type(
}
// In-place methods that are expected to return `Self`.
if INPLACE_BINOP_METHODS.contains(&name) {
if is_inplace_bin_op(name) {
if !is_self(returns, checker.semantic()) {
checker.diagnostics.push(Diagnostic::new(
NonSelfReturnType {
@ -214,21 +214,25 @@ pub(crate) fn non_self_return_type(
}
}
const INPLACE_BINOP_METHODS: &[&str] = &[
"__iadd__",
"__isub__",
"__imul__",
"__imatmul__",
"__itruediv__",
"__ifloordiv__",
"__imod__",
"__ipow__",
"__ilshift__",
"__irshift__",
"__iand__",
"__ixor__",
"__ior__",
];
/// Returns `true` if the method is an in-place binary operator.
fn is_inplace_bin_op(name: &str) -> bool {
matches!(
name,
"__iadd__"
| "__isub__"
| "__imul__"
| "__imatmul__"
| "__itruediv__"
| "__ifloordiv__"
| "__imod__"
| "__ipow__"
| "__ilshift__"
| "__irshift__"
| "__iand__"
| "__ixor__"
| "__ior__"
)
}
/// Return `true` if the given expression resolves to the given name.
fn is_name(expr: &Expr, name: &str) -> bool {

View file

@ -3,7 +3,7 @@ use rustpython_parser::ast::{self, Expr, Keyword, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::compose_call_path;
use ruff_python_ast::call_path::collect_call_path;
use ruff_python_ast::helpers::{collect_arg_names, SimpleCallArgs};
use ruff_python_ast::visitor;
use ruff_python_ast::visitor::Visitor;
@ -18,26 +18,6 @@ impl Violation for PytestPatchWithLambda {
}
}
const PATCH_NAMES: &[&str] = &[
"mocker.patch",
"class_mocker.patch",
"module_mocker.patch",
"package_mocker.patch",
"session_mocker.patch",
"mock.patch",
"unittest.mock.patch",
];
const PATCH_OBJECT_NAMES: &[&str] = &[
"mocker.patch.object",
"class_mocker.patch.object",
"module_mocker.patch.object",
"package_mocker.patch.object",
"session_mocker.patch.object",
"mock.patch.object",
"unittest.mock.patch.object",
];
#[derive(Default)]
/// Visitor that checks references the argument names in the lambda body.
struct LambdaBodyVisitor<'a> {
@ -98,15 +78,36 @@ pub(crate) fn patch_with_lambda(
args: &[Expr],
keywords: &[Keyword],
) -> Option<Diagnostic> {
if let Some(call_path) = compose_call_path(call) {
if PATCH_NAMES.contains(&call_path.as_str()) {
let call_path = collect_call_path(call)?;
if matches!(
call_path.as_slice(),
[
"mocker"
| "class_mocker"
| "module_mocker"
| "package_mocker"
| "session_mocker"
| "mock",
"patch"
] | ["unittest", "mock", "patch"]
) {
check_patch_call(call, args, keywords, 1)
} else if PATCH_OBJECT_NAMES.contains(&call_path.as_str()) {
} else if matches!(
call_path.as_slice(),
[
"mocker"
| "class_mocker"
| "module_mocker"
| "package_mocker"
| "session_mocker"
| "mock",
"patch",
"object"
] | ["unittest", "mock", "patch", "object"]
) {
check_patch_call(call, args, keywords, 2)
} else {
None
}
} else {
None
}
}

View file

@ -119,7 +119,12 @@ impl AlwaysAutofixableViolation for DoubleNegation {
}
}
const DUNDER_METHODS: &[&str] = &["__eq__", "__ne__", "__lt__", "__le__", "__gt__", "__ge__"];
fn is_dunder_method(name: &str) -> bool {
matches!(
name,
"__eq__" | "__ne__" | "__lt__" | "__le__" | "__gt__" | "__ge__"
)
}
fn is_exception_check(stmt: &Stmt) -> bool {
let Stmt::If(ast::StmtIf {test: _, body, orelse: _, range: _ })= stmt else {
@ -159,7 +164,7 @@ pub(crate) fn negation_with_equal_op(
| ScopeKind::AsyncFunction(ast::StmtAsyncFunctionDef { name, .. }) =
&checker.semantic().scope().kind
{
if DUNDER_METHODS.contains(&name.as_str()) {
if is_dunder_method(name) {
return;
}
}
@ -211,7 +216,7 @@ pub(crate) fn negation_with_not_equal_op(
| ScopeKind::AsyncFunction(ast::StmtAsyncFunctionDef { name, .. }) =
&checker.semantic().scope().kind
{
if DUNDER_METHODS.contains(&name.as_str()) {
if is_dunder_method(name) {
return;
}
}

View file

@ -2,7 +2,7 @@ use rustpython_parser::ast::{Alias, Ranged};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_stdlib::future::ALL_FEATURE_NAMES;
use ruff_python_stdlib::future::is_feature_name;
use crate::checkers::ast::Checker;
@ -30,7 +30,10 @@ impl Violation for FutureFeatureNotDefined {
}
pub(crate) fn future_feature_not_defined(checker: &mut Checker, alias: &Alias) {
if !ALL_FEATURE_NAMES.contains(&alias.name.as_str()) {
if is_feature_name(&alias.name) {
return;
}
checker.diagnostics.push(Diagnostic::new(
FutureFeatureNotDefined {
name: alias.name.to_string(),
@ -38,4 +41,3 @@ pub(crate) fn future_feature_not_defined(checker: &mut Checker, alias: &Alias) {
alias.range(),
));
}
}

View file

@ -94,15 +94,13 @@ impl Violation for DeprecatedImport {
}
}
// A list of modules that may involve import rewrites.
const RELEVANT_MODULES: &[&str] = &[
"collections",
"pipes",
"mypy_extensions",
"typing_extensions",
"typing",
"typing.re",
];
/// Returns `true` if the module may contain deprecated imports.
fn is_relevant_module(module: &str) -> bool {
matches!(
module,
"collections" | "pipes" | "mypy_extensions" | "typing_extensions" | "typing" | "typing.re"
)
}
// Members of `collections` that were moved to `collections.abc`.
const COLLECTIONS_TO_ABC: &[&str] = &[
@ -560,7 +558,7 @@ pub(crate) fn deprecated_import(
return;
};
if !RELEVANT_MODULES.contains(&module) {
if !is_relevant_module(module) {
return;
}

View file

@ -52,37 +52,6 @@ impl AlwaysAutofixableViolation for UnnecessaryBuiltinImport {
}
}
const BUILTINS: &[&str] = &[
"*",
"ascii",
"bytes",
"chr",
"dict",
"filter",
"hex",
"input",
"int",
"isinstance",
"list",
"map",
"max",
"min",
"next",
"object",
"oct",
"open",
"pow",
"range",
"round",
"str",
"super",
"zip",
];
const IO: &[&str] = &["open"];
const SIX_MOVES_BUILTINS: &[&str] = BUILTINS;
const SIX: &[&str] = &["callable", "next"];
const SIX_MOVES: &[&str] = &["filter", "input", "map", "range", "zip"];
/// UP029
pub(crate) fn unnecessary_builtin_import(
checker: &mut Checker,
@ -90,25 +59,52 @@ pub(crate) fn unnecessary_builtin_import(
module: &str,
names: &[Alias],
) {
let deprecated_names = match module {
"builtins" => BUILTINS,
"io" => IO,
"six" => SIX,
"six.moves" => SIX_MOVES,
"six.moves.builtins" => SIX_MOVES_BUILTINS,
_ => return,
};
// Ignore irrelevant modules.
if !matches!(
module,
"builtins" | "io" | "six" | "six.moves" | "six.moves.builtins"
) {
return;
}
// Do this with a filter?
let mut unused_imports: Vec<&Alias> = vec![];
for alias in names {
if alias.asname.is_some() {
continue;
}
if deprecated_names.contains(&alias.name.as_str()) {
unused_imports.push(alias);
}
}
// Identify unaliased, builtin imports.
let unused_imports: Vec<&Alias> = names
.iter()
.filter(|alias| alias.asname.is_none())
.filter(|alias| {
matches!(
(module, alias.name.as_str()),
(
"builtins" | "six.moves.builtins",
"*" | "ascii"
| "bytes"
| "chr"
| "dict"
| "filter"
| "hex"
| "input"
| "int"
| "isinstance"
| "list"
| "map"
| "max"
| "min"
| "next"
| "object"
| "oct"
| "open"
| "pow"
| "range"
| "round"
| "str"
| "super"
| "zip"
) | ("io", "open")
| ("six", "callable" | "next")
| ("six.moves", "filter" | "input" | "map" | "range" | "zip")
)
})
.collect();
if unused_imports.is_empty() {
return;

View file

@ -1,13 +1,17 @@
/// A copy of `__future__.all_feature_names`.
pub const ALL_FEATURE_NAMES: &[&str] = &[
"nested_scopes",
"generators",
"division",
"absolute_import",
"with_statement",
"print_function",
"unicode_literals",
"barry_as_FLUFL",
"generator_stop",
"annotations",
];
/// Returns `true` if `name` is a valid `__future__` feature name, as defined by
/// `__future__.all_feature_names`.
pub fn is_feature_name(name: &str) -> bool {
matches!(
name,
"nested_scopes"
| "generators"
| "division"
| "absolute_import"
| "with_statement"
| "print_function"
| "unicode_literals"
| "barry_as_FLUFL"
| "generator_stop"
| "annotations"
)
}