Skip to content

Commit

Permalink
Remove context from ComparableExpr
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Sep 13, 2023
1 parent ebe9c03 commit 00e79d5
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 30 deletions.
22 changes: 12 additions & 10 deletions crates/ruff/src/rules/pylint/rules/repeated_equality_comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand All @@ -95,24 +96,25 @@ 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(
Expand Down
30 changes: 10 additions & 20 deletions crates/ruff_python_ast/src/comparable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,38 +665,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)]
Expand Down Expand Up @@ -915,50 +909,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,
Expand Down

0 comments on commit 00e79d5

Please sign in to comment.