Repository: astral-sh/ruff
File: crates/ty_python_semantic/src/types/narrow.rs (Rust)

Code before:
        let mut last_rhs_ty: Option<Type> = None;

        for (op, (left, right)) in std::iter::zip(&**ops, comparator_tuples) {
            let lhs_ty = last_rhs_ty.unwrap_or_else(|| inference.expression_type(left));
            let rhs_ty = inference.expression_type(right);
            last_rhs_ty = Some(rhs_ty);

            match left {
                ast::Expr::Name(_)
                | ast::Expr::Attribute(_)
                | ast::Expr::Subscript(_)
                | ast::Expr::Named(_) => {
                    if let Some(left) = place_expr(left)
                        && let Some(ty) =
                            self.evaluate_expr_compare_op(lhs_ty, rhs_ty, *op, is_positive)
                    {
                        let place = self.expect_place(&left);
                        constraints.insert(place, NarrowingConstraint::regular(ty));
                    }
                }
                // If left is not a narrowable target and not a Call expression try to narrow the
                // right operand instead. For symmetric operators (==, !=, is, is not), we can swap
                // the operands.
                //
                // E.g., `None != x` should narrow `x` the same way as `x != None`.
                _ if !matches!(left, ast::Expr::Call(_))
                    && matches!(
                        op,
                        ast::CmpOp::Eq | ast::CmpOp::NotEq | ast::CmpOp::Is | ast::CmpOp::IsNot
                    )
                    && matches!(
                        right,
                        ast::Expr::Name(_)
                            | ast::Expr::Attribute(_)
                            | ast::Expr::Subscript(_)
                            | ast::Expr::Named(_)
                    ) =>
                {
                    if let Some(right_place) = place_expr(right)
                        // Swap lhs_ty and rhs_ty since we're narrowing the right operand
                        && let Some(ty) =
                            self.evaluate_expr_compare_op(rhs_ty, lhs_ty, *op, is_positive)
                    {
                        let place = self.expect_place(&right_place);
                        constraints.insert(place, NarrowingConstraint::regular(ty));
                    }
                }
                ast::Expr::Call(ast::ExprCall {
                    range: _,
                    node_index: _,
                    func: callable,

Review comment (bug):
Why do we exclude call expressions specifically? Is it just so that it doesn't "clobber" the `ast::Expr::Call()` branch immediately below this one? If so, maybe this branch could just go below that one?

No tests fail if I make this change to your PR branch locally:

```diff
diff --git a/crates/ty_python_semantic/src/types/narrow.rs b/crates/ty_python_semantic/src/types/narrow.rs
index 927f0f6943..c1ff380a84 100644
--- a/crates/ty_python_semantic/src/types/narrow.rs
+++ b/crates/ty_python_semantic/src/types/narrow.rs
@@ -1196,33 +1196,6 @@ impl<'db, 'ast> NarrowingConstraintsBuilder<'db, 'ast> {
                         constraints.insert(place, NarrowingConstraint::regular(ty));
                     }
                 }
-                // If left is not a narrowable target and not a Call expression try to narrow the
-                // right operand instead. For symmetric operators (==, !=, is, is not), we can swap
-                // the operands.
-                //
-                // E.g., `None != x` should narrow `x` the same way as `x != None`.
-                _ if !matches!(left, ast::Expr::Call(_))
-                    && matches!(
-                        op,
-                        ast::CmpOp::Eq | ast::CmpOp::NotEq | ast::CmpOp::Is | ast::CmpOp::IsNot
-                    )
-                    && matches!(
-                        right,
-                        ast::Expr::Name(_)
-                            | ast::Expr::Attribute(_)
-                            | ast::Expr::Subscript(_)
-                            | ast::Expr::Named(_)
-                    ) =>
-                {
-                    if let Some(right_place) = place_expr(right)
-                        // Swap lhs_ty and rhs_ty since we're narrowing the right operand
-                        && let Some(ty) =
-                            self.evaluate_expr_compare_op(rhs_ty, lhs_ty, *op, is_positive)
-                    {
-                        let place = self.expect_place(&right_place);
-                        constraints.insert(place, NarrowingConstraint::regular(ty));
-                    }
-                }
                 ast::Expr::Call(ast::ExprCall {
                     range: _,
                     node_index: _,
@@ -1275,6 +1248,31 @@ impl<'db, 'ast> NarrowingConstraintsBuilder<'db, 'ast> {
                         );
                     }
                 }
+                // If left is not a narrowable target and not a Call expression try to narrow the
+                // right operand instead. For symmetric operators (==, !=, is, is not), we can swap
+                // the operands.
+                //
+                // E.g., `None != x` should narrow `x` the same way as `x != None`.
+                _ if matches!(
+                    op,
+                    ast::CmpOp::Eq | ast::CmpOp::NotEq | ast::CmpOp::Is | ast::CmpOp::IsNot
+                ) && matches!(
+                    right,
+                    ast::Expr::Name(_)
+                        | ast::Expr::Attribute(_)
+                        | ast::Expr::Subscript(_)
+                        | ast::Expr::Named(_)
+                ) =>
+                {
+                    if let Some(right_place) = place_expr(right)
+                        // Swap lhs_ty and rhs_ty since we're narrowing the right operand
+                        && let Some(ty) =
+                            self.evaluate_expr_compare_op(rhs_ty, lhs_ty, *op, is_positive)
+                    {
+                        let place = self.expect_place(&right_place);
+                        constraints.insert(place, NarrowingConstraint::regular(ty));
+                    }
+                }
                 _ => {}
             }
         }
```

I was also a bit confused about why you had the `matches!(op, ast::CmpOp::Eq | ast::CmpOp::NotEq | ast::CmpOp::Is | ast::CmpOp::IsNot)` check in there before calling `self.evaluate_expr_compare_op()`, since `self.evaluate_expr_compare_op()` does its own check to see whether the `op` is valid. After staring at it a bit, I think it's because (unlike narrowing for the left-hand side) narrowing the right-hand side is only valid if `op` is a _symmetric_ operator, and `in`/`not in` are not symmetric operators? But it would be great to add a test that would fail without that check -- no tests currently fail if I get rid of it.

Code after:
        let mut last_rhs_ty: Option<Type> = None;

        for (op, (left, right)) in std::iter::zip(&**ops, comparator_tuples) {
            let lhs_ty = last_rhs_ty.unwrap_or_else(|| inference.expression_type(left));
            let rhs_ty = inference.expression_type(right);
            last_rhs_ty = Some(rhs_ty);

            match left {
                ast::Expr::Name(_)
                | ast::Expr::Attribute(_)
                | ast::Expr::Subscript(_)
                | ast::Expr::Named(_) => {
                    if let Some(left) = place_expr(left)
                        && let Some(ty) =
                            self.evaluate_expr_compare_op(lhs_ty, rhs_ty, *op, is_positive)
                    {
                        let place = self.expect_place(&left);
                        constraints.insert(place, NarrowingConstraint::regular(ty));
                    }
                }
                ast::Expr::Call(ast::ExprCall {
                    range: _,
                    node_index: _,
                    func: callable,
                    arguments:
                        ast::Arguments {
                            args,
                            keywords,
                            range: _,
                            node_index: _,
                        },
                }) if keywords.is_empty() => {
                    let Type::ClassLiteral(rhs_class) = rhs_ty else {
                        continue;
                    };

                    let target = match &**args {
                        [first] => match place_expr(first) {
                            Some(target) => target,
                            None => continue,
                        },
                        _ => continue,
                    };

                    let is_positive = if is_positive {
                        op == &ast::CmpOp::Is
                    } else {
                        op == &ast::CmpOp::IsNot
                    };

                    // `else`-branch narrowing for `if type(x) is Y` can only be done