From be8f8e62b52294ea0ffe0f68f7ce58695d913085 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 18 Dec 2023 14:39:52 -0500 Subject: [PATCH] Reverse order of arguments for `operator.contains` (#9192) Closes https://github.com/astral-sh/ruff/issues/9191. --- .../resources/test/fixtures/refurb/FURB118.py | 9 +- .../refurb/rules/reimplemented_operator.rs | 89 ++++++++++++++++--- ...es__refurb__tests__FURB118_FURB118.py.snap | 54 +++++------ 3 files changed, 106 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 9cabba5f683c0..51c136f976558 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB118.py @@ -26,24 +26,29 @@ op_gte = lambda x, y: x >= y op_is = lambda x, y: x is y op_isnot = lambda x, y: x is not y -op_in = lambda x, y: x in y +op_in = lambda x, y: y in x + def op_not2(x): return not x + def op_add2(x, y): return x + y + class Adder: def add(x, y): return x + y -# Ok. +# OK. op_add3 = lambda x, y = 1: x + y op_neg2 = lambda x, y: y - x op_notin = lambda x, y: y not in x op_and = lambda x, y: y and x op_or = lambda x, y: y or x +op_in = lambda x, y: x in y + def op_neg3(x, y): return y - x 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 1c3d81c834545..0ee71d6f967d2 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/reimplemented_operator.rs @@ -223,23 +223,88 @@ fn cmp_op(expr: &ast::ExprCompare, params: &ast::Parameters) -> Option<&'static let [right] = expr.comparators.as_slice() else { return None; }; - if !is_same_expression(arg1, &expr.left) || !is_same_expression(arg2, right) { - return None; - } + match op { - ast::CmpOp::Eq => Some("eq"), - ast::CmpOp::NotEq => Some("ne"), - ast::CmpOp::Lt => Some("lt"), - ast::CmpOp::LtE => Some("le"), - ast::CmpOp::Gt => Some("gt"), - ast::CmpOp::GtE => Some("ge"), - ast::CmpOp::Is => Some("is_"), - ast::CmpOp::IsNot => Some("is_not"), - ast::CmpOp::In => Some("contains"), + ast::CmpOp::Eq => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("eq") + } else { + None + } + } + ast::CmpOp::NotEq => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("ne") + } else { + None + } + } + ast::CmpOp::Lt => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("lt") + } else { + None + } + } + ast::CmpOp::LtE => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("le") + } else { + None + } + } + ast::CmpOp::Gt => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("gt") + } else { + None + } + } + ast::CmpOp::GtE => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("ge") + } else { + None + } + } + ast::CmpOp::Is => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("is_") + } else { + None + } + } + ast::CmpOp::IsNot => { + if match_arguments(arg1, arg2, &expr.left, right) { + Some("is_not") + } else { + None + } + } + ast::CmpOp::In => { + // Note: `operator.contains` reverses the order of arguments. That is: + // `operator.contains` is equivalent to `lambda x, y: y in x`, rather than + // `lambda x, y: x in y`. + if match_arguments(arg1, arg2, right, &expr.left) { + Some("contains") + } else { + None + } + } ast::CmpOp::NotIn => None, } } +/// Returns `true` if the given arguments match the expected operands. +fn match_arguments( + arg1: &ast::ParameterWithDefault, + arg2: &ast::ParameterWithDefault, + operand1: &Expr, + operand2: &Expr, +) -> bool { + is_same_expression(arg1, operand1) && is_same_expression(arg2, operand2) +} + /// Returns `true` if the given argument is the "same" as the given expression. For example, if /// the argument has a default, it is not considered the same as any expression; if both match the /// same name, they are considered the same. 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 67e2f228c9216..828a95ee3b5ea 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 @@ -590,7 +590,7 @@ FURB118.py:26:10: FURB118 [*] Use `operator.ge` instead of defining a lambda 27 |+op_gte = operator.ge 27 28 | op_is = lambda x, y: x is y 28 29 | op_isnot = lambda x, y: x is not y -29 30 | op_in = lambda x, y: x in y +29 30 | op_in = lambda x, y: y in x FURB118.py:27:9: FURB118 [*] Use `operator.is_` instead of defining a lambda | @@ -599,7 +599,7 @@ FURB118.py:27:9: FURB118 [*] Use `operator.is_` instead of defining a lambda 27 | op_is = lambda x, y: x is y | ^^^^^^^^^^^^^^^^^^^ FURB118 28 | op_isnot = lambda x, y: x is not y -29 | op_in = lambda x, y: x in y +29 | op_in = lambda x, y: y in x | = help: Replace with `operator.is_` @@ -616,7 +616,7 @@ FURB118.py:27:9: FURB118 [*] Use `operator.is_` instead of defining a lambda 27 |-op_is = lambda x, y: x is y 28 |+op_is = operator.is_ 28 29 | op_isnot = lambda x, y: x is not y -29 30 | op_in = lambda x, y: x in y +29 30 | op_in = lambda x, y: y in x 30 31 | FURB118.py:28:12: FURB118 [*] Use `operator.is_not` instead of defining a lambda @@ -625,7 +625,7 @@ FURB118.py:28:12: FURB118 [*] Use `operator.is_not` instead of defining a lambda 27 | op_is = lambda x, y: x is y 28 | op_isnot = lambda x, y: x is not y | ^^^^^^^^^^^^^^^^^^^^^^^ FURB118 -29 | op_in = lambda x, y: x in y +29 | op_in = lambda x, y: y in x | = help: Replace with `operator.is_not` @@ -641,18 +641,16 @@ FURB118.py:28:12: FURB118 [*] Use `operator.is_not` instead of defining a lambda 27 28 | op_is = lambda x, y: x is y 28 |-op_isnot = lambda x, y: x is not y 29 |+op_isnot = operator.is_not -29 30 | op_in = lambda x, y: x in y +29 30 | op_in = lambda x, y: y in x 30 31 | -31 32 | def op_not2(x): +31 32 | FURB118.py:29:9: FURB118 [*] Use `operator.contains` instead of defining a lambda | 27 | op_is = lambda x, y: x is y 28 | op_isnot = lambda x, y: x is not y -29 | op_in = lambda x, y: x in y +29 | op_in = lambda x, y: y in x | ^^^^^^^^^^^^^^^^^^^ FURB118 -30 | -31 | def op_not2(x): | = help: Replace with `operator.contains` @@ -666,45 +664,37 @@ FURB118.py:29:9: FURB118 [*] Use `operator.contains` instead of defining a lambd 26 27 | op_gte = lambda x, y: x >= y 27 28 | op_is = lambda x, y: x is y 28 29 | op_isnot = lambda x, y: x is not y -29 |-op_in = lambda x, y: x in y +29 |-op_in = lambda x, y: y in x 30 |+op_in = operator.contains 30 31 | -31 32 | def op_not2(x): -32 33 | return not x +31 32 | +32 33 | def op_not2(x): -FURB118.py:31:1: FURB118 Use `operator.not_` instead of defining a function +FURB118.py:32:1: FURB118 Use `operator.not_` instead of defining a function | -29 | op_in = lambda x, y: x in y -30 | -31 | / def op_not2(x): -32 | | return not x +32 | / def op_not2(x): +33 | | return not x | |________________^ FURB118 -33 | -34 | def op_add2(x, y): | = help: Replace with `operator.not_` -FURB118.py:34:1: FURB118 Use `operator.add` instead of defining a function +FURB118.py:36:1: FURB118 Use `operator.add` instead of defining a function | -32 | return not x -33 | -34 | / def op_add2(x, y): -35 | | return x + y +36 | / def op_add2(x, y): +37 | | return x + y | |________________^ FURB118 -36 | -37 | class Adder: | = help: Replace with `operator.add` -FURB118.py:38:5: FURB118 Use `operator.add` instead of defining a function +FURB118.py:41:5: FURB118 Use `operator.add` instead of defining a function | -37 | class Adder: -38 | def add(x, y): +40 | class Adder: +41 | def add(x, y): | _____^ -39 | | return x + y +42 | | return x + y | |____________________^ FURB118 -40 | -41 | # Ok. +43 | +44 | # OK. | = help: Replace with `operator.add`