Skip to content

Commit

Permalink
Avoid suggesting set rewrites for non-hashable types
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Feb 12, 2024
1 parent e2785f3 commit c4d5763
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
1 in (
1, 2, 3
)

# OK
fruits = ["cherry", "grapes"]
"cherry" in fruits

# OK
fruits in [[1, 2, 3], [4, 5, 6]]
fruits in [1, 2, 3]
1 in [[1, 2, 3], [4, 5, 6]]
42 changes: 40 additions & 2 deletions crates/ruff_linter/src/rules/pylint/rules/literal_membership.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, CmpOp, Expr};
use ruff_python_semantic::analyze::typing;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand All @@ -25,7 +26,8 @@ use crate::checkers::ast::Checker;
/// ## Fix safety
/// This rule's fix is marked as unsafe, as the use of a `set` literal will
/// error at runtime if the sequence contains unhashable elements (like lists
/// or dictionaries).
/// or dictionaries). While Ruff will attempt to infer the hashability of the
/// elements, it may not always be able to do so.
///
/// ## References
/// - [What’s New In Python 3.2](https://docs.python.org/3/whatsnew/3.2.html#optimizations)
Expand Down Expand Up @@ -57,7 +59,43 @@ pub(crate) fn literal_membership(checker: &mut Checker, compare: &ast::ExprCompa
return;
};

if !matches!(right, Expr::List(_) | Expr::Tuple(_)) {
let elts = match right {
Expr::List(ast::ExprList { elts, .. }) => elts,
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts,
_ => return,
};

// If `left`, or any of the elements in `right`, are known to _not_ be hashable, return.
if std::iter::once(compare.left.as_ref())
.chain(elts)
.any(|expr| match expr {
// Expressions that are known _not_ to be hashable.
Expr::List(_)
| Expr::Set(_)
| Expr::Dict(_)
| Expr::ListComp(_)
| Expr::SetComp(_)
| Expr::DictComp(_)
| Expr::GeneratorExp(_)
| Expr::Await(_)
| Expr::Yield(_)
| Expr::YieldFrom(_) => true,
// Expressions that can be _inferred_ not to be hashable.
Expr::Name(name) => {
let Some(binding) = checker
.semantic()
.only_binding(name)
.map(|id| checker.semantic().binding(id))
else {
return false;
};
typing::is_list(binding, checker.semantic())
|| typing::is_dict(binding, checker.semantic())
|| typing::is_set(binding, checker.semantic())
}
_ => false,
})
{
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ literal_membership.py:4:6: PLR6201 [*] Use a `set` literal when testing for memb
5 | | 1, 2, 3
6 | | )
| |_^ PLR6201
7 |
8 | # OK
7 | fruits = ["cherry", "grapes"]
8 | "cherry" in fruits
|
= help: Convert to `set`

Expand All @@ -62,8 +62,8 @@ literal_membership.py:4:6: PLR6201 [*] Use a `set` literal when testing for memb
5 5 | 1, 2, 3
6 |-)
6 |+}
7 7 |
8 8 | # OK
9 9 | fruits = ["cherry", "grapes"]
7 7 | fruits = ["cherry", "grapes"]
8 8 | "cherry" in fruits
9 9 |


0 comments on commit c4d5763

Please sign in to comment.