Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Refactor literal-comparison and not-test rules (#6636) 

## Summary

No behavior changes, but these need some refactoring to support
https://github.com/astral-sh/ruff/pull/6575 (namely, they need to take
the `ast::ExprCompare` or similar node instead of the attribute fields),
and I don't want to muddy that PR.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2023-08-16 21:02:30 -04:00 committed by GitHub
parent 97ae9e7433
commit 036035bc50
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 113 additions and 135 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
crates/ruff/src/checkers/ast/analyze/expression.rs
View File

@ -1101,22 +1101,15 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
}
}
}
Expr::UnaryOp(ast::ExprUnaryOp {
op,
operand,
range: _,
}) => {
let check_not_in = checker.enabled(Rule::NotInTest);
let check_not_is = checker.enabled(Rule::NotIsTest);
if check_not_in || check_not_is {
pycodestyle::rules::not_tests(
checker,
expr,
*op,
operand,
check_not_in,
check_not_is,
);
Expr::UnaryOp(
unary_op @ ast::ExprUnaryOp {
op,
operand,
range: _,
},
) => {
if checker.any_enabled(&[Rule::NotInTest, Rule::NotIsTest]) {
pycodestyle::rules::not_tests(checker, unary_op);
}
if checker.enabled(Rule::UnaryPrefixIncrementDecrement) {
flake8_bugbear::rules::unary_prefix_increment_decrement(
@ -1141,18 +1134,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
range: _,
},
) => {
let check_none_comparisons = checker.enabled(Rule::NoneComparison);
let check_true_false_comparisons = checker.enabled(Rule::TrueFalseComparison);
if check_none_comparisons || check_true_false_comparisons {
pycodestyle::rules::literal_comparisons(
checker,
expr,
left,
ops,
comparators,
check_none_comparisons,
check_true_false_comparisons,
);
if checker.any_enabled(&[Rule::NoneComparison, Rule::TrueFalseComparison]) {
pycodestyle::rules::literal_comparisons(checker, compare);
}
if checker.enabled(Rule::IsLiteral) {
pyflakes::rules::invalid_literal_comparison(checker, left, ops, comparators, expr);

2
crates/ruff/src/rules/pycodestyle/helpers.rs
View File

@ -10,7 +10,7 @@ pub(super) fn is_ambiguous_name(name: &str) -> bool {
name == "l" || name == "I" || name == "O"
}
pub(super) fn compare(
pub(super) fn generate_comparison(
left: &Expr,
ops: &[CmpOp],
comparators: &[Expr],

116
crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs
View File

@ -1,15 +1,15 @@
use itertools::izip;
use ruff_python_ast::{self as ast, CmpOp, Constant, Expr, Ranged};
use rustc_hash::FxHashMap;
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers;
use ruff_python_ast::helpers::is_const_none;
use ruff_python_ast::{self as ast, CmpOp, Constant, Expr, Ranged};
use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::registry::AsRule;
use crate::rules::pycodestyle::helpers::compare;
use crate::rules::pycodestyle::helpers::generate_comparison;
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
enum EqCmpOp {
@ -130,15 +130,7 @@ impl AlwaysAutofixableViolation for TrueFalseComparison {
}
/// E711, E712
pub(crate) fn literal_comparisons(
checker: &mut Checker,
expr: &Expr,
left: &Expr,
ops: &[CmpOp],
comparators: &[Expr],
check_none_comparisons: bool,
check_true_false_comparisons: bool,
) {
pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprCompare) {
// Mapping from (bad operator index) to (replacement operator). As we iterate
// through the list of operators, we apply "dummy" fixes for each error,
// then replace the entire expression at the end with one "real" fix, to
@ -146,15 +138,18 @@ pub(crate) fn literal_comparisons(
let mut bad_ops: FxHashMap<usize, CmpOp> = FxHashMap::default();
let mut diagnostics: Vec<Diagnostic> = vec![];
let op = ops.first().unwrap();
// Check `left`.
let mut comparator = left;
let next = &comparators[0];
let mut comparator = compare.left.as_ref();
let [op, ..] = compare.ops.as_slice() else {
return;
};
let [next, ..] = compare.comparators.as_slice() else {
return;
};
if !helpers::is_constant_non_singleton(next) {
if let Some(op) = EqCmpOp::try_from(*op) {
if check_none_comparisons && is_const_none(comparator) {
if checker.enabled(Rule::NoneComparison) && is_const_none(comparator) {
match op {
EqCmpOp::Eq => {
let diagnostic = Diagnostic::new(NoneComparison(op), comparator.range());
@ -173,7 +168,7 @@ pub(crate) fn literal_comparisons(
}
}
if check_true_false_comparisons {
if checker.enabled(Rule::TrueFalseComparison) {
if let Expr::Constant(ast::ExprConstant {
value: Constant::Bool(value),
kind: None,
@ -208,59 +203,66 @@ pub(crate) fn literal_comparisons(
}
// Check each comparator in order.
for (idx, (op, next)) in izip!(ops, comparators).enumerate() {
for (index, (op, next)) in compare
.ops
.iter()
.zip(compare.comparators.iter())
.enumerate()
{
if helpers::is_constant_non_singleton(comparator) {
comparator = next;
continue;
}
if let Some(op) = EqCmpOp::try_from(*op) {
if check_none_comparisons && is_const_none(next) {
let Some(op) = EqCmpOp::try_from(*op) else {
continue;
};
if checker.enabled(Rule::NoneComparison) && is_const_none(next) {
match op {
EqCmpOp::Eq => {
let diagnostic = Diagnostic::new(NoneComparison(op), next.range());
if checker.patch(diagnostic.kind.rule()) {
bad_ops.insert(index, CmpOp::Is);
}
diagnostics.push(diagnostic);
}
EqCmpOp::NotEq => {
let diagnostic = Diagnostic::new(NoneComparison(op), next.range());
if checker.patch(diagnostic.kind.rule()) {
bad_ops.insert(index, CmpOp::IsNot);
}
diagnostics.push(diagnostic);
}
}
}
if checker.enabled(Rule::TrueFalseComparison) {
if let Expr::Constant(ast::ExprConstant {
value: Constant::Bool(value),
kind: None,
range: _,
}) = next
{
match op {
EqCmpOp::Eq => {
let diagnostic = Diagnostic::new(NoneComparison(op), next.range());
let diagnostic =
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
if checker.patch(diagnostic.kind.rule()) {
bad_ops.insert(idx, CmpOp::Is);
bad_ops.insert(index, CmpOp::Is);
}
diagnostics.push(diagnostic);
}
EqCmpOp::NotEq => {
let diagnostic = Diagnostic::new(NoneComparison(op), next.range());
let diagnostic =
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
if checker.patch(diagnostic.kind.rule()) {
bad_ops.insert(idx, CmpOp::IsNot);
bad_ops.insert(index, CmpOp::IsNot);
}
diagnostics.push(diagnostic);
}
}
}
if check_true_false_comparisons {
if let Expr::Constant(ast::ExprConstant {
value: Constant::Bool(value),
kind: None,
range: _,
}) = next
{
match op {
EqCmpOp::Eq => {
let diagnostic =
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
if checker.patch(diagnostic.kind.rule()) {
bad_ops.insert(idx, CmpOp::Is);
}
diagnostics.push(diagnostic);
}
EqCmpOp::NotEq => {
let diagnostic =
Diagnostic::new(TrueFalseComparison(*value, op), next.range());
if checker.patch(diagnostic.kind.rule()) {
bad_ops.insert(idx, CmpOp::IsNot);
}
diagnostics.push(diagnostic);
}
}
}
}
}
comparator = next;
@ -270,17 +272,19 @@ pub(crate) fn literal_comparisons(
// `noqa`, but another doesn't, both will be removed here.
if !bad_ops.is_empty() {
// Replace the entire comparison expression.
let ops = ops
let ops = compare
.ops
.iter()
.enumerate()
.map(|(idx, op)| bad_ops.get(&idx).unwrap_or(op))
.copied()
.collect::<Vec<_>>();
let content = compare(left, &ops, comparators, checker.locator());
let content =
generate_comparison(&compare.left, &ops, &compare.comparators, checker.locator());
for diagnostic in &mut diagnostics {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
content.to_string(),
expr.range(),
compare.range(),
)));
}
}

91
crates/ruff/src/rules/pycodestyle/rules/not_tests.rs
View File

@ -1,11 +1,10 @@
use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged, UnaryOp};
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, CmpOp, Expr, Ranged};
use crate::checkers::ast::Checker;
use crate::registry::AsRule;
use crate::rules::pycodestyle::helpers::compare;
use crate::registry::{AsRule, Rule};
use crate::rules::pycodestyle::helpers::generate_comparison;
/// ## What it does
/// Checks for negative comparison using `not {foo} in {bar}`.
@ -74,54 +73,46 @@ impl AlwaysAutofixableViolation for NotIsTest {
}
/// E713, E714
pub(crate) fn not_tests(
checker: &mut Checker,
expr: &Expr,
op: UnaryOp,
operand: &Expr,
check_not_in: bool,
check_not_is: bool,
) {
if matches!(op, UnaryOp::Not) {
if let Expr::Compare(ast::ExprCompare {
left,
ops,
comparators,
range: _,
}) = operand
{
if !matches!(&ops[..], [CmpOp::In | CmpOp::Is]) {
return;
}
for op in ops {
match op {
CmpOp::In => {
if check_not_in {
let mut diagnostic = Diagnostic::new(NotInTest, operand.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
compare(left, &[CmpOp::NotIn], comparators, checker.locator()),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
CmpOp::Is => {
if check_not_is {
let mut diagnostic = Diagnostic::new(NotIsTest, operand.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
compare(left, &[CmpOp::IsNot], comparators, checker.locator()),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
_ => {}
pub(crate) fn not_tests(checker: &mut Checker, unary_op: &ast::ExprUnaryOp) {
if !unary_op.op.is_not() {
return;
}
let Expr::Compare(ast::ExprCompare {
left,
ops,
comparators,
range: _,
}) = unary_op.operand.as_ref()
else {
return;
};
match ops.as_slice() {
[CmpOp::In] => {
if checker.enabled(Rule::NotInTest) {
let mut diagnostic = Diagnostic::new(NotInTest, unary_op.operand.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
generate_comparison(left, &[CmpOp::NotIn], comparators, checker.locator()),
unary_op.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
[CmpOp::Is] => {
if checker.enabled(Rule::NotIsTest) {
let mut diagnostic = Diagnostic::new(NotIsTest, unary_op.operand.range());
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
generate_comparison(left, &[CmpOp::IsNot], comparators, checker.locator()),
unary_op.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
_ => {}
}
}