From e00594e8d2fd4a3ce8cef84374c8eccab4c0d56b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 3 Nov 2024 13:44:52 -0500 Subject: [PATCH] Respect hash-equivalent literals in `iteration-over-set` (#14063) ## Summary Closes https://github.com/astral-sh/ruff/issues/14049. --- .../fixtures/pylint/iteration_over_set.py | 3 + .../rules/pylint/rules/iteration_over_set.rs | 9 +- crates/ruff_python_ast/src/comparable.rs | 86 +++++++++++++++++-- 3 files changed, 88 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py b/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py index f8d32e59a5..ae8ccb3513 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py @@ -60,3 +60,6 @@ for item in {1, 2, 3, 4, 5, 6, int("7")}: # calls in set literals are fine for item in {1, 2, 2}: # duplicate literals will be ignored # B033 catches this print(f"I like {item}.") + +for item in {False, 0, 0.0, 0j, True, 1, 1.0}: + print(item) diff --git a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs index 0abc2d89b0..9b37135517 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs @@ -1,8 +1,10 @@ +use rustc_hash::{FxBuildHasher, FxHashSet}; + use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{comparable::ComparableExpr, Expr}; +use ruff_python_ast::comparable::HashableExpr; +use ruff_python_ast::Expr; use ruff_text_size::Ranged; -use rustc_hash::{FxBuildHasher, FxHashSet}; use crate::checkers::ast::Checker; @@ -54,8 +56,7 @@ pub(crate) fn iteration_over_set(checker: &mut Checker, expr: &Expr) { let mut seen_values = FxHashSet::with_capacity_and_hasher(set.len(), FxBuildHasher); for value in set { - let comparable_value = ComparableExpr::from(value); - if !seen_values.insert(comparable_value) { + if !seen_values.insert(HashableExpr::from(value)) { // if the set contains a duplicate literal value, early exit. // rule `B033` can catch that. return; diff --git a/crates/ruff_python_ast/src/comparable.rs b/crates/ruff_python_ast/src/comparable.rs index a6825b6eb2..8dd4c0dd85 100644 --- a/crates/ruff_python_ast/src/comparable.rs +++ b/crates/ruff_python_ast/src/comparable.rs @@ -15,9 +15,10 @@ //! 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 std::borrow::Cow; - use crate as ast; +use crate::{Expr, Number}; +use std::borrow::Cow; +use std::hash::Hash; #[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)] pub enum ComparableBoolOp { @@ -861,8 +862,8 @@ pub struct ExprNumberLiteral<'a> { } #[derive(Debug, PartialEq, Eq, Hash)] -pub struct ExprBoolLiteral<'a> { - value: &'a bool, +pub struct ExprBoolLiteral { + value: bool, } #[derive(Debug, PartialEq, Eq, Hash)] @@ -934,7 +935,7 @@ pub enum ComparableExpr<'a> { StringLiteral(ExprStringLiteral<'a>), BytesLiteral(ExprBytesLiteral<'a>), NumberLiteral(ExprNumberLiteral<'a>), - BoolLiteral(ExprBoolLiteral<'a>), + BoolLiteral(ExprBoolLiteral), NoneLiteral, EllipsisLiteral, Attribute(ExprAttribute<'a>), @@ -1109,7 +1110,7 @@ impl<'a> From<&'a ast::Expr> for ComparableExpr<'a> { }) } ast::Expr::BooleanLiteral(ast::ExprBooleanLiteral { value, range: _ }) => { - Self::BoolLiteral(ExprBoolLiteral { value }) + Self::BoolLiteral(ExprBoolLiteral { value: *value }) } ast::Expr::NoneLiteral(_) => Self::NoneLiteral, ast::Expr::EllipsisLiteral(_) => Self::EllipsisLiteral, @@ -1675,3 +1676,76 @@ impl<'a> From<&'a ast::ModExpression> for ComparableModExpression<'a> { } } } + +/// Wrapper around [`Expr`] that implements [`Hash`] and [`PartialEq`] according to Python +/// semantics: +/// +/// > Values that compare equal (such as 1, 1.0, and True) can be used interchangeably to index the +/// > same dictionary entry. +/// +/// For example, considers `True`, `1`, and `1.0` to be equal, as they hash to the same value +/// in Python, along with `False`, `0`, and `0.0`. +/// +/// See: +#[derive(Debug)] +pub struct HashableExpr<'a>(ComparableExpr<'a>); + +impl Hash for HashableExpr<'_> { + fn hash(&self, state: &mut H) { + self.0.hash(state); + } +} + +impl PartialEq for HashableExpr<'_> { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } +} + +impl Eq for HashableExpr<'_> {} + +impl<'a> From<&'a Expr> for HashableExpr<'a> { + fn from(expr: &'a Expr) -> Self { + /// Returns a version of the given expression that can be hashed and compared according to + /// Python semantics. + fn as_hashable(expr: &Expr) -> ComparableExpr { + match expr { + Expr::Named(named) => ComparableExpr::NamedExpr(ExprNamed { + target: Box::new(ComparableExpr::from(&named.target)), + value: Box::new(as_hashable(&named.value)), + }), + Expr::NumberLiteral(number) => as_bool(number) + .map(|value| ComparableExpr::BoolLiteral(ExprBoolLiteral { value })) + .unwrap_or_else(|| ComparableExpr::from(expr)), + Expr::Tuple(tuple) => ComparableExpr::Tuple(ExprTuple { + elts: tuple.iter().map(as_hashable).collect(), + }), + _ => ComparableExpr::from(expr), + } + } + + /// Returns the `bool` value of the given expression, if it has an equivalent hash to + /// `True` or `False`. + fn as_bool(number: &crate::ExprNumberLiteral) -> Option { + match &number.value { + Number::Int(int) => match int.as_u8() { + Some(0) => Some(false), + Some(1) => Some(true), + _ => None, + }, + Number::Float(float) => match float { + 0.0 => Some(false), + 1.0 => Some(true), + _ => None, + }, + Number::Complex { real, imag } => match (real, imag) { + (0.0, 0.0) => Some(false), + (1.0, 0.0) => Some(true), + _ => None, + }, + } + } + + Self(as_hashable(expr)) + } +}