Skip to content

Commit

Permalink
Omit repeated equality comparison for sys (astral-sh#10054)
Browse files Browse the repository at this point in the history
## Summary
Update PLR1714 to ignore `sys.platform` and `sys.version` checks. 
I'm not sure if these checks or if we need to add more. Please advise.

Fixes astral-sh#10017

## Test Plan
Added a new test case and ran `cargo nextest run`
  • Loading branch information
arjunnn authored and nkxxll committed Mar 4, 2024
1 parent aa59a37 commit 92e3445
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,7 @@
foo[0] == "a" or foo[0] == "b" # Subscripts.

foo() == "a" or foo() == "b" # Calls.

import sys

sys.platform == "win32" or sys.platform == "emscripten" # sys attributes
Original file line number Diff line number Diff line change
Expand Up @@ -9,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};

Expand Down Expand Up @@ -74,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;
}
Expand Down Expand Up @@ -157,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,
Expand Down Expand Up @@ -196,6 +198,16 @@ fn is_allowed_value(bool_op: BoolOp, value: &Expr) -> bool {
return false;
}

// 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
}

Expand Down

0 comments on commit 92e3445

Please sign in to comment.