Skip to content

Commit

Permalink
[pylint] Implement consider-using-ternary (R1706) (#7811)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
danbi2990 authored Oct 13, 2023
1 parent 6f9c317 commit c03a693
Show file tree
Hide file tree
Showing 9 changed files with 445 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -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
]
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
133 changes: 133 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/and_or_ternary.rs
Original file line number Diff line number Diff line change
@@ -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<String> {
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);
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit c03a693

Please sign in to comment.