diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI006.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI006.pyi index a0037bde07..72a06595dd 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI006.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI006.pyi @@ -14,5 +14,7 @@ if sys.version_info <= (3, 10): ... # Error: PYI006 Use only `<` and `>=` for v if sys.version_info <= (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons if sys.version_info > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons +elif sys.version_info > (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons if python_version > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons +elif python_version == (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI066.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI066.py new file mode 100644 index 0000000000..b227280131 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI066.py @@ -0,0 +1,54 @@ +import sys + +if sys.version_info < (3, 10): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + def foo(x): ... +else: + def foo(x, *, bar=True): ... + +if sys.version_info < (3, 8): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 8)" + def bar(x): ... +elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 9)" + def bar(x, *, bar=True): ... +elif sys.version_info < (3, 11): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + def bar(x, *, bar=True, baz=False): ... +else: + def bar(x, *, bar=True, baz=False, qux=1): ... + + +if sys.version_info >= (3, 5): + ... +elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + ... +else: + ... + +# Negative cases + +if sys.version_info[0] == 2: ... +if sys.version_info[:1] == (2,): ... +if sys.version_info[:1] == (True,): ... +if sys.version_info < ('3', '0'): ... +if sys.version_info >= (3, 4, 3): ... +if sys.version_info == (3, 4): ... +if sys.version_info < (3, 5): ... +if sys.version_info >= (3, 5): ... +if (2, 7) <= sys.version_info < (3, 5): ... + + +if sys.version_info >= (3, 5): + ... +else: + ... + +if sys.version_info >= (3, 10): + def foo1(x, *, bar=True, baz=False): ... +elif sys.version_info >= (3, 9): + def foo1(x, *, bar=True): ... +else: + def foo1(x): ... + +if sys.version_info < (3, 9): + def foo2(x): ... +elif sys.version_info < (3, 10): + def foo2(x, *, bar=True): ... +# no else case, no raise diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI066.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI066.pyi new file mode 100644 index 0000000000..b227280131 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI066.pyi @@ -0,0 +1,54 @@ +import sys + +if sys.version_info < (3, 10): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + def foo(x): ... +else: + def foo(x, *, bar=True): ... + +if sys.version_info < (3, 8): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 8)" + def bar(x): ... +elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 9)" + def bar(x, *, bar=True): ... +elif sys.version_info < (3, 11): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + def bar(x, *, bar=True, baz=False): ... +else: + def bar(x, *, bar=True, baz=False, qux=1): ... + + +if sys.version_info >= (3, 5): + ... +elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + ... +else: + ... + +# Negative cases + +if sys.version_info[0] == 2: ... +if sys.version_info[:1] == (2,): ... +if sys.version_info[:1] == (True,): ... +if sys.version_info < ('3', '0'): ... +if sys.version_info >= (3, 4, 3): ... +if sys.version_info == (3, 4): ... +if sys.version_info < (3, 5): ... +if sys.version_info >= (3, 5): ... +if (2, 7) <= sys.version_info < (3, 5): ... + + +if sys.version_info >= (3, 5): + ... +else: + ... + +if sys.version_info >= (3, 10): + def foo1(x, *, bar=True, baz=False): ... +elif sys.version_info >= (3, 9): + def foo1(x, *, bar=True): ... +else: + def foo1(x): ... + +if sys.version_info < (3, 9): + def foo2(x): ... +elif sys.version_info < (3, 10): + def foo2(x, *, bar=True): ... +# no else case, no raise diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 57671fb771..d97d7f9449 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1092,7 +1092,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { ruff::rules::sort_dunder_all_aug_assign(checker, aug_assign); } } - Stmt::If(if_ @ ast::StmtIf { test, .. }) => { + Stmt::If( + if_ @ ast::StmtIf { + test, + elif_else_clauses, + .. + }, + ) => { if checker.enabled(Rule::TooManyNestedBlocks) { pylint::rules::too_many_nested_blocks(checker, stmt); } @@ -1172,13 +1178,38 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_pyi::rules::unrecognized_platform(checker, test); } } - if checker.enabled(Rule::BadVersionInfoComparison) { - if let Expr::BoolOp(ast::ExprBoolOp { values, .. }) = test.as_ref() { - for value in values { - flake8_pyi::rules::bad_version_info_comparison(checker, value); + if checker.any_enabled(&[Rule::BadVersionInfoComparison, Rule::BadVersionInfoOrder]) + { + fn bad_version_info_comparison( + checker: &mut Checker, + test: &Expr, + has_else_clause: bool, + ) { + if let Expr::BoolOp(ast::ExprBoolOp { values, .. }) = test { + for value in values { + flake8_pyi::rules::bad_version_info_comparison( + checker, + value, + has_else_clause, + ); + } + } else { + flake8_pyi::rules::bad_version_info_comparison( + checker, + test, + has_else_clause, + ); + } + } + + let has_else_clause = + elif_else_clauses.iter().any(|clause| clause.test.is_none()); + + bad_version_info_comparison(checker, test.as_ref(), has_else_clause); + for clause in elif_else_clauses { + if let Some(test) = clause.test.as_ref() { + bad_version_info_comparison(checker, test, has_else_clause); } - } else { - flake8_pyi::rules::bad_version_info_comparison(checker, test); } } if checker.enabled(Rule::ComplexIfStatementInStub) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 56e18af821..23abc54145 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -812,6 +812,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass), (Flake8Pyi, "062") => (RuleGroup::Preview, rules::flake8_pyi::rules::DuplicateLiteralMember), (Flake8Pyi, "064") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantFinalLiteral), + (Flake8Pyi, "066") => (RuleGroup::Preview, rules::flake8_pyi::rules::BadVersionInfoOrder), // flake8-pytest-style (Flake8PytestStyle, "001") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestFixtureIncorrectParenthesesStyle), diff --git a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs index f53bd1e766..62b1c1ba9a 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs @@ -23,6 +23,8 @@ mod tests { #[test_case(Rule::BadExitAnnotation, Path::new("PYI036.pyi"))] #[test_case(Rule::BadVersionInfoComparison, Path::new("PYI006.py"))] #[test_case(Rule::BadVersionInfoComparison, Path::new("PYI006.pyi"))] + #[test_case(Rule::BadVersionInfoOrder, Path::new("PYI066.py"))] + #[test_case(Rule::BadVersionInfoOrder, Path::new("PYI066.pyi"))] #[test_case(Rule::CollectionsNamedTuple, Path::new("PYI024.py"))] #[test_case(Rule::CollectionsNamedTuple, Path::new("PYI024.pyi"))] #[test_case(Rule::ComplexAssignmentInStub, Path::new("PYI017.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/bad_version_info_comparison.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/bad_version_info_comparison.rs index 056e89e3ea..ecafa4a1b7 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/bad_version_info_comparison.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/bad_version_info_comparison.rs @@ -5,6 +5,7 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::registry::Rule; /// ## What it does /// Checks for uses of comparators other than `<` and `>=` for @@ -57,8 +58,61 @@ impl Violation for BadVersionInfoComparison { } } -/// PYI006 -pub(crate) fn bad_version_info_comparison(checker: &mut Checker, test: &Expr) { +/// ## What it does +/// Checks for if-else statements with `sys.version_info` comparisons that use +/// `<` comparators. +/// +/// ## Why is this bad? +/// As a convention, branches that correspond to newer Python versions should +/// come first when using `sys.version_info` comparisons. This makes it easier +/// to understand the desired behavior, which typically corresponds to the +/// latest Python versions. +/// +/// ## Example +/// +/// ```python +/// import sys +/// +/// if sys.version_info < (3, 10): +/// +/// def read_data(x, *, preserve_order=True): +/// ... +/// +/// else: +/// +/// def read_data(x): +/// ... +/// ``` +/// +/// Use instead: +/// +/// ```python +/// if sys.version_info >= (3, 10): +/// +/// def read_data(x): +/// ... +/// +/// else: +/// +/// def read_data(x, *, preserve_order=True): +/// ... +/// ``` +#[violation] +pub struct BadVersionInfoOrder; + +impl Violation for BadVersionInfoOrder { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use `>=` when using `if`-`else` with `sys.version_info` comparisons") + } +} + +/// PYI006, PYI066 +pub(crate) fn bad_version_info_comparison( + checker: &mut Checker, + test: &Expr, + has_else_clause: bool, +) { let Expr::Compare(ast::ExprCompare { left, ops, @@ -81,11 +135,24 @@ pub(crate) fn bad_version_info_comparison(checker: &mut Checker, test: &Expr) { return; } - if matches!(op, CmpOp::Lt | CmpOp::GtE) { + if matches!(op, CmpOp::GtE) { + // No issue to be raised, early exit. return; } - checker - .diagnostics - .push(Diagnostic::new(BadVersionInfoComparison, test.range())); + if matches!(op, CmpOp::Lt) { + if checker.enabled(Rule::BadVersionInfoOrder) { + if has_else_clause { + checker + .diagnostics + .push(Diagnostic::new(BadVersionInfoOrder, test.range())); + } + } + } else { + if checker.enabled(Rule::BadVersionInfoComparison) { + checker + .diagnostics + .push(Diagnostic::new(BadVersionInfoComparison, test.range())); + }; + } } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI006_PYI006.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI006_PYI006.pyi.snap index b928d5d4ad..55abdd253b 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI006_PYI006.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI006_PYI006.pyi.snap @@ -47,16 +47,30 @@ PYI006.pyi:16:4: PYI006 Use `<` or `>=` for `sys.version_info` comparisons 15 | 16 | if sys.version_info > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI006 -17 | -18 | if python_version > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons +17 | elif sys.version_info > (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons | -PYI006.pyi:18:4: PYI006 Use `<` or `>=` for `sys.version_info` comparisons +PYI006.pyi:17:6: PYI006 Use `<` or `>=` for `sys.version_info` comparisons | 16 | if sys.version_info > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons -17 | -18 | if python_version > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons - | ^^^^^^^^^^^^^^^^^^^^^^^^ PYI006 +17 | elif sys.version_info > (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI006 +18 | +19 | if python_version > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons | +PYI006.pyi:19:4: PYI006 Use `<` or `>=` for `sys.version_info` comparisons + | +17 | elif sys.version_info > (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons +18 | +19 | if python_version > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons + | ^^^^^^^^^^^^^^^^^^^^^^^^ PYI006 +20 | elif python_version == (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons + | +PYI006.pyi:20:6: PYI006 Use `<` or `>=` for `sys.version_info` comparisons + | +19 | if python_version > (3, 10): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons +20 | elif python_version == (3, 11): ... # Error: PYI006 Use only `<` and `>=` for version info comparisons + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PYI006 + | diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI066_PYI066.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI066_PYI066.py.snap new file mode 100644 index 0000000000..f0a4f44d2b --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI066_PYI066.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- + diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI066_PYI066.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI066_PYI066.pyi.snap new file mode 100644 index 0000000000..fbc165d719 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI066_PYI066.pyi.snap @@ -0,0 +1,52 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI066.pyi:3:4: PYI066 Use `>=` when using `if`-`else` with `sys.version_info` comparisons + | +1 | import sys +2 | +3 | if sys.version_info < (3, 10): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI066 +4 | def foo(x): ... +5 | else: + | + +PYI066.pyi:8:4: PYI066 Use `>=` when using `if`-`else` with `sys.version_info` comparisons + | + 6 | def foo(x, *, bar=True): ... + 7 | + 8 | if sys.version_info < (3, 8): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 8)" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PYI066 + 9 | def bar(x): ... +10 | elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 9)" + | + +PYI066.pyi:10:6: PYI066 Use `>=` when using `if`-`else` with `sys.version_info` comparisons + | + 8 | if sys.version_info < (3, 8): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 8)" + 9 | def bar(x): ... +10 | elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 9)" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PYI066 +11 | def bar(x, *, bar=True): ... +12 | elif sys.version_info < (3, 11): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + | + +PYI066.pyi:12:6: PYI066 Use `>=` when using `if`-`else` with `sys.version_info` comparisons + | +10 | elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 9)" +11 | def bar(x, *, bar=True): ... +12 | elif sys.version_info < (3, 11): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI066 +13 | def bar(x, *, bar=True, baz=False): ... +14 | else: + | + +PYI066.pyi:20:6: PYI066 Use `>=` when using `if`-`else` with `sys.version_info` comparisons + | +18 | if sys.version_info >= (3, 5): +19 | ... +20 | elif sys.version_info < (3, 9): # Y066 When using if/else with sys.version_info, put the code for new Python versions first, e.g. "if sys.version_info >= (3, 10)" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PYI066 +21 | ... +22 | else: + | diff --git a/ruff.schema.json b/ruff.schema.json index 2a409bc133..36b2ea1db7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3581,6 +3581,7 @@ "PYI06", "PYI062", "PYI064", + "PYI066", "Q", "Q0", "Q00",