diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index 52c8c0ac205d5f..27f81b4715b1ca 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -93,3 +93,53 @@ def add(x, y): # Without a slice, trivia is retained op_itemgetter = lambda x: x[1, 2] + + +# All methods in classes are ignored, even those defined using lambdas: +class Foo: + def x(self, other): + return self == other + +class Bar: + y = lambda self, other: self == other + +from typing import Callable +class Baz: + z: Callable = lambda self, other: self == other + + +# Lambdas wrapped in function calls could also still be method definitions! +# To avoid false positives, we shouldn't flag any of these either: +from typing import final, override, no_type_check + + +class Foo: + a = final(lambda self, other: self == other) + b = override(lambda self, other: self == other) + c = no_type_check(lambda self, other: self == other) + d = final(override(no_type_check(lambda self, other: self == other))) + + +# lambdas used in decorators do not constitute method definitions, +# so these *should* be flagged: +class TheLambdasHereAreNotMethods: + @pytest.mark.parametrize( + "slicer, expected", + [ + (lambda x: x[-2:], "foo"), + (lambda x: x[-5:-3], "bar"), + ], + ) + def test_inlet_asset_alias_extra_slice(self, slicer, expected): + assert slice("whatever") == expected + + +class NotAMethodButHardToDetect: + # In an ideal world, perhaps we'd emit a diagnostic here, + # since this `lambda` is clearly not a method definition, + # and *could* be safely replaced with an `operator` function. + # Practically speaking, however, it's hard to see how we'd accurately determine + # that the `lambda` is *not* a method definition + # without risking false positives elsewhere or introducing complex heuristics + # that users would find surprising and confusing + FOO = sorted([x for x in BAR], key=lambda x: x.baz) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs index 83af7589a2c77c..2ec55d48509ac9 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_lambdas.rs @@ -2,7 +2,7 @@ use ruff_python_ast::Expr; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::{flake8_builtins, flake8_pie, pylint, refurb}; +use crate::rules::{flake8_builtins, flake8_pie, pylint}; /// Run lint rules over all deferred lambdas in the [`SemanticModel`]. pub(crate) fn deferred_lambdas(checker: &mut Checker) { @@ -21,9 +21,6 @@ pub(crate) fn deferred_lambdas(checker: &mut Checker) { if checker.enabled(Rule::ReimplementedContainerBuiltin) { flake8_pie::rules::reimplemented_container_builtin(checker, lambda); } - if checker.enabled(Rule::ReimplementedOperator) { - refurb::rules::reimplemented_operator(checker, &lambda.into()); - } if checker.enabled(Rule::BuiltinLambdaArgumentShadowing) { flake8_builtins::rules::builtin_lambda_argument_shadowing(checker, lambda); } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 4a0c1f8724fc42..7d7e5fcc7028ef 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1650,6 +1650,11 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ruff::rules::assignment_in_assert(checker, expr); } } + Expr::Lambda(lambda_expr) => { + if checker.enabled(Rule::ReimplementedOperator) { + refurb::rules::reimplemented_operator(checker, &lambda_expr.into()); + } + } _ => {} }; } diff --git a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs index 42f7760027cfa5..5644cc2c72776f 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -22,7 +22,7 @@ use crate::Locator; /// /// ## Why is this bad? /// The `operator` module provides functions that implement the same functionality as the -/// corresponding operators. For example, `operator.add` is equivalent to `lambda x, y: x + y`. +/// corresponding operators. For example, `operator.add` is often equivalent to `lambda x, y: x + y`. /// Using the functions from the `operator` module is more concise and communicates the intent of /// the code more clearly. /// @@ -44,10 +44,30 @@ use crate::Locator; /// ``` /// /// ## Fix safety -/// This fix is usually safe, but if the lambda is called with keyword arguments, e.g., -/// `add = lambda x, y: x + y; add(x=1, y=2)`, replacing the lambda with an operator function, e.g., -/// `operator.add`, will cause the call to raise a `TypeError`, as functions in `operator` do not allow -/// keyword arguments. +/// The fix offered by this rule is always marked as unsafe. While the changes the fix would make +/// would rarely break your code, there are two ways in which functions from the `operator` module +/// differ from user-defined functions. It would be non-trivial for Ruff to detect whether or not +/// these differences would matter in a specific situation where Ruff is emitting a diagnostic for +/// this rule. +/// +/// The first difference is that `operator` functions cannot be called with keyword arguments, but +/// most user-defined functions can. If an `add` function is defined as `add = lambda x, y: x + y`, +/// replacing this function with `operator.add` will cause the later call to raise `TypeError` if +/// the function is later called with keyword arguments, e.g. `add(x=1, y=2)`. +/// +/// The second difference is that user-defined functions are [descriptors], but this is not true of +/// the functions defined in the `operator` module. Practically speaking, this means that defining +/// a function in a class body (either by using a `def` statement or assigning a `lambda` function +/// to a variable) is a valid way of defining an instance method on that class; monkeypatching a +/// user-defined function onto a class after the class has been created also has the same effect. +/// The same is not true of an `operator` function: assigning an `operator` function to a variable +/// in a class body or monkeypatching one onto a class will not create a valid instance method. +/// Ruff will refrain from emitting diagnostics for this rule on function definitions in class +/// bodies; however, it does not currently have sophisticated enough type inference to avoid +/// emitting this diagnostic if a user-defined function is being monkeypatched onto a class after +/// the class has been constructed. +/// +/// [descriptors]: https://docs.python.org/3/howto/descriptor.html #[derive(ViolationMetadata)] pub(crate) struct ReimplementedOperator { operator: Operator, @@ -60,14 +80,7 @@ impl Violation for ReimplementedOperator { #[derive_message_formats] fn message(&self) -> String { let ReimplementedOperator { operator, target } = self; - match target { - FunctionLikeKind::Function => { - format!("Use `operator.{operator}` instead of defining a function") - } - FunctionLikeKind::Lambda => { - format!("Use `operator.{operator}` instead of defining a lambda") - } - } + format!("Use `operator.{operator}` instead of defining a {target}") } fn fix_title(&self) -> Option { @@ -79,10 +92,16 @@ impl Violation for ReimplementedOperator { /// FURB118 pub(crate) fn reimplemented_operator(checker: &mut Checker, target: &FunctionLike) { // Ignore methods. - if target.kind() == FunctionLikeKind::Function { - if checker.semantic().current_scope().kind.is_class() { - return; - } + // Methods can be defined via a `def` statement in a class scope, + // or via a lambda appearing on the right-hand side of an assignment in a class scope. + if checker.semantic().current_scope().kind.is_class() + && (target.is_function_def() + || checker + .semantic() + .current_statements() + .any(|stmt| matches!(stmt, Stmt::AnnAssign(_) | Stmt::Assign(_)))) + { + return; } let Some(params) = target.parameters() else { @@ -141,6 +160,10 @@ impl FunctionLike<'_> { } } + const fn is_function_def(&self) -> bool { + matches!(self, Self::Function(_)) + } + /// Return the body of the function-like node. /// /// If the node is a function definition that consists of more than a single return statement, @@ -327,12 +350,27 @@ fn get_operator(expr: &Expr, params: &Parameters, locator: &Locator) -> Option &'static str { + match self { + Self::Lambda => "lambda", + Self::Function => "function", + } + } +} + +impl Display for FunctionLikeKind { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + /// Return the name of the `operator` implemented by the given unary expression. fn unary_op(expr: &ast::ExprUnaryOp, params: &Parameters) -> Option<&'static str> { let [arg] = params.args.as_slice() else { diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap index 8cf0314f1f23ca..3d21ba0d73107f 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/refurb/mod.rs -snapshot_kind: text --- FURB118.py:2:13: FURB118 [*] Use `operator.invert` instead of defining a lambda | @@ -947,3 +946,64 @@ FURB118.py:95:17: FURB118 [*] Use `operator.itemgetter((1, 2))` instead 94 95 | # Without a slice, trivia is retained 95 |-op_itemgetter = lambda x: x[1, 2] 96 |+op_itemgetter = operator.itemgetter((1, 2)) +96 97 | +97 98 | +98 99 | # All methods in classes are ignored, even those defined using lambdas: + +FURB118.py:129:14: FURB118 [*] Use `operator.itemgetter(slice(-2, None))` instead of defining a lambda + | +127 | "slicer, expected", +128 | [ +129 | (lambda x: x[-2:], "foo"), + | ^^^^^^^^^^^^^^^^ FURB118 +130 | (lambda x: x[-5:-3], "bar"), +131 | ], + | + = help: Replace with `operator.itemgetter(slice(-2, None))` + +ℹ Unsafe fix +111 111 | # Lambdas wrapped in function calls could also still be method definitions! +112 112 | # To avoid false positives, we shouldn't flag any of these either: +113 113 | from typing import final, override, no_type_check + 114 |+import operator +114 115 | +115 116 | +116 117 | class Foo: +-------------------------------------------------------------------------------- +126 127 | @pytest.mark.parametrize( +127 128 | "slicer, expected", +128 129 | [ +129 |- (lambda x: x[-2:], "foo"), + 130 |+ (operator.itemgetter(slice(-2, None)), "foo"), +130 131 | (lambda x: x[-5:-3], "bar"), +131 132 | ], +132 133 | ) + +FURB118.py:130:14: FURB118 [*] Use `operator.itemgetter(slice(-5, -3))` instead of defining a lambda + | +128 | [ +129 | (lambda x: x[-2:], "foo"), +130 | (lambda x: x[-5:-3], "bar"), + | ^^^^^^^^^^^^^^^^^^ FURB118 +131 | ], +132 | ) + | + = help: Replace with `operator.itemgetter(slice(-5, -3))` + +ℹ Unsafe fix +111 111 | # Lambdas wrapped in function calls could also still be method definitions! +112 112 | # To avoid false positives, we shouldn't flag any of these either: +113 113 | from typing import final, override, no_type_check + 114 |+import operator +114 115 | +115 116 | +116 117 | class Foo: +-------------------------------------------------------------------------------- +127 128 | "slicer, expected", +128 129 | [ +129 130 | (lambda x: x[-2:], "foo"), +130 |- (lambda x: x[-5:-3], "bar"), + 131 |+ (operator.itemgetter(slice(-5, -3)), "bar"), +131 132 | ], +132 133 | ) +133 134 | def test_inlet_asset_alias_extra_slice(self, slicer, expected):