diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs b/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs index 842d97e325..45ce25a4ec 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs @@ -54,7 +54,7 @@ pub fn useless_expression(checker: &mut Checker, value: &Expr) { } // Ignore statements that have side effects. - if contains_effect(&checker.ctx, value) { + if contains_effect(value, |id| checker.ctx.is_builtin(id)) { // Flag attributes as useless expressions, even if they're attached to calls or other // expressions. if matches!(value.node, ExprKind::Attribute { .. }) { diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs index 4341d6508b..5a16f41b93 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs @@ -213,7 +213,7 @@ pub fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) { } else { unreachable!("Indices should only contain `isinstance` calls") }; - let fixable = !contains_effect(&checker.ctx, target); + let fixable = !contains_effect(target, |id| checker.ctx.is_builtin(id)); let mut diagnostic = Diagnostic::new( DuplicateIsinstanceCall { name: if let ExprKind::Name { id, .. } = &target.node { @@ -341,7 +341,7 @@ pub fn compare_with_tuple(checker: &mut Checker, expr: &Expr) { // Avoid rewriting (e.g.) `a == "foo" or a == f()`. if comparators .iter() - .any(|expr| contains_effect(&checker.ctx, expr)) + .any(|expr| contains_effect(expr, |id| checker.ctx.is_builtin(id))) { continue; } @@ -423,7 +423,7 @@ pub fn expr_and_not_expr(checker: &mut Checker, expr: &Expr) { return; } - if contains_effect(&checker.ctx, expr) { + if contains_effect(expr, |id| checker.ctx.is_builtin(id)) { return; } @@ -477,7 +477,7 @@ pub fn expr_or_not_expr(checker: &mut Checker, expr: &Expr) { return; } - if contains_effect(&checker.ctx, expr) { + if contains_effect(expr, |id| checker.ctx.is_builtin(id)) { return; } @@ -522,7 +522,7 @@ pub fn is_short_circuit( let mut location = expr.location; for (value, next_value) in values.iter().tuple_windows() { // Keep track of the location of the furthest-right, non-effectful expression. - if contains_effect(ctx, value) { + if contains_effect(value, |id| ctx.is_builtin(id)) { location = next_value.location; continue; } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs index 7c96ce9b57..fea7226802 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs @@ -639,10 +639,9 @@ pub fn manual_dict_lookup( let StmtKind::Return { value, .. } = &body[0].node else { return; }; - if value - .as_ref() - .map_or(false, |value| contains_effect(&checker.ctx, value)) - { + if value.as_ref().map_or(false, |value| { + contains_effect(value, |id| checker.ctx.is_builtin(id)) + }) { return; } @@ -712,10 +711,9 @@ pub fn manual_dict_lookup( let StmtKind::Return { value, .. } = &body[0].node else { return; }; - if value - .as_ref() - .map_or(false, |value| contains_effect(&checker.ctx, value)) - { + if value.as_ref().map_or(false, |value| { + contains_effect(value, |id| checker.ctx.is_builtin(id)) + }) { return; }; @@ -757,13 +755,13 @@ pub fn use_dict_get_with_default( if body.len() != 1 || orelse.len() != 1 { return; } - let StmtKind::Assign { targets: body_var, value: body_val, ..} = &body[0].node else { + let StmtKind::Assign { targets: body_var, value: body_value, ..} = &body[0].node else { return; }; if body_var.len() != 1 { return; }; - let StmtKind::Assign { targets: orelse_var, value: orelse_val, .. } = &orelse[0].node else { + let StmtKind::Assign { targets: orelse_var, value: orelse_value, .. } = &orelse[0].node else { return; }; if orelse_var.len() != 1 { @@ -775,15 +773,15 @@ pub fn use_dict_get_with_default( if test_dict.len() != 1 { return; } - let (expected_var, expected_val, default_var, default_val) = match ops[..] { - [Cmpop::In] => (&body_var[0], body_val, &orelse_var[0], orelse_val), - [Cmpop::NotIn] => (&orelse_var[0], orelse_val, &body_var[0], body_val), + let (expected_var, expected_value, default_var, default_value) = match ops[..] { + [Cmpop::In] => (&body_var[0], body_value, &orelse_var[0], orelse_value), + [Cmpop::NotIn] => (&orelse_var[0], orelse_value, &body_var[0], body_value), _ => { return; } }; let test_dict = &test_dict[0]; - let ExprKind::Subscript { value: expected_subscript, slice: expected_slice, .. } = &expected_val.node else { + let ExprKind::Subscript { value: expected_subscript, slice: expected_slice, .. } = &expected_value.node else { return; }; @@ -797,7 +795,7 @@ pub fn use_dict_get_with_default( } // Check that the default value is not "complex". - if contains_effect(&checker.ctx, default_val) { + if contains_effect(default_value, |id| checker.ctx.is_builtin(id)) { return; } @@ -842,7 +840,7 @@ pub fn use_dict_get_with_default( })), args: vec![ create_expr(test_key.node.clone()), - create_expr(default_val.node.clone()), + create_expr(default_value.node.clone()), ], keywords: vec![], })), diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs index 172a26eaa9..efbb12cf20 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_variable.rs @@ -202,7 +202,9 @@ fn remove_unused_variable( range.location == target.location && range.end_location == target.end_location.unwrap() }) { if matches!(target.node, ExprKind::Name { .. }) { - return if targets.len() > 1 || contains_effect(&checker.ctx, value) { + return if targets.len() > 1 + || contains_effect(value, |id| checker.ctx.is_builtin(id)) + { // If the expression is complex (`x = foo()`), remove the assignment, // but preserve the right-hand side. Some(( @@ -248,7 +250,7 @@ fn remove_unused_variable( } = &stmt.node { if matches!(target.node, ExprKind::Name { .. }) { - return if contains_effect(&checker.ctx, value) { + return if contains_effect(value, |id| checker.ctx.is_builtin(id)) { // If the expression is complex (`x = foo()`), remove the assignment, // but preserve the right-hand side. Some(( diff --git a/crates/ruff/src/rules/tryceratops/rules/try_consider_else.rs b/crates/ruff/src/rules/tryceratops/rules/try_consider_else.rs index fb50c685cf..d29d1d2b00 100644 --- a/crates/ruff/src/rules/tryceratops/rules/try_consider_else.rs +++ b/crates/ruff/src/rules/tryceratops/rules/try_consider_else.rs @@ -28,7 +28,7 @@ pub fn try_consider_else( if let Some(stmt) = body.last() { if let StmtKind::Return { value } = &stmt.node { if let Some(value) = value { - if contains_effect(&checker.ctx, value) { + if contains_effect(value, |id| checker.ctx.is_builtin(id)) { return; } } diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 7c0f301362..841c8764d5 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -60,7 +60,12 @@ pub fn contains_call_path(ctx: &Context, expr: &Expr, target: &[&str]) -> bool { /// Return `true` if the `Expr` contains an expression that appears to include a /// side-effect (like a function call). -pub fn contains_effect(ctx: &Context, expr: &Expr) -> bool { +/// +/// Accepts a closure that determines whether a given name (e.g., `"list"`) is a Python builtin. +pub fn contains_effect(expr: &Expr, is_builtin: F) -> bool +where + F: Fn(&str) -> bool, +{ any_over_expr(expr, &|expr| { // Accept empty initializers. if let ExprKind::Call { @@ -71,13 +76,13 @@ pub fn contains_effect(ctx: &Context, expr: &Expr) -> bool { { if args.is_empty() && keywords.is_empty() { if let ExprKind::Name { id, .. } = &func.node { - let is_empty_initializer = (id == "set" - || id == "list" - || id == "tuple" - || id == "dict" - || id == "frozenset") - && ctx.is_builtin(id); - return !is_empty_initializer; + if !matches!(id.as_str(), "set" | "list" | "tuple" | "dict" | "frozenset") { + return true; + } + if !is_builtin(id) { + return true; + } + return false; } } }