Skip to content

Commit

Permalink
Merge rules
Browse files Browse the repository at this point in the history
  • Loading branch information
@tibor-reiss
tibor-reiss committed Feb 17, 2024
1 parent 595ec4c commit 320aea4
Show file tree
Hide file tree
Showing 10 changed files with 205 additions and 343 deletions.
82 changes: 0 additions & 82 deletions crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py
View file

This file was deleted.

47 changes: 47 additions & 0 deletions ...test/fixtures/pylint/max_instead_of_if.py → .../fixtures/pylint/min_max_instead_of_if.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
if value < value2: # [max-instead-of-if]
value = value2

if value > 10: # [min-instead-of-if]
value = 10

if value >= 10: # [min-instead-of-if]
value = 10

if value > value2: # [min-instead-of-if]
value = value2


class A:
def __init__(self):
Expand All @@ -24,6 +33,9 @@ def __init__(self):
if A1.value < 10: # [max-instead-of-if]
A1.value = 10

if A1.value > 10: # [min-instead-of-if]
A1.value = 10


class AA:
def __init__(self, value):
Expand Down Expand Up @@ -51,6 +63,12 @@ def __le__(self, b):
if A2 <= A1: # [max-instead-of-if]
A2 = A1

if A2 > A1: # [min-instead-of-if]
A2 = A1

if A2 >= A1: # [min-instead-of-if]
A2 = A1

# Negative
if value < 10:
value = 2
Expand Down Expand Up @@ -80,3 +98,32 @@ def __le__(self, b):
value = 5
elif value == 3:
value = 2

if value > 10:
value = 2

if value >= 3:
value = 5

if value > 10:
value = 2
value2 = 3

if value > value2:
value = value3

if value > 5:
value = value3

if 2 > value >= 3:
value = 1

if value > 10:
value = 10
else:
value = 3

if value >= 3:
value = 5
elif value == 3:
value = 2
7 changes: 2 additions & 5 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1130,11 +1130,8 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::TooManyBooleanExpressions) {
pylint::rules::too_many_boolean_expressions(checker, if_);
}
if checker.enabled(Rule::MaxInsteadOfIf) {
pylint::rules::max_instead_of_if(checker, if_);
}
if checker.enabled(Rule::MinInsteadOfIf) {
pylint::rules::min_instead_of_if(checker, if_);
if checker.enabled(Rule::MinMaxInsteadOfIf) {
pylint::rules::min_max_instead_of_if(checker, if_);
}
if checker.source_type.is_stub() {
if checker.any_enabled(&[
Expand Down
3 changes: 1 addition & 2 deletions crates/ruff_linter/src/codes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
(Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary),
(Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias),
(Pylint, "R1730") => (RuleGroup::Preview, rules::pylint::rules::MinInsteadOfIf),
(Pylint, "R1731") => (RuleGroup::Preview, rules::pylint::rules::MaxInsteadOfIf),
(Pylint, "R1730") => (RuleGroup::Preview, rules::pylint::rules::MinMaxInsteadOfIf),
(Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),
(Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup),
(Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison),
Expand Down
3 changes: 1 addition & 2 deletions crates/ruff_linter/src/rules/pylint/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ mod tests {
#[test_case(Rule::EqWithoutHash, Path::new("eq_without_hash.py"))]
#[test_case(Rule::EmptyComment, Path::new("empty_comment.py"))]
#[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))]
#[test_case(Rule::MaxInsteadOfIf, Path::new("max_instead_of_if.py"))]
#[test_case(Rule::MinInsteadOfIf, Path::new("min_instead_of_if.py"))]
#[test_case(Rule::MinMaxInsteadOfIf, Path::new("min_max_instead_of_if.py"))]
#[test_case(Rule::SingleStringSlots, Path::new("single_string_slots.py"))]
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_0.py"))]
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_1.py"))]
Expand Down
147 changes: 24 additions & 123 deletions crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ use ruff_text_size::{Ranged, TextRange};
use crate::checkers::ast::Checker;

/// ## What it does
/// Check for an if node that can be refactored as a max python builtin.
/// Check for an if node that can be refactored as a min/max python builtin.
///
/// ## Why is this bad?
/// An if block where the test and assignment have the same structure can
/// be expressed more concisely by using the python builtin max function.
/// be expressed more concisely by using the python builtin min/max function.
///
/// ## Example
/// ```python
Expand All @@ -28,54 +28,22 @@ use crate::checkers::ast::Checker;
///
/// ## References
/// - [Python documentation: max function](https://docs.python.org/3/library/functions.html#max)
#[violation]
pub struct MaxInsteadOfIf {
contents: String,
}

impl Violation for MaxInsteadOfIf {
#[derive_message_formats]
fn message(&self) -> String {
let MaxInsteadOfIf { contents } = self;
format!("Consider using `{contents}` instead of unnecessary if block")
}
}

/// ## What it does
/// Check for an if node that can be refactored as a min python builtin.
///
/// ## Why is this bad?
/// An if block where the test and assignment have the same structure can
/// be expressed more concisely by using the python builtin min function.
///
/// ## Example
/// ```python
/// if value > 10:
/// value = 10
/// ```
///
/// Use instead:
/// ```python
/// value = min(value, 10)
/// ```
///
/// ## References
/// - [Python documentation: min function](https://docs.python.org/3/library/functions.html#min)
#[violation]
pub struct MinInsteadOfIf {
pub struct MinMaxInsteadOfIf {
contents: String,
}

impl Violation for MinInsteadOfIf {
impl Violation for MinMaxInsteadOfIf {
#[derive_message_formats]
fn message(&self) -> String {
let MinInsteadOfIf { contents } = self;
let MinMaxInsteadOfIf { contents } = self;
format!("Consider using `{contents}` instead of unnecessary if block")
}
}

/// R1730
pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) {
/// R1730 (and also R1731)
pub(crate) fn min_max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) {
let ast::StmtIf {
test,
body,
Expand Down Expand Up @@ -117,9 +85,11 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) {
return;
};

if !matches!(op, CmpOp::Gt | CmpOp::GtE) {
return;
}
let min_or_max = match op {
CmpOp::Gt | CmpOp::GtE => MinMax::Min,
CmpOp::Lt | CmpOp::LtE => MinMax::Max,
_ => return,
};

let left_cmp = ComparableExpr::from(left);
let body_target_cmp = ComparableExpr::from(body_target);
Expand All @@ -130,7 +100,7 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) {
}

let func_node = ast::ExprName {
id: "min".into(),
id: min_or_max.as_str().into(),
ctx: ExprContext::Load,
range: TextRange::default(),
};
Expand All @@ -149,93 +119,24 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) {
range: TextRange::default(),
};
let diagnostic = Diagnostic::new(
MinInsteadOfIf {
MinMaxInsteadOfIf {
contents: checker.generator().stmt(&assign_node.into()),
},
stmt_if.range(),
);
checker.diagnostics.push(diagnostic);
}

/// R1731
pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) {
let ast::StmtIf {
test,
body,
elif_else_clauses,
range: _,
} = stmt_if;

if !elif_else_clauses.is_empty() {
return;
}

let [Stmt::Assign(ast::StmtAssign {
targets: body_targets,
value: body_value,
..
})] = body.as_slice()
else {
return;
};
let [body_target] = body_targets.as_slice() else {
return;
};

let Some(ast::ExprCompare {
ops,
left,
comparators,
..
}) = test.as_compare_expr()
else {
return;
};

if !(!body_target.is_subscript_expr() && !left.is_subscript_expr()) {
return;
}

let ([op], [right_statement]) = (&**ops, &**comparators) else {
return;
};

if !matches!(op, CmpOp::Lt | CmpOp::LtE) {
return;
}
enum MinMax {
Min,
Max,
}

let left_cmp = ComparableExpr::from(left);
let body_target_cmp = ComparableExpr::from(body_target);
let right_statement_cmp = ComparableExpr::from(right_statement);
let body_value_cmp = ComparableExpr::from(body_value);
if left_cmp != body_target_cmp || right_statement_cmp != body_value_cmp {
return;
impl MinMax {
fn as_str(&self) -> &'static str {
match self {
Self::Min => "min",
Self::Max => "max",
}
}

let func_node = ast::ExprName {
id: "max".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
};
let value_node = ast::ExprCall {
func: Box::new(func_node.into()),
arguments: Arguments {
args: Box::from([body_target.clone(), body_value.deref().clone()]),
keywords: Box::from([]),
range: TextRange::default(),
},
range: TextRange::default(),
};
let assign_node = ast::StmtAssign {
targets: vec![body_target.clone()],
value: Box::new(value_node.into()),
range: TextRange::default(),
};
let diagnostic = Diagnostic::new(
MaxInsteadOfIf {
contents: checker.generator().stmt(&assign_node.into()),
},
stmt_if.range(),
);
checker.diagnostics.push(diagnostic);
}

0 comments on commit 320aea4

Please sign in to comment.