Remove ExprContext from ComparableExpr (#7362)

`ComparableExpr` includes the `ExprContext` field on an expression, so,
e.g., the two tuples in `(a, b) = (a, b)` won't be considered equal.
Similarly, the tuples in `[(a, b) for (a, b) in c]` _also_ wouldn't be
considered equal. I find this behavior surprising, since
`ComparableExpr` is intended to allow you to compare two ASTs, but
`ExprContext` is really encoding information about the broader context
for the expression.

crates/ruff/src/rules/pylint/rules/repeated_equality_comparison.rs

@@ -10,7 +10,7 @@ use ruff_python_ast::comparable::ComparableExpr;
 use ruff_python_ast::hashable::HashableExpr;
 use ruff_python_ast::{self as ast, BoolOp, CmpOp, Expr};
 use ruff_source_file::Locator;
-use ruff_text_size::Ranged;
+use ruff_text_size::{Ranged, TextSize};
 
 use crate::autofix::snippet::SourceCodeSnippet;
 use crate::checkers::ast::Checker;
@@ -74,7 +74,8 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
         return;
     }
 
-    let mut value_to_comparators: FxHashMap<HashableExpr, (usize, Vec<&Expr>)> =
+    // Map from expression hash to (starting offset, number of comparisons, list
+    let mut value_to_comparators: FxHashMap<HashableExpr, (TextSize, Vec<&Expr>)> =
         FxHashMap::with_capacity_and_hasher(
             bool_op.values.len() * 2,
             BuildHasherDefault::default(),
@@ -95,30 +96,31 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
         };
 
         if matches!(left.as_ref(), Expr::Name(_) | Expr::Attribute(_)) {
-            let (left_count, left_matches) = value_to_comparators
+            let (_, left_matches) = value_to_comparators
                 .entry(left.deref().into())
-                .or_insert_with(|| (0, Vec::new()));
+                .or_insert_with(|| (left.start(), Vec::new()));
-            *left_count += 1;
             left_matches.push(right);
         }
 
         if matches!(right, Expr::Name(_) | Expr::Attribute(_)) {
-            let (right_count, right_matches) = value_to_comparators
+            let (_, right_matches) = value_to_comparators
                 .entry(right.into())
-                .or_insert_with(|| (0, Vec::new()));
+                .or_insert_with(|| (right.start(), Vec::new()));
-            *right_count += 1;
             right_matches.push(left);
         }
     }
 
-    for (value, (count, comparators)) in value_to_comparators {
+    for (value, (_, comparators)) in value_to_comparators
-        if count > 1 {
+        .iter()
+        .sorted_by_key(|(_, (start, _))| *start)
+    {
+        if comparators.len() > 1 {
             checker.diagnostics.push(Diagnostic::new(
                 RepeatedEqualityComparison {
                     expression: SourceCodeSnippet::new(merged_membership_test(
                         value.as_expr(),
                         bool_op.op,
-                        &comparators,
+                        comparators,
                         checker.locator(),
                     )),
                 },

crates/ruff_python_ast/src/comparable.rs

@@ -1,25 +1,23 @@
 //! An equivalent object hierarchy to the `RustPython` AST hierarchy, but with the
 //! ability to compare expressions for equality (via [`Eq`] and [`Hash`]).
+//!
+//! Two [`ComparableExpr`]s are considered equal if the underlying AST nodes have the
+//! same shape, ignoring trivia (e.g., parentheses, comments, and whitespace), the
+//! location in the source code, and other contextual information (e.g., whether they
+//! represent reads or writes, which is typically encoded in the Python AST).
+//!
+//! For example, in `[(a, b) for a, b in c]`, the `(a, b)` and `a, b` expressions are
+//! considered equal, despite the former being parenthesized, and despite the former
+//! being a write ([`ast::ExprContext::Store`]) and the latter being a read
+//! ([`ast::ExprContext::Load`]).
+//!
+//! Similarly, `"a" "b"` and `"ab"` would be considered equal, despite the former being
+//! an implicit concatenation of string literals, as these expressions are considered to
+//! have the same shape in that they evaluate to the same value.
 
-use crate as ast;
 use num_bigint::BigInt;
 
-#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)]
+use crate as ast;
-pub enum ComparableExprContext {
-    Load,
-    Store,
-    Del,
-}
-
-impl From<&ast::ExprContext> for ComparableExprContext {
-    fn from(ctx: &ast::ExprContext) -> Self {
-        match ctx {
-            ast::ExprContext::Load => Self::Load,
-            ast::ExprContext::Store => Self::Store,
-            ast::ExprContext::Del => Self::Del,
-        }
-    }
-}
 
 #[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)]
 pub enum ComparableBoolOp {
@@ -665,38 +663,32 @@ pub struct ExprConstant<'a> {
 pub struct ExprAttribute<'a> {
     value: Box<ComparableExpr<'a>>,
     attr: &'a str,
-    ctx: ComparableExprContext,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct ExprSubscript<'a> {
     value: Box<ComparableExpr<'a>>,
     slice: Box<ComparableExpr<'a>>,
-    ctx: ComparableExprContext,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct ExprStarred<'a> {
     value: Box<ComparableExpr<'a>>,
-    ctx: ComparableExprContext,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct ExprName<'a> {
     id: &'a str,
-    ctx: ComparableExprContext,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct ExprList<'a> {
     elts: Vec<ComparableExpr<'a>>,
-    ctx: ComparableExprContext,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
 pub struct ExprTuple<'a> {
     elts: Vec<ComparableExpr<'a>>,
-    ctx: ComparableExprContext,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
@@ -915,50 +907,46 @@ impl<'a> From<&'a ast::Expr> for ComparableExpr<'a> {
             ast::Expr::Attribute(ast::ExprAttribute {
                 value,
                 attr,
-                ctx,
+                ctx: _,
                 range: _,
             }) => Self::Attribute(ExprAttribute {
                 value: value.into(),
                 attr: attr.as_str(),
-                ctx: ctx.into(),
             }),
             ast::Expr::Subscript(ast::ExprSubscript {
                 value,
                 slice,
-                ctx,
+                ctx: _,
                 range: _,
             }) => Self::Subscript(ExprSubscript {
                 value: value.into(),
                 slice: slice.into(),
-                ctx: ctx.into(),
             }),
             ast::Expr::Starred(ast::ExprStarred {
                 value,
-                ctx,
+                ctx: _,
                 range: _,
             }) => Self::Starred(ExprStarred {
                 value: value.into(),
-                ctx: ctx.into(),
-            }),
-            ast::Expr::Name(ast::ExprName { id, ctx, range: _ }) => Self::Name(ExprName {
-                id: id.as_str(),
-                ctx: ctx.into(),
             }),
+            ast::Expr::Name(ast::ExprName {
+                id,
+                ctx: _,
+                range: _,
+            }) => Self::Name(ExprName { id: id.as_str() }),
             ast::Expr::List(ast::ExprList {
                 elts,
-                ctx,
+                ctx: _,
                 range: _,
             }) => Self::List(ExprList {
                 elts: elts.iter().map(Into::into).collect(),
-                ctx: ctx.into(),
             }),
             ast::Expr::Tuple(ast::ExprTuple {
                 elts,
-                ctx,
+                ctx: _,
                 range: _,
             }) => Self::Tuple(ExprTuple {
                 elts: elts.iter().map(Into::into).collect(),
-                ctx: ctx.into(),
             }),
             ast::Expr::Slice(ast::ExprSlice {
                 lower,