From 969a6f0d53d1642d904a8bbe566b8eb931e67d98 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 18 Jan 2023 18:42:49 -0500 Subject: [PATCH] Replace misplaced-comparison-constant with SIM300 (#1980) Closes: #1954. --- BREAKING_CHANGES.md | 7 ++ README.md | 1 - .../test/fixtures/flake8_simplify/SIM300.py | 3 + ruff.schema.json | 4 - src/checkers/ast.rs | 10 -- src/registry.rs | 1 - .../flake8_simplify/rules/yoda_conditions.rs | 32 ++++-- ...ke8_simplify__tests__SIM300_SIM300.py.snap | 63 ++++++++++- src/rules/pylint/mod.rs | 1 - .../rules/misplaced_comparison_constant.rs | 56 --------- src/rules/pylint/rules/mod.rs | 2 - ...2201_misplaced_comparison_constant.py.snap | 107 ------------------ src/violations.rs | 33 +----- 13 files changed, 96 insertions(+), 224 deletions(-) delete mode 100644 src/rules/pylint/rules/misplaced_comparison_constant.rs delete mode 100644 src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC2201_misplaced_comparison_constant.py.snap diff --git a/BREAKING_CHANGES.md b/BREAKING_CHANGES.md index ea3700f369c0d..b0577a6c2b333 100644 --- a/BREAKING_CHANGES.md +++ b/BREAKING_CHANGES.md @@ -1,5 +1,12 @@ # Breaking Changes +## 0.0.226 + +### `misplaced-comparison-constant` (`PLC2201`) was deprecated in favor of `SIM300` ([#1980](https://github.com/charliermarsh/ruff/pull/1980)) + +These two rules contain (nearly) identical logic. To deduplicate the rule set, we've upgraded +`SIM300` to handle a few more cases, and deprecated `PLC2201` in favor of `SIM300`. + ## 0.0.225 ### `@functools.cache` rewrites have been moved to a standalone rule (`UP033`) ([#1938](https://github.com/charliermarsh/ruff/pull/1938)) diff --git a/README.md b/README.md index 52cd272fe3872..b0a70fc8b9271 100644 --- a/README.md +++ b/README.md @@ -1112,7 +1112,6 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI. | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | | PLC0414 | UselessImportAlias | Import alias does not rename original package | 🛠 | -| PLC2201 | MisplacedComparisonConstant | Comparison should be ... | 🛠 | | PLC3002 | UnnecessaryDirectLambdaCall | Lambda expression called directly. Execute the expression inline instead. | | #### Error (PLE) diff --git a/resources/test/fixtures/flake8_simplify/SIM300.py b/resources/test/fixtures/flake8_simplify/SIM300.py index 7cf4d1fba5516..6aac1c20f45a9 100644 --- a/resources/test/fixtures/flake8_simplify/SIM300.py +++ b/resources/test/fixtures/flake8_simplify/SIM300.py @@ -2,6 +2,9 @@ "yoda" == compare # SIM300 'yoda' == compare # SIM300 42 == age # SIM300 +"yoda" <= compare # SIM300 +'yoda' < compare # SIM300 +42 > age # SIM300 # OK compare == "yoda" diff --git a/ruff.schema.json b/ruff.schema.json index 821cd7cf43439..2152c6ae12ff2 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1484,10 +1484,6 @@ "PLC04", "PLC041", "PLC0414", - "PLC2", - "PLC22", - "PLC220", - "PLC2201", "PLC3", "PLC30", "PLC300", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 4688acd21774a..c8c949068fa71 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -2552,16 +2552,6 @@ where ); } - if self.settings.rules.enabled(&RuleCode::PLC2201) { - pylint::rules::misplaced_comparison_constant( - self, - expr, - left, - ops, - comparators, - ); - } - if self.settings.rules.enabled(&RuleCode::PLR0133) { pylint::rules::constant_comparison(self, left, ops, comparators); } diff --git a/src/registry.rs b/src/registry.rs index a9bb57d67f3ff..036c282cf3b0e 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -80,7 +80,6 @@ ruff_macros::define_rule_mapping!( F901 => violations::RaiseNotImplemented, // pylint PLC0414 => violations::UselessImportAlias, - PLC2201 => violations::MisplacedComparisonConstant, PLC3002 => violations::UnnecessaryDirectLambdaCall, PLE0117 => violations::NonlocalWithoutBinding, PLE0118 => violations::UsedPriorGlobalDeclaration, diff --git a/src/rules/flake8_simplify/rules/yoda_conditions.rs b/src/rules/flake8_simplify/rules/yoda_conditions.rs index dccf39c74c7fb..72763ace36d20 100644 --- a/src/rules/flake8_simplify/rules/yoda_conditions.rs +++ b/src/rules/flake8_simplify/rules/yoda_conditions.rs @@ -14,17 +14,20 @@ pub fn yoda_conditions( ops: &[Cmpop], comparators: &[Expr], ) { - if !matches!(ops[..], [Cmpop::Eq]) { + let ([op], [right]) = (ops, comparators) else { return; - } - if comparators.len() != 1 { + }; + + if !matches!( + op, + Cmpop::Eq | Cmpop::NotEq | Cmpop::Lt | Cmpop::LtE | Cmpop::Gt | Cmpop::GtE, + ) { return; } - if !matches!(left.node, ExprKind::Constant { .. }) { + if !matches!(&left.node, &ExprKind::Constant { .. }) { return; } - let right = comparators.first().unwrap(); - if matches!(right.node, ExprKind::Constant { .. }) { + if matches!(&right.node, &ExprKind::Constant { .. }) { return; } @@ -36,16 +39,27 @@ pub fn yoda_conditions( .locator .slice_source_code_range(&Range::from_located(right)); + // Reverse the operation. + let reversed_op = match op { + Cmpop::Eq => "==", + Cmpop::NotEq => "!=", + Cmpop::Lt => ">", + Cmpop::LtE => ">=", + Cmpop::Gt => "<", + Cmpop::GtE => "<=", + _ => unreachable!("Expected comparison operator"), + }; + + let suggestion = format!("{variable} {reversed_op} {constant}"); let mut diagnostic = Diagnostic::new( violations::YodaConditions { - constant: constant.to_string(), - variable: variable.to_string(), + suggestion: suggestion.to_string(), }, Range::from_located(expr), ); if checker.patch(diagnostic.kind.code()) { diagnostic.amend(Fix::replacement( - format!("{variable} == {constant}"), + suggestion, left.location, right.end_location.unwrap(), )); diff --git a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap index e01845138b0b2..e5a216023e5f3 100644 --- a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap +++ b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap @@ -4,8 +4,7 @@ expression: diagnostics --- - kind: YodaConditions: - variable: compare - constant: "\"yoda\"" + suggestion: "compare == \"yoda\"" location: row: 2 column: 0 @@ -23,8 +22,7 @@ expression: diagnostics parent: ~ - kind: YodaConditions: - variable: compare - constant: "'yoda'" + suggestion: "compare == 'yoda'" location: row: 3 column: 0 @@ -42,8 +40,7 @@ expression: diagnostics parent: ~ - kind: YodaConditions: - variable: age - constant: "42" + suggestion: age == 42 location: row: 4 column: 0 @@ -59,4 +56,58 @@ expression: diagnostics row: 4 column: 9 parent: ~ +- kind: + YodaConditions: + suggestion: "compare >= \"yoda\"" + location: + row: 5 + column: 0 + end_location: + row: 5 + column: 17 + fix: + content: "compare >= \"yoda\"" + location: + row: 5 + column: 0 + end_location: + row: 5 + column: 17 + parent: ~ +- kind: + YodaConditions: + suggestion: "compare > 'yoda'" + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 16 + fix: + content: "compare > 'yoda'" + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 16 + parent: ~ +- kind: + YodaConditions: + suggestion: age < 42 + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 8 + fix: + content: age < 42 + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 8 + parent: ~ diff --git a/src/rules/pylint/mod.rs b/src/rules/pylint/mod.rs index 1842b6277710b..de02b819a724a 100644 --- a/src/rules/pylint/mod.rs +++ b/src/rules/pylint/mod.rs @@ -13,7 +13,6 @@ mod tests { use crate::settings::Settings; #[test_case(RuleCode::PLC0414, Path::new("import_aliasing.py"); "PLC0414")] - #[test_case(RuleCode::PLC2201, Path::new("misplaced_comparison_constant.py"); "PLC2201")] #[test_case(RuleCode::PLC3002, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")] #[test_case(RuleCode::PLE0117, Path::new("nonlocal_without_binding.py"); "PLE0117")] #[test_case(RuleCode::PLE0118, Path::new("used_prior_global_declaration.py"); "PLE0118")] diff --git a/src/rules/pylint/rules/misplaced_comparison_constant.rs b/src/rules/pylint/rules/misplaced_comparison_constant.rs deleted file mode 100644 index 36e3545963c5f..0000000000000 --- a/src/rules/pylint/rules/misplaced_comparison_constant.rs +++ /dev/null @@ -1,56 +0,0 @@ -use rustpython_ast::{Cmpop, Expr, ExprKind}; - -use crate::ast::types::Range; -use crate::checkers::ast::Checker; -use crate::fix::Fix; -use crate::registry::Diagnostic; -use crate::violations; - -/// PLC2201 -pub fn misplaced_comparison_constant( - checker: &mut Checker, - expr: &Expr, - left: &Expr, - ops: &[Cmpop], - comparators: &[Expr], -) { - let ([op], [right]) = (ops, comparators) else { - return; - }; - - if !matches!( - op, - Cmpop::Eq | Cmpop::NotEq | Cmpop::Lt | Cmpop::LtE | Cmpop::Gt | Cmpop::GtE, - ) { - return; - } - if !matches!(&left.node, &ExprKind::Constant { .. }) { - return; - } - if matches!(&right.node, &ExprKind::Constant { .. }) { - return; - } - - let reversed_op = match op { - Cmpop::Eq => "==", - Cmpop::NotEq => "!=", - Cmpop::Lt => ">", - Cmpop::LtE => ">=", - Cmpop::Gt => "<", - Cmpop::GtE => "<=", - _ => unreachable!("Expected comparison operator"), - }; - let suggestion = format!("{right} {reversed_op} {left}"); - let mut diagnostic = Diagnostic::new( - violations::MisplacedComparisonConstant(suggestion.clone()), - Range::from_located(expr), - ); - if checker.patch(diagnostic.kind.code()) { - diagnostic.amend(Fix::replacement( - suggestion, - expr.location, - expr.end_location.unwrap(), - )); - } - checker.diagnostics.push(diagnostic); -} diff --git a/src/rules/pylint/rules/mod.rs b/src/rules/pylint/rules/mod.rs index 82cb47d13e867..b146b831ca9a5 100644 --- a/src/rules/pylint/rules/mod.rs +++ b/src/rules/pylint/rules/mod.rs @@ -2,7 +2,6 @@ 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; pub use property_with_parameters::property_with_parameters; pub use unnecessary_direct_lambda_call::unnecessary_direct_lambda_call; pub use use_from_import::use_from_import; @@ -15,7 +14,6 @@ mod await_outside_async; mod constant_comparison; mod magic_value_comparison; mod merge_isinstance; -mod misplaced_comparison_constant; mod property_with_parameters; mod unnecessary_direct_lambda_call; mod use_from_import; diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC2201_misplaced_comparison_constant.py.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC2201_misplaced_comparison_constant.py.snap deleted file mode 100644 index 36ee592bc8035..0000000000000 --- a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC2201_misplaced_comparison_constant.py.snap +++ /dev/null @@ -1,107 +0,0 @@ ---- -source: src/rules/pylint/mod.rs -expression: diagnostics ---- -- kind: - MisplacedComparisonConstant: i >= 5 - location: - row: 20 - column: 11 - end_location: - row: 20 - column: 17 - fix: - content: i >= 5 - location: - row: 20 - column: 11 - end_location: - row: 20 - column: 17 - parent: ~ -- kind: - MisplacedComparisonConstant: i == 1 - location: - row: 22 - column: 11 - end_location: - row: 22 - column: 17 - fix: - content: i == 1 - location: - row: 22 - column: 11 - end_location: - row: 22 - column: 17 - parent: ~ -- kind: - MisplacedComparisonConstant: dummy_return() > 3 - location: - row: 24 - column: 11 - end_location: - row: 24 - column: 29 - fix: - content: dummy_return() > 3 - location: - row: 24 - column: 11 - end_location: - row: 24 - column: 29 - parent: ~ -- kind: - MisplacedComparisonConstant: instance.dummy_return() != 4 - location: - row: 26 - column: 11 - end_location: - row: 26 - column: 39 - fix: - content: instance.dummy_return() != 4 - location: - row: 26 - column: 11 - end_location: - row: 26 - column: 39 - parent: ~ -- kind: - MisplacedComparisonConstant: instance.attr == 1 - location: - row: 28 - column: 11 - end_location: - row: 28 - column: 29 - fix: - content: instance.attr == 1 - location: - row: 28 - column: 11 - end_location: - row: 28 - column: 29 - parent: ~ -- kind: - MisplacedComparisonConstant: "instance.attr == 'aaa'" - location: - row: 30 - column: 11 - end_location: - row: 30 - column: 33 - fix: - content: "instance.attr == 'aaa'" - location: - row: 30 - column: 11 - end_location: - row: 30 - column: 33 - parent: ~ - diff --git a/src/violations.rs b/src/violations.rs index 29e0a90bf0236..ad38a6385de18 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -1056,25 +1056,6 @@ impl AlwaysAutofixableViolation for UselessImportAlias { } } -define_violation!( - pub struct MisplacedComparisonConstant(pub String); -); -impl AlwaysAutofixableViolation for MisplacedComparisonConstant { - fn message(&self) -> String { - let MisplacedComparisonConstant(comparison) = self; - format!("Comparison should be {comparison}") - } - - fn autofix_title(&self) -> String { - let MisplacedComparisonConstant(comparison) = self; - format!("Replace with {comparison}") - } - - fn placeholder() -> Self { - MisplacedComparisonConstant("...".to_string()) - } -} - define_violation!( pub struct UnnecessaryDirectLambdaCall; ); @@ -3135,25 +3116,23 @@ impl AlwaysAutofixableViolation for AndFalse { define_violation!( pub struct YodaConditions { - pub variable: String, - pub constant: String, + pub suggestion: String, } ); impl AlwaysAutofixableViolation for YodaConditions { fn message(&self) -> String { - let YodaConditions { variable, constant } = self; - format!("Yoda conditions are discouraged, use `{variable} == {constant}` instead") + let YodaConditions { suggestion } = self; + format!("Yoda conditions are discouraged, use `{suggestion}` instead") } fn autofix_title(&self) -> String { - let YodaConditions { variable, constant } = self; - format!("Replace Yoda condition with `{variable} == {constant}`") + let YodaConditions { suggestion } = self; + format!("Replace Yoda condition with `{suggestion}`") } fn placeholder() -> Self { YodaConditions { - variable: "x".to_string(), - constant: "1".to_string(), + suggestion: "x == 1".to_string(), } } }