From c03a693ebcf721466576738340e3c74dfc7e9df4 Mon Sep 17 00:00:00 2001 From: Jake Park Date: Fri, 13 Oct 2023 10:29:19 +0900 Subject: [PATCH] [pylint] Implement consider-using-ternary (R1706) (#7811) This is my first PR. Please feel free to give me any feedback for even small drawbacks. ## Summary Checks if pre-python 2.5 ternary syntax is used. Before ```python x, y = 1, 2 maximum = x >= y and x or y # [consider-using-ternary] ``` After ```python x, y = 1, 2 maximum = x if x >= y else y ``` References: [pylint](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/consider-using-ternary.html) #970 [and_or_ternary distinction logic](https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/refactoring/refactoring_checker.py#L1813) ## Test Plan Unit test, python file, snapshot added. --- .../test/fixtures/pylint/and_or_ternary.py | 73 ++++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../src/rules/pylint/rules/and_or_ternary.rs | 133 ++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ...int__tests__PLR1706_and_or_ternary.py.snap | 229 ++++++++++++++++++ crates/ruff_workspace/src/configuration.rs | 3 +- ruff.schema.json | 1 + 9 files changed, 445 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/and_or_ternary.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/and_or_ternary.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1706_and_or_ternary.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/and_or_ternary.py b/crates/ruff_linter/resources/test/fixtures/pylint/and_or_ternary.py new file mode 100644 index 0000000000000..a65f0b02a0dc1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/and_or_ternary.py @@ -0,0 +1,73 @@ +# OK + +1<2 and 'b' and 'c' + +1<2 or 'a' and 'b' + +1<2 and 'a' + +1<2 or 'a' + +2>1 + +1<2 and 'a' or 'b' and 'c' + +1<2 and 'a' or 'b' or 'c' + +1<2 and 'a' or 'b' or 'c' or (lambda x: x+1) + +1<2 and 'a' or 'b' or (lambda x: x+1) or 'c' + +default = 'default' +if (not isinstance(default, bool) and isinstance(default, int)) \ + or (isinstance(default, str) and default): + pass + +docid, token = None, None +(docid is None and token is None) or (docid is not None and token is not None) + +vendor, os_version = 'darwin', '14' +vendor == "debian" and os_version in ["12"] or vendor == "ubuntu" and os_version in [] + +# Don't emit if the parent is an `if` statement. +if (task_id in task_dict and task_dict[task_id] is not task) \ + or task_id in used_group_ids: + pass + +no_target, is_x64, target = True, False, 'target' +if (no_target and not is_x64) or target == 'ARM_APPL_RUST_TARGET': + pass + +# Don't emit if the parent is a `bool_op` expression. +isinstance(val, str) and ((len(val) == 7 and val[0] == "#") or val in enums.NamedColor) + +# Errors + +1<2 and 'a' or 'b' + +(lambda x: x+1) and 'a' or 'b' + +'a' and (lambda x: x+1) or 'orange' + +val = '#0000FF' +(len(val) == 7 and val[0] == "#") or val in {'green'} + +marker = 'marker' +isinstance(marker, dict) and 'field' in marker or marker in {} + +def has_oranges(oranges, apples=None) -> bool: + return apples and False or oranges + +[x for x in l if a and b or c] + +{x: y for x in l if a and b or c} + +{x for x in l if a and b or c} + +new_list = [ + x + for sublist in all_lists + if a and b or c + for x in sublist + if (isinstance(operator, list) and x in operator) or x != operator +] diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 041d566f2abb5..58ec6f6e4bdcd 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1412,6 +1412,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::RepeatedEqualityComparison) { pylint::rules::repeated_equality_comparison(checker, bool_op); } + if checker.enabled(Rule::AndOrTernary) { + pylint::rules::and_or_ternary(checker, bool_op); + } if checker.enabled(Rule::UnnecessaryKeyCheck) { ruff::rules::unnecessary_key_check(checker, expr); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index dea29feeb0679..a0722c0741b9b 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -249,6 +249,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1701") => (RuleGroup::Unspecified, rules::pylint::rules::RepeatedIsinstanceCalls), (Pylint, "R1711") => (RuleGroup::Unspecified, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Unspecified, rules::pylint::rules::RepeatedEqualityComparison), + (Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary), (Pylint, "R1722") => (RuleGroup::Unspecified, rules::pylint::rules::SysExitAlias), (Pylint, "R2004") => (RuleGroup::Unspecified, rules::pylint::rules::MagicValueComparison), (Pylint, "R5501") => (RuleGroup::Unspecified, rules::pylint::rules::CollapsibleElseIf), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index aee24e4e62037..21b0c0250f911 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -18,6 +18,7 @@ mod tests { use crate::settings::LinterSettings; use crate::test::test_path; + #[test_case(Rule::AndOrTernary, Path::new("and_or_ternary.py"))] #[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))] #[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))] #[test_case( diff --git a/crates/ruff_linter/src/rules/pylint/rules/and_or_ternary.rs b/crates/ruff_linter/src/rules/pylint/rules/and_or_ternary.rs new file mode 100644 index 0000000000000..ed7c2ef634a82 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/and_or_ternary.rs @@ -0,0 +1,133 @@ +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{ + BoolOp, Expr, ExprBoolOp, ExprDictComp, ExprIfExp, ExprListComp, ExprSetComp, +}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; +use crate::fix::snippet::SourceCodeSnippet; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for uses of the known pre-Python 2.5 ternary syntax. +/// +/// ## Why is this bad? +/// Prior to the introduction of the if-expression (ternary) operator in Python +/// 2.5, the only way to express a conditional expression was to use the `and` +/// and `or` operators. +/// +/// The if-expression construct is clearer and more explicit, and should be +/// preferred over the use of `and` and `or` for ternary expressions. +/// +/// ## Example +/// ```python +/// x, y = 1, 2 +/// maximum = x >= y and x or y +/// ``` +/// +/// Use instead: +/// ```python +/// x, y = 1, 2 +/// maximum = x if x >= y else y +/// ``` +#[violation] +pub struct AndOrTernary { + ternary: SourceCodeSnippet, +} + +impl Violation for AndOrTernary { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + if let Some(ternary) = self.ternary.full_display() { + format!("Consider using if-else expression (`{ternary}`)") + } else { + format!("Consider using if-else expression") + } + } + + fn fix_title(&self) -> Option { + Some(format!("Convert to if-else expression")) + } +} + +/// Returns `Some((condition, true_value, false_value))`, if `bool_op` is of the form `condition and true_value or false_value`. +fn parse_and_or_ternary(bool_op: &ExprBoolOp) -> Option<(&Expr, &Expr, &Expr)> { + if bool_op.op != BoolOp::Or { + return None; + } + let [expr, false_value] = bool_op.values.as_slice() else { + return None; + }; + let Some(and_op) = expr.as_bool_op_expr() else { + return None; + }; + if and_op.op != BoolOp::And { + return None; + } + let [condition, true_value] = and_op.values.as_slice() else { + return None; + }; + if false_value.is_bool_op_expr() || true_value.is_bool_op_expr() { + return None; + } + Some((condition, true_value, false_value)) +} + +/// Returns `true` if the expression is used within a comprehension. +fn is_comprehension_if(parent: Option<&Expr>, expr: &ExprBoolOp) -> bool { + let comprehensions = match parent { + Some(Expr::ListComp(ExprListComp { generators, .. })) => generators, + Some(Expr::SetComp(ExprSetComp { generators, .. })) => generators, + Some(Expr::DictComp(ExprDictComp { generators, .. })) => generators, + _ => { + return false; + } + }; + comprehensions + .iter() + .any(|comp| comp.ifs.iter().any(|ifs| ifs.range() == expr.range())) +} + +/// PLR1706 +pub(crate) fn and_or_ternary(checker: &mut Checker, bool_op: &ExprBoolOp) { + if checker.semantic().current_statement().is_if_stmt() { + return; + } + let parent_expr = checker.semantic().current_expression_parent(); + if parent_expr.is_some_and(Expr::is_bool_op_expr) { + return; + } + let Some((condition, true_value, false_value)) = parse_and_or_ternary(bool_op) else { + return; + }; + + let if_expr = Expr::IfExp(ExprIfExp { + test: Box::new(condition.clone()), + body: Box::new(true_value.clone()), + orelse: Box::new(false_value.clone()), + range: TextRange::default(), + }); + + let ternary = if is_comprehension_if(parent_expr, bool_op) { + format!("({})", checker.generator().expr(&if_expr)) + } else { + checker.generator().expr(&if_expr) + }; + + let mut diagnostic = Diagnostic::new( + AndOrTernary { + ternary: SourceCodeSnippet::new(ternary.clone()), + }, + bool_op.range, + ); + if checker.enabled(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( + ternary, + bool_op.range, + ))); + } + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 7643b55b52140..317657266794b 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -1,3 +1,4 @@ +pub(crate) use and_or_ternary::*; pub(crate) use assert_on_string_literal::*; pub(crate) use await_outside_async::*; pub(crate) use bad_dunder_method_name::*; @@ -57,6 +58,7 @@ pub(crate) use useless_return::*; pub(crate) use yield_from_in_async_function::*; pub(crate) use yield_in_init::*; +mod and_or_ternary; mod assert_on_string_literal; mod await_outside_async; mod bad_dunder_method_name; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1706_and_or_ternary.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1706_and_or_ternary.py.snap new file mode 100644 index 0000000000000..044d05d789a1a --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1706_and_or_ternary.py.snap @@ -0,0 +1,229 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +and_or_ternary.py:46:1: PLR1706 [*] Consider using if-else expression (`'a' if 1 < 2 else 'b'`) + | +44 | # Errors +45 | +46 | 1<2 and 'a' or 'b' + | ^^^^^^^^^^^^^^^^^^ PLR1706 +47 | +48 | (lambda x: x+1) and 'a' or 'b' + | + = help: Convert to if-else expression + +ℹ Suggested fix +43 43 | +44 44 | # Errors +45 45 | +46 |-1<2 and 'a' or 'b' + 46 |+'a' if 1 < 2 else 'b' +47 47 | +48 48 | (lambda x: x+1) and 'a' or 'b' +49 49 | + +and_or_ternary.py:48:1: PLR1706 [*] Consider using if-else expression (`'a' if (lambda x: x + 1) else 'b'`) + | +46 | 1<2 and 'a' or 'b' +47 | +48 | (lambda x: x+1) and 'a' or 'b' + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1706 +49 | +50 | 'a' and (lambda x: x+1) or 'orange' + | + = help: Convert to if-else expression + +ℹ Suggested fix +45 45 | +46 46 | 1<2 and 'a' or 'b' +47 47 | +48 |-(lambda x: x+1) and 'a' or 'b' + 48 |+'a' if (lambda x: x + 1) else 'b' +49 49 | +50 50 | 'a' and (lambda x: x+1) or 'orange' +51 51 | + +and_or_ternary.py:50:1: PLR1706 [*] Consider using if-else expression (`(lambda x: x + 1) if 'a' else 'orange'`) + | +48 | (lambda x: x+1) and 'a' or 'b' +49 | +50 | 'a' and (lambda x: x+1) or 'orange' + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1706 +51 | +52 | val = '#0000FF' + | + = help: Convert to if-else expression + +ℹ Suggested fix +47 47 | +48 48 | (lambda x: x+1) and 'a' or 'b' +49 49 | +50 |-'a' and (lambda x: x+1) or 'orange' + 50 |+(lambda x: x + 1) if 'a' else 'orange' +51 51 | +52 52 | val = '#0000FF' +53 53 | (len(val) == 7 and val[0] == "#") or val in {'green'} + +and_or_ternary.py:53:1: PLR1706 [*] Consider using if-else expression + | +52 | val = '#0000FF' +53 | (len(val) == 7 and val[0] == "#") or val in {'green'} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1706 +54 | +55 | marker = 'marker' + | + = help: Convert to if-else expression + +ℹ Suggested fix +50 50 | 'a' and (lambda x: x+1) or 'orange' +51 51 | +52 52 | val = '#0000FF' +53 |-(len(val) == 7 and val[0] == "#") or val in {'green'} + 53 |+val[0] == '#' if len(val) == 7 else val in {'green'} +54 54 | +55 55 | marker = 'marker' +56 56 | isinstance(marker, dict) and 'field' in marker or marker in {} + +and_or_ternary.py:56:1: PLR1706 [*] Consider using if-else expression + | +55 | marker = 'marker' +56 | isinstance(marker, dict) and 'field' in marker or marker in {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1706 +57 | +58 | def has_oranges(oranges, apples=None) -> bool: + | + = help: Convert to if-else expression + +ℹ Suggested fix +53 53 | (len(val) == 7 and val[0] == "#") or val in {'green'} +54 54 | +55 55 | marker = 'marker' +56 |-isinstance(marker, dict) and 'field' in marker or marker in {} + 56 |+'field' in marker if isinstance(marker, dict) else marker in {} +57 57 | +58 58 | def has_oranges(oranges, apples=None) -> bool: +59 59 | return apples and False or oranges + +and_or_ternary.py:59:12: PLR1706 [*] Consider using if-else expression (`False if apples else oranges`) + | +58 | def has_oranges(oranges, apples=None) -> bool: +59 | return apples and False or oranges + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1706 +60 | +61 | [x for x in l if a and b or c] + | + = help: Convert to if-else expression + +ℹ Suggested fix +56 56 | isinstance(marker, dict) and 'field' in marker or marker in {} +57 57 | +58 58 | def has_oranges(oranges, apples=None) -> bool: +59 |- return apples and False or oranges + 59 |+ return False if apples else oranges +60 60 | +61 61 | [x for x in l if a and b or c] +62 62 | + +and_or_ternary.py:61:18: PLR1706 [*] Consider using if-else expression (`(b if a else c)`) + | +59 | return apples and False or oranges +60 | +61 | [x for x in l if a and b or c] + | ^^^^^^^^^^^^ PLR1706 +62 | +63 | {x: y for x in l if a and b or c} + | + = help: Convert to if-else expression + +ℹ Suggested fix +58 58 | def has_oranges(oranges, apples=None) -> bool: +59 59 | return apples and False or oranges +60 60 | +61 |-[x for x in l if a and b or c] + 61 |+[x for x in l if (b if a else c)] +62 62 | +63 63 | {x: y for x in l if a and b or c} +64 64 | + +and_or_ternary.py:63:21: PLR1706 [*] Consider using if-else expression (`(b if a else c)`) + | +61 | [x for x in l if a and b or c] +62 | +63 | {x: y for x in l if a and b or c} + | ^^^^^^^^^^^^ PLR1706 +64 | +65 | {x for x in l if a and b or c} + | + = help: Convert to if-else expression + +ℹ Suggested fix +60 60 | +61 61 | [x for x in l if a and b or c] +62 62 | +63 |-{x: y for x in l if a and b or c} + 63 |+{x: y for x in l if (b if a else c)} +64 64 | +65 65 | {x for x in l if a and b or c} +66 66 | + +and_or_ternary.py:65:18: PLR1706 [*] Consider using if-else expression (`(b if a else c)`) + | +63 | {x: y for x in l if a and b or c} +64 | +65 | {x for x in l if a and b or c} + | ^^^^^^^^^^^^ PLR1706 +66 | +67 | new_list = [ + | + = help: Convert to if-else expression + +ℹ Suggested fix +62 62 | +63 63 | {x: y for x in l if a and b or c} +64 64 | +65 |-{x for x in l if a and b or c} + 65 |+{x for x in l if (b if a else c)} +66 66 | +67 67 | new_list = [ +68 68 | x + +and_or_ternary.py:70:8: PLR1706 [*] Consider using if-else expression (`(b if a else c)`) + | +68 | x +69 | for sublist in all_lists +70 | if a and b or c + | ^^^^^^^^^^^^ PLR1706 +71 | for x in sublist +72 | if (isinstance(operator, list) and x in operator) or x != operator + | + = help: Convert to if-else expression + +ℹ Suggested fix +67 67 | new_list = [ +68 68 | x +69 69 | for sublist in all_lists +70 |- if a and b or c + 70 |+ if (b if a else c) +71 71 | for x in sublist +72 72 | if (isinstance(operator, list) and x in operator) or x != operator +73 73 | ] + +and_or_ternary.py:72:8: PLR1706 [*] Consider using if-else expression + | +70 | if a and b or c +71 | for x in sublist +72 | if (isinstance(operator, list) and x in operator) or x != operator + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1706 +73 | ] + | + = help: Convert to if-else expression + +ℹ Suggested fix +69 69 | for sublist in all_lists +70 70 | if a and b or c +71 71 | for x in sublist +72 |- if (isinstance(operator, list) and x in operator) or x != operator + 72 |+ if (x in operator if isinstance(operator, list) else x != operator) +73 73 | ] + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 62383d7486ffa..1ef320cd20fe9 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1076,6 +1076,8 @@ mod tests { ]; const PREVIEW_RULES: &[Rule] = &[ + Rule::AndOrTernary, + Rule::AssignmentInAssert, Rule::DirectLoggerInstantiation, Rule::InvalidGetLoggerArgument, Rule::ManualDictComprehension, @@ -1085,7 +1087,6 @@ mod tests { Rule::TooManyPublicMethods, Rule::UndocumentedWarn, Rule::UnnecessaryEnumerate, - Rule::AssignmentInAssert, ]; #[allow(clippy::needless_pass_by_value)] diff --git a/ruff.schema.json b/ruff.schema.json index 77b716a27d24d..7be275a4100e0 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2960,6 +2960,7 @@ "PLR17", "PLR170", "PLR1701", + "PLR1706", "PLR171", "PLR1711", "PLR1714",