From 39bdfe14b88584c9db30a2f70ea6401f6e9139c5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 13 Sep 2023 14:40:53 -0400 Subject: [PATCH] Remove context from ComparableExpr --- .../rules/repeated_equality_comparison.rs | 24 ++++---- crates/ruff_python_ast/src/comparable.rs | 58 +++++++------------ 2 files changed, 34 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 03841a7adc9429..67b7c2c069c88c 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 e0d7c99ba123fd..9f5f4e942428fa 100644 --- a/crates/ruff_python_ast/src/comparable.rs +++ b/crates/ruff_python_ast/src/comparable.rs @@ -1,25 +1,19 @@ //! 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`]). -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 +659,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 +903,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,