diff --git a/README.md b/README.md index 654d65dcb9..bcc2e01146 100644 --- a/README.md +++ b/README.md @@ -1094,6 +1094,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI. | PLE1142 | AwaitOutsideAsync | `await` should be used within an async function | | | PLR0206 | PropertyWithParameters | Cannot have defined parameters for properties | | | PLR0402 | ConsiderUsingFromImport | Use `from ... import ...` in lieu of alias | | +| PLR0133 | ConstantComparison | Two constants compared in a comparison, consider replacing `0 == 0` | | | PLR1701 | ConsiderMergingIsinstance | Merge these isinstance calls: `isinstance(..., (...))` | | | PLR1722 | UseSysExit | Use `sys.exit()` instead of `exit` | 🛠 | | PLR2004 | MagicValueComparison | Magic number used in comparison, consider replacing magic with a constant variable | | diff --git a/resources/test/fixtures/pylint/constant_comparison.py b/resources/test/fixtures/pylint/constant_comparison.py new file mode 100644 index 0000000000..e54b0a7059 --- /dev/null +++ b/resources/test/fixtures/pylint/constant_comparison.py @@ -0,0 +1,59 @@ +"""Check that magic values are not used in comparisons""" + +if 100 == 100: # [comparison-of-constants] + pass + +if 1 == 3: # [comparison-of-constants] + pass + +if 1 != 3: # [comparison-of-constants] + pass + +x = 0 +if 4 == 3 == x: # [comparison-of-constants] + pass + +if x == 0: # correct + pass + +y = 1 +if x == y: # correct + pass + +if 1 > 0: # [comparison-of-constants] + pass + +if x > 0: # correct + pass + +if 1 >= 0: # [comparison-of-constants] + pass + +if x >= 0: # correct + pass + +if 1 < 0: # [comparison-of-constants] + pass + +if x < 0: # correct + pass + +if 1 <= 0: # [comparison-of-constants] + pass + +if x <= 0: # correct + pass + +word = "hello" +if word == "": # correct + pass + +if "hello" == "": # [comparison-of-constants] + pass + +truthy = True +if truthy == True: # correct + pass + +if True == False: # [comparison-of-constants] + pass diff --git a/ruff.schema.json b/ruff.schema.json index 847819bfd4..04598a3405 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1443,6 +1443,9 @@ "PLE1142", "PLR", "PLR0", + "PLR01", + "PLR013", + "PLR0133", "PLR02", "PLR020", "PLR0206", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 337f815d7f..917c2f8679 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -2590,6 +2590,10 @@ where ); } + if self.settings.enabled.contains(&RuleCode::PLR0133) { + pylint::rules::constant_comparison(self, left, ops, comparators); + } + if self.settings.enabled.contains(&RuleCode::PLR2004) { pylint::rules::magic_value_comparison(self, left, comparators); } diff --git a/src/pylint/mod.rs b/src/pylint/mod.rs index 55e11bcb17..11b977e38a 100644 --- a/src/pylint/mod.rs +++ b/src/pylint/mod.rs @@ -17,6 +17,7 @@ mod tests { #[test_case(RuleCode::PLE0117, Path::new("nonlocal_without_binding.py"); "PLE0117")] #[test_case(RuleCode::PLE0118, Path::new("used_prior_global_declaration.py"); "PLE0118")] #[test_case(RuleCode::PLE1142, Path::new("await_outside_async.py"); "PLE1142")] + #[test_case(RuleCode::PLR0133, Path::new("constant_comparison.py"); "PLR0133")] #[test_case(RuleCode::PLR0206, Path::new("property_with_parameters.py"); "PLR0206")] #[test_case(RuleCode::PLR0402, Path::new("import_aliasing.py"); "PLR0402")] #[test_case(RuleCode::PLR1701, Path::new("consider_merging_isinstance.py"); "PLR1701")] diff --git a/src/pylint/rules/constant_comparison.rs b/src/pylint/rules/constant_comparison.rs new file mode 100644 index 0000000000..825f67557b --- /dev/null +++ b/src/pylint/rules/constant_comparison.rs @@ -0,0 +1,44 @@ +use itertools::Itertools; +use rustpython_ast::{Cmpop, Expr, ExprKind, Located}; + +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::registry::Diagnostic; +use crate::violations; + +/// PLR0133 +pub fn constant_comparison( + checker: &mut Checker, + left: &Expr, + ops: &[Cmpop], + comparators: &[Expr], +) { + for ((left, right), op) in std::iter::once(left) + .chain(comparators.iter()) + .tuple_windows::<(&Located<_>, &Located<_>)>() + .zip(ops) + { + if let ( + ExprKind::Constant { + value: left_constant, + .. + }, + ExprKind::Constant { + value: right_constant, + .. + }, + ) = (&left.node, &right.node) + { + let diagnostic = Diagnostic::new( + violations::ConstantComparison { + left_constant: left_constant.to_string(), + op: op.into(), + right_constant: right_constant.to_string(), + }, + Range::from_located(left), + ); + + checker.diagnostics.push(diagnostic); + }; + } +} diff --git a/src/pylint/rules/mod.rs b/src/pylint/rules/mod.rs index b9d3f87919..82cb47d13e 100644 --- a/src/pylint/rules/mod.rs +++ b/src/pylint/rules/mod.rs @@ -1,4 +1,5 @@ pub use await_outside_async::await_outside_async; +pub use constant_comparison::constant_comparison; pub use magic_value_comparison::magic_value_comparison; pub use merge_isinstance::merge_isinstance; pub use misplaced_comparison_constant::misplaced_comparison_constant; @@ -11,6 +12,7 @@ pub use useless_else_on_loop::useless_else_on_loop; pub use useless_import_alias::useless_import_alias; mod await_outside_async; +mod constant_comparison; mod magic_value_comparison; mod merge_isinstance; mod misplaced_comparison_constant; diff --git a/src/pylint/snapshots/ruff__pylint__tests__PLR0133_constant_comparison.py.snap b/src/pylint/snapshots/ruff__pylint__tests__PLR0133_constant_comparison.py.snap new file mode 100644 index 0000000000..1f2e93a8dc --- /dev/null +++ b/src/pylint/snapshots/ruff__pylint__tests__PLR0133_constant_comparison.py.snap @@ -0,0 +1,135 @@ +--- +source: src/pylint/mod.rs +expression: diagnostics +--- +- kind: + ConstantComparison: + left_constant: "100" + op: Eq + right_constant: "100" + location: + row: 3 + column: 3 + end_location: + row: 3 + column: 6 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "1" + op: Eq + right_constant: "3" + location: + row: 6 + column: 3 + end_location: + row: 6 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "1" + op: NotEq + right_constant: "3" + location: + row: 9 + column: 3 + end_location: + row: 9 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "4" + op: Eq + right_constant: "3" + location: + row: 13 + column: 3 + end_location: + row: 13 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "1" + op: Gt + right_constant: "0" + location: + row: 23 + column: 3 + end_location: + row: 23 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "1" + op: GtE + right_constant: "0" + location: + row: 29 + column: 3 + end_location: + row: 29 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "1" + op: Lt + right_constant: "0" + location: + row: 35 + column: 3 + end_location: + row: 35 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "1" + op: LtE + right_constant: "0" + location: + row: 41 + column: 3 + end_location: + row: 41 + column: 4 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "'hello'" + op: Eq + right_constant: "''" + location: + row: 51 + column: 3 + end_location: + row: 51 + column: 10 + fix: ~ + parent: ~ +- kind: + ConstantComparison: + left_constant: "True" + op: Eq + right_constant: "False" + location: + row: 58 + column: 3 + end_location: + row: 58 + column: 7 + fix: ~ + parent: ~ + diff --git a/src/registry.rs b/src/registry.rs index 4c8f3bd6e7..04afdbc1ec 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -187,6 +187,7 @@ define_rule_mapping!( PLE1142 => violations::AwaitOutsideAsync, PLR0206 => violations::PropertyWithParameters, PLR0402 => violations::ConsiderUsingFromImport, + PLR0133 => violations::ConstantComparison, PLR1701 => violations::ConsiderMergingIsinstance, PLR1722 => violations::UseSysExit, PLR2004 => violations::MagicValueComparison, diff --git a/src/violations.rs b/src/violations.rs index c920cb1ff7..25742ac599 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -1157,6 +1157,85 @@ impl Violation for ConsiderUsingFromImport { } } +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] +pub enum ViolationsCmpop { + Eq, + NotEq, + Lt, + LtE, + Gt, + GtE, + Is, + IsNot, + In, + NotIn, +} + +impl From<&Cmpop> for ViolationsCmpop { + fn from(cmpop: &Cmpop) -> Self { + match cmpop { + Cmpop::Eq => Self::Eq, + Cmpop::NotEq => Self::NotEq, + Cmpop::Lt => Self::Lt, + Cmpop::LtE => Self::LtE, + Cmpop::Gt => Self::Gt, + Cmpop::GtE => Self::GtE, + Cmpop::Is => Self::Is, + Cmpop::IsNot => Self::IsNot, + Cmpop::In => Self::In, + Cmpop::NotIn => Self::NotIn, + } + } +} + +impl fmt::Display for ViolationsCmpop { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let representation = match self { + Self::Eq => "==", + Self::NotEq => "!=", + Self::Lt => "<", + Self::LtE => "<=", + Self::Gt => ">", + Self::GtE => ">=", + Self::Is => "is", + Self::IsNot => "is not", + Self::In => "in", + Self::NotIn => "not in", + }; + write!(f, "{representation}") + } +} + +define_violation!( + pub struct ConstantComparison { + pub left_constant: String, + pub op: ViolationsCmpop, + pub right_constant: String, + } +); +impl Violation for ConstantComparison { + fn message(&self) -> String { + let ConstantComparison { + left_constant, + op, + right_constant, + } = self; + + format!( + "Two constants compared in a comparison, consider replacing `{left_constant} {op} \ + {right_constant}`" + ) + } + + fn placeholder() -> Self { + ConstantComparison { + left_constant: "0".to_string(), + op: ViolationsCmpop::Eq, + right_constant: "0".to_string(), + } + } +} + define_violation!( pub struct ConsiderMergingIsinstance(pub String, pub Vec); );