Fix bug where methods defined using lambdas were flagged by FURB118 (#14639)

Author: AlexWaygood

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)

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);
             }

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());
+            }
+        }
         _ => {}
     };
 }

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<String> {
@@ -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<O
     }
 }
 
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
 enum FunctionLikeKind {
     Lambda,
     Function,
 }
 
+impl FunctionLikeKind {
+    const fn as_str(self) -> &'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 {

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):