From 51fde586d219e526e4963b20ee3ed1c7902e48d5 Mon Sep 17 00:00:00 2001 From: Arjun Munji Date: Mon, 19 Feb 2024 00:19:29 +0000 Subject: [PATCH 1/3] omit repeatd equality comparision rule for sys.{version,platform} --- .../rules/pylint/rules/repeated_equality_comparison.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs index b4c5a792256d09..8651c10cd79fa0 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs @@ -196,6 +196,14 @@ fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool { return false; } + for comparator in comparators.iter() { + if let Some(expr_str) = comparator.to_str() { + if expr_str.starts_with("sys.platform") || expr_str.starts_with("sys.version") { + return false; + } + } + } + true } From ce4374ea94c1f561d8e35033d51bf80369d4e9b8 Mon Sep 17 00:00:00 2001 From: Arjun Munji Date: Mon, 19 Feb 2024 23:41:22 +0000 Subject: [PATCH 2/3] fix corectness & add test case --- .../pylint/repeated_equality_comparison.py | 2 ++ .../rules/repeated_equality_comparison.rs | 17 +++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py b/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py index 6c3f694ba87cdc..4455d4140faee5 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py @@ -51,3 +51,5 @@ foo[0] == "a" or foo[0] == "b" # Subscripts. foo() == "a" or foo() == "b" # Calls. + +sys.platform == "win32" or sys.platform == "emscripten" # sys attributes diff --git a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs index 8651c10cd79fa0..350a02d7835f4c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs @@ -2,6 +2,7 @@ use std::hash::BuildHasherDefault; use std::ops::Deref; use itertools::{any, Itertools}; +use regex::Regex; use rustc_hash::FxHashMap; use ast::ExprContext; @@ -196,12 +197,16 @@ fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool { return false; } - for comparator in comparators.iter() { - if let Some(expr_str) = comparator.to_str() { - if expr_str.starts_with("sys.platform") || expr_str.starts_with("sys.version") { - return false; - } - } + if left.is_attribute_expr() + && left + .to_owned() + .expect_attribute_expr() + .value + .name_expr() + .is_some_and(|f| f.id == "sys") + { + let excluder = Regex::new(r"^(platform|version)").unwrap(); + return !excluder.is_match(left.clone().expect_attribute_expr().attr.as_str()); } true From 8678008e7cfa20587b1020ced5186ad9cc4db3e5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 20 Feb 2024 13:56:42 -0500 Subject: [PATCH 3/3] Use semantic model --- .../pylint/repeated_equality_comparison.py | 4 ++- .../rules/repeated_equality_comparison.rs | 25 +++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py b/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py index 4455d4140faee5..8eeec8bdafbbce 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/repeated_equality_comparison.py @@ -52,4 +52,6 @@ foo() == "a" or foo() == "b" # Calls. -sys.platform == "win32" or sys.platform == "emscripten" # sys attributes +import sys + +sys.platform == "win32" or sys.platform == "emscripten" # sys attributes diff --git a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs index 350a02d7835f4c..25f36d63a1bd96 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/repeated_equality_comparison.rs @@ -2,7 +2,6 @@ use std::hash::BuildHasherDefault; use std::ops::Deref; use itertools::{any, Itertools}; -use regex::Regex; use rustc_hash::FxHashMap; use ast::ExprContext; @@ -10,7 +9,9 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::hashable::HashableExpr; +use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::{self as ast, BoolOp, CmpOp, Expr}; +use ruff_python_semantic::SemanticModel; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -75,7 +76,7 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast: if bool_op .values .iter() - .any(|value| !is_allowed_value(bool_op.op, value)) + .any(|value| !is_allowed_value(bool_op.op, value, checker.semantic())) { return; } @@ -158,7 +159,7 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast: /// Return `true` if the given expression is compatible with a membership test. /// E.g., `==` operators can be joined with `or` and `!=` operators can be /// joined with `and`. -fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool { +fn is_allowed_value(bool_op: BoolOp, value: &Expr, semantic: &SemanticModel) -> bool { let Expr::Compare(ast::ExprCompare { left, ops, @@ -197,16 +198,14 @@ fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool { return false; } - if left.is_attribute_expr() - && left - .to_owned() - .expect_attribute_expr() - .value - .name_expr() - .is_some_and(|f| f.id == "sys") - { - let excluder = Regex::new(r"^(platform|version)").unwrap(); - return !excluder.is_match(left.clone().expect_attribute_expr().attr.as_str()); + // Ignore `sys.version_info` and `sys.platform` comparisons, which are only + // respected by type checkers when enforced via equality. + if any_over_expr(value, &|expr| { + semantic.resolve_call_path(expr).is_some_and(|call_path| { + matches!(call_path.as_slice(), ["sys", "version_info" | "platform"]) + }) + }) { + return false; } true