Reverse order of arguments for operator.contains (#9192)

Closes #9191.
astral-sh · Dec 18, 2023 · be8f8e6 · be8f8e6
1 parent c97d3dd
commit be8f8e6
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 46 deletions.
diff --git 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
@@ -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/...ter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB118_FURB118.py.snap b/...ter/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`