From ec2f229a457d5f31bd74729f9b5fff591e83b490 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 14 Sep 2023 11:40:02 -0400 Subject: [PATCH] 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. --- .../rules/repeated_equality_comparison.rs | 24 +++---- crates/ruff_python_ast/src/comparable.rs | 62 ++++++++----------- 2 files changed, 38 insertions(+), 48 deletions(-) diff --git a/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison.rs b/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison.rs index 03841a7adc..67b7c2c069 100644 --- a/crates/ruff/src/rules/pylint/rules/repeated_equality_comparison.rs +++ b/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)> = + // Map from expression hash to (starting offset, number of comparisons, list + let mut value_to_comparators: FxHashMap)> = 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())); - *left_count += 1; + .or_insert_with(|| (left.start(), Vec::new())); 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())); - *right_count += 1; + .or_insert_with(|| (right.start(), Vec::new())); right_matches.push(left); } } - for (value, (count, comparators)) in value_to_comparators { - if count > 1 { + for (value, (_, comparators)) in value_to_comparators + .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(), )), }, diff --git a/crates/ruff_python_ast/src/comparable.rs b/crates/ruff_python_ast/src/comparable.rs index e0d7c99ba1..1c4f01967b 100644 --- a/crates/ruff_python_ast/src/comparable.rs +++ b/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)] -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, - } - } -} +use crate as ast; #[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)] pub enum ComparableBoolOp { @@ -665,38 +663,32 @@ pub struct ExprConstant<'a> { pub struct ExprAttribute<'a> { value: Box>, attr: &'a str, - ctx: ComparableExprContext, } #[derive(Debug, PartialEq, Eq, Hash)] pub struct ExprSubscript<'a> { value: Box>, slice: Box>, - ctx: ComparableExprContext, } #[derive(Debug, PartialEq, Eq, Hash)] pub struct ExprStarred<'a> { value: Box>, - 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>, - ctx: ComparableExprContext, } #[derive(Debug, PartialEq, Eq, Hash)] pub struct ExprTuple<'a> { elts: Vec>, - 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,