From 1e2fde668e7a5fe7de6953bf103c37ca94b6810c Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 27 Nov 2024 17:06:55 +0000 Subject: [PATCH 1/7] Fix bug where methods defined using lambdas were flagged by FURB118 --- .../resources/test/fixtures/refurb/FURB118.py | 9 +++++ .../checkers/ast/analyze/deferred_lambdas.rs | 5 +-- .../src/checkers/ast/analyze/expression.rs | 5 +++ .../refurb/rules/reimplemented_operator.rs | 34 +++++++++++-------- ...es__refurb__tests__FURB118_FURB118.py.snap | 4 ++- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index 52c8c0ac205d5..b96be0a0c90f4 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -93,3 +93,12 @@ 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 selfff, other: self == other 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 83af7589a2c77..2ec55d48509ac 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 4a0c1f8724fc4..7d7e5fcc7028e 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 42f7760027cfa..86bcacbc4408e 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -60,14 +60,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 { @@ -78,11 +71,9 @@ 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; - } + // Ignore methods, whether defined using the `def` keyword or via a `lambda` assignment. + if checker.semantic().current_scope().kind.is_class() { + return; } let Some(params) = target.parameters() else { @@ -327,12 +318,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 8cf0314f1f23c..1bc82ac000cf2 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,6 @@ 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: From 8a954e6bfacc0fe29b99d0b7a432e5e23eac97b8 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 27 Nov 2024 17:25:23 +0000 Subject: [PATCH 2/7] Improve docs on fix safety --- .../resources/test/fixtures/refurb/FURB118.py | 2 +- .../refurb/rules/reimplemented_operator.rs | 30 +++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index b96be0a0c90f4..693cb2b2d0f93 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -101,4 +101,4 @@ def x(self, other): return self == other class Bar: - y = lambda selfff, other: self == other + y = lambda self, other: self == other 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 86bcacbc4408e..cd5896825629a 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 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 it 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, From 5ace156b75d4897bd8f6a7dd51f49294d8b033ed Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 27 Nov 2024 18:18:46 +0000 Subject: [PATCH 3/7] Fix false negative on lambdas inside decorators --- .../resources/test/fixtures/refurb/FURB118.py | 18 ++++++ .../refurb/rules/reimplemented_operator.rs | 12 +++- ...es__refurb__tests__FURB118_FURB118.py.snap | 58 +++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index 693cb2b2d0f93..f49b068cf5511 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -102,3 +102,21 @@ def x(self, other): class Bar: y = lambda self, other: self == other + +from typing import Callable +class Baz: + z: Callable = 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 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 cd5896825629a..ad68ed8509fa3 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -92,7 +92,13 @@ impl Violation for ReimplementedOperator { /// FURB118 pub(crate) fn reimplemented_operator(checker: &mut Checker, target: &FunctionLike) { // Ignore methods, whether defined using the `def` keyword or via a `lambda` assignment. - if checker.semantic().current_scope().kind.is_class() { + 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; } @@ -152,6 +158,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, 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 1bc82ac000cf2..d245b7c98bc57 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 @@ -949,3 +949,61 @@ FURB118.py:95:17: FURB118 [*] Use `operator.itemgetter((1, 2))` instead 96 97 | 97 98 | 98 99 | # All methods in classes are ignored, even those defined using lambdas: + +FURB118.py:117:14: FURB118 [*] Use `operator.itemgetter(slice(-2, None))` instead of defining a lambda + | +115 | "slicer, expected", +116 | [ +117 | (lambda x: x[-2:], "foo"), + | ^^^^^^^^^^^^^^^^ FURB118 +118 | (lambda x: x[-5:-3], "bar"), +119 | ], + | + = help: Replace with `operator.itemgetter(slice(-2, None))` + +ℹ Unsafe fix +104 104 | y = lambda self, other: self == other +105 105 | +106 106 | from typing import Callable + 107 |+import operator +107 108 | class Baz: +108 109 | z: Callable = lambda self, other: self == other +109 110 | +-------------------------------------------------------------------------------- +114 115 | @pytest.mark.parametrize( +115 116 | "slicer, expected", +116 117 | [ +117 |- (lambda x: x[-2:], "foo"), + 118 |+ (operator.itemgetter(slice(-2, None)), "foo"), +118 119 | (lambda x: x[-5:-3], "bar"), +119 120 | ], +120 121 | ) + +FURB118.py:118:14: FURB118 [*] Use `operator.itemgetter(slice(-5, -3))` instead of defining a lambda + | +116 | [ +117 | (lambda x: x[-2:], "foo"), +118 | (lambda x: x[-5:-3], "bar"), + | ^^^^^^^^^^^^^^^^^^ FURB118 +119 | ], +120 | ) + | + = help: Replace with `operator.itemgetter(slice(-5, -3))` + +ℹ Unsafe fix +104 104 | y = lambda self, other: self == other +105 105 | +106 106 | from typing import Callable + 107 |+import operator +107 108 | class Baz: +108 109 | z: Callable = lambda self, other: self == other +109 110 | +-------------------------------------------------------------------------------- +115 116 | "slicer, expected", +116 117 | [ +117 118 | (lambda x: x[-2:], "foo"), +118 |- (lambda x: x[-5:-3], "bar"), + 119 |+ (operator.itemgetter(slice(-5, -3)), "bar"), +119 120 | ], +120 121 | ) +121 122 | def test_inlet_asset_alias_extra_slice(self, slicer, expected): From 9090af00f49d626cc2b241019a66520c9e2812bf Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 28 Nov 2024 12:42:48 +0000 Subject: [PATCH 4/7] address review --- .../resources/test/fixtures/refurb/FURB118.py | 12 +++ .../refurb/rules/reimplemented_operator.rs | 4 +- ...es__refurb__tests__FURB118_FURB118.py.snap | 90 +++++++++---------- 3 files changed, 60 insertions(+), 46 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index f49b068cf5511..72b1af66185b8 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -108,6 +108,18 @@ 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: 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 ad68ed8509fa3..b94610c4dd5ae 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -91,7 +91,9 @@ impl Violation for ReimplementedOperator { /// FURB118 pub(crate) fn reimplemented_operator(checker: &mut Checker, target: &FunctionLike) { - // Ignore methods, whether defined using the `def` keyword or via a `lambda` assignment. + // Ignore methods. + // 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 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 d245b7c98bc57..3d21ba0d73107 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 @@ -950,60 +950,60 @@ FURB118.py:95:17: FURB118 [*] Use `operator.itemgetter((1, 2))` instead 97 98 | 98 99 | # All methods in classes are ignored, even those defined using lambdas: -FURB118.py:117:14: FURB118 [*] Use `operator.itemgetter(slice(-2, None))` instead of defining a lambda +FURB118.py:129:14: FURB118 [*] Use `operator.itemgetter(slice(-2, None))` instead of defining a lambda | -115 | "slicer, expected", -116 | [ -117 | (lambda x: x[-2:], "foo"), +127 | "slicer, expected", +128 | [ +129 | (lambda x: x[-2:], "foo"), | ^^^^^^^^^^^^^^^^ FURB118 -118 | (lambda x: x[-5:-3], "bar"), -119 | ], +130 | (lambda x: x[-5:-3], "bar"), +131 | ], | = help: Replace with `operator.itemgetter(slice(-2, None))` ℹ Unsafe fix -104 104 | y = lambda self, other: self == other -105 105 | -106 106 | from typing import Callable - 107 |+import operator -107 108 | class Baz: -108 109 | z: Callable = lambda self, other: self == other -109 110 | --------------------------------------------------------------------------------- -114 115 | @pytest.mark.parametrize( -115 116 | "slicer, expected", -116 117 | [ -117 |- (lambda x: x[-2:], "foo"), - 118 |+ (operator.itemgetter(slice(-2, None)), "foo"), -118 119 | (lambda x: x[-5:-3], "bar"), -119 120 | ], -120 121 | ) - -FURB118.py:118:14: FURB118 [*] Use `operator.itemgetter(slice(-5, -3))` instead of defining a lambda +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 | -116 | [ -117 | (lambda x: x[-2:], "foo"), -118 | (lambda x: x[-5:-3], "bar"), +128 | [ +129 | (lambda x: x[-2:], "foo"), +130 | (lambda x: x[-5:-3], "bar"), | ^^^^^^^^^^^^^^^^^^ FURB118 -119 | ], -120 | ) +131 | ], +132 | ) | = help: Replace with `operator.itemgetter(slice(-5, -3))` ℹ Unsafe fix -104 104 | y = lambda self, other: self == other -105 105 | -106 106 | from typing import Callable - 107 |+import operator -107 108 | class Baz: -108 109 | z: Callable = lambda self, other: self == other -109 110 | --------------------------------------------------------------------------------- -115 116 | "slicer, expected", -116 117 | [ -117 118 | (lambda x: x[-2:], "foo"), -118 |- (lambda x: x[-5:-3], "bar"), - 119 |+ (operator.itemgetter(slice(-5, -3)), "bar"), -119 120 | ], -120 121 | ) -121 122 | def test_inlet_asset_alias_extra_slice(self, slicer, expected): +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): From 85711072f57f6b0b163eaf2602d9ac75b6b6ae64 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 28 Nov 2024 12:47:40 +0000 Subject: [PATCH 5/7] document the edge case in `zerver` --- .../resources/test/fixtures/refurb/FURB118.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index 72b1af66185b8..315bacaf5f180 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -132,3 +132,12 @@ class TheLambdasHereAreNotMethods: ) def test_inlet_asset_alias_extra_slice(self, slicer, expected): assert slice("whatever") == expected + + +class NotAMethodButHardToDetect: + # in an ideal world, perhaps we'd flag this, + # but practically speaking 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) From d8c937c91b96ccf868774a012b7d0f868d2bb155 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 28 Nov 2024 12:50:45 +0000 Subject: [PATCH 6/7] final docs tweak --- .../refurb/rules/reimplemented_operator.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) 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 b94610c4dd5ae..5644cc2c72776 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -51,21 +51,21 @@ use crate::Locator; /// 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` +/// 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 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 +/// 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 it 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. +/// 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)] From a8f4cd3dbcc10856a2dbc30c373e3e0a25198383 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 28 Nov 2024 12:53:35 +0000 Subject: [PATCH 7/7] improve comment --- .../ruff_linter/resources/test/fixtures/refurb/FURB118.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py index 315bacaf5f180..27f81b4715b1c 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -135,8 +135,10 @@ def test_inlet_asset_alias_extra_slice(self, slicer, expected): class NotAMethodButHardToDetect: - # in an ideal world, perhaps we'd flag this, - # but practically speaking it's hard to see how we'd accurately determine + # 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