diff --git a/Cargo.lock b/Cargo.lock index 7b3a67a298..b58fdb4ab8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2226,7 +2226,7 @@ dependencies = [ [[package]] name = "ruff_text_size" version = "0.0.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=b996b21ffca562ecb2086f632a6a0b05c245c24a#b996b21ffca562ecb2086f632a6a0b05c245c24a" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db04fd415774032e1e2ceb03bcbf5305e0d22c8c#db04fd415774032e1e2ceb03bcbf5305e0d22c8c" dependencies = [ "schemars", "serde", @@ -2327,7 +2327,7 @@ dependencies = [ [[package]] name = "rustpython-ast" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=b996b21ffca562ecb2086f632a6a0b05c245c24a#b996b21ffca562ecb2086f632a6a0b05c245c24a" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db04fd415774032e1e2ceb03bcbf5305e0d22c8c#db04fd415774032e1e2ceb03bcbf5305e0d22c8c" dependencies = [ "is-macro", "num-bigint", @@ -2338,7 +2338,7 @@ dependencies = [ [[package]] name = "rustpython-format" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=b996b21ffca562ecb2086f632a6a0b05c245c24a#b996b21ffca562ecb2086f632a6a0b05c245c24a" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db04fd415774032e1e2ceb03bcbf5305e0d22c8c#db04fd415774032e1e2ceb03bcbf5305e0d22c8c" dependencies = [ "bitflags 2.3.3", "itertools", @@ -2350,7 +2350,7 @@ dependencies = [ [[package]] name = "rustpython-literal" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=b996b21ffca562ecb2086f632a6a0b05c245c24a#b996b21ffca562ecb2086f632a6a0b05c245c24a" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db04fd415774032e1e2ceb03bcbf5305e0d22c8c#db04fd415774032e1e2ceb03bcbf5305e0d22c8c" dependencies = [ "hexf-parse", "is-macro", @@ -2362,7 +2362,7 @@ dependencies = [ [[package]] name = "rustpython-parser" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=b996b21ffca562ecb2086f632a6a0b05c245c24a#b996b21ffca562ecb2086f632a6a0b05c245c24a" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db04fd415774032e1e2ceb03bcbf5305e0d22c8c#db04fd415774032e1e2ceb03bcbf5305e0d22c8c" dependencies = [ "anyhow", "is-macro", @@ -2385,7 +2385,7 @@ dependencies = [ [[package]] name = "rustpython-parser-core" version = "0.2.0" -source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=b996b21ffca562ecb2086f632a6a0b05c245c24a#b996b21ffca562ecb2086f632a6a0b05c245c24a" +source = "git+https://github.com/astral-sh/RustPython-Parser.git?rev=db04fd415774032e1e2ceb03bcbf5305e0d22c8c#db04fd415774032e1e2ceb03bcbf5305e0d22c8c" dependencies = [ "is-macro", "memchr", diff --git a/Cargo.toml b/Cargo.toml index 04360b6360..a96cd6f742 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,12 +54,12 @@ libcst = { git = "https://github.com/Instagram/LibCST.git", rev = "3cacca1a1029f # Please tag the RustPython version every time you update its revision here and in fuzz/Cargo.toml # Tagging the version ensures that older ruff versions continue to build from source even when we rebase our RustPython fork. -# Current tag: v0.0.7 -ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" } -rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" , default-features = false, features = ["num-bigint"]} -rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a", default-features = false, features = ["num-bigint"] } -rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a", default-features = false } -rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" , default-features = false, features = ["full-lexer", "num-bigint"] } +# Current tag: v0.0.9 +ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" } +rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" , default-features = false, features = ["num-bigint"]} +rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c", default-features = false, features = ["num-bigint"] } +rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c", default-features = false } +rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" , default-features = false, features = ["full-lexer", "num-bigint"] } [profile.release] lto = "fat" diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B007.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B007.py index 6c855a7989..4e82accaaa 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bugbear/B007.py +++ b/crates/ruff/resources/test/fixtures/flake8_bugbear/B007.py @@ -97,3 +97,10 @@ def f(): # variable name). for line_ in range(self.header_lines): fp.readline() + +# Regression test: visitor didn't walk the elif test +for key, value in current_crawler_tags.items(): + if key: + pass + elif wanted_tag_value != value: + pass diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM102.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM102.py index 3b47a3e335..a0c3656550 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM102.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM102.py @@ -100,6 +100,14 @@ if node.module0123456789: ): print("Bad module!") +# SIM102 +# Regression test for https://github.com/apache/airflow/blob/145b16caaa43f0c42bffd97344df916c602cddde/airflow/configuration.py#L1161 +if a: + if b: + if c: + print("if") +elif d: + print("elif") # OK if a: diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM108.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM108.py index 94b14f911a..956caff2b3 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM108.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM108.py @@ -23,7 +23,7 @@ elif a: else: b = 2 -# OK (false negative) +# SIM108 if True: pass else: diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py index 2a4f155468..d18bb76d3e 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py @@ -94,3 +94,10 @@ if result.eofs == "F": errors = 1 else: errors = 1 + +if a: + # Ignore branches with diverging comments because it means we're repeating + # the bodies because we have different reasons for each branch + x = 1 +elif c: + x = 1 diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py index d5ddf88107..155c3fccc4 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py @@ -84,3 +84,15 @@ elif func_name == "remove": return "D" elif func_name == "move": return "MV" + +# OK +def no_return_in_else(platform): + if platform == "linux": + return "auditwheel repair -w {dest_dir} {wheel}" + elif platform == "macos": + return "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel}" + elif platform == "windows": + return "" + else: + msg = f"Unknown platform: {platform!r}" + raise ValueError(msg) diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM401.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM401.py index 487f15a5f7..79b48a57dd 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM401.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM401.py @@ -38,6 +38,15 @@ if key in a_dict: else: vars[idx] = "defaultß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789" +# SIM401 +if foo(): + pass +else: + if key in a_dict: + vars[idx] = a_dict[key] + else: + vars[idx] = "default" + ### # Negative cases ### @@ -105,12 +114,3 @@ elif key in a_dict: vars[idx] = a_dict[key] else: vars[idx] = "default" - -# OK (false negative for nested else) -if foo(): - pass -else: - if key in a_dict: - vars[idx] = a_dict[key] - else: - vars[idx] = "default" diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F634.py b/crates/ruff/resources/test/fixtures/pyflakes/F634.py index f23ac85a36..022e334a7a 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F634.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F634.py @@ -1,6 +1,11 @@ if (1, 2): pass +if (3, 4): + pass +elif foo: + pass + for _ in range(5): if True: pass diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F811_25.py b/crates/ruff/resources/test/fixtures/pyflakes/F811_25.py new file mode 100644 index 0000000000..c33acf4b4f --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pyflakes/F811_25.py @@ -0,0 +1,15 @@ +# Regression test for branch detection from +# https://github.com/pypa/build/blob/5800521541e5e749d4429617420d1ef8cdb40b46/src/build/_importlib.py +import sys + +if sys.version_info < (3, 8): + import importlib_metadata as metadata +elif sys.version_info < (3, 9, 10) or (3, 10, 0) <= sys.version_info < (3, 10, 2): + try: + import importlib_metadata as metadata + except ModuleNotFoundError: + from importlib import metadata +else: + from importlib import metadata + +__all__ = ["metadata"] diff --git a/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py b/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py index 2d12216144..fb00f5d329 100644 --- a/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py +++ b/crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py @@ -47,3 +47,17 @@ def not_ok1(): pass else: pass + + +# Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737 +def not_ok2(): + if True: + print(1) + elif True: + print(2) + else: + if True: + print(3) + else: + print(4) + diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP036_4.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP036_4.py index e3eead1251..57184c9c9c 100644 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP036_4.py +++ b/crates/ruff/resources/test/fixtures/pyupgrade/UP036_4.py @@ -7,20 +7,20 @@ if True: if True: if foo: - pass + print() elif sys.version_info < (3, 3): cmd = [sys.executable, "-m", "test.regrtest"] if True: if foo: - pass + print() elif sys.version_info < (3, 3): cmd = [sys.executable, "-m", "test.regrtest"] elif foo: cmd = [sys.executable, "-m", "test", "-j0"] if foo: - pass + print() elif sys.version_info < (3, 3): cmd = [sys.executable, "-m", "test.regrtest"] @@ -28,7 +28,7 @@ if True: cmd = [sys.executable, "-m", "test.regrtest"] if foo: - pass + print() elif sys.version_info < (3, 3): cmd = [sys.executable, "-m", "test.regrtest"] else: diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY004.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY004.py index 2c228e8929..8825737059 100644 --- a/crates/ruff/resources/test/fixtures/tryceratops/TRY004.py +++ b/crates/ruff/resources/test/fixtures/tryceratops/TRY004.py @@ -230,6 +230,15 @@ def incorrect_multi_conditional(arg1, arg2): raise Exception("...") # should be typeerror +def multiple_is_instance_checks(some_arg): + if isinstance(some_arg, str): + pass + elif isinstance(some_arg, int): + pass + else: + raise Exception("...") # should be typeerror + + class MyCustomTypeValidation(Exception): pass @@ -296,6 +305,17 @@ def multiple_ifs(some_args): pass +def else_body(obj): + if isinstance(obj, datetime.timedelta): + return "TimeDelta" + elif isinstance(obj, relativedelta.relativedelta): + return "RelativeDelta" + elif isinstance(obj, CronExpression): + return "CronExpression" + else: + raise Exception(f"Unknown object type: {obj.__class__.__name__}") + + def early_return(): if isinstance(this, some_type): if x in this: diff --git a/crates/ruff/src/autofix/edits.rs b/crates/ruff/src/autofix/edits.rs index 8526815d2a..5bdff428c9 100644 --- a/crates/ruff/src/autofix/edits.rs +++ b/crates/ruff/src/autofix/edits.rs @@ -190,12 +190,24 @@ fn is_lone_child(child: &Stmt, parent: &Stmt) -> bool { } Stmt::For(ast::StmtFor { body, orelse, .. }) | Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. }) - | Stmt::While(ast::StmtWhile { body, orelse, .. }) - | Stmt::If(ast::StmtIf { body, orelse, .. }) => { + | Stmt::While(ast::StmtWhile { body, orelse, .. }) => { if is_only(body, child) || is_only(orelse, child) { return true; } } + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + if is_only(body, child) + || elif_else_clauses + .iter() + .any(|ast::ElifElseClause { body, .. }| is_only(body, child)) + { + return true; + } + } Stmt::Try(ast::StmtTry { body, handlers, diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 61f09c6950..1c39c784a4 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -5,8 +5,8 @@ use log::error; use ruff_text_size::{TextRange, TextSize}; use rustpython_format::cformat::{CFormatError, CFormatErrorType}; use rustpython_parser::ast::{ - self, Arg, ArgWithDefault, Arguments, Comprehension, Constant, ExceptHandler, Expr, - ExprContext, Keyword, Operator, Pattern, Ranged, Stmt, Suite, UnaryOp, + self, Arg, ArgWithDefault, Arguments, Comprehension, Constant, ElifElseClause, ExceptHandler, + Expr, ExprContext, Keyword, Operator, Pattern, Ranged, Stmt, Suite, UnaryOp, }; use ruff_diagnostics::{Diagnostic, Fix, IsolationLevel}; @@ -588,6 +588,7 @@ where name, bases, keywords, + type_params: _, decorator_list, body, range: _, @@ -1159,9 +1160,8 @@ where Stmt::If( stmt_if @ ast::StmtIf { test, - body, - orelse, - range: _, + elif_else_clauses, + .. }, ) => { if self.enabled(Rule::EmptyTypeCheckingBlock) { @@ -1170,70 +1170,42 @@ where } } if self.enabled(Rule::IfTuple) { - pyflakes::rules::if_tuple(self, stmt, test); + pyflakes::rules::if_tuple(self, stmt_if); } if self.enabled(Rule::CollapsibleIf) { flake8_simplify::rules::nested_if_statements( self, - stmt, - test, - body, - orelse, + stmt_if, self.semantic.stmt_parent(), ); } if self.enabled(Rule::IfWithSameArms) { - flake8_simplify::rules::if_with_same_arms( - self, - stmt, - self.semantic.stmt_parent(), - ); + flake8_simplify::rules::if_with_same_arms(self, self.locator, stmt_if); } if self.enabled(Rule::NeedlessBool) { flake8_simplify::rules::needless_bool(self, stmt); } if self.enabled(Rule::IfElseBlockInsteadOfDictLookup) { - flake8_simplify::rules::manual_dict_lookup( - self, - stmt, - test, - body, - orelse, - self.semantic.stmt_parent(), - ); + flake8_simplify::rules::manual_dict_lookup(self, stmt_if); } if self.enabled(Rule::IfElseBlockInsteadOfIfExp) { - flake8_simplify::rules::use_ternary_operator( - self, - stmt, - self.semantic.stmt_parent(), - ); + flake8_simplify::rules::use_ternary_operator(self, stmt); } if self.enabled(Rule::IfElseBlockInsteadOfDictGet) { - flake8_simplify::rules::use_dict_get_with_default( - self, - stmt, - test, - body, - orelse, - self.semantic.stmt_parent(), - ); + flake8_simplify::rules::use_dict_get_with_default(self, stmt_if); } if self.enabled(Rule::TypeCheckWithoutTypeError) { tryceratops::rules::type_check_without_type_error( self, - body, - test, - orelse, + stmt_if, self.semantic.stmt_parent(), ); } if self.enabled(Rule::OutdatedVersionBlock) { - pyupgrade::rules::outdated_version_block(self, stmt, test, body, orelse); + pyupgrade::rules::outdated_version_block(self, stmt_if); } if self.enabled(Rule::CollapsibleElseIf) { - if let Some(diagnostic) = - pylint::rules::collapsible_else_if(orelse, self.locator) + if let Some(diagnostic) = pylint::rules::collapsible_else_if(elif_else_clauses) { self.diagnostics.push(diagnostic); } @@ -2053,7 +2025,7 @@ where stmt_if @ ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _, }, ) => { @@ -2068,7 +2040,9 @@ where self.visit_body(body); } - self.visit_body(orelse); + for clause in elif_else_clauses { + self.visit_elif_else_clause(clause); + } } _ => visitor::walk_stmt(self, stmt), }; @@ -4344,6 +4318,14 @@ impl<'a> Checker<'a> { self.semantic.flags = snapshot; } + /// Visit an [`ElifElseClause`] + fn visit_elif_else_clause(&mut self, clause: &'a ElifElseClause) { + if let Some(test) = &clause.test { + self.visit_boolean_test(test); + } + self.visit_body(&clause.body); + } + /// Add a [`Binding`] to the current scope, bound to the given name. fn add_binding( &mut self, diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs b/crates/ruff/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs index 1618f0f9b2..736470b3b9 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs @@ -55,8 +55,6 @@ struct GroupNameFinder<'a> { /// A flag indicating that the `group_name` variable has been overridden /// during the visit. overridden: bool, - /// A stack of `if` statements. - parent_ifs: Vec<&'a Stmt>, /// A stack of counters where each counter is itself a list of usage count. /// This is used specifically for mutually exclusive statements such as an /// `if` or `match`. @@ -77,7 +75,6 @@ impl<'a> GroupNameFinder<'a> { usage_count: 0, nested: false, overridden: false, - parent_ifs: Vec::new(), counter_stack: Vec::new(), exprs: Vec::new(), } @@ -146,56 +143,28 @@ where Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _, }) => { - // Determine whether we're on an `if` arm (as opposed to an `elif`). - let is_if_arm = !self.parent_ifs.iter().any(|parent| { - if let Stmt::If(ast::StmtIf { orelse, .. }) = parent { - orelse.len() == 1 && &orelse[0] == stmt - } else { - false - } - }); + // base if plus branches + let mut if_stack = Vec::with_capacity(1 + elif_else_clauses.len()); + // Initialize the vector with the count for the if branch. + if_stack.push(0); + self.counter_stack.push(if_stack); - if is_if_arm { - // Initialize the vector with the count for current branch. - self.counter_stack.push(vec![0]); - } else { - // SAFETY: `unwrap` is safe because we're either in `elif` or - // `else` branch which can come only after an `if` branch. - // When inside an `if` branch, a new vector will be pushed - // onto the stack. + self.visit_expr(test); + self.visit_body(body); + + for clause in elif_else_clauses { self.counter_stack.last_mut().unwrap().push(0); + self.visit_elif_else_clause(clause); } - let has_else = orelse - .first() - .map_or(false, |expr| !matches!(expr, Stmt::If(_))); - - self.parent_ifs.push(stmt); - if has_else { - // There's no `Stmt::Else`; instead, the `else` contents are directly on - // the `orelse` of the `Stmt::If` node. We want to add a new counter for - // the `orelse` branch, but first, we need to visit the `if` body manually. - self.visit_expr(test); - self.visit_body(body); - - // Now, we're in an `else` block. - self.counter_stack.last_mut().unwrap().push(0); - self.visit_body(orelse); - } else { - visitor::walk_stmt(self, stmt); - } - self.parent_ifs.pop(); - - if is_if_arm { - if let Some(last) = self.counter_stack.pop() { - // This is the max number of group usage from all the - // branches of this `if` statement. - let max_count = last.into_iter().max().unwrap_or(0); - self.increment_usage_count(max_count); - } + if let Some(last) = self.counter_stack.pop() { + // This is the max number of group usage from all the + // branches of this `if` statement. + let max_count = last.into_iter().max().unwrap_or(0); + self.increment_usage_count(max_count); } } Stmt::Match(ast::StmtMatch { diff --git a/crates/ruff/src/rules/flake8_return/rules/function.rs b/crates/ruff/src/rules/flake8_return/rules/function.rs index 01d27636d3..c55088654a 100644 --- a/crates/ruff/src/rules/flake8_return/rules/function.rs +++ b/crates/ruff/src/rules/flake8_return/rules/function.rs @@ -1,13 +1,14 @@ use std::ops::Add; use ruff_text_size::{TextRange, TextSize}; -use rustpython_parser::ast::{self, Expr, Ranged, Stmt}; +use rustpython_parser::ast::{self, ElifElseClause, Expr, Ranged, Stmt}; use ruff_diagnostics::{AlwaysAutofixableViolation, Violation}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::is_const_none; -use ruff_python_ast::helpers::{elif_else_range, is_const_false, is_const_true}; +use ruff_python_ast::helpers::{is_const_false, is_const_true}; +use ruff_python_ast::stmt_if::elif_else_range; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::whitespace::indentation; use ruff_python_semantic::SemanticModel; @@ -387,13 +388,25 @@ fn is_noreturn_func(func: &Expr, semantic: &SemanticModel) -> bool { /// RET503 fn implicit_return(checker: &mut Checker, stmt: &Stmt) { match stmt { - Stmt::If(ast::StmtIf { body, orelse, .. }) => { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { if let Some(last_stmt) = body.last() { implicit_return(checker, last_stmt); } - if let Some(last_stmt) = orelse.last() { - implicit_return(checker, last_stmt); - } else { + for clause in elif_else_clauses { + if let Some(last_stmt) = clause.body.last() { + implicit_return(checker, last_stmt); + } + } + + // Check if we don't have an else clause + if matches!( + elif_else_clauses.last(), + None | Some(ast::ElifElseClause { test: Some(_), .. }) + ) { let mut diagnostic = Diagnostic::new(ImplicitReturn, stmt.range()); if checker.patch(diagnostic.kind.rule()) { if let Some(indent) = indentation(checker.locator, stmt) { @@ -564,13 +577,21 @@ fn unnecessary_assign(checker: &mut Checker, stack: &Stack) { } /// RET505, RET506, RET507, RET508 -fn superfluous_else_node(checker: &mut Checker, stmt: &ast::StmtIf, branch: Branch) -> bool { - let ast::StmtIf { body, .. } = stmt; - for child in body { +fn superfluous_else_node( + checker: &mut Checker, + if_elif_body: &[Stmt], + elif_else: &ElifElseClause, +) -> bool { + let branch = if elif_else.test.is_some() { + Branch::Elif + } else { + Branch::Else + }; + for child in if_elif_body { if child.is_return_stmt() { let diagnostic = Diagnostic::new( SuperfluousElseReturn { branch }, - elif_else_range(stmt, checker.locator).unwrap_or_else(|| stmt.range()), + elif_else_range(elif_else, checker.locator).unwrap_or_else(|| elif_else.range()), ); if checker.enabled(diagnostic.kind.rule()) { checker.diagnostics.push(diagnostic); @@ -579,7 +600,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &ast::StmtIf, branch: Bran } else if child.is_break_stmt() { let diagnostic = Diagnostic::new( SuperfluousElseBreak { branch }, - elif_else_range(stmt, checker.locator).unwrap_or_else(|| stmt.range()), + elif_else_range(elif_else, checker.locator).unwrap_or_else(|| elif_else.range()), ); if checker.enabled(diagnostic.kind.rule()) { checker.diagnostics.push(diagnostic); @@ -588,7 +609,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &ast::StmtIf, branch: Bran } else if child.is_raise_stmt() { let diagnostic = Diagnostic::new( SuperfluousElseRaise { branch }, - elif_else_range(stmt, checker.locator).unwrap_or_else(|| stmt.range()), + elif_else_range(elif_else, checker.locator).unwrap_or_else(|| elif_else.range()), ); if checker.enabled(diagnostic.kind.rule()) { checker.diagnostics.push(diagnostic); @@ -597,7 +618,7 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &ast::StmtIf, branch: Bran } else if child.is_continue_stmt() { let diagnostic = Diagnostic::new( SuperfluousElseContinue { branch }, - elif_else_range(stmt, checker.locator).unwrap_or_else(|| stmt.range()), + elif_else_range(elif_else, checker.locator).unwrap_or_else(|| elif_else.range()), ); if checker.enabled(diagnostic.kind.rule()) { checker.diagnostics.push(diagnostic); @@ -609,16 +630,9 @@ fn superfluous_else_node(checker: &mut Checker, stmt: &ast::StmtIf, branch: Bran } /// RET505, RET506, RET507, RET508 -fn superfluous_elif(checker: &mut Checker, stack: &Stack) { - for stmt in &stack.elifs { - superfluous_else_node(checker, stmt, Branch::Elif); - } -} - -/// RET505, RET506, RET507, RET508 -fn superfluous_else(checker: &mut Checker, stack: &Stack) { - for stmt in &stack.elses { - superfluous_else_node(checker, stmt, Branch::Else); +fn superfluous_elif_else(checker: &mut Checker, stack: &Stack) { + for (if_elif_body, elif_else) in &stack.elifs_elses { + superfluous_else_node(checker, if_elif_body, elif_else); } } @@ -655,8 +669,7 @@ pub(crate) fn function(checker: &mut Checker, body: &[Stmt], returns: Option<&Ex Rule::SuperfluousElseContinue, Rule::SuperfluousElseBreak, ]) { - superfluous_elif(checker, &stack); - superfluous_else(checker, &stack); + superfluous_elif_else(checker, &stack); } // Skip any functions without return statements. diff --git a/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET508_RET508.py.snap b/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET508_RET508.py.snap index 9ae299e91b..5154b39429 100644 --- a/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET508_RET508.py.snap +++ b/crates/ruff/src/rules/flake8_return/snapshots/ruff__rules__flake8_return__tests__RET508_RET508.py.snap @@ -70,4 +70,13 @@ RET508.py:82:9: RET508 Unnecessary `else` after `break` statement 84 | return | +RET508.py:158:13: RET508 Unnecessary `else` after `break` statement + | +156 | if i > w: +157 | break +158 | else: + | ^^^^ RET508 +159 | a = z + | + diff --git a/crates/ruff/src/rules/flake8_return/visitor.rs b/crates/ruff/src/rules/flake8_return/visitor.rs index 32b7daa93d..9d157f0e6e 100644 --- a/crates/ruff/src/rules/flake8_return/visitor.rs +++ b/crates/ruff/src/rules/flake8_return/visitor.rs @@ -1,5 +1,5 @@ use rustc_hash::FxHashSet; -use rustpython_parser::ast::{self, Expr, Identifier, Stmt}; +use rustpython_parser::ast::{self, ElifElseClause, Expr, Identifier, Stmt}; use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; @@ -8,10 +8,8 @@ use ruff_python_ast::visitor::Visitor; pub(super) struct Stack<'a> { /// The `return` statements in the current function. pub(super) returns: Vec<&'a ast::StmtReturn>, - /// The `else` statements in the current function. - pub(super) elses: Vec<&'a ast::StmtIf>, - /// The `elif` statements in the current function. - pub(super) elifs: Vec<&'a ast::StmtIf>, + /// The `elif` or `else` statements in the current function. + pub(super) elifs_elses: Vec<(&'a [Stmt], &'a ElifElseClause)>, /// The non-local variables in the current function. pub(super) non_locals: FxHashSet<&'a str>, /// Whether the current function is a generator. @@ -117,27 +115,13 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> { self.stack.returns.push(stmt_return); } - Stmt::If(stmt_if) => { - let is_elif_arm = self.parents.iter().any(|parent| { - if let Stmt::If(ast::StmtIf { orelse, .. }) = parent { - orelse.len() == 1 && &orelse[0] == stmt - } else { - false - } - }); - - if !is_elif_arm { - let has_elif = - stmt_if.orelse.len() == 1 && stmt_if.orelse.first().unwrap().is_if_stmt(); - let has_else = !stmt_if.orelse.is_empty(); - - if has_elif { - // `stmt` is an `if` block followed by an `elif` clause. - self.stack.elifs.push(stmt_if); - } else if has_else { - // `stmt` is an `if` block followed by an `else` clause. - self.stack.elses.push(stmt_if); - } + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + if let Some(first) = elif_else_clauses.first() { + self.stack.elifs_elses.push((body, first)); } } _ => {} diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs index 7c181d1405..ffa33185cf 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs @@ -1,12 +1,16 @@ use log::error; use ruff_text_size::TextRange; use rustc_hash::FxHashSet; -use rustpython_parser::ast::{self, CmpOp, Constant, Expr, ExprContext, Identifier, Ranged, Stmt}; +use rustpython_parser::ast::{ + self, CmpOp, Constant, ElifElseClause, Expr, ExprContext, Identifier, Ranged, Stmt, StmtIf, +}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::{ComparableConstant, ComparableExpr, ComparableStmt}; use ruff_python_ast::helpers::{any_over_expr, contains_effect, first_colon_range, has_comments}; +use ruff_python_ast::source_code::Locator; +use ruff_python_ast::stmt_if::if_elif_branches; use ruff_python_semantic::SemanticModel; use ruff_python_whitespace::UniversalNewlines; @@ -23,16 +27,6 @@ fn compare_stmt(stmt1: &ComparableStmt, stmt2: &ComparableStmt) -> bool { stmt1.eq(stmt2) } -fn compare_body(body1: &[Stmt], body2: &[Stmt]) -> bool { - if body1.len() != body2.len() { - return false; - } - body1 - .iter() - .zip(body2.iter()) - .all(|(stmt1, stmt2)| compare_stmt(&stmt1.into(), &stmt2.into())) -} - /// ## What it does /// Checks for nested `if` statements that can be collapsed into a single `if` /// statement. @@ -287,7 +281,7 @@ fn is_main_check(expr: &Expr) -> bool { } /// Find the last nested if statement and return the test expression and the -/// first statement. +/// last statement. /// /// ```python /// if xxx: @@ -301,13 +295,13 @@ fn find_last_nested_if(body: &[Stmt]) -> Option<(&Expr, &Stmt)> { let [Stmt::If(ast::StmtIf { test, body: inner_body, - orelse, + elif_else_clauses, .. })] = body else { return None; }; - if !orelse.is_empty() { + if !elif_else_clauses.is_empty() { return None; } find_last_nested_if(inner_body).or_else(|| { @@ -318,30 +312,36 @@ fn find_last_nested_if(body: &[Stmt]) -> Option<(&Expr, &Stmt)> { }) } -/// SIM102 -pub(crate) fn nested_if_statements( - checker: &mut Checker, - stmt: &Stmt, - test: &Expr, - body: &[Stmt], - orelse: &[Stmt], - parent: Option<&Stmt>, -) { - // If the parent could contain a nested if-statement, abort. - if let Some(Stmt::If(ast::StmtIf { body, orelse, .. })) = parent { - if orelse.is_empty() && body.len() == 1 { - return; - } - } +fn nested_if_body(stmt_if: &StmtIf) -> Option<(&[Stmt], TextRange)> { + let StmtIf { + test, + body, + elif_else_clauses, + .. + } = stmt_if; - // If this if-statement has an else clause, or more than one child, abort. - if !(orelse.is_empty() && body.len() == 1) { - return; + // It must be the last condition, otherwise there could be another `elif` or `else` that only + // depends on the outer of the two conditions + let (test, body, range) = if let Some(clause) = elif_else_clauses.last() { + if let Some(test) = &clause.test { + (test, &clause.body, clause.range()) + } else { + // The last condition is an `else` (different rule) + return None; + } + } else { + (test.as_ref(), body, stmt_if.range()) + }; + + // The nested if must be the only child, otherwise there is at least one more statement that + // only depends on the outer condition + if body.len() > 1 { + return None; } // Allow `if __name__ == "__main__":` statements. if is_main_check(test) { - return; + return None; } // Allow `if True:` and `if False:` statements. @@ -352,9 +352,18 @@ pub(crate) fn nested_if_statements( .. }) ) { - return; + return None; } + Some((body, range)) +} + +/// SIM102 +pub(crate) fn nested_if_statements(checker: &mut Checker, stmt_if: &StmtIf, parent: Option<&Stmt>) { + let Some((body, range)) = nested_if_body(stmt_if) else { + return; + }; + // Find the deepest nested if-statement, to inform the range. let Some((test, first_stmt)) = find_last_nested_if(body) else { return; @@ -365,12 +374,22 @@ pub(crate) fn nested_if_statements( checker.locator, ); + // Check if the parent is already emitting a larger diagnostic including this if statement + if let Some(Stmt::If(stmt_if)) = parent { + if let Some((body, _range)) = nested_if_body(stmt_if) { + // In addition to repeating the `nested_if_body` and `find_last_nested_if` check, we + // also need to be the first child in the parent + if matches!(&body[0], Stmt::If(inner) if inner == stmt_if) + && find_last_nested_if(body).is_some() + { + return; + } + } + } + let mut diagnostic = Diagnostic::new( CollapsibleIf, - colon.map_or_else( - || stmt.range(), - |colon| TextRange::new(stmt.start(), colon.end()), - ), + colon.map_or(range, |colon| TextRange::new(range.start(), colon.end())), ); if checker.patch(diagnostic.kind.rule()) { // The fixer preserves comments in the nested body, but removes comments between @@ -379,9 +398,9 @@ pub(crate) fn nested_if_statements( if !checker .indexer .comment_ranges() - .intersects(TextRange::new(stmt.start(), nested_if.start())) + .intersects(TextRange::new(range.start(), nested_if.start())) { - match fix_if::fix_nested_if_statements(checker.locator, checker.stylist, stmt) { + match fix_if::fix_nested_if_statements(checker.locator, checker.stylist, range) { Ok(edit) => { if edit .content() @@ -437,17 +456,43 @@ fn is_one_line_return_bool(stmts: &[Stmt]) -> Option { /// SIM103 pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) { let Stmt::If(ast::StmtIf { - test, - body, - orelse, + test: if_test, + body: if_body, + elif_else_clauses, range: _, }) = stmt else { return; }; + // Extract an `if` or `elif` (that returns) followed by an else (that returns the same value) + let (if_test, if_body, else_body, range) = match elif_else_clauses.as_slice() { + // if-else case + [ElifElseClause { + body: else_body, + test: None, + .. + }] => (if_test.as_ref(), if_body, else_body, stmt.range()), + // elif-else case + [.., ElifElseClause { + body: elif_body, + test: Some(elif_test), + range: elif_range, + }, ElifElseClause { + body: else_body, + test: None, + range: else_range, + }] => ( + elif_test, + elif_body, + else_body, + TextRange::new(elif_range.start(), else_range.end()), + ), + _ => return, + }; + let (Some(if_return), Some(else_return)) = ( - is_one_line_return_bool(body), - is_one_line_return_bool(orelse), + is_one_line_return_bool(if_body), + is_one_line_return_bool(else_body), ) else { return; }; @@ -458,23 +503,23 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) { return; } - let condition = checker.generator().expr(test); - let mut diagnostic = Diagnostic::new(NeedlessBool { condition }, stmt.range()); + let condition = checker.generator().expr(if_test); + let mut diagnostic = Diagnostic::new(NeedlessBool { condition }, range); if checker.patch(diagnostic.kind.rule()) { if matches!(if_return, Bool::True) && matches!(else_return, Bool::False) - && !has_comments(stmt, checker.locator, checker.indexer) - && (test.is_compare_expr() || checker.semantic().is_builtin("bool")) + && !has_comments(&range, checker.locator, checker.indexer) + && (if_test.is_compare_expr() || checker.semantic().is_builtin("bool")) { - if test.is_compare_expr() { + if if_test.is_compare_expr() { // If the condition is a comparison, we can replace it with the condition. let node = ast::StmtReturn { - value: Some(test.clone()), + value: Some(Box::new(if_test.clone())), range: TextRange::default(), }; diagnostic.set_fix(Fix::suggested(Edit::range_replacement( checker.generator().stmt(&node.into()), - stmt.range(), + range, ))); } else { // Otherwise, we need to wrap the condition in a call to `bool`. (We've already @@ -486,7 +531,7 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) { }; let node1 = ast::ExprCall { func: Box::new(node.into()), - args: vec![(**test).clone()], + args: vec![if_test.clone()], keywords: vec![], range: TextRange::default(), }; @@ -496,7 +541,7 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) { }; diagnostic.set_fix(Fix::suggested(Edit::range_replacement( checker.generator().stmt(&node2.into()), - stmt.range(), + range, ))); }; } @@ -520,99 +565,71 @@ fn ternary(target_var: &Expr, body_value: &Expr, test: &Expr, orelse_value: &Exp node1.into() } -/// Return `true` if the `Expr` contains a reference to `${module}.${target}`. -fn contains_call_path(expr: &Expr, target: &[&str], semantic: &SemanticModel) -> bool { +/// Return `true` if the `Expr` contains a reference to any of the given `${module}.${target}`. +fn contains_call_path(expr: &Expr, targets: &[&[&str]], semantic: &SemanticModel) -> bool { any_over_expr(expr, &|expr| { - semantic - .resolve_call_path(expr) - .map_or(false, |call_path| call_path.as_slice() == target) + semantic.resolve_call_path(expr).map_or(false, |call_path| { + targets.iter().any(|target| &call_path.as_slice() == target) + }) }) } /// SIM108 -pub(crate) fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: Option<&Stmt>) { +pub(crate) fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt) { let Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _, }) = stmt else { return; }; - if body.len() != 1 || orelse.len() != 1 { + // `test: None` to only match an `else` clause + let [ElifElseClause { + body: else_body, + test: None, + .. + }] = elif_else_clauses.as_slice() + else { return; - } - let Stmt::Assign(ast::StmtAssign { + }; + let [Stmt::Assign(ast::StmtAssign { targets: body_targets, value: body_value, .. - }) = &body[0] + })] = body.as_slice() else { return; }; - let Stmt::Assign(ast::StmtAssign { - targets: orelse_targets, - value: orelse_value, + let [Stmt::Assign(ast::StmtAssign { + targets: else_targets, + value: else_value, .. - }) = &orelse[0] + })] = else_body.as_slice() else { return; }; - if body_targets.len() != 1 || orelse_targets.len() != 1 { - return; - } - let Expr::Name(ast::ExprName { id: body_id, .. }) = &body_targets[0] else { + let ([body_target], [else_target]) = (body_targets.as_slice(), else_targets.as_slice()) else { return; }; - let Expr::Name(ast::ExprName { id: orelse_id, .. }) = &orelse_targets[0] else { + let Expr::Name(ast::ExprName { id: body_id, .. }) = body_target else { return; }; - if body_id != orelse_id { + let Expr::Name(ast::ExprName { id: else_id, .. }) = else_target else { + return; + }; + if body_id != else_id { return; } - // Avoid suggesting ternary for `if sys.version_info >= ...`-style checks. - if contains_call_path(test, &["sys", "version_info"], checker.semantic()) { + // Avoid suggesting ternary for `if sys.version_info >= ...`-style and + // `if sys.platform.startswith("...")`-style checks. + let ignored_call_paths: &[&[&str]] = &[&["sys", "version_info"], &["sys", "platform"]]; + if contains_call_path(test, ignored_call_paths, checker.semantic()) { return; } - // Avoid suggesting ternary for `if sys.platform.startswith("...")`-style - // checks. - if contains_call_path(test, &["sys", "platform"], checker.semantic()) { - return; - } - - // It's part of a bigger if-elif block: - // https://github.com/MartinThoma/flake8-simplify/issues/115 - if let Some(Stmt::If(ast::StmtIf { - orelse: parent_orelse, - .. - })) = parent - { - if parent_orelse.len() == 1 && stmt == &parent_orelse[0] { - // TODO(charlie): These two cases have the same AST: - // - // if True: - // pass - // elif a: - // b = 1 - // else: - // b = 2 - // - // if True: - // pass - // else: - // if a: - // b = 1 - // else: - // b = 2 - // - // We want to flag the latter, but not the former. Right now, we flag neither. - return; - } - } - // Avoid suggesting ternary for `if (yield ...)`-style checks. // TODO(charlie): Fix precedence handling for yields in generator. if matches!( @@ -622,14 +639,14 @@ pub(crate) fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: O return; } if matches!( - orelse_value.as_ref(), + else_value.as_ref(), Expr::Yield(_) | Expr::YieldFrom(_) | Expr::Await(_) ) { return; } - let target_var = &body_targets[0]; - let ternary = ternary(target_var, body_value, test, orelse_value); + let target_var = &body_target; + let ternary = ternary(target_var, body_value, test, else_value); let contents = checker.generator().stmt(&ternary); // Don't flag if the resulting expression would exceed the maximum line length. @@ -659,135 +676,85 @@ pub(crate) fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: O checker.diagnostics.push(diagnostic); } -fn get_if_body_pairs<'a>( - test: &'a Expr, - body: &'a [Stmt], - orelse: &'a [Stmt], -) -> Vec<(&'a Expr, &'a [Stmt])> { - let mut pairs = vec![(test, body)]; - let mut orelse = orelse; - loop { - if orelse.len() != 1 { - break; - } - let Stmt::If(ast::StmtIf { - test, - body, - orelse: orelse_orelse, - range: _, - }) = &orelse[0] - else { - break; - }; - pairs.push((test, body)); - orelse = orelse_orelse; - } - pairs -} - /// SIM114 -pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt: &Stmt, parent: Option<&Stmt>) { - let Stmt::If(ast::StmtIf { - test, - body, - orelse, - range: _, - }) = stmt - else { - return; - }; +pub(crate) fn if_with_same_arms(checker: &mut Checker, locator: &Locator, stmt_if: &StmtIf) { + let mut branches_iter = if_elif_branches(stmt_if).peekable(); + while let Some(current_branch) = branches_iter.next() { + let Some(following_branch) = branches_iter.peek() else { + continue; + }; - // It's part of a bigger if-elif block: - // https://github.com/MartinThoma/flake8-simplify/issues/115 - if let Some(Stmt::If(ast::StmtIf { - orelse: parent_orelse, - .. - })) = parent - { - if parent_orelse.len() == 1 && stmt == &parent_orelse[0] { - // TODO(charlie): These two cases have the same AST: - // - // if True: - // pass - // elif a: - // b = 1 - // else: - // b = 2 - // - // if True: - // pass - // else: - // if a: - // b = 1 - // else: - // b = 2 - // - // We want to flag the latter, but not the former. Right now, we flag neither. - return; + // The bodies must have the same code ... + if current_branch.body.len() != following_branch.body.len() { + continue; + } + if !current_branch + .body + .iter() + .zip(following_branch.body.iter()) + .all(|(stmt1, stmt2)| compare_stmt(&stmt1.into(), &stmt2.into())) + { + continue; } - } - let if_body_pairs = get_if_body_pairs(test, body, orelse); - for i in 0..(if_body_pairs.len() - 1) { - let (test, body) = &if_body_pairs[i]; - let (.., next_body) = &if_body_pairs[i + 1]; - if compare_body(body, next_body) { - checker.diagnostics.push(Diagnostic::new( - IfWithSameArms, - TextRange::new( - if i == 0 { stmt.start() } else { test.start() }, - next_body.last().unwrap().end(), - ), - )); + // ...and the same comments + let first_comments: Vec<_> = checker + .indexer + .comments_in_range(current_branch.range, locator) + .collect(); + let second_comments: Vec<_> = checker + .indexer + .comments_in_range(following_branch.range, locator) + .collect(); + if first_comments != second_comments { + continue; } + + checker.diagnostics.push(Diagnostic::new( + IfWithSameArms, + TextRange::new( + current_branch.range.start(), + following_branch.body.last().unwrap().end(), + ), + )); } } /// SIM116 -pub(crate) fn manual_dict_lookup( - checker: &mut Checker, - stmt: &Stmt, - test: &Expr, - body: &[Stmt], - orelse: &[Stmt], - parent: Option<&Stmt>, -) { +pub(crate) fn manual_dict_lookup(checker: &mut Checker, stmt_if: &StmtIf) { // Throughout this rule: - // * Each if-statement's test must consist of a constant equality check with the same variable. - // * Each if-statement's body must consist of a single `return`. - // * Each if-statement's orelse must be either another if-statement or empty. - // * The final if-statement's orelse must be empty, or a single `return`. + // * Each if or elif statement's test must consist of a constant equality check with the same variable. + // * Each if or elif statement's body must consist of a single `return`. + // * The else clause must be empty, or a single `return`. + let StmtIf { + body, + test, + elif_else_clauses, + .. + } = stmt_if; + let Expr::Compare(ast::ExprCompare { left, ops, comparators, range: _, - }) = &test + }) = test.as_ref() else { return; }; let Expr::Name(ast::ExprName { id: target, .. }) = left.as_ref() else { return; }; - if body.len() != 1 { + if ops != &[CmpOp::Eq] { return; } - if orelse.len() != 1 { - return; - } - if !(ops.len() == 1 && ops[0] == CmpOp::Eq) { - return; - } - if comparators.len() != 1 { - return; - } - let Expr::Constant(ast::ExprConstant { + let [Expr::Constant(ast::ExprConstant { value: constant, .. - }) = &comparators[0] + })] = comparators.as_slice() else { return; }; - let Stmt::Return(ast::StmtReturn { value, range: _ }) = &body[0] else { + let [Stmt::Return(ast::StmtReturn { value, range: _ })] = body.as_slice() else { return; }; if value.as_ref().map_or(false, |value| { @@ -796,99 +763,60 @@ pub(crate) fn manual_dict_lookup( return; } - // It's part of a bigger if-elif block: - // https://github.com/MartinThoma/flake8-simplify/issues/115 - if let Some(Stmt::If(ast::StmtIf { - orelse: parent_orelse, - .. - })) = parent - { - if parent_orelse.len() == 1 && stmt == &parent_orelse[0] { - // TODO(charlie): These two cases have the same AST: - // - // if True: - // pass - // elif a: - // b = 1 - // else: - // b = 2 - // - // if True: - // pass - // else: - // if a: - // b = 1 - // else: - // b = 2 - // - // We want to flag the latter, but not the former. Right now, we flag neither. - return; - } - } - let mut constants: FxHashSet = FxHashSet::default(); constants.insert(constant.into()); - let mut child: Option<&Stmt> = orelse.get(0); - while let Some(current) = child.take() { - let Stmt::If(ast::StmtIf { - test, - body, - orelse, - range: _, - }) = ¤t - else { - return; - }; - if body.len() != 1 { - return; - } - if orelse.len() > 1 { - return; - } - let Expr::Compare(ast::ExprCompare { - left, - ops, - comparators, - range: _, - }) = test.as_ref() - else { - return; - }; - let Expr::Name(ast::ExprName { id, .. }) = left.as_ref() else { - return; - }; - if !(id == target && matches!(ops.as_slice(), [CmpOp::Eq])) { - return; - } - let [Expr::Constant(ast::ExprConstant { - value: constant, .. - })] = comparators.as_slice() - else { - return; - }; - let Stmt::Return(ast::StmtReturn { value, range: _ }) = &body[0] else { - return; - }; - if value.as_ref().map_or(false, |value| { - contains_effect(value, |id| checker.semantic().is_builtin(id)) - }) { + for clause in elif_else_clauses { + let ElifElseClause { test, body, .. } = clause; + let [Stmt::Return(ast::StmtReturn { value, range: _ })] = body.as_slice() else { return; }; - constants.insert(constant.into()); - if let Some(orelse) = orelse.first() { - match orelse { - Stmt::If(_) => { - child = Some(orelse); - } - Stmt::Return(_) => { - child = None; - } - _ => return, + match test.as_ref() { + // `else` + None => { + // The else must also be a single effect-free return statement + let [Stmt::Return(ast::StmtReturn { value, range: _ })] = body.as_slice() else { + return; + }; + if value.as_ref().map_or(false, |value| { + contains_effect(value, |id| checker.semantic().is_builtin(id)) + }) { + return; + }; + } + // `elif` + Some(Expr::Compare(ast::ExprCompare { + left, + ops, + comparators, + range: _, + })) => { + let Expr::Name(ast::ExprName { id, .. }) = left.as_ref() else { + return; + }; + if id != target || ops != &[CmpOp::Eq] { + return; + } + let [Expr::Constant(ast::ExprConstant { + value: constant, .. + })] = comparators.as_slice() + else { + return; + }; + + if value.as_ref().map_or(false, |value| { + contains_effect(value, |id| checker.semantic().is_builtin(id)) + }) { + return; + }; + + constants.insert(constant.into()); + } + // Different `elif` + _ => { + return; } - } else { - child = None; } } @@ -898,27 +826,38 @@ pub(crate) fn manual_dict_lookup( checker.diagnostics.push(Diagnostic::new( IfElseBlockInsteadOfDictLookup, - stmt.range(), + stmt_if.range(), )); } /// SIM401 -pub(crate) fn use_dict_get_with_default( - checker: &mut Checker, - stmt: &Stmt, - test: &Expr, - body: &[Stmt], - orelse: &[Stmt], - parent: Option<&Stmt>, -) { - if body.len() != 1 || orelse.len() != 1 { +pub(crate) fn use_dict_get_with_default(checker: &mut Checker, stmt_if: &StmtIf) { + let StmtIf { + test, + body, + elif_else_clauses, + .. + } = stmt_if; + + let [body_stmt] = body.as_slice() else { return; - } + }; + let [ElifElseClause { + body: else_body, + test: None, + .. + }] = elif_else_clauses.as_slice() + else { + return; + }; + let [else_body_stmt] = else_body.as_slice() else { + return; + }; let Stmt::Assign(ast::StmtAssign { targets: body_var, value: body_value, .. - }) = &body[0] + }) = &body_stmt else { return; }; @@ -929,7 +868,7 @@ pub(crate) fn use_dict_get_with_default( targets: orelse_var, value: orelse_value, .. - }) = &orelse[0] + }) = &else_body_stmt else { return; }; @@ -941,7 +880,7 @@ pub(crate) fn use_dict_get_with_default( ops, comparators: test_dict, range: _, - }) = &test + }) = test.as_ref() else { return; }; @@ -949,8 +888,18 @@ pub(crate) fn use_dict_get_with_default( return; } let (expected_var, expected_value, default_var, default_value) = match ops[..] { - [CmpOp::In] => (&body_var[0], body_value, &orelse_var[0], orelse_value), - [CmpOp::NotIn] => (&orelse_var[0], orelse_value, &body_var[0], body_value), + [CmpOp::In] => ( + &body_var[0], + body_value, + &orelse_var[0], + orelse_value.as_ref(), + ), + [CmpOp::NotIn] => ( + &orelse_var[0], + orelse_value, + &body_var[0], + body_value.as_ref(), + ), _ => { return; } @@ -979,37 +928,7 @@ pub(crate) fn use_dict_get_with_default( return; } - // It's part of a bigger if-elif block: - // https://github.com/MartinThoma/flake8-simplify/issues/115 - if let Some(Stmt::If(ast::StmtIf { - orelse: parent_orelse, - .. - })) = parent - { - if parent_orelse.len() == 1 && stmt == &parent_orelse[0] { - // TODO(charlie): These two cases have the same AST: - // - // if True: - // pass - // elif a: - // b = 1 - // else: - // b = 2 - // - // if True: - // pass - // else: - // if a: - // b = 1 - // else: - // b = 2 - // - // We want to flag the latter, but not the former. Right now, we flag neither. - return; - } - } - - let node = *default_value.clone(); + let node = default_value.clone(); let node1 = *test_key.clone(); let node2 = ast::ExprAttribute { value: expected_subscript.clone(), @@ -1033,9 +952,9 @@ pub(crate) fn use_dict_get_with_default( let contents = checker.generator().stmt(&node5.into()); // Don't flag if the resulting expression would exceed the maximum line length. - let line_start = checker.locator.line_start(stmt.start()); + let line_start = checker.locator.line_start(stmt_if.start()); if LineWidth::new(checker.settings.tab_size) - .add_str(&checker.locator.contents()[TextRange::new(line_start, stmt.start())]) + .add_str(&checker.locator.contents()[TextRange::new(line_start, stmt_if.start())]) .add_str(&contents) > checker.settings.line_length { @@ -1046,13 +965,13 @@ pub(crate) fn use_dict_get_with_default( IfElseBlockInsteadOfDictGet { contents: contents.clone(), }, - stmt.range(), + stmt_if.range(), ); if checker.patch(diagnostic.kind.rule()) { - if !has_comments(stmt, checker.locator, checker.indexer) { + if !has_comments(stmt_if, checker.locator, checker.indexer) { diagnostic.set_fix(Fix::suggested(Edit::range_replacement( contents, - stmt.range(), + stmt_if.range(), ))); } } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_unary_op.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_unary_op.rs index b8a4246c2f..9079807755 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_unary_op.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_unary_op.rs @@ -127,13 +127,7 @@ fn is_dunder_method(name: &str) -> bool { } fn is_exception_check(stmt: &Stmt) -> bool { - let Stmt::If(ast::StmtIf { - test: _, - body, - orelse: _, - range: _, - }) = stmt - else { + let Stmt::If(ast::StmtIf { body, .. }) = stmt else { return false; }; matches!(body.as_slice(), [Stmt::Raise(_)]) diff --git a/crates/ruff/src/rules/flake8_simplify/rules/fix_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/fix_if.rs index 3199c6c509..3b21d0539d 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/fix_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/fix_if.rs @@ -5,14 +5,13 @@ use libcst_native::{ BooleanOp, BooleanOperation, CompoundStatement, Expression, If, LeftParen, ParenthesizableWhitespace, ParenthesizedNode, RightParen, SimpleWhitespace, Statement, Suite, }; -use rustpython_parser::ast::Ranged; +use ruff_text_size::TextRange; -use crate::autofix::codemods::CodegenStylist; use ruff_diagnostics::Edit; use ruff_python_ast::source_code::{Locator, Stylist}; use ruff_python_ast::whitespace; -use ruff_python_whitespace::PythonWhitespace; +use crate::autofix::codemods::CodegenStylist; use crate::cst::matchers::{match_function_def, match_if, match_indented_block, match_statement}; fn parenthesize_and_operand(expr: Expression) -> Expression { @@ -34,21 +33,19 @@ fn parenthesize_and_operand(expr: Expression) -> Expression { pub(crate) fn fix_nested_if_statements( locator: &Locator, stylist: &Stylist, - stmt: &rustpython_parser::ast::Stmt, + range: TextRange, ) -> Result { // Infer the indentation of the outer block. - let Some(outer_indent) = whitespace::indentation(locator, stmt) else { + let Some(outer_indent) = whitespace::indentation(locator, &range) else { bail!("Unable to fix multiline statement"); }; // Extract the module text. - let contents = locator.lines(stmt.range()); - - // Handle `elif` blocks differently; detect them upfront. - let is_elif = contents.trim_whitespace_start().starts_with("elif"); + let contents = locator.lines(range); // If this is an `elif`, we have to remove the `elif` keyword for now. (We'll // restore the `el` later on.) + let is_elif = contents.starts_with("elif"); let module_text = if is_elif { Cow::Owned(contents.replacen("elif", "if", 1)) } else { @@ -128,6 +125,6 @@ pub(crate) fn fix_nested_if_statements( Cow::Borrowed(module_text) }; - let range = locator.lines_range(stmt.range()); + let range = locator.lines_range(range); Ok(Edit::range_replacement(contents.to_string(), range)) } diff --git a/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs b/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs index f0e757b111..4a30d94859 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/reimplemented_builtin.rs @@ -237,7 +237,7 @@ fn return_values_for_else(stmt: &Stmt) -> Option { let Stmt::If(ast::StmtIf { body: nested_body, test: nested_test, - orelse: nested_orelse, + elif_else_clauses: nested_elif_else_clauses, range: _, }) = &body[0] else { @@ -246,7 +246,7 @@ fn return_values_for_else(stmt: &Stmt) -> Option { if nested_body.len() != 1 { return None; } - if !nested_orelse.is_empty() { + if !nested_elif_else_clauses.is_empty() { return None; } let Stmt::Return(ast::StmtReturn { value, range: _ }) = &nested_body[0] else { @@ -317,7 +317,7 @@ fn return_values_for_siblings<'a>(stmt: &'a Stmt, sibling: &'a Stmt) -> Option(stmt: &'a Stmt, sibling: &'a Stmt) -> Option 0 else -x` instead of `if`-`else`-block | 57 | # SIM108 (without fix due to comments) diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index e9a21dee80..ed2db659c5 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -127,12 +127,11 @@ SIM114.py:38:1: SIM114 Combine `if` branches using logical `or` operator 58 | if result.eofs == "O": | -SIM114.py:62:6: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:62:1: SIM114 Combine `if` branches using logical `or` operator | 60 | elif result.eofs == "S": 61 | skipped = 1 -62 | elif result.eofs == "F": - | ______^ +62 | / elif result.eofs == "F": 63 | | errors = 1 64 | | elif result.eofs == "E": 65 | | errors = 1 diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap index 0b151c3d72..d2ef5ee577 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM116_SIM116.py.snap @@ -105,6 +105,8 @@ SIM116.py:79:1: SIM116 Use a dictionary instead of consecutive `if` statements 85 | | elif func_name == "move": 86 | | return "MV" | |_______________^ SIM116 +87 | +88 | # OK | diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap index 9ad99c7484..9408fda39e 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap @@ -114,7 +114,7 @@ SIM401.py:36:1: SIM401 [*] Use `vars[idx] = a_dict.get(key, "defaultß9πŸ’£2ℝ6 39 | | vars[idx] = "defaultß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789" | |___________________________________________________________________________^ SIM401 40 | -41 | ### +41 | # SIM401 | = help: Replace with `vars[idx] = a_dict.get(key, "defaultß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789")` @@ -128,7 +128,35 @@ SIM401.py:36:1: SIM401 [*] Use `vars[idx] = a_dict.get(key, "defaultß9πŸ’£2ℝ6 39 |- vars[idx] = "defaultß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789" 36 |+vars[idx] = a_dict.get(key, "defaultß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789ß9πŸ’£2ℝ6789") 40 37 | -41 38 | ### -42 39 | # Negative cases +41 38 | # SIM401 +42 39 | if foo(): + +SIM401.py:45:5: SIM401 [*] Use `vars[idx] = a_dict.get(key, "default")` instead of an `if` block + | +43 | pass +44 | else: +45 | if key in a_dict: + | _____^ +46 | | vars[idx] = a_dict[key] +47 | | else: +48 | | vars[idx] = "default" + | |_____________________________^ SIM401 +49 | +50 | ### + | + = help: Replace with `vars[idx] = a_dict.get(key, "default")` + +β„Ή Suggested fix +42 42 | if foo(): +43 43 | pass +44 44 | else: +45 |- if key in a_dict: +46 |- vars[idx] = a_dict[key] +47 |- else: +48 |- vars[idx] = "default" + 45 |+ vars[idx] = a_dict.get(key, "default") +49 46 | +50 47 | ### +51 48 | # Negative cases diff --git a/crates/ruff/src/rules/isort/block.rs b/crates/ruff/src/rules/isort/block.rs index e0eea8ae35..1f5d4f56c1 100644 --- a/crates/ruff/src/rules/isort/block.rs +++ b/crates/ruff/src/rules/isort/block.rs @@ -241,14 +241,18 @@ where } self.finalize(None); } - Stmt::If(ast::StmtIf { body, orelse, .. }) => { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { for stmt in body { self.visit_stmt(stmt); } self.finalize(None); - for stmt in orelse { - self.visit_stmt(stmt); + for clause in elif_else_clauses { + self.visit_elif_else_clause(clause); } self.finalize(None); } diff --git a/crates/ruff/src/rules/mccabe/rules/function_is_too_complex.rs b/crates/ruff/src/rules/mccabe/rules/function_is_too_complex.rs index d67de8efc1..e5242e69e6 100644 --- a/crates/ruff/src/rules/mccabe/rules/function_is_too_complex.rs +++ b/crates/ruff/src/rules/mccabe/rules/function_is_too_complex.rs @@ -68,10 +68,19 @@ fn get_complexity_number(stmts: &[Stmt]) -> usize { let mut complexity = 0; for stmt in stmts { match stmt { - Stmt::If(ast::StmtIf { body, orelse, .. }) => { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { complexity += 1; complexity += get_complexity_number(body); - complexity += get_complexity_number(orelse); + for clause in elif_else_clauses { + if clause.test.is_some() { + complexity += 1; + } + complexity += get_complexity_number(&clause.body); + } } Stmt::For(ast::StmtFor { body, orelse, .. }) | Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. }) => { diff --git a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs index b5bf5ea1ae..73b6ee60be 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs @@ -64,9 +64,12 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo // filtered.append(x) // ``` [Stmt::If(ast::StmtIf { - body, orelse, test, .. + body, + elif_else_clauses, + test, + .. })] => { - if !orelse.is_empty() { + if !elif_else_clauses.is_empty() { return; } let [stmt] = body.as_slice() else { diff --git a/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs b/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs index 8e3f4930e2..0bb158482b 100644 --- a/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs +++ b/crates/ruff/src/rules/pycodestyle/rules/lambda_assignment.rs @@ -224,6 +224,7 @@ fn function( body: vec![body], decorator_list: vec![], returns: Some(Box::new(return_type)), + type_params: vec![], type_comment: None, range: TextRange::default(), }); @@ -236,6 +237,7 @@ fn function( body: vec![body], decorator_list: vec![], returns: None, + type_params: vec![], type_comment: None, range: TextRange::default(), }); diff --git a/crates/ruff/src/rules/pyflakes/mod.rs b/crates/ruff/src/rules/pyflakes/mod.rs index e796ac4563..1f13edf973 100644 --- a/crates/ruff/src/rules/pyflakes/mod.rs +++ b/crates/ruff/src/rules/pyflakes/mod.rs @@ -107,6 +107,7 @@ mod tests { #[test_case(Rule::RedefinedWhileUnused, Path::new("F811_22.py"))] #[test_case(Rule::RedefinedWhileUnused, Path::new("F811_23.py"))] #[test_case(Rule::RedefinedWhileUnused, Path::new("F811_24.py"))] + #[test_case(Rule::RedefinedWhileUnused, Path::new("F811_25.py"))] #[test_case(Rule::UndefinedName, Path::new("F821_0.py"))] #[test_case(Rule::UndefinedName, Path::new("F821_1.py"))] #[test_case(Rule::UndefinedName, Path::new("F821_2.py"))] diff --git a/crates/ruff/src/rules/pyflakes/rules/if_tuple.rs b/crates/ruff/src/rules/pyflakes/rules/if_tuple.rs index 98ce068c20..0fe40fd842 100644 --- a/crates/ruff/src/rules/pyflakes/rules/if_tuple.rs +++ b/crates/ruff/src/rules/pyflakes/rules/if_tuple.rs @@ -1,7 +1,8 @@ -use rustpython_parser::ast::{self, Expr, Ranged, Stmt}; +use rustpython_parser::ast::{self, Expr, Ranged, StmtIf}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::stmt_if::if_elif_branches; use crate::checkers::ast::Checker; @@ -37,12 +38,16 @@ impl Violation for IfTuple { } /// F634 -pub(crate) fn if_tuple(checker: &mut Checker, stmt: &Stmt, test: &Expr) { - if let Expr::Tuple(ast::ExprTuple { elts, .. }) = &test { - if !elts.is_empty() { - checker - .diagnostics - .push(Diagnostic::new(IfTuple, stmt.range())); +pub(crate) fn if_tuple(checker: &mut Checker, stmt_if: &StmtIf) { + for branch in if_elif_branches(stmt_if) { + let Expr::Tuple(ast::ExprTuple { elts, .. }) = &branch.test else { + continue; + }; + if elts.is_empty() { + continue; } + checker + .diagnostics + .push(Diagnostic::new(IfTuple, branch.test.range())); } } diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F634_F634.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F634_F634.py.snap index 7f3a889816..d753d6d96e 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F634_F634.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F634_F634.py.snap @@ -1,25 +1,31 @@ --- source: crates/ruff/src/rules/pyflakes/mod.rs --- -F634.py:1:1: F634 If test is a tuple, which is always `True` +F634.py:1:4: F634 If test is a tuple, which is always `True` | -1 | / if (1, 2): -2 | | pass - | |________^ F634 -3 | -4 | for _ in range(5): +1 | if (1, 2): + | ^^^^^^ F634 +2 | pass | -F634.py:7:5: F634 If test is a tuple, which is always `True` +F634.py:4:4: F634 If test is a tuple, which is always `True` + | +2 | pass +3 | +4 | if (3, 4): + | ^^^^^^ F634 +5 | pass +6 | elif foo: + | + +F634.py:12:10: F634 If test is a tuple, which is always `True` | - 5 | if True: - 6 | pass - 7 | elif (3, 4): - | _____^ - 8 | | pass - 9 | | elif (): -10 | | pass - | |____________^ F634 +10 | if True: +11 | pass +12 | elif (3, 4): + | ^^^^^^ F634 +13 | pass +14 | elif (): | diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F811_F811_25.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F811_F811_25.py.snap new file mode 100644 index 0000000000..1976c4331d --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F811_F811_25.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- + diff --git a/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs b/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs index 57a1bf8405..1ad81b4cf7 100644 --- a/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs +++ b/crates/ruff/src/rules/pylint/rules/collapsible_else_if.rs @@ -1,8 +1,8 @@ -use rustpython_parser::ast::{Ranged, Stmt}; +use ruff_text_size::TextRange; +use rustpython_parser::ast::{ElifElseClause, Ranged, Stmt}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::source_code::Locator; /// ## What it does /// Checks for `else` blocks that consist of a single `if` statement. @@ -47,15 +47,20 @@ impl Violation for CollapsibleElseIf { } /// PLR5501 -pub(crate) fn collapsible_else_if(orelse: &[Stmt], locator: &Locator) -> Option { - if orelse.len() == 1 { - let first = &orelse[0]; - if matches!(first, Stmt::If(_)) { - // Determine whether this is an `elif`, or an `if` in an `else` block. - if locator.slice(first.range()).starts_with("if") { - return Some(Diagnostic::new(CollapsibleElseIf, first.range())); - } - } +pub(crate) fn collapsible_else_if(elif_else_clauses: &[ElifElseClause]) -> Option { + let Some(ElifElseClause { + body, + test: None, + range, + }) = elif_else_clauses.last() + else { + return None; + }; + if let [first @ Stmt::If(_)] = body.as_slice() { + return Some(Diagnostic::new( + CollapsibleElseIf, + TextRange::new(range.start(), first.start()), + )); } None } diff --git a/crates/ruff/src/rules/pylint/rules/continue_in_finally.rs b/crates/ruff/src/rules/pylint/rules/continue_in_finally.rs index 600ee4306e..84a553c929 100644 --- a/crates/ruff/src/rules/pylint/rules/continue_in_finally.rs +++ b/crates/ruff/src/rules/pylint/rules/continue_in_finally.rs @@ -54,8 +54,17 @@ fn traverse_body(checker: &mut Checker, body: &[Stmt]) { } match stmt { - Stmt::If(ast::StmtIf { body, orelse, .. }) - | Stmt::Try(ast::StmtTry { body, orelse, .. }) + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + traverse_body(checker, body); + for clause in elif_else_clauses { + traverse_body(checker, &clause.body); + } + } + Stmt::Try(ast::StmtTry { body, orelse, .. }) | Stmt::TryStar(ast::StmtTryStar { body, orelse, .. }) => { traverse_body(checker, body); traverse_body(checker, orelse); diff --git a/crates/ruff/src/rules/pylint/rules/too_many_branches.rs b/crates/ruff/src/rules/pylint/rules/too_many_branches.rs index 5d997ab843..32e2cf728b 100644 --- a/crates/ruff/src/rules/pylint/rules/too_many_branches.rs +++ b/crates/ruff/src/rules/pylint/rules/too_many_branches.rs @@ -88,74 +88,74 @@ impl Violation for TooManyBranches { fn num_branches(stmts: &[Stmt]) -> usize { stmts .iter() - .map(|stmt| { - match stmt { - Stmt::If(ast::StmtIf { body, orelse, .. }) => { - 1 + num_branches(body) - + (if let Some(stmt) = orelse.first() { - // `elif:` and `else: if:` have the same AST representation. - // Avoid treating `elif:` as two statements. - usize::from(!stmt.is_if_stmt()) - } else { - 0 - }) - + num_branches(orelse) - } - Stmt::Match(ast::StmtMatch { cases, .. }) => { - 1 + cases + .map(|stmt| match stmt { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + 1 + num_branches(body) + + elif_else_clauses.len() + + elif_else_clauses .iter() - .map(|case| num_branches(&case.body)) + .map(|clause| num_branches(&clause.body)) .sum::() - } - Stmt::For(ast::StmtFor { body, orelse, .. }) - | Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. }) - | Stmt::While(ast::StmtWhile { body, orelse, .. }) => { - 1 + num_branches(body) - + (if orelse.is_empty() { - 0 - } else { - 1 + num_branches(orelse) - }) - } - Stmt::Try(ast::StmtTry { - body, - handlers, - orelse, - finalbody, - range: _, - }) - | Stmt::TryStar(ast::StmtTryStar { - body, - handlers, - orelse, - finalbody, - range: _, - }) => { - 1 + num_branches(body) - + (if orelse.is_empty() { - 0 - } else { - 1 + num_branches(orelse) - }) - + (if finalbody.is_empty() { - 0 - } else { - 1 + num_branches(finalbody) - }) - + handlers - .iter() - .map(|handler| { - 1 + { - let ExceptHandler::ExceptHandler( - ast::ExceptHandlerExceptHandler { body, .. }, - ) = handler; - num_branches(body) - } - }) - .sum::() - } - _ => 0, } + Stmt::Match(ast::StmtMatch { cases, .. }) => { + 1 + cases + .iter() + .map(|case| num_branches(&case.body)) + .sum::() + } + Stmt::For(ast::StmtFor { body, orelse, .. }) + | Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. }) + | Stmt::While(ast::StmtWhile { body, orelse, .. }) => { + 1 + num_branches(body) + + (if orelse.is_empty() { + 0 + } else { + 1 + num_branches(orelse) + }) + } + Stmt::Try(ast::StmtTry { + body, + handlers, + orelse, + finalbody, + range: _, + }) + | Stmt::TryStar(ast::StmtTryStar { + body, + handlers, + orelse, + finalbody, + range: _, + }) => { + 1 + num_branches(body) + + (if orelse.is_empty() { + 0 + } else { + 1 + num_branches(orelse) + }) + + (if finalbody.is_empty() { + 0 + } else { + 1 + num_branches(finalbody) + }) + + handlers + .iter() + .map(|handler| { + 1 + { + let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { + body, + .. + }) = handler; + num_branches(body) + } + }) + .sum::() + } + _ => 0, }) .sum() } @@ -205,8 +205,7 @@ else: else: pass "#; - - test_helper(source, 3)?; + test_helper(source, 4)?; Ok(()) } diff --git a/crates/ruff/src/rules/pylint/rules/too_many_statements.rs b/crates/ruff/src/rules/pylint/rules/too_many_statements.rs index b22bffdbd8..0832b55c83 100644 --- a/crates/ruff/src/rules/pylint/rules/too_many_statements.rs +++ b/crates/ruff/src/rules/pylint/rules/too_many_statements.rs @@ -66,16 +66,16 @@ fn num_statements(stmts: &[Stmt]) -> usize { let mut count = 0; for stmt in stmts { match stmt { - Stmt::If(ast::StmtIf { body, orelse, .. }) => { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { count += 1; count += num_statements(body); - if let Some(stmt) = orelse.first() { - // `elif:` and `else: if:` have the same AST representation. - // Avoid treating `elif:` as two statements. - if !stmt.is_if_stmt() { - count += 1; - } - count += num_statements(orelse); + for clause in elif_else_clauses { + count += 1; + count += num_statements(&clause.body); } } Stmt::For(ast::StmtFor { body, orelse, .. }) @@ -207,7 +207,7 @@ def f(): print() "#; let stmts = Suite::parse(source, "")?; - assert_eq!(num_statements(&stmts), 5); + assert_eq!(num_statements(&stmts), 6); Ok(()) } diff --git a/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs b/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs index 32fb1b8b59..2005541405 100644 --- a/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs +++ b/crates/ruff/src/rules/pylint/rules/useless_else_on_loop.rs @@ -53,8 +53,15 @@ impl Violation for UselessElseOnLoop { fn loop_exits_early(body: &[Stmt]) -> bool { body.iter().any(|stmt| match stmt { - Stmt::If(ast::StmtIf { body, orelse, .. }) => { - loop_exits_early(body) || loop_exits_early(orelse) + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { + loop_exits_early(body) + || elif_else_clauses + .iter() + .any(|clause| loop_exits_early(&clause.body)) } Stmt::With(ast::StmtWith { body, .. }) | Stmt::AsyncWith(ast::StmtAsyncWith { body, .. }) => loop_exits_early(body), diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap index 426513185c..cfb1bb48fe 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap @@ -1,26 +1,39 @@ --- source: crates/ruff/src/rules/pylint/mod.rs --- -collapsible_else_if.py:38:9: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation +collapsible_else_if.py:37:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | +35 | if 1: 36 | pass 37 | else: -38 | if 2: - | _________^ -39 | | pass - | |________________^ PLR5501 + | _____^ +38 | | if 2: + | |________^ PLR5501 +39 | pass | -collapsible_else_if.py:46:9: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation +collapsible_else_if.py:45:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation | +43 | if 1: 44 | pass 45 | else: -46 | if 2: - | _________^ -47 | | pass -48 | | else: -49 | | pass - | |________________^ PLR5501 + | _____^ +46 | | if 2: + | |________^ PLR5501 +47 | pass +48 | else: + | + +collapsible_else_if.py:58:5: PLR5501 Use `elif` instead of `else` then `if`, to reduce indentation + | +56 | elif True: +57 | print(2) +58 | else: + | _____^ +59 | | if True: + | |________^ PLR5501 +60 | print(3) +61 | else: | diff --git a/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs b/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs index 2d59a3df80..857871708f 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs @@ -170,6 +170,7 @@ fn create_class_def_stmt(typename: &str, body: Vec, base_class: &Expr) -> bases: vec![base_class.clone()], keywords: vec![], body, + type_params: vec![], decorator_list: vec![], range: TextRange::default(), } diff --git a/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs b/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs index afce256896..8e3b541ec9 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs @@ -127,6 +127,7 @@ fn create_class_def_stmt( bases: vec![base_class.clone()], keywords, body, + type_params: vec![], decorator_list: vec![], range: TextRange::default(), } diff --git a/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs b/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs index e84e0ab64b..cde8398e0e 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/outdated_version_block.rs @@ -1,13 +1,12 @@ use std::cmp::Ordering; use num_bigint::{BigInt, Sign}; -use ruff_text_size::{TextRange, TextSize}; -use rustpython_parser::ast::{self, CmpOp, Constant, Expr, Ranged, Stmt}; -use rustpython_parser::{lexer, Mode, Tok}; +use ruff_text_size::{TextLen, TextRange}; +use rustpython_parser::ast::{self, CmpOp, Constant, ElifElseClause, Expr, Ranged, StmtIf}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::source_code::Locator; +use ruff_python_ast::stmt_if::{if_elif_branches, BranchKind, IfElifBranch}; use ruff_python_ast::whitespace::indentation; use crate::autofix::edits::delete_stmt; @@ -61,98 +60,6 @@ impl AlwaysAutofixableViolation for OutdatedVersionBlock { } } -/// The metadata for a version-comparison block. -#[derive(Debug)] -struct BlockMetadata { - /// The first `if` or `elif` token in the block, used to signal the start of the - /// version-comparison block. - leading_token: StartToken, - /// The first `elif` or `else` token following the start token, if any, used to signal the end - /// of the version-comparison block. - trailing_token: Option, -} - -/// The set of tokens that can start a block, i.e., the first token in an `if` statement. -#[derive(Debug)] -enum StartTok { - If, - Elif, -} - -impl StartTok { - fn from_tok(tok: &Tok) -> Option { - match tok { - Tok::If => Some(Self::If), - Tok::Elif => Some(Self::Elif), - _ => None, - } - } -} - -#[derive(Debug)] -struct StartToken { - tok: StartTok, - range: TextRange, -} - -/// The set of tokens that can end a block, i.e., the first token in the subsequent `elif` or `else` -/// branch that follows an `if` or `elif` statement. -#[derive(Debug)] -enum EndTok { - Elif, - Else, -} - -impl EndTok { - fn from_tok(tok: &Tok) -> Option { - match tok { - Tok::Elif => Some(Self::Elif), - Tok::Else => Some(Self::Else), - _ => None, - } - } -} - -#[derive(Debug)] -struct EndToken { - tok: EndTok, - range: TextRange, -} - -fn metadata(locator: &Locator, located: &T, body: &[Stmt]) -> Option -where - T: Ranged, -{ - indentation(locator, located)?; - - let mut iter = lexer::lex_starts_at( - locator.slice(located.range()), - Mode::Module, - located.start(), - ) - .flatten(); - - // First the leading `if` or `elif` token. - let (tok, range) = iter.next()?; - let leading_token = StartToken { - tok: StartTok::from_tok(&tok)?, - range, - }; - - // Skip any tokens until we reach the end of the `if` body. - let body_end = body.last()?.range().end(); - - // Find the trailing `elif` or `else` token, if any. - let trailing_token = iter - .skip_while(|(_, range)| range.start() < body_end) - .find_map(|(tok, range)| EndTok::from_tok(&tok).map(|tok| EndToken { tok, range })); - - Some(BlockMetadata { - leading_token, - trailing_token, - }) -} - /// Converts a `BigInt` to a `u32`. If the number is negative, it will return 0. fn bigint_to_u32(number: &BigInt) -> u32 { let the_number = number.to_u32_digits(); @@ -207,96 +114,110 @@ fn compare_version(if_version: &[u32], py_version: PythonVersion, or_equal: bool } } -/// Convert a [`Stmt::If`], retaining the `else`. -fn fix_py2_block( - checker: &Checker, - stmt: &Stmt, - orelse: &[Stmt], - block: &BlockMetadata, -) -> Option { - let leading_token = &block.leading_token; - let Some(trailing_token) = &block.trailing_token else { - // Delete the entire statement. If this is an `elif`, know it's the only child - // of its parent, so avoid passing in the parent at all. Otherwise, - // `delete_stmt` will erroneously include a `pass`. - let stmt = checker.semantic().stmt(); - let parent = checker.semantic().stmt_parent(); - let edit = delete_stmt( - stmt, - if matches!(block.leading_token.tok, StartTok::If) { - parent - } else { - None - }, - checker.locator, - checker.indexer, - ); - return Some(Fix::suggested(edit)); - }; - - match (&leading_token.tok, &trailing_token.tok) { - // If we only have an `if` and an `else`, dedent the `else` block. - (StartTok::If, EndTok::Else) => { - let start = orelse.first()?; - let end = orelse.last()?; - if indentation(checker.locator, start).is_none() { - // Inline `else` block (e.g., `else: x = 1`). - Some(Fix::suggested(Edit::range_replacement( - checker - .locator - .slice(TextRange::new(start.start(), end.end())) - .to_string(), - stmt.range(), - ))) - } else { - indentation(checker.locator, stmt) - .and_then(|indentation| { - adjust_indentation( - TextRange::new(checker.locator.line_start(start.start()), end.end()), - indentation, - checker.locator, - checker.stylist, - ) - .ok() - }) - .map(|contents| { - Fix::suggested(Edit::replacement( - contents, - checker.locator.line_start(stmt.start()), - stmt.end(), - )) - }) +/// For fixing, we have 4 cases: +/// * Just an if: delete as statement (insert pass in parent if required) +/// * If with an elif: delete, turn elif into if +/// * If with an else: delete, dedent else +/// * Just an elif: delete, `elif False` can always be removed +fn fix_py2_block(checker: &Checker, stmt_if: &StmtIf, branch: &IfElifBranch) -> Option { + match branch.kind { + BranchKind::If => match stmt_if.elif_else_clauses.first() { + // If we have a lone `if`, delete as statement (insert pass in parent if required) + None => { + let stmt = checker.semantic().stmt(); + let parent = checker.semantic().stmt_parent(); + let edit = delete_stmt(stmt, parent, checker.locator, checker.indexer); + Some(Fix::suggested(edit)) } - } - (StartTok::If, EndTok::Elif) => { - // If we have an `if` and an `elif`, turn the `elif` into an `if`. - let start_location = leading_token.range.start(); - let end_location = trailing_token.range.start() + TextSize::from(2); - Some(Fix::suggested(Edit::deletion(start_location, end_location))) - } - (StartTok::Elif, _) => { - // If we have an `elif`, delete up to the `else` or the end of the statement. - let start_location = leading_token.range.start(); - let end_location = trailing_token.range.start(); - Some(Fix::suggested(Edit::deletion(start_location, end_location))) + // If we have an `if` and an `elif`, turn the `elif` into an `if` + Some(ElifElseClause { + test: Some(_), + range, + .. + }) => { + debug_assert!( + &checker.locator.contents()[TextRange::at(range.start(), "elif".text_len())] + == "elif" + ); + let end_location = range.start() + ("elif".text_len() - "if".text_len()); + Some(Fix::suggested(Edit::deletion( + stmt_if.start(), + end_location, + ))) + } + // If we only have an `if` and an `else`, dedent the `else` block + Some(ElifElseClause { + body, test: None, .. + }) => { + let start = body.first()?; + let end = body.last()?; + if indentation(checker.locator, start).is_none() { + // Inline `else` block (e.g., `else: x = 1`). + Some(Fix::suggested(Edit::range_replacement( + checker + .locator + .slice(TextRange::new(start.start(), end.end())) + .to_string(), + stmt_if.range(), + ))) + } else { + indentation(checker.locator, stmt_if) + .and_then(|indentation| { + adjust_indentation( + TextRange::new( + checker.locator.line_start(start.start()), + end.end(), + ), + indentation, + checker.locator, + checker.stylist, + ) + .ok() + }) + .map(|contents| { + Fix::suggested(Edit::replacement( + contents, + checker.locator.line_start(stmt_if.start()), + stmt_if.end(), + )) + }) + } + } + }, + BranchKind::Elif => { + // The range of the `ElifElseClause` ends in the line of the last statement. To avoid + // inserting an empty line between the end of `if` branch and the beginning `elif` or + // `else` branch after the deleted branch we find the next branch after the current, if + // any, and delete to its start. + // ```python + // if cond: + // x = 1 + // elif sys.version < (3.0): + // delete from here ... ^ x = 2 + // else: + // ... to here (exclusive) ^ x = 3 + // ``` + let next_start = stmt_if + .elif_else_clauses + .iter() + .map(Ranged::start) + .find(|start| *start > branch.range.start()); + Some(Fix::suggested(Edit::deletion( + branch.range.start(), + next_start.unwrap_or(branch.range.end()), + ))) } } } /// Convert a [`Stmt::If`], removing the `else` block. -fn fix_py3_block( - checker: &mut Checker, - stmt: &Stmt, - test: &Expr, - body: &[Stmt], - block: &BlockMetadata, -) -> Option { - match block.leading_token.tok { - StartTok::If => { - // If the first statement is an if, use the body of this statement, and ignore +fn fix_py3_block(checker: &mut Checker, stmt_if: &StmtIf, branch: &IfElifBranch) -> Option { + match branch.kind { + BranchKind::If => { + // If the first statement is an `if`, use the body of this statement, and ignore // the rest. - let start = body.first()?; - let end = body.last()?; + let start = branch.body.first()?; + let end = branch.body.last()?; if indentation(checker.locator, start).is_none() { // Inline `if` block (e.g., `if ...: x = 1`). Some(Fix::suggested(Edit::range_replacement( @@ -304,10 +225,10 @@ fn fix_py3_block( .locator .slice(TextRange::new(start.start(), end.end())) .to_string(), - stmt.range(), + stmt_if.range, ))) } else { - indentation(checker.locator, stmt) + indentation(checker.locator, &stmt_if) .and_then(|indentation| { adjust_indentation( TextRange::new(checker.locator.line_start(start.start()), end.end()), @@ -320,81 +241,76 @@ fn fix_py3_block( .map(|contents| { Fix::suggested(Edit::replacement( contents, - checker.locator.line_start(stmt.start()), - stmt.end(), + checker.locator.line_start(stmt_if.start()), + stmt_if.end(), )) }) } } - StartTok::Elif => { - // Replace the `elif` with an `else, preserve the body of the elif, and remove + BranchKind::Elif => { + // Replace the `elif` with an `else`, preserve the body of the elif, and remove // the rest. - let end = body.last()?; - let text = checker.locator.slice(TextRange::new(test.end(), end.end())); + let end = branch.body.last()?; + let text = checker + .locator + .slice(TextRange::new(branch.test.end(), end.end())); Some(Fix::suggested(Edit::range_replacement( format!("else{text}"), - stmt.range(), + TextRange::new(branch.range.start(), stmt_if.end()), ))) } } } /// UP036 -pub(crate) fn outdated_version_block( - checker: &mut Checker, - stmt: &Stmt, - test: &Expr, - body: &[Stmt], - orelse: &[Stmt], -) { - let Expr::Compare(ast::ExprCompare { - left, - ops, - comparators, - range: _, - }) = &test - else { - return; - }; +pub(crate) fn outdated_version_block(checker: &mut Checker, stmt_if: &StmtIf) { + for branch in if_elif_branches(stmt_if) { + let Expr::Compare(ast::ExprCompare { + left, + ops, + comparators, + range: _, + }) = &branch.test + else { + continue; + }; - if !checker - .semantic() - .resolve_call_path(left) - .map_or(false, |call_path| { - matches!(call_path.as_slice(), ["sys", "version_info"]) - }) - { - return; - } + let ([op], [comparison]) = (ops.as_slice(), comparators.as_slice()) else { + continue; + }; + + if !checker + .semantic() + .resolve_call_path(left) + .map_or(false, |call_path| { + matches!(call_path.as_slice(), ["sys", "version_info"]) + }) + { + continue; + } - if ops.len() == 1 && comparators.len() == 1 { - let comparison = &comparators[0]; - let op = &ops[0]; match comparison { Expr::Tuple(ast::ExprTuple { elts, .. }) => { let version = extract_version(elts); let target = checker.settings.target_version; if op == &CmpOp::Lt || op == &CmpOp::LtE { if compare_version(&version, target, op == &CmpOp::LtE) { - let mut diagnostic = Diagnostic::new(OutdatedVersionBlock, stmt.range()); + let mut diagnostic = + Diagnostic::new(OutdatedVersionBlock, branch.test.range()); if checker.patch(diagnostic.kind.rule()) { - if let Some(block) = metadata(checker.locator, stmt, body) { - if let Some(fix) = fix_py2_block(checker, stmt, orelse, &block) { - diagnostic.set_fix(fix); - } + if let Some(fix) = fix_py2_block(checker, stmt_if, &branch) { + diagnostic.set_fix(fix); } } checker.diagnostics.push(diagnostic); } } else if op == &CmpOp::Gt || op == &CmpOp::GtE { if compare_version(&version, target, op == &CmpOp::GtE) { - let mut diagnostic = Diagnostic::new(OutdatedVersionBlock, stmt.range()); + let mut diagnostic = + Diagnostic::new(OutdatedVersionBlock, branch.test.range()); if checker.patch(diagnostic.kind.rule()) { - if let Some(block) = metadata(checker.locator, stmt, body) { - if let Some(fix) = fix_py3_block(checker, stmt, test, body, &block) - { - diagnostic.set_fix(fix); - } + if let Some(fix) = fix_py3_block(checker, stmt_if, &branch) { + diagnostic.set_fix(fix); } } checker.diagnostics.push(diagnostic); @@ -407,22 +323,18 @@ pub(crate) fn outdated_version_block( }) => { let version_number = bigint_to_u32(number); if version_number == 2 && op == &CmpOp::Eq { - let mut diagnostic = Diagnostic::new(OutdatedVersionBlock, stmt.range()); + let mut diagnostic = Diagnostic::new(OutdatedVersionBlock, branch.test.range()); if checker.patch(diagnostic.kind.rule()) { - if let Some(block) = metadata(checker.locator, stmt, body) { - if let Some(fix) = fix_py2_block(checker, stmt, orelse, &block) { - diagnostic.set_fix(fix); - } + if let Some(fix) = fix_py2_block(checker, stmt_if, &branch) { + diagnostic.set_fix(fix); } } checker.diagnostics.push(diagnostic); } else if version_number == 3 && op == &CmpOp::Eq { - let mut diagnostic = Diagnostic::new(OutdatedVersionBlock, stmt.range()); + let mut diagnostic = Diagnostic::new(OutdatedVersionBlock, branch.test.range()); if checker.patch(diagnostic.kind.rule()) { - if let Some(block) = metadata(checker.locator, stmt, body) { - if let Some(fix) = fix_py3_block(checker, stmt, test, body, &block) { - diagnostic.set_fix(fix); - } + if let Some(fix) = fix_py3_block(checker, stmt_if, &branch) { + diagnostic.set_fix(fix); } } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_0.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_0.py.snap index bb8998dbcc..4fccfbe7b3 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_0.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_0.py.snap @@ -1,17 +1,14 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP036_0.py:3:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:3:4: UP036 [*] Version block is outdated for minimum Python version | -1 | import sys -2 | -3 | / if sys.version_info < (3,0): -4 | | print("py2") -5 | | else: -6 | | print("py3") - | |________________^ UP036 -7 | -8 | if sys.version_info < (3,0): +1 | import sys +2 | +3 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +4 | print("py2") +5 | else: | = help: Remove outdated version block @@ -27,20 +24,14 @@ UP036_0.py:3:1: UP036 [*] Version block is outdated for minimum Python version 8 5 | if sys.version_info < (3,0): 9 6 | if True: -UP036_0.py:8:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:8:4: UP036 [*] Version block is outdated for minimum Python version | - 6 | print("py3") - 7 | - 8 | / if sys.version_info < (3,0): - 9 | | if True: -10 | | print("py2!") -11 | | else: -12 | | print("???") -13 | | else: -14 | | print("py3") - | |________________^ UP036 -15 | -16 | if sys.version_info < (3,0): print("PY2!") + 6 | print("py3") + 7 | + 8 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 + 9 | if True: +10 | print("py2!") | = help: Remove outdated version block @@ -60,15 +51,13 @@ UP036_0.py:8:1: UP036 [*] Version block is outdated for minimum Python version 16 10 | if sys.version_info < (3,0): print("PY2!") 17 11 | else: print("PY3!") -UP036_0.py:16:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:16:4: UP036 [*] Version block is outdated for minimum Python version | -14 | print("py3") -15 | -16 | / if sys.version_info < (3,0): print("PY2!") -17 | | else: print("PY3!") - | |___________________^ UP036 -18 | -19 | if True: +14 | print("py3") +15 | +16 | if sys.version_info < (3,0): print("PY2!") + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +17 | else: print("PY3!") | = help: Remove outdated version block @@ -83,17 +72,13 @@ UP036_0.py:16:1: UP036 [*] Version block is outdated for minimum Python version 19 18 | if True: 20 19 | if sys.version_info < (3,0): -UP036_0.py:20:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:20:8: UP036 [*] Version block is outdated for minimum Python version | -19 | if True: -20 | if sys.version_info < (3,0): - | _____^ -21 | | print("PY2") -22 | | else: -23 | | print("PY3") - | |____________________^ UP036 -24 | -25 | if sys.version_info < (3,0): print(1 if True else 3) +19 | if True: +20 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +21 | print("PY2") +22 | else: | = help: Remove outdated version block @@ -110,16 +95,14 @@ UP036_0.py:20:5: UP036 [*] Version block is outdated for minimum Python version 25 22 | if sys.version_info < (3,0): print(1 if True else 3) 26 23 | else: -UP036_0.py:25:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:25:4: UP036 [*] Version block is outdated for minimum Python version | -23 | print("PY3") -24 | -25 | / if sys.version_info < (3,0): print(1 if True else 3) -26 | | else: -27 | | print("py3") - | |________________^ UP036 -28 | -29 | if sys.version_info < (3,0): +23 | print("PY3") +24 | +25 | if sys.version_info < (3,0): print(1 if True else 3) + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +26 | else: +27 | print("py3") | = help: Remove outdated version block @@ -135,20 +118,14 @@ UP036_0.py:25:1: UP036 [*] Version block is outdated for minimum Python version 29 27 | if sys.version_info < (3,0): 30 28 | def f(): -UP036_0.py:29:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:29:4: UP036 [*] Version block is outdated for minimum Python version | -27 | print("py3") -28 | -29 | / if sys.version_info < (3,0): -30 | | def f(): -31 | | print("py2") -32 | | else: -33 | | def f(): -34 | | print("py3") -35 | | print("This the next") - | |______________________________^ UP036 -36 | -37 | if sys.version_info > (3,0): +27 | print("py3") +28 | +29 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +30 | def f(): +31 | print("py2") | = help: Remove outdated version block @@ -170,15 +147,14 @@ UP036_0.py:29:1: UP036 [*] Version block is outdated for minimum Python version 37 33 | if sys.version_info > (3,0): 38 34 | print("py3") -UP036_0.py:37:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:37:4: UP036 [*] Version block is outdated for minimum Python version | -35 | print("This the next") -36 | -37 | / if sys.version_info > (3,0): -38 | | print("py3") -39 | | else: -40 | | print("py2") - | |________________^ UP036 +35 | print("This the next") +36 | +37 | if sys.version_info > (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +38 | print("py3") +39 | else: | = help: Remove outdated version block @@ -195,16 +171,14 @@ UP036_0.py:37:1: UP036 [*] Version block is outdated for minimum Python version 42 39 | 43 40 | x = 1 -UP036_0.py:45:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:45:4: UP036 [*] Version block is outdated for minimum Python version | -43 | x = 1 -44 | -45 | / if sys.version_info > (3,0): -46 | | print("py3") -47 | | else: -48 | | print("py2") - | |________________^ UP036 -49 | # ohai +43 | x = 1 +44 | +45 | if sys.version_info > (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +46 | print("py3") +47 | else: | = help: Remove outdated version block @@ -221,15 +195,13 @@ UP036_0.py:45:1: UP036 [*] Version block is outdated for minimum Python version 50 47 | 51 48 | x = 1 -UP036_0.py:53:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:53:4: UP036 [*] Version block is outdated for minimum Python version | -51 | x = 1 -52 | -53 | / if sys.version_info > (3,0): print("py3") -54 | | else: print("py2") - | |__________________^ UP036 -55 | -56 | if sys.version_info > (3,): +51 | x = 1 +52 | +53 | if sys.version_info > (3,0): print("py3") + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +54 | else: print("py2") | = help: Remove outdated version block @@ -244,17 +216,14 @@ UP036_0.py:53:1: UP036 [*] Version block is outdated for minimum Python version 56 55 | if sys.version_info > (3,): 57 56 | print("py3") -UP036_0.py:56:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:56:4: UP036 [*] Version block is outdated for minimum Python version | -54 | else: print("py2") -55 | -56 | / if sys.version_info > (3,): -57 | | print("py3") -58 | | else: -59 | | print("py2") - | |________________^ UP036 -60 | -61 | if True: +54 | else: print("py2") +55 | +56 | if sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +57 | print("py3") +58 | else: | = help: Remove outdated version block @@ -271,17 +240,13 @@ UP036_0.py:56:1: UP036 [*] Version block is outdated for minimum Python version 61 58 | if True: 62 59 | if sys.version_info > (3,): -UP036_0.py:62:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:62:8: UP036 [*] Version block is outdated for minimum Python version | -61 | if True: -62 | if sys.version_info > (3,): - | _____^ -63 | | print("py3") -64 | | else: -65 | | print("py2") - | |____________________^ UP036 -66 | -67 | if sys.version_info < (3,): +61 | if True: +62 | if sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +63 | print("py3") +64 | else: | = help: Remove outdated version block @@ -298,17 +263,14 @@ UP036_0.py:62:5: UP036 [*] Version block is outdated for minimum Python version 67 64 | if sys.version_info < (3,): 68 65 | print("py2") -UP036_0.py:67:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:67:4: UP036 [*] Version block is outdated for minimum Python version | -65 | print("py2") -66 | -67 | / if sys.version_info < (3,): -68 | | print("py2") -69 | | else: -70 | | print("py3") - | |________________^ UP036 -71 | -72 | def f(): +65 | print("py2") +66 | +67 | if sys.version_info < (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +68 | print("py2") +69 | else: | = help: Remove outdated version block @@ -325,18 +287,13 @@ UP036_0.py:67:1: UP036 [*] Version block is outdated for minimum Python version 72 69 | def f(): 73 70 | if sys.version_info < (3,0): -UP036_0.py:73:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:73:8: UP036 [*] Version block is outdated for minimum Python version | -72 | def f(): -73 | if sys.version_info < (3,0): - | _____^ -74 | | try: -75 | | yield -76 | | finally: -77 | | pass -78 | | else: -79 | | yield - | |_____________^ UP036 +72 | def f(): +73 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +74 | try: +75 | yield | = help: Remove outdated version block @@ -356,20 +313,14 @@ UP036_0.py:73:5: UP036 [*] Version block is outdated for minimum Python version 81 75 | 82 76 | class C: -UP036_0.py:86:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:86:8: UP036 [*] Version block is outdated for minimum Python version | -84 | pass -85 | -86 | if sys.version_info < (3,0): - | _____^ -87 | | def f(py2): -88 | | pass -89 | | else: -90 | | def f(py3): -91 | | pass - | |________________^ UP036 -92 | -93 | def h(): +84 | pass +85 | +86 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +87 | def f(py2): +88 | pass | = help: Remove outdated version block @@ -389,19 +340,15 @@ UP036_0.py:86:5: UP036 [*] Version block is outdated for minimum Python version 93 89 | def h(): 94 90 | pass -UP036_0.py:97:5: UP036 [*] Version block is outdated for minimum Python version - | - 96 | if True: - 97 | if sys.version_info < (3,0): - | _____^ - 98 | | 2 - 99 | | else: -100 | | 3 - | |_________^ UP036 -101 | -102 | # comment - | - = help: Remove outdated version block +UP036_0.py:97:8: UP036 [*] Version block is outdated for minimum Python version + | +96 | if True: +97 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +98 | 2 +99 | else: + | + = help: Remove outdated version block β„Ή Suggested fix 94 94 | pass @@ -416,23 +363,14 @@ UP036_0.py:97:5: UP036 [*] Version block is outdated for minimum Python version 102 99 | # comment 103 100 | -UP036_0.py:104:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:104:4: UP036 [*] Version block is outdated for minimum Python version | -102 | # comment -103 | -104 | / if sys.version_info < (3,0): -105 | | def f(): -106 | | print("py2") -107 | | def g(): -108 | | print("py2") -109 | | else: -110 | | def f(): -111 | | print("py3") -112 | | def g(): -113 | | print("py3") - | |____________________^ UP036 -114 | -115 | if True: +102 | # comment +103 | +104 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +105 | def f(): +106 | print("py2") | = help: Remove outdated version block @@ -458,15 +396,13 @@ UP036_0.py:104:1: UP036 [*] Version block is outdated for minimum Python version 115 109 | if True: 116 110 | if sys.version_info > (3,): -UP036_0.py:116:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:116:8: UP036 [*] Version block is outdated for minimum Python version | -115 | if True: -116 | if sys.version_info > (3,): - | _____^ -117 | | print(3) - | |________________^ UP036 -118 | # comment -119 | print(2+3) +115 | if True: +116 | if sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +117 | print(3) +118 | # comment | = help: Remove outdated version block @@ -481,11 +417,11 @@ UP036_0.py:116:5: UP036 [*] Version block is outdated for minimum Python version 119 118 | print(2+3) 120 119 | -UP036_0.py:122:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:122:8: UP036 [*] Version block is outdated for minimum Python version | 121 | if True: 122 | if sys.version_info > (3,): print(3) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 123 | 124 | if True: | @@ -501,13 +437,12 @@ UP036_0.py:122:5: UP036 [*] Version block is outdated for minimum Python version 124 124 | if True: 125 125 | if sys.version_info > (3,): -UP036_0.py:125:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:125:8: UP036 [*] Version block is outdated for minimum Python version | -124 | if True: -125 | if sys.version_info > (3,): - | _____^ -126 | | print(3) - | |________________^ UP036 +124 | if True: +125 | if sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +126 | print(3) | = help: Remove outdated version block @@ -522,19 +457,13 @@ UP036_0.py:125:5: UP036 [*] Version block is outdated for minimum Python version 128 127 | 129 128 | if True: -UP036_0.py:130:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:130:8: UP036 [*] Version block is outdated for minimum Python version | -129 | if True: -130 | if sys.version_info <= (3, 0): - | _____^ -131 | | expected_error = [] -132 | | else: -133 | | expected_error = [ -134 | | ":1:5: Generator expression must be parenthesized", -135 | | "max(1 for i in range(10), key=lambda x: x+1)", -136 | | " ^", -137 | | ] - | |_________^ UP036 +129 | if True: +130 | if sys.version_info <= (3, 0): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +131 | expected_error = [] +132 | else: | = help: Remove outdated version block @@ -556,17 +485,12 @@ UP036_0.py:130:5: UP036 [*] Version block is outdated for minimum Python version 139 136 | 140 137 | if sys.version_info <= (3, 0): -UP036_0.py:140:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:140:4: UP036 [*] Version block is outdated for minimum Python version | -140 | / if sys.version_info <= (3, 0): -141 | | expected_error = [] -142 | | else: -143 | | expected_error = [ -144 | | ":1:5: Generator expression must be parenthesized", -145 | | "max(1 for i in range(10), key=lambda x: x+1)", -146 | | " ^", -147 | | ] - | |_____^ UP036 +140 | if sys.version_info <= (3, 0): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +141 | expected_error = [] +142 | else: | = help: Remove outdated version block @@ -588,23 +512,12 @@ UP036_0.py:140:1: UP036 [*] Version block is outdated for minimum Python version 149 146 | 150 147 | if sys.version_info > (3,0): -UP036_0.py:150:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:150:4: UP036 [*] Version block is outdated for minimum Python version | -150 | / if sys.version_info > (3,0): -151 | | """this -152 | | is valid""" -153 | | -154 | | """the indentation on -155 | | this line is significant""" -156 | | -157 | | "this is" \ -158 | | "allowed too" -159 | | -160 | | ("so is" -161 | | "this for some reason") - | |____________________________^ UP036 -162 | -163 | if sys.version_info > (3, 0): expected_error = \ +150 | if sys.version_info > (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +151 | """this +152 | is valid""" | = help: Remove outdated version block @@ -633,15 +546,13 @@ UP036_0.py:150:1: UP036 [*] Version block is outdated for minimum Python version 163 162 | if sys.version_info > (3, 0): expected_error = \ 164 163 | [] -UP036_0.py:163:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:163:4: UP036 [*] Version block is outdated for minimum Python version | -161 | "this for some reason") -162 | -163 | / if sys.version_info > (3, 0): expected_error = \ -164 | | [] - | |______^ UP036 -165 | -166 | if sys.version_info > (3, 0): expected_error = [] +161 | "this for some reason") +162 | +163 | if sys.version_info > (3, 0): expected_error = \ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +164 | [] | = help: Remove outdated version block @@ -655,12 +566,12 @@ UP036_0.py:163:1: UP036 [*] Version block is outdated for minimum Python version 165 165 | 166 166 | if sys.version_info > (3, 0): expected_error = [] -UP036_0.py:166:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:166:4: UP036 [*] Version block is outdated for minimum Python version | 164 | [] 165 | 166 | if sys.version_info > (3, 0): expected_error = [] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 167 | 168 | if sys.version_info > (3, 0): \ | @@ -676,15 +587,13 @@ UP036_0.py:166:1: UP036 [*] Version block is outdated for minimum Python version 168 168 | if sys.version_info > (3, 0): \ 169 169 | expected_error = [] -UP036_0.py:168:1: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:168:4: UP036 [*] Version block is outdated for minimum Python version | -166 | if sys.version_info > (3, 0): expected_error = [] -167 | -168 | / if sys.version_info > (3, 0): \ -169 | | expected_error = [] - | |_______________________^ UP036 -170 | -171 | if True: +166 | if sys.version_info > (3, 0): expected_error = [] +167 | +168 | if sys.version_info > (3, 0): \ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +169 | expected_error = [] | = help: Remove outdated version block @@ -699,15 +608,12 @@ UP036_0.py:168:1: UP036 [*] Version block is outdated for minimum Python version 171 170 | if True: 172 171 | if sys.version_info > (3, 0): expected_error = \ -UP036_0.py:172:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:172:8: UP036 [*] Version block is outdated for minimum Python version | -171 | if True: -172 | if sys.version_info > (3, 0): expected_error = \ - | _____^ -173 | | [] - | |______^ UP036 -174 | -175 | if True: +171 | if True: +172 | if sys.version_info > (3, 0): expected_error = \ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +173 | [] | = help: Remove outdated version block @@ -721,11 +627,11 @@ UP036_0.py:172:5: UP036 [*] Version block is outdated for minimum Python version 174 174 | 175 175 | if True: -UP036_0.py:176:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:176:8: UP036 [*] Version block is outdated for minimum Python version | 175 | if True: 176 | if sys.version_info > (3, 0): expected_error = [] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 177 | 178 | if True: | @@ -741,13 +647,12 @@ UP036_0.py:176:5: UP036 [*] Version block is outdated for minimum Python version 178 178 | if True: 179 179 | if sys.version_info > (3, 0): \ -UP036_0.py:179:5: UP036 [*] Version block is outdated for minimum Python version +UP036_0.py:179:8: UP036 [*] Version block is outdated for minimum Python version | -178 | if True: -179 | if sys.version_info > (3, 0): \ - | _____^ -180 | | expected_error = [] - | |_______________________^ UP036 +178 | if True: +179 | if sys.version_info > (3, 0): \ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +180 | expected_error = [] | = help: Remove outdated version block diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_1.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_1.py.snap index e64022388a..5d385f538d 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_1.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_1.py.snap @@ -1,17 +1,14 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP036_1.py:3:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:3:4: UP036 [*] Version block is outdated for minimum Python version | -1 | import sys -2 | -3 | / if sys.version_info == 2: -4 | | 2 -5 | | else: -6 | | 3 - | |_____^ UP036 -7 | -8 | if sys.version_info < (3,): +1 | import sys +2 | +3 | if sys.version_info == 2: + | ^^^^^^^^^^^^^^^^^^^^^ UP036 +4 | 2 +5 | else: | = help: Remove outdated version block @@ -27,17 +24,14 @@ UP036_1.py:3:1: UP036 [*] Version block is outdated for minimum Python version 8 5 | if sys.version_info < (3,): 9 6 | 2 -UP036_1.py:8:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:8:4: UP036 [*] Version block is outdated for minimum Python version | - 6 | 3 - 7 | - 8 | / if sys.version_info < (3,): - 9 | | 2 -10 | | else: -11 | | 3 - | |_____^ UP036 -12 | -13 | if sys.version_info < (3,0): + 6 | 3 + 7 | + 8 | if sys.version_info < (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 + 9 | 2 +10 | else: | = help: Remove outdated version block @@ -54,17 +48,14 @@ UP036_1.py:8:1: UP036 [*] Version block is outdated for minimum Python version 13 10 | if sys.version_info < (3,0): 14 11 | 2 -UP036_1.py:13:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:13:4: UP036 [*] Version block is outdated for minimum Python version | -11 | 3 -12 | -13 | / if sys.version_info < (3,0): -14 | | 2 -15 | | else: -16 | | 3 - | |_____^ UP036 -17 | -18 | if sys.version_info == 3: +11 | 3 +12 | +13 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +14 | 2 +15 | else: | = help: Remove outdated version block @@ -81,17 +72,14 @@ UP036_1.py:13:1: UP036 [*] Version block is outdated for minimum Python version 18 15 | if sys.version_info == 3: 19 16 | 3 -UP036_1.py:18:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:18:4: UP036 [*] Version block is outdated for minimum Python version | -16 | 3 -17 | -18 | / if sys.version_info == 3: -19 | | 3 -20 | | else: -21 | | 2 - | |_____^ UP036 -22 | -23 | if sys.version_info > (3,): +16 | 3 +17 | +18 | if sys.version_info == 3: + | ^^^^^^^^^^^^^^^^^^^^^ UP036 +19 | 3 +20 | else: | = help: Remove outdated version block @@ -108,17 +96,14 @@ UP036_1.py:18:1: UP036 [*] Version block is outdated for minimum Python version 23 20 | if sys.version_info > (3,): 24 21 | 3 -UP036_1.py:23:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:23:4: UP036 [*] Version block is outdated for minimum Python version | -21 | 2 -22 | -23 | / if sys.version_info > (3,): -24 | | 3 -25 | | else: -26 | | 2 - | |_____^ UP036 -27 | -28 | if sys.version_info >= (3,): +21 | 2 +22 | +23 | if sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +24 | 3 +25 | else: | = help: Remove outdated version block @@ -135,17 +120,14 @@ UP036_1.py:23:1: UP036 [*] Version block is outdated for minimum Python version 28 25 | if sys.version_info >= (3,): 29 26 | 3 -UP036_1.py:28:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:28:4: UP036 [*] Version block is outdated for minimum Python version | -26 | 2 -27 | -28 | / if sys.version_info >= (3,): -29 | | 3 -30 | | else: -31 | | 2 - | |_____^ UP036 -32 | -33 | from sys import version_info +26 | 2 +27 | +28 | if sys.version_info >= (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +29 | 3 +30 | else: | = help: Remove outdated version block @@ -162,17 +144,14 @@ UP036_1.py:28:1: UP036 [*] Version block is outdated for minimum Python version 33 30 | from sys import version_info 34 31 | -UP036_1.py:35:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:35:4: UP036 [*] Version block is outdated for minimum Python version | -33 | from sys import version_info -34 | -35 | / if version_info > (3,): -36 | | 3 -37 | | else: -38 | | 2 - | |_____^ UP036 -39 | -40 | if True: +33 | from sys import version_info +34 | +35 | if version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^ UP036 +36 | 3 +37 | else: | = help: Remove outdated version block @@ -189,17 +168,14 @@ UP036_1.py:35:1: UP036 [*] Version block is outdated for minimum Python version 40 37 | if True: 41 38 | print(1) -UP036_1.py:42:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:42:6: UP036 [*] Version block is outdated for minimum Python version | -40 | if True: -41 | print(1) -42 | / elif sys.version_info < (3,0): -43 | | print(2) -44 | | else: -45 | | print(3) - | |____________^ UP036 -46 | -47 | if True: +40 | if True: +41 | print(1) +42 | elif sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +43 | print(2) +44 | else: | = help: Remove outdated version block @@ -213,17 +189,14 @@ UP036_1.py:42:1: UP036 [*] Version block is outdated for minimum Python version 45 43 | print(3) 46 44 | -UP036_1.py:49:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:49:6: UP036 [*] Version block is outdated for minimum Python version | -47 | if True: -48 | print(1) -49 | / elif sys.version_info > (3,): -50 | | print(3) -51 | | else: -52 | | print(2) - | |____________^ UP036 -53 | -54 | if True: +47 | if True: +48 | print(1) +49 | elif sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +50 | print(3) +51 | else: | = help: Remove outdated version block @@ -240,15 +213,13 @@ UP036_1.py:49:1: UP036 [*] Version block is outdated for minimum Python version 54 52 | if True: 55 53 | print(1) -UP036_1.py:56:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:56:6: UP036 [*] Version block is outdated for minimum Python version | -54 | if True: -55 | print(1) -56 | / elif sys.version_info > (3,): -57 | | print(3) - | |____________^ UP036 -58 | -59 | def f(): +54 | if True: +55 | print(1) +56 | elif sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +57 | print(3) | = help: Remove outdated version block @@ -262,16 +233,13 @@ UP036_1.py:56:1: UP036 [*] Version block is outdated for minimum Python version 58 58 | 59 59 | def f(): -UP036_1.py:62:5: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:62:10: UP036 [*] Version block is outdated for minimum Python version | -60 | if True: -61 | print(1) -62 | elif sys.version_info > (3,): - | _____^ -63 | | print(3) - | |________________^ UP036 -64 | -65 | if True: +60 | if True: +61 | print(1) +62 | elif sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +63 | print(3) | = help: Remove outdated version block @@ -285,17 +253,14 @@ UP036_1.py:62:5: UP036 [*] Version block is outdated for minimum Python version 64 64 | 65 65 | if True: -UP036_1.py:67:1: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:67:6: UP036 [*] Version block is outdated for minimum Python version | -65 | if True: -66 | print(1) -67 | / elif sys.version_info < (3,0): -68 | | print(2) -69 | | else: -70 | | print(3) - | |____________^ UP036 -71 | -72 | def f(): +65 | if True: +66 | print(1) +67 | elif sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +68 | print(2) +69 | else: | = help: Remove outdated version block @@ -309,14 +274,13 @@ UP036_1.py:67:1: UP036 [*] Version block is outdated for minimum Python version 70 68 | print(3) 71 69 | -UP036_1.py:75:5: UP036 [*] Version block is outdated for minimum Python version +UP036_1.py:75:10: UP036 [*] Version block is outdated for minimum Python version | -73 | if True: -74 | print(1) -75 | elif sys.version_info > (3,): - | _____^ -76 | | print(3) - | |________________^ UP036 +73 | if True: +74 | print(1) +75 | elif sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +76 | print(3) | = help: Remove outdated version block diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_2.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_2.py.snap index 773502a44f..9ca87c972a 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_2.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_2.py.snap @@ -1,17 +1,14 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP036_2.py:4:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:4:4: UP036 [*] Version block is outdated for minimum Python version | -2 | from sys import version_info -3 | -4 | / if sys.version_info > (3, 5): -5 | | 3+6 -6 | | else: -7 | | 3-5 - | |_______^ UP036 -8 | -9 | if version_info > (3, 5): +2 | from sys import version_info +3 | +4 | if sys.version_info > (3, 5): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +5 | 3+6 +6 | else: | = help: Remove outdated version block @@ -28,17 +25,14 @@ UP036_2.py:4:1: UP036 [*] Version block is outdated for minimum Python version 9 6 | if version_info > (3, 5): 10 7 | 3+6 -UP036_2.py:9:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:9:4: UP036 [*] Version block is outdated for minimum Python version | - 7 | 3-5 - 8 | - 9 | / if version_info > (3, 5): -10 | | 3+6 -11 | | else: -12 | | 3-5 - | |_______^ UP036 -13 | -14 | if sys.version_info >= (3,6): + 7 | 3-5 + 8 | + 9 | if version_info > (3, 5): + | ^^^^^^^^^^^^^^^^^^^^^ UP036 +10 | 3+6 +11 | else: | = help: Remove outdated version block @@ -55,17 +49,14 @@ UP036_2.py:9:1: UP036 [*] Version block is outdated for minimum Python version 14 11 | if sys.version_info >= (3,6): 15 12 | 3+6 -UP036_2.py:14:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:14:4: UP036 [*] Version block is outdated for minimum Python version | -12 | 3-5 -13 | -14 | / if sys.version_info >= (3,6): -15 | | 3+6 -16 | | else: -17 | | 3-5 - | |_______^ UP036 -18 | -19 | if version_info >= (3,6): +12 | 3-5 +13 | +14 | if sys.version_info >= (3,6): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +15 | 3+6 +16 | else: | = help: Remove outdated version block @@ -82,17 +73,14 @@ UP036_2.py:14:1: UP036 [*] Version block is outdated for minimum Python version 19 16 | if version_info >= (3,6): 20 17 | 3+6 -UP036_2.py:19:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:19:4: UP036 [*] Version block is outdated for minimum Python version | -17 | 3-5 -18 | -19 | / if version_info >= (3,6): -20 | | 3+6 -21 | | else: -22 | | 3-5 - | |_______^ UP036 -23 | -24 | if sys.version_info < (3,6): +17 | 3-5 +18 | +19 | if version_info >= (3,6): + | ^^^^^^^^^^^^^^^^^^^^^ UP036 +20 | 3+6 +21 | else: | = help: Remove outdated version block @@ -109,17 +97,14 @@ UP036_2.py:19:1: UP036 [*] Version block is outdated for minimum Python version 24 21 | if sys.version_info < (3,6): 25 22 | 3-5 -UP036_2.py:24:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:24:4: UP036 [*] Version block is outdated for minimum Python version | -22 | 3-5 -23 | -24 | / if sys.version_info < (3,6): -25 | | 3-5 -26 | | else: -27 | | 3+6 - | |_______^ UP036 -28 | -29 | if sys.version_info <= (3,5): +22 | 3-5 +23 | +24 | if sys.version_info < (3,6): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +25 | 3-5 +26 | else: | = help: Remove outdated version block @@ -136,17 +121,14 @@ UP036_2.py:24:1: UP036 [*] Version block is outdated for minimum Python version 29 26 | if sys.version_info <= (3,5): 30 27 | 3-5 -UP036_2.py:29:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:29:4: UP036 [*] Version block is outdated for minimum Python version | -27 | 3+6 -28 | -29 | / if sys.version_info <= (3,5): -30 | | 3-5 -31 | | else: -32 | | 3+6 - | |_______^ UP036 -33 | -34 | if sys.version_info <= (3, 5): +27 | 3+6 +28 | +29 | if sys.version_info <= (3,5): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +30 | 3-5 +31 | else: | = help: Remove outdated version block @@ -163,17 +145,14 @@ UP036_2.py:29:1: UP036 [*] Version block is outdated for minimum Python version 34 31 | if sys.version_info <= (3, 5): 35 32 | 3-5 -UP036_2.py:34:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:34:4: UP036 [*] Version block is outdated for minimum Python version | -32 | 3+6 -33 | -34 | / if sys.version_info <= (3, 5): -35 | | 3-5 -36 | | else: -37 | | 3+6 - | |_______^ UP036 -38 | -39 | if sys.version_info >= (3, 5): +32 | 3+6 +33 | +34 | if sys.version_info <= (3, 5): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +35 | 3-5 +36 | else: | = help: Remove outdated version block @@ -190,15 +169,13 @@ UP036_2.py:34:1: UP036 [*] Version block is outdated for minimum Python version 39 36 | if sys.version_info >= (3, 5): 40 37 | pass -UP036_2.py:39:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:39:4: UP036 [*] Version block is outdated for minimum Python version | -37 | 3+6 -38 | -39 | / if sys.version_info >= (3, 5): -40 | | pass - | |________^ UP036 -41 | -42 | if sys.version_info < (3,0): +37 | 3+6 +38 | +39 | if sys.version_info >= (3, 5): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +40 | pass | = help: Remove outdated version block @@ -213,15 +190,13 @@ UP036_2.py:39:1: UP036 [*] Version block is outdated for minimum Python version 42 41 | if sys.version_info < (3,0): 43 42 | pass -UP036_2.py:42:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:42:4: UP036 [*] Version block is outdated for minimum Python version | -40 | pass -41 | -42 | / if sys.version_info < (3,0): -43 | | pass - | |________^ UP036 -44 | -45 | if True: +40 | pass +41 | +42 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +43 | pass | = help: Remove outdated version block @@ -235,15 +210,12 @@ UP036_2.py:42:1: UP036 [*] Version block is outdated for minimum Python version 45 43 | if True: 46 44 | if sys.version_info < (3,0): -UP036_2.py:46:5: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:46:8: UP036 [*] Version block is outdated for minimum Python version | -45 | if True: -46 | if sys.version_info < (3,0): - | _____^ -47 | | pass - | |____________^ UP036 -48 | -49 | if sys.version_info < (3,0): +45 | if True: +46 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +47 | pass | = help: Remove outdated version block @@ -258,17 +230,14 @@ UP036_2.py:46:5: UP036 [*] Version block is outdated for minimum Python version 49 48 | if sys.version_info < (3,0): 50 49 | pass -UP036_2.py:49:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:49:4: UP036 [*] Version block is outdated for minimum Python version | -47 | pass -48 | -49 | / if sys.version_info < (3,0): -50 | | pass -51 | | elif False: -52 | | pass - | |________^ UP036 -53 | -54 | if sys.version_info > (3,): +47 | pass +48 | +49 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +50 | pass +51 | elif False: | = help: Remove outdated version block @@ -284,17 +253,14 @@ UP036_2.py:49:1: UP036 [*] Version block is outdated for minimum Python version 53 51 | 54 52 | if sys.version_info > (3,): -UP036_2.py:54:1: UP036 [*] Version block is outdated for minimum Python version +UP036_2.py:54:4: UP036 [*] Version block is outdated for minimum Python version | -52 | pass -53 | -54 | / if sys.version_info > (3,): -55 | | pass -56 | | elif False: -57 | | pass - | |________^ UP036 -58 | -59 | if sys.version_info[0] > "2": +52 | pass +53 | +54 | if sys.version_info > (3,): + | ^^^^^^^^^^^^^^^^^^^^^^^ UP036 +55 | pass +56 | elif False: | = help: Remove outdated version block diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_3.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_3.py.snap index f7365f6174..d82d1aba25 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_3.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_3.py.snap @@ -1,23 +1,16 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP036_3.py:3:1: UP036 [*] Version block is outdated for minimum Python version - | - 1 | import sys - 2 | - 3 | / if sys.version_info < (3,0): - 4 | | print("py2") - 5 | | for item in range(10): - 6 | | print(f"PY2-{item}") - 7 | | else : - 8 | | print("py3") - 9 | | for item in range(10): -10 | | print(f"PY3-{item}") - | |____________________________^ UP036 -11 | -12 | if False: - | - = help: Remove outdated version block +UP036_3.py:3:15: UP036 [*] Version block is outdated for minimum Python version + | +1 | import sys +2 | +3 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +4 | print("py2") +5 | for item in range(10): + | + = help: Remove outdated version block β„Ή Suggested fix 1 1 | import sys @@ -37,19 +30,13 @@ UP036_3.py:3:1: UP036 [*] Version block is outdated for minimum Python version 12 7 | if False: 13 8 | if sys.version_info < (3,0): -UP036_3.py:13:5: UP036 [*] Version block is outdated for minimum Python version +UP036_3.py:13:19: UP036 [*] Version block is outdated for minimum Python version | -12 | if False: -13 | if sys.version_info < (3,0): - | _____^ -14 | | print("py2") -15 | | for item in range(10): -16 | | print(f"PY2-{item}") -17 | | else : -18 | | print("py3") -19 | | for item in range(10): -20 | | print(f"PY3-{item}") - | |________________________________^ UP036 +12 | if False: +13 | if sys.version_info < (3,0): + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +14 | print("py2") +15 | for item in range(10): | = help: Remove outdated version block @@ -72,11 +59,11 @@ UP036_3.py:13:5: UP036 [*] Version block is outdated for minimum Python version 22 17 | 23 18 | if sys.version_info < (3,0): print("PY2!") -UP036_3.py:23:1: UP036 [*] Version block is outdated for minimum Python version +UP036_3.py:23:15: UP036 [*] Version block is outdated for minimum Python version | -23 | / if sys.version_info < (3,0): print("PY2!") -24 | | else : print("PY3!") - | |__________________________________________________^ UP036 +23 | if sys.version_info < (3,0): print("PY2!") + | ^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +24 | else : print("PY3!") | = help: Remove outdated version block diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_4.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_4.py.snap index 9ddd273855..a0bacbe7af 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_4.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_4.py.snap @@ -1,13 +1,12 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP036_4.py:4:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:4:8: UP036 [*] Version block is outdated for minimum Python version | -3 | if True: -4 | if sys.version_info < (3, 3): - | _____^ -5 | | cmd = [sys.executable, "-m", "test.regrtest"] - | |_____________________________________________________^ UP036 +3 | if True: +4 | if sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +5 | cmd = [sys.executable, "-m", "test.regrtest"] | = help: Remove outdated version block @@ -22,87 +21,76 @@ UP036_4.py:4:5: UP036 [*] Version block is outdated for minimum Python version 7 6 | 8 7 | if True: -UP036_4.py:11:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:11:10: UP036 [*] Version block is outdated for minimum Python version | - 9 | if foo: -10 | pass -11 | elif sys.version_info < (3, 3): - | _____^ -12 | | cmd = [sys.executable, "-m", "test.regrtest"] - | |_____________________________________________________^ UP036 -13 | -14 | if True: + 9 | if foo: +10 | print() +11 | elif sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +12 | cmd = [sys.executable, "-m", "test.regrtest"] | = help: Remove outdated version block β„Ή Suggested fix 8 8 | if True: 9 9 | if foo: -10 10 | pass +10 10 | print() 11 |- elif sys.version_info < (3, 3): 12 |- cmd = [sys.executable, "-m", "test.regrtest"] -13 11 | -14 12 | if True: -15 13 | if foo: + 11 |+ +13 12 | +14 13 | if True: +15 14 | if foo: -UP036_4.py:17:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:17:10: UP036 [*] Version block is outdated for minimum Python version | -15 | if foo: -16 | pass -17 | elif sys.version_info < (3, 3): - | _____^ -18 | | cmd = [sys.executable, "-m", "test.regrtest"] -19 | | elif foo: -20 | | cmd = [sys.executable, "-m", "test", "-j0"] - | |___________________________________________________^ UP036 -21 | -22 | if foo: +15 | if foo: +16 | print() +17 | elif sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +18 | cmd = [sys.executable, "-m", "test.regrtest"] +19 | elif foo: | = help: Remove outdated version block β„Ή Suggested fix 14 14 | if True: 15 15 | if foo: -16 16 | pass +16 16 | print() 17 |- elif sys.version_info < (3, 3): 18 |- cmd = [sys.executable, "-m", "test.regrtest"] 19 17 | elif foo: 20 18 | cmd = [sys.executable, "-m", "test", "-j0"] 21 19 | -UP036_4.py:24:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:24:10: UP036 [*] Version block is outdated for minimum Python version | -22 | if foo: -23 | pass -24 | elif sys.version_info < (3, 3): - | _____^ -25 | | cmd = [sys.executable, "-m", "test.regrtest"] - | |_____________________________________________________^ UP036 -26 | -27 | if sys.version_info < (3, 3): +22 | if foo: +23 | print() +24 | elif sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +25 | cmd = [sys.executable, "-m", "test.regrtest"] | = help: Remove outdated version block β„Ή Suggested fix 21 21 | 22 22 | if foo: -23 23 | pass +23 23 | print() 24 |- elif sys.version_info < (3, 3): 25 |- cmd = [sys.executable, "-m", "test.regrtest"] -26 24 | -27 25 | if sys.version_info < (3, 3): -28 26 | cmd = [sys.executable, "-m", "test.regrtest"] + 24 |+ +26 25 | +27 26 | if sys.version_info < (3, 3): +28 27 | cmd = [sys.executable, "-m", "test.regrtest"] -UP036_4.py:27:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:27:8: UP036 [*] Version block is outdated for minimum Python version | -25 | cmd = [sys.executable, "-m", "test.regrtest"] -26 | -27 | if sys.version_info < (3, 3): - | _____^ -28 | | cmd = [sys.executable, "-m", "test.regrtest"] - | |_____________________________________________________^ UP036 -29 | -30 | if foo: +25 | cmd = [sys.executable, "-m", "test.regrtest"] +26 | +27 | if sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +28 | cmd = [sys.executable, "-m", "test.regrtest"] | = help: Remove outdated version block @@ -114,45 +102,37 @@ UP036_4.py:27:5: UP036 [*] Version block is outdated for minimum Python version 28 |- cmd = [sys.executable, "-m", "test.regrtest"] 29 27 | 30 28 | if foo: -31 29 | pass +31 29 | print() -UP036_4.py:32:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:32:10: UP036 [*] Version block is outdated for minimum Python version | -30 | if foo: -31 | pass -32 | elif sys.version_info < (3, 3): - | _____^ -33 | | cmd = [sys.executable, "-m", "test.regrtest"] -34 | | else: -35 | | cmd = [sys.executable, "-m", "test", "-j0"] - | |___________________________________________________^ UP036 -36 | -37 | if sys.version_info < (3, 3): +30 | if foo: +31 | print() +32 | elif sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +33 | cmd = [sys.executable, "-m", "test.regrtest"] +34 | else: | = help: Remove outdated version block β„Ή Suggested fix 29 29 | 30 30 | if foo: -31 31 | pass +31 31 | print() 32 |- elif sys.version_info < (3, 3): 33 |- cmd = [sys.executable, "-m", "test.regrtest"] 34 32 | else: 35 33 | cmd = [sys.executable, "-m", "test", "-j0"] 36 34 | -UP036_4.py:37:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:37:8: UP036 [*] Version block is outdated for minimum Python version | -35 | cmd = [sys.executable, "-m", "test", "-j0"] -36 | -37 | if sys.version_info < (3, 3): - | _____^ -38 | | cmd = [sys.executable, "-m", "test.regrtest"] -39 | | else: -40 | | cmd = [sys.executable, "-m", "test", "-j0"] - | |___________________________________________________^ UP036 -41 | -42 | if sys.version_info < (3, 3): +35 | cmd = [sys.executable, "-m", "test", "-j0"] +36 | +37 | if sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +38 | cmd = [sys.executable, "-m", "test.regrtest"] +39 | else: | = help: Remove outdated version block @@ -169,16 +149,14 @@ UP036_4.py:37:5: UP036 [*] Version block is outdated for minimum Python version 42 39 | if sys.version_info < (3, 3): 43 40 | cmd = [sys.executable, "-m", "test.regrtest"] -UP036_4.py:42:5: UP036 [*] Version block is outdated for minimum Python version +UP036_4.py:42:8: UP036 [*] Version block is outdated for minimum Python version | -40 | cmd = [sys.executable, "-m", "test", "-j0"] -41 | -42 | if sys.version_info < (3, 3): - | _____^ -43 | | cmd = [sys.executable, "-m", "test.regrtest"] -44 | | elif foo: -45 | | cmd = [sys.executable, "-m", "test", "-j0"] - | |___________________________________________________^ UP036 +40 | cmd = [sys.executable, "-m", "test", "-j0"] +41 | +42 | if sys.version_info < (3, 3): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +43 | cmd = [sys.executable, "-m", "test.regrtest"] +44 | elif foo: | = help: Remove outdated version block diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_5.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_5.py.snap index 59edd620b5..3bc04c32cb 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_5.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP036_5.py.snap @@ -1,24 +1,16 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP036_5.py:3:1: UP036 [*] Version block is outdated for minimum Python version - | - 1 | import sys - 2 | - 3 | / if sys.version_info < (3, 8): - 4 | | - 5 | | def a(): - 6 | | if b: - 7 | | print(1) - 8 | | elif c: - 9 | | print(2) -10 | | return None -11 | | -12 | | else: -13 | | pass - | |________^ UP036 - | - = help: Remove outdated version block +UP036_5.py:3:4: UP036 [*] Version block is outdated for minimum Python version + | +1 | import sys +2 | +3 | if sys.version_info < (3, 8): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +4 | +5 | def a(): + | + = help: Remove outdated version block β„Ή Suggested fix 1 1 | import sys @@ -39,24 +31,13 @@ UP036_5.py:3:1: UP036 [*] Version block is outdated for minimum Python version 15 5 | 16 6 | import sys -UP036_5.py:18:1: UP036 [*] Version block is outdated for minimum Python version +UP036_5.py:18:4: UP036 [*] Version block is outdated for minimum Python version | -16 | import sys -17 | -18 | / if sys.version_info < (3, 8): -19 | | pass -20 | | -21 | | else: -22 | | -23 | | def a(): -24 | | if b: -25 | | print(1) -26 | | elif c: -27 | | print(2) -28 | | else: -29 | | print(3) -30 | | return None - | |___________________^ UP036 +16 | import sys +17 | +18 | if sys.version_info < (3, 8): + | ^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +19 | pass | = help: Remove outdated version block diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap index 1ab3a11c7d..9294427514 100644 --- a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap @@ -286,9 +286,11 @@ flowchart TD start(("Start")) return(("End")) block0["return 1\n"] - block1["return 2\n"] - block2["return 0\n"] - block3["elif False: + block1["return 0\n"] + block2["return 2\n"] + block3["if True: + return 1 + elif False: return 2 else: return 0\n"] @@ -302,8 +304,8 @@ flowchart TD start --> block4 block4 -- "True" --> block0 block4 -- "else" --> block3 - block3 -- "False" --> block1 - block3 -- "else" --> block2 + block3 -- "False" --> block2 + block3 -- "else" --> block1 block2 --> return block1 --> return block0 --> return @@ -327,9 +329,11 @@ flowchart TD start(("Start")) return(("End")) block0["return 1\n"] - block1["return 2\n"] - block2["return 0\n"] - block3["elif True: + block1["return 0\n"] + block2["return 2\n"] + block3["if False: + return 1 + elif True: return 2 else: return 0\n"] @@ -343,8 +347,8 @@ flowchart TD start --> block4 block4 -- "False" --> block0 block4 -- "else" --> block3 - block3 -- "True" --> block1 - block3 -- "else" --> block2 + block3 -- "True" --> block2 + block3 -- "else" --> block1 block2 --> return block1 --> return block0 --> return @@ -377,9 +381,11 @@ flowchart TD block0["return 6\n"] block1["return 3\n"] block2["return 0\n"] - block3["return 1\n"] - block4["return 2\n"] - block5["elif True: + block3["return 2\n"] + block4["return 1\n"] + block5["if False: + return 0 + elif True: return 1 else: return 2\n"] @@ -389,9 +395,17 @@ flowchart TD return 1 else: return 2\n"] - block7["return 4\n"] - block8["return 5\n"] - block9["elif True: + block7["return 5\n"] + block8["return 4\n"] + block9["if True: + if False: + return 0 + elif True: + return 1 + else: + return 2 + return 3 + elif True: return 4 else: return 5\n"] @@ -411,14 +425,14 @@ flowchart TD start --> block10 block10 -- "True" --> block6 block10 -- "else" --> block9 - block9 -- "True" --> block7 - block9 -- "else" --> block8 + block9 -- "True" --> block8 + block9 -- "else" --> block7 block8 --> return block7 --> return block6 -- "False" --> block2 block6 -- "else" --> block5 - block5 -- "True" --> block3 - block5 -- "else" --> block4 + block5 -- "True" --> block4 + block5 -- "else" --> block3 block4 --> return block3 --> return block2 --> return @@ -445,7 +459,9 @@ flowchart TD block0["return #quot;reached#quot;\n"] block1["return #quot;unreached#quot;\n"] block2["return #quot;also unreached#quot;\n"] - block3["elif False: + block3["if False: + return #quot;unreached#quot; + elif False: return #quot;also unreached#quot;\n"] block4["if False: return #quot;unreached#quot; @@ -490,14 +506,16 @@ flowchart TD return(("End")) block0["return buffer.data\n"] block1["return base64.b64decode(data)\n"] - block2["buffer = data\n"] - block3["buffer = self._buffers[id]\n"] - block4["self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] - block5["id = data[#quot;id#quot;]\nif id in self._buffers: + block2["buffer = self._buffers[id]\n"] + block3["self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] + block4["id = data[#quot;id#quot;]\nif id in self._buffers: buffer = self._buffers[id] else: self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] - block6["elif isinstance(data, Buffer): + block5["buffer = data\n"] + block6["if isinstance(data, str): + return base64.b64decode(data) + elif isinstance(data, Buffer): buffer = data else: id = data[#quot;id#quot;] @@ -521,11 +539,11 @@ flowchart TD start --> block7 block7 -- "isinstance(data, str)" --> block1 block7 -- "else" --> block6 - block6 -- "isinstance(data, Buffer)" --> block2 - block6 -- "else" --> block5 - block5 -- "id in self._buffers" --> block3 - block5 -- "else" --> block4 - block4 --> block0 + block6 -- "isinstance(data, Buffer)" --> block5 + block6 -- "else" --> block4 + block5 --> block0 + block4 -- "id in self._buffers" --> block2 + block4 -- "else" --> block3 block3 --> block0 block2 --> block0 block1 --> return diff --git a/crates/ruff/src/rules/ruff/rules/unreachable.rs b/crates/ruff/src/rules/ruff/rules/unreachable.rs index 5c81acd9be..872eb50c88 100644 --- a/crates/ruff/src/rules/ruff/rules/unreachable.rs +++ b/crates/ruff/src/rules/ruff/rules/unreachable.rs @@ -484,17 +484,44 @@ impl<'stmt> BasicBlocksBuilder<'stmt> { self.unconditional_next_block(after) } // Statements that (can) divert the control flow. - Stmt::If(stmt) => { - let next_after_block = - self.maybe_next_block_index(after, || needs_next_block(&stmt.body)); - let orelse_after_block = - self.maybe_next_block_index(after, || needs_next_block(&stmt.orelse)); - let next = self.append_blocks_if_not_empty(&stmt.body, next_after_block); - let orelse = self.append_blocks_if_not_empty(&stmt.orelse, orelse_after_block); + Stmt::If(stmt_if) => { + let after_consequent_block = + self.maybe_next_block_index(after, || needs_next_block(&stmt_if.body)); + let after_alternate_block = self.maybe_next_block_index(after, || { + stmt_if + .elif_else_clauses + .last() + .map_or(true, |clause| needs_next_block(&clause.body)) + }); + + let consequent = + self.append_blocks_if_not_empty(&stmt_if.body, after_consequent_block); + + // Block ID of the next elif or else clause. + let mut next_branch = after_alternate_block; + + for clause in stmt_if.elif_else_clauses.iter().rev() { + let consequent = + self.append_blocks_if_not_empty(&clause.body, after_consequent_block); + + next_branch = if let Some(test) = &clause.test { + let next = NextBlock::If { + condition: Condition::Test(test), + next: consequent, + orelse: next_branch, + }; + let stmts = std::slice::from_ref(stmt); + let block = BasicBlock { stmts, next }; + self.blocks.push(block) + } else { + consequent + }; + } + NextBlock::If { - condition: Condition::Test(&stmt.test), - next, - orelse, + condition: Condition::Test(&stmt_if.test), + next: consequent, + orelse: next_branch, } } Stmt::While(StmtWhile { @@ -648,6 +675,7 @@ impl<'stmt> BasicBlocksBuilder<'stmt> { } // The tough branches are done, here is an easy one. Stmt::Return(_) => NextBlock::Terminate, + Stmt::TypeAlias(_) => todo!(), }; // Include any statements in the block that don't divert the control flow. @@ -867,7 +895,7 @@ fn needs_next_block(stmts: &[Stmt]) -> bool { match last { Stmt::Return(_) | Stmt::Raise(_) => false, - Stmt::If(stmt) => needs_next_block(&stmt.body) || needs_next_block(&stmt.orelse), + Stmt::If(stmt) => needs_next_block(&stmt.body) || stmt.elif_else_clauses.last().map_or(true, |clause| needs_next_block(&clause.body)), Stmt::FunctionDef(_) | Stmt::AsyncFunctionDef(_) | Stmt::Import(_) @@ -893,6 +921,7 @@ fn needs_next_block(stmts: &[Stmt]) -> bool { | Stmt::Try(_) | Stmt::TryStar(_) | Stmt::Assert(_) => true, + Stmt::TypeAlias(_) => todo!(), } } @@ -927,6 +956,7 @@ fn is_control_flow_stmt(stmt: &Stmt) -> bool { | Stmt::Assert(_) | Stmt::Break(_) | Stmt::Continue(_) => true, + Stmt::TypeAlias(_) => todo!(), } } @@ -1063,6 +1093,11 @@ mod tests { "first block should always terminate" ); + let got_mermaid = MermaidGraph { + graph: &got, + source: &source, + }; + // All block index should be valid. let valid = BlockIndex::from_usize(got.blocks.len()); for block in &got.blocks { @@ -1076,11 +1111,6 @@ mod tests { } } - let got_mermaid = MermaidGraph { - graph: &got, - source: &source, - }; - writeln!( output, "## Function {i}\n### Source\n```python\n{}\n```\n\n### Control Flow Graph\n```mermaid\n{}```\n", diff --git a/crates/ruff/src/rules/tryceratops/rules/type_check_without_type_error.rs b/crates/ruff/src/rules/tryceratops/rules/type_check_without_type_error.rs index 193da3a5c9..a38078120d 100644 --- a/crates/ruff/src/rules/tryceratops/rules/type_check_without_type_error.rs +++ b/crates/ruff/src/rules/tryceratops/rules/type_check_without_type_error.rs @@ -1,8 +1,7 @@ -use rustpython_parser::ast::{self, Expr, Ranged, Stmt}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor}; +use rustpython_parser::ast::{self, Expr, Ranged, Stmt, StmtIf}; use crate::checkers::ast::Checker; @@ -164,34 +163,18 @@ fn check_body(checker: &mut Checker, body: &[Stmt]) { } } -/// Search the orelse of an if-condition for raises. -fn check_orelse(checker: &mut Checker, body: &[Stmt]) { - for item in body { - if has_control_flow(item) { - return; - } - match item { - Stmt::If(ast::StmtIf { test, .. }) => { - if !check_type_check_test(checker, test) { - return; - } - } - Stmt::Raise(ast::StmtRaise { exc: Some(exc), .. }) => { - check_raise(checker, exc, item); - } - _ => {} - } - } -} - /// TRY004 pub(crate) fn type_check_without_type_error( checker: &mut Checker, - body: &[Stmt], - test: &Expr, - orelse: &[Stmt], + stmt_if: &StmtIf, parent: Option<&Stmt>, ) { + let StmtIf { + body, + test, + elif_else_clauses, + .. + } = stmt_if; if let Some(Stmt::If(ast::StmtIf { test, .. })) = parent { if !check_type_check_test(checker, test) { return; @@ -199,8 +182,20 @@ pub(crate) fn type_check_without_type_error( } // Only consider the body when the `if` condition is all type-related - if check_type_check_test(checker, test) { - check_body(checker, body); - check_orelse(checker, orelse); + if !check_type_check_test(checker, test) { + return; + } + check_body(checker, body); + + for clause in elif_else_clauses { + if let Some(test) = &clause.test { + // If there are any `elif`, they must all also be type-related + if !check_type_check_test(checker, test) { + return; + } + } + + // The `elif` or `else` body raises the wrong exception + check_body(checker, &clause.body); } } diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__type-check-without-type-error_TRY004.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__type-check-without-type-error_TRY004.py.snap index 0d2f2202ef..be351bfdc4 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__type-check-without-type-error_TRY004.py.snap +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__type-check-without-type-error_TRY004.py.snap @@ -252,32 +252,48 @@ TRY004.py:230:9: TRY004 Prefer `TypeError` exception for invalid type | ^^^^^^^^^^^^^^^^^^^^^^ TRY004 | -TRY004.py:267:9: TRY004 Prefer `TypeError` exception for invalid type +TRY004.py:239:9: TRY004 Prefer `TypeError` exception for invalid type | -265 | def check_body(some_args): -266 | if isinstance(some_args, int): -267 | raise ValueError("...") # should be typeerror +237 | pass +238 | else: +239 | raise Exception("...") # should be typeerror + | ^^^^^^^^^^^^^^^^^^^^^^ TRY004 + | + +TRY004.py:276:9: TRY004 Prefer `TypeError` exception for invalid type + | +274 | def check_body(some_args): +275 | if isinstance(some_args, int): +276 | raise ValueError("...") # should be typeerror | ^^^^^^^^^^^^^^^^^^^^^^^ TRY004 | -TRY004.py:277:9: TRY004 Prefer `TypeError` exception for invalid type +TRY004.py:286:9: TRY004 Prefer `TypeError` exception for invalid type | -275 | def multiple_elifs(some_args): -276 | if not isinstance(some_args, int): -277 | raise ValueError("...") # should be typerror +284 | def multiple_elifs(some_args): +285 | if not isinstance(some_args, int): +286 | raise ValueError("...") # should be typerror | ^^^^^^^^^^^^^^^^^^^^^^^ TRY004 -278 | elif some_args < 3: -279 | raise ValueError("...") # this is ok +287 | elif some_args < 3: +288 | raise ValueError("...") # this is ok | -TRY004.py:288:9: TRY004 Prefer `TypeError` exception for invalid type +TRY004.py:297:9: TRY004 Prefer `TypeError` exception for invalid type | -286 | def multiple_ifs(some_args): -287 | if not isinstance(some_args, int): -288 | raise ValueError("...") # should be typerror +295 | def multiple_ifs(some_args): +296 | if not isinstance(some_args, int): +297 | raise ValueError("...") # should be typerror | ^^^^^^^^^^^^^^^^^^^^^^^ TRY004 -289 | else: -290 | if some_args < 3: +298 | else: +299 | if some_args < 3: + | + +TRY004.py:316:9: TRY004 Prefer `TypeError` exception for invalid type + | +314 | return "CronExpression" +315 | else: +316 | raise Exception(f"Unknown object type: {obj.__class__.__name__}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY004 | diff --git a/crates/ruff_python_ast/src/comparable.rs b/crates/ruff_python_ast/src/comparable.rs index cfda94d4b5..5ebc6468ba 100644 --- a/crates/ruff_python_ast/src/comparable.rs +++ b/crates/ruff_python_ast/src/comparable.rs @@ -466,6 +466,26 @@ impl<'a> From<&'a ast::ExceptHandler> for ComparableExceptHandler<'a> { } } +#[derive(Debug, PartialEq, Eq, Hash)] +pub struct ComparableElifElseClause<'a> { + test: Option>, + body: Vec>, +} + +impl<'a> From<&'a ast::ElifElseClause> for ComparableElifElseClause<'a> { + fn from(elif_else_clause: &'a ast::ElifElseClause) -> Self { + let ast::ElifElseClause { + range: _, + test, + body, + } = elif_else_clause; + Self { + test: test.as_ref().map(Into::into), + body: body.iter().map(Into::into).collect(), + } + } +} + #[derive(Debug, PartialEq, Eq, Hash)] pub struct ExprBoolOp<'a> { op: ComparableBoolOp, @@ -999,7 +1019,7 @@ pub struct StmtWhile<'a> { pub struct StmtIf<'a> { test: ComparableExpr<'a>, body: Vec>, - orelse: Vec>, + elif_else_clauses: Vec>, } #[derive(Debug, PartialEq, Eq, Hash)] @@ -1118,7 +1138,8 @@ impl<'a> From<&'a ast::Stmt> for ComparableStmt<'a> { decorator_list, returns, type_comment, - range: _range, + range: _, + type_params: _, }) => Self::FunctionDef(StmtFunctionDef { name: name.as_str(), args: args.into(), @@ -1134,7 +1155,8 @@ impl<'a> From<&'a ast::Stmt> for ComparableStmt<'a> { decorator_list, returns, type_comment, - range: _range, + range: _, + type_params: _, }) => Self::AsyncFunctionDef(StmtAsyncFunctionDef { name: name.as_str(), args: args.into(), @@ -1149,7 +1171,8 @@ impl<'a> From<&'a ast::Stmt> for ComparableStmt<'a> { keywords, body, decorator_list, - range: _range, + range: _, + type_params: _, }) => Self::ClassDef(StmtClassDef { name: name.as_str(), bases: bases.iter().map(Into::into).collect(), @@ -1242,12 +1265,12 @@ impl<'a> From<&'a ast::Stmt> for ComparableStmt<'a> { ast::Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _range, }) => Self::If(StmtIf { test: test.into(), body: body.iter().map(Into::into).collect(), - orelse: orelse.iter().map(Into::into).collect(), + elif_else_clauses: elif_else_clauses.iter().map(Into::into).collect(), }), ast::Stmt::With(ast::StmtWith { items, @@ -1354,6 +1377,7 @@ impl<'a> From<&'a ast::Stmt> for ComparableStmt<'a> { ast::Stmt::Pass(_) => Self::Pass, ast::Stmt::Break(_) => Self::Break, ast::Stmt::Continue(_) => Self::Continue, + ast::Stmt::TypeAlias(_) => todo!(), } } } diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index b946a17e5c..0dfad2d123 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -437,9 +437,19 @@ where Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _range, - }) => any_over_expr(test, func) || any_over_body(body, func) || any_over_body(orelse, func), + }) => { + any_over_expr(test, func) + || any_over_body(body, func) + || elif_else_clauses.iter().any(|clause| { + clause + .test + .as_ref() + .map_or(false, |test| any_over_expr(test, func)) + || any_over_body(&clause.body, func) + }) + } Stmt::With(ast::StmtWith { items, body, .. }) | Stmt::AsyncWith(ast::StmtAsyncWith { items, body, .. }) => { items.iter().any(|with_item| { @@ -529,6 +539,7 @@ where range: _range, }) => any_over_expr(value, func), Stmt::Pass(_) | Stmt::Break(_) | Stmt::Continue(_) => false, + Stmt::TypeAlias(_) => todo!(), } } @@ -944,9 +955,15 @@ where | Stmt::AsyncFunctionDef(_) | Stmt::Try(_) | Stmt::TryStar(_) => {} - Stmt::If(ast::StmtIf { body, orelse, .. }) => { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { walk_body(self, body); - walk_body(self, orelse); + for clause in elif_else_clauses { + self.visit_elif_else_clause(clause); + } } Stmt::While(ast::StmtWhile { body, .. }) | Stmt::With(ast::StmtWith { body, .. }) @@ -1063,25 +1080,6 @@ pub fn first_colon_range(range: TextRange, locator: &Locator) -> Option Option { - let ast::StmtIf { body, orelse, .. } = stmt; - - let start = body.last().expect("Expected body to be non-empty").end(); - - let end = match &orelse[..] { - [Stmt::If(ast::StmtIf { test, .. })] => test.start(), - [stmt, ..] => stmt.start(), - _ => return None, - }; - - let contents = &locator.contents()[TextRange::new(start, end)]; - lexer::lex_starts_at(contents, Mode::Module, start) - .flatten() - .find(|(kind, _)| matches!(kind, Tok::Elif | Tok::Else)) - .map(|(_, range)| range) -} - /// Given an offset at the end of a line (including newlines), return the offset of the /// continuation at the end of that line. fn find_continuation(offset: TextSize, locator: &Locator, indexer: &Indexer) -> Option { @@ -1568,13 +1566,13 @@ mod tests { use anyhow::Result; use ruff_text_size::{TextLen, TextRange, TextSize}; - use rustpython_ast::{CmpOp, Expr, Ranged, Stmt}; + use rustpython_ast::{CmpOp, Expr, Ranged}; use rustpython_parser::ast::Suite; use rustpython_parser::Parse; use crate::helpers::{ - elif_else_range, first_colon_range, has_trailing_content, locate_cmp_ops, - resolve_imported_module_path, LocatedCmpOp, + first_colon_range, has_trailing_content, locate_cmp_ops, resolve_imported_module_path, + LocatedCmpOp, }; use crate::source_code::Locator; @@ -1667,35 +1665,6 @@ y = 2 assert_eq!(range, TextRange::new(TextSize::from(6), TextSize::from(7))); } - #[test] - fn extract_elif_else_range() -> Result<()> { - let contents = "if a: - ... -elif b: - ... -"; - let stmt = Stmt::parse(contents, "")?; - let stmt = Stmt::as_if_stmt(&stmt).unwrap(); - let locator = Locator::new(contents); - let range = elif_else_range(stmt, &locator).unwrap(); - assert_eq!(range.start(), TextSize::from(14)); - assert_eq!(range.end(), TextSize::from(18)); - - let contents = "if a: - ... -else: - ... -"; - let stmt = Stmt::parse(contents, "")?; - let stmt = Stmt::as_if_stmt(&stmt).unwrap(); - let locator = Locator::new(contents); - let range = elif_else_range(stmt, &locator).unwrap(); - assert_eq!(range.start(), TextSize::from(14)); - assert_eq!(range.end(), TextSize::from(18)); - - Ok(()) - } - #[test] fn extract_cmp_op_location() -> Result<()> { let contents = "x == 1"; diff --git a/crates/ruff_python_ast/src/lib.rs b/crates/ruff_python_ast/src/lib.rs index 72928ec9e9..424e1be6ff 100644 --- a/crates/ruff_python_ast/src/lib.rs +++ b/crates/ruff_python_ast/src/lib.rs @@ -12,6 +12,7 @@ pub mod node; pub mod relocate; pub mod source_code; pub mod statement_visitor; +pub mod stmt_if; pub mod str; pub mod token_kind; pub mod types; diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index ccbc44c723..ef3e6b4309 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -98,6 +98,7 @@ pub enum AnyNode { WithItem(WithItem), MatchCase(MatchCase), Decorator(Decorator), + ElifElseClause(ast::ElifElseClause), } impl AnyNode { @@ -180,7 +181,8 @@ impl AnyNode { | AnyNode::Alias(_) | AnyNode::WithItem(_) | AnyNode::MatchCase(_) - | AnyNode::Decorator(_) => None, + | AnyNode::Decorator(_) + | AnyNode::ElifElseClause(_) => None, } } @@ -263,7 +265,8 @@ impl AnyNode { | AnyNode::Alias(_) | AnyNode::WithItem(_) | AnyNode::MatchCase(_) - | AnyNode::Decorator(_) => None, + | AnyNode::Decorator(_) + | AnyNode::ElifElseClause(_) => None, } } @@ -346,7 +349,8 @@ impl AnyNode { | AnyNode::Alias(_) | AnyNode::WithItem(_) | AnyNode::MatchCase(_) - | AnyNode::Decorator(_) => None, + | AnyNode::Decorator(_) + | AnyNode::ElifElseClause(_) => None, } } @@ -429,7 +433,8 @@ impl AnyNode { | AnyNode::Alias(_) | AnyNode::WithItem(_) | AnyNode::MatchCase(_) - | AnyNode::Decorator(_) => None, + | AnyNode::Decorator(_) + | AnyNode::ElifElseClause(_) => None, } } @@ -512,7 +517,8 @@ impl AnyNode { | AnyNode::Alias(_) | AnyNode::WithItem(_) | AnyNode::MatchCase(_) - | AnyNode::Decorator(_) => None, + | AnyNode::Decorator(_) + | AnyNode::ElifElseClause(_) => None, } } @@ -595,7 +601,8 @@ impl AnyNode { | AnyNode::Alias(_) | AnyNode::WithItem(_) | AnyNode::MatchCase(_) - | AnyNode::Decorator(_) => None, + | AnyNode::Decorator(_) + | AnyNode::ElifElseClause(_) => None, } } @@ -702,6 +709,7 @@ impl AnyNode { Self::WithItem(node) => AnyNodeRef::WithItem(node), Self::MatchCase(node) => AnyNodeRef::MatchCase(node), Self::Decorator(node) => AnyNodeRef::Decorator(node), + Self::ElifElseClause(node) => AnyNodeRef::ElifElseClause(node), } } @@ -1159,6 +1167,34 @@ impl AstNode for ast::StmtIf { AnyNode::from(self) } } +impl AstNode for ast::ElifElseClause { + fn cast(kind: AnyNode) -> Option + where + Self: Sized, + { + if let AnyNode::ElifElseClause(node) = kind { + Some(node) + } else { + None + } + } + + fn cast_ref(kind: AnyNodeRef) -> Option<&Self> { + if let AnyNodeRef::ElifElseClause(node) = kind { + Some(node) + } else { + None + } + } + + fn as_any_node_ref(&self) -> AnyNodeRef { + AnyNodeRef::from(self) + } + + fn into_any_node(self) -> AnyNode { + AnyNode::from(self) + } +} impl AstNode for ast::StmtWith { fn cast(kind: AnyNode) -> Option where @@ -2900,6 +2936,7 @@ impl From for AnyNode { Stmt::Pass(node) => AnyNode::StmtPass(node), Stmt::Break(node) => AnyNode::StmtBreak(node), Stmt::Continue(node) => AnyNode::StmtContinue(node), + Stmt::TypeAlias(_) => todo!(), } } } @@ -3076,6 +3113,12 @@ impl From for AnyNode { } } +impl From for AnyNode { + fn from(node: ast::ElifElseClause) -> Self { + AnyNode::ElifElseClause(node) + } +} + impl From for AnyNode { fn from(node: ast::StmtWith) -> Self { AnyNode::StmtWith(node) @@ -3514,6 +3557,7 @@ impl Ranged for AnyNode { AnyNode::WithItem(node) => node.range(), AnyNode::MatchCase(node) => node.range(), AnyNode::Decorator(node) => node.range(), + AnyNode::ElifElseClause(node) => node.range(), } } } @@ -3597,6 +3641,7 @@ pub enum AnyNodeRef<'a> { WithItem(&'a WithItem), MatchCase(&'a MatchCase), Decorator(&'a Decorator), + ElifElseClause(&'a ast::ElifElseClause), } impl AnyNodeRef<'_> { @@ -3679,6 +3724,7 @@ impl AnyNodeRef<'_> { AnyNodeRef::WithItem(node) => NonNull::from(*node).cast(), AnyNodeRef::MatchCase(node) => NonNull::from(*node).cast(), AnyNodeRef::Decorator(node) => NonNull::from(*node).cast(), + AnyNodeRef::ElifElseClause(node) => NonNull::from(*node).cast(), } } @@ -3767,6 +3813,7 @@ impl AnyNodeRef<'_> { AnyNodeRef::WithItem(_) => NodeKind::WithItem, AnyNodeRef::MatchCase(_) => NodeKind::MatchCase, AnyNodeRef::Decorator(_) => NodeKind::Decorator, + AnyNodeRef::ElifElseClause(_) => NodeKind::ElifElseClause, } } @@ -3849,7 +3896,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::Alias(_) | AnyNodeRef::WithItem(_) | AnyNodeRef::MatchCase(_) - | AnyNodeRef::Decorator(_) => false, + | AnyNodeRef::Decorator(_) + | AnyNodeRef::ElifElseClause(_) => false, } } @@ -3932,7 +3980,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::Alias(_) | AnyNodeRef::WithItem(_) | AnyNodeRef::MatchCase(_) - | AnyNodeRef::Decorator(_) => false, + | AnyNodeRef::Decorator(_) + | AnyNodeRef::ElifElseClause(_) => false, } } @@ -4015,7 +4064,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::Alias(_) | AnyNodeRef::WithItem(_) | AnyNodeRef::MatchCase(_) - | AnyNodeRef::Decorator(_) => false, + | AnyNodeRef::Decorator(_) + | AnyNodeRef::ElifElseClause(_) => false, } } @@ -4098,7 +4148,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::Alias(_) | AnyNodeRef::WithItem(_) | AnyNodeRef::MatchCase(_) - | AnyNodeRef::Decorator(_) => false, + | AnyNodeRef::Decorator(_) + | AnyNodeRef::ElifElseClause(_) => false, } } @@ -4181,7 +4232,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::Alias(_) | AnyNodeRef::WithItem(_) | AnyNodeRef::MatchCase(_) - | AnyNodeRef::Decorator(_) => false, + | AnyNodeRef::Decorator(_) + | AnyNodeRef::ElifElseClause(_) => false, } } @@ -4264,7 +4316,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::Alias(_) | AnyNodeRef::WithItem(_) | AnyNodeRef::MatchCase(_) - | AnyNodeRef::Decorator(_) => false, + | AnyNodeRef::Decorator(_) + | AnyNodeRef::ElifElseClause(_) => false, } } @@ -4383,6 +4436,12 @@ impl<'a> From<&'a ast::StmtIf> for AnyNodeRef<'a> { } } +impl<'a> From<&'a ast::ElifElseClause> for AnyNodeRef<'a> { + fn from(node: &'a ast::ElifElseClause) -> Self { + AnyNodeRef::ElifElseClause(node) + } +} + impl<'a> From<&'a ast::StmtWith> for AnyNodeRef<'a> { fn from(node: &'a ast::StmtWith) -> Self { AnyNodeRef::StmtWith(node) @@ -4731,6 +4790,7 @@ impl<'a> From<&'a Stmt> for AnyNodeRef<'a> { Stmt::Pass(node) => AnyNodeRef::StmtPass(node), Stmt::Break(node) => AnyNodeRef::StmtBreak(node), Stmt::Continue(node) => AnyNodeRef::StmtContinue(node), + Stmt::TypeAlias(_) => todo!(), } } } @@ -4934,6 +4994,7 @@ impl Ranged for AnyNodeRef<'_> { AnyNodeRef::WithItem(node) => node.range(), AnyNodeRef::MatchCase(node) => node.range(), AnyNodeRef::Decorator(node) => node.range(), + AnyNodeRef::ElifElseClause(node) => node.range(), } } } @@ -5017,4 +5078,5 @@ pub enum NodeKind { WithItem, MatchCase, Decorator, + ElifElseClause, } diff --git a/crates/ruff_python_ast/src/source_code/comment_ranges.rs b/crates/ruff_python_ast/src/source_code/comment_ranges.rs index 977bc83b0d..1c296666eb 100644 --- a/crates/ruff_python_ast/src/source_code/comment_ranges.rs +++ b/crates/ruff_python_ast/src/source_code/comment_ranges.rs @@ -1,3 +1,4 @@ +use itertools::Itertools; use std::fmt::{Debug, Formatter}; use std::ops::Deref; @@ -25,6 +26,21 @@ impl CommentRanges { }) .is_ok() } + + /// Returns the comments who are within the range + pub fn comments_in_range(&self, range: TextRange) -> &[TextRange] { + let start = self + .raw + .partition_point(|comment| comment.start() < range.start()); + // We expect there are few comments, so switching to find should be faster + match self.raw[start..] + .iter() + .find_position(|comment| comment.end() > range.end()) + { + Some((in_range, _element)) => &self.raw[start..start + in_range], + None => &self.raw[start..], + } + } } impl Deref for CommentRanges { diff --git a/crates/ruff_python_ast/src/source_code/generator.rs b/crates/ruff_python_ast/src/source_code/generator.rs index 5c34362bee..412b897eed 100644 --- a/crates/ruff_python_ast/src/source_code/generator.rs +++ b/crates/ruff_python_ast/src/source_code/generator.rs @@ -271,7 +271,8 @@ impl<'a> Generator<'a> { keywords, body, decorator_list, - range: _range, + range: _, + type_params: _, }) => { self.newlines(if self.indent_depth == 0 { 2 } else { 1 }); for decorator in decorator_list { @@ -457,7 +458,7 @@ impl<'a> Generator<'a> { Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _range, }) => { statement!({ @@ -467,33 +468,19 @@ impl<'a> Generator<'a> { }); self.body(body); - let mut orelse_: &[Stmt] = orelse; - loop { - if orelse_.len() == 1 && matches!(orelse_[0], Stmt::If(_)) { - if let Stmt::If(ast::StmtIf { - body, - test, - orelse, - range: _range, - }) = &orelse_[0] - { - statement!({ - self.p("elif "); - self.unparse_expr(test, precedence::IF); - self.p(":"); - }); - self.body(body); - orelse_ = orelse; - } + for clause in elif_else_clauses { + if let Some(test) = &clause.test { + statement!({ + self.p("elif "); + self.unparse_expr(test, precedence::IF); + self.p(":"); + }); } else { - if !orelse_.is_empty() { - statement!({ - self.p("else:"); - }); - self.body(orelse_); - } - break; + statement!({ + self.p("else:"); + }); } + self.body(&clause.body); } } Stmt::With(ast::StmtWith { items, body, .. }) => { @@ -715,6 +702,7 @@ impl<'a> Generator<'a> { self.p("continue"); }); } + Stmt::TypeAlias(_) => todo!(), } } diff --git a/crates/ruff_python_ast/src/source_code/indexer.rs b/crates/ruff_python_ast/src/source_code/indexer.rs index c70e8378fc..2a60e64c86 100644 --- a/crates/ruff_python_ast/src/source_code/indexer.rs +++ b/crates/ruff_python_ast/src/source_code/indexer.rs @@ -97,6 +97,18 @@ impl Indexer { &self.comment_ranges } + /// Returns the comments in the given range as source code slices + pub fn comments_in_range<'a>( + &'a self, + range: TextRange, + locator: &'a Locator, + ) -> impl Iterator { + self.comment_ranges + .comments_in_range(range) + .iter() + .map(move |comment_range| locator.slice(*comment_range)) + } + /// Returns the line start positions of continuations (backslash). pub fn continuation_line_starts(&self) -> &[TextSize] { &self.continuation_lines diff --git a/crates/ruff_python_ast/src/statement_visitor.rs b/crates/ruff_python_ast/src/statement_visitor.rs index df35b6bb2e..bda9fa8f64 100644 --- a/crates/ruff_python_ast/src/statement_visitor.rs +++ b/crates/ruff_python_ast/src/statement_visitor.rs @@ -1,5 +1,6 @@ //! Specialized AST visitor trait and walk functions that only visit statements. +use rustpython_ast::ElifElseClause; use rustpython_parser::ast::{self, ExceptHandler, MatchCase, Stmt}; /// A trait for AST visitors that only need to visit statements. @@ -13,6 +14,9 @@ pub trait StatementVisitor<'a> { fn visit_except_handler(&mut self, except_handler: &'a ExceptHandler) { walk_except_handler(self, except_handler); } + fn visit_elif_else_clause(&mut self, elif_else_clause: &'a ElifElseClause) { + walk_elif_else_clause(self, elif_else_clause); + } fn visit_match_case(&mut self, match_case: &'a MatchCase) { walk_match_case(self, match_case); } @@ -47,9 +51,15 @@ pub fn walk_stmt<'a, V: StatementVisitor<'a> + ?Sized>(visitor: &mut V, stmt: &' visitor.visit_body(body); visitor.visit_body(orelse); } - Stmt::If(ast::StmtIf { body, orelse, .. }) => { + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => { visitor.visit_body(body); - visitor.visit_body(orelse); + for clause in elif_else_clauses { + visitor.visit_elif_else_clause(clause); + } } Stmt::With(ast::StmtWith { body, .. }) => { visitor.visit_body(body); @@ -105,6 +115,13 @@ pub fn walk_except_handler<'a, V: StatementVisitor<'a> + ?Sized>( } } +pub fn walk_elif_else_clause<'a, V: StatementVisitor<'a> + ?Sized>( + visitor: &mut V, + elif_else_clause: &'a ElifElseClause, +) { + visitor.visit_body(&elif_else_clause.body); +} + pub fn walk_match_case<'a, V: StatementVisitor<'a> + ?Sized>( visitor: &mut V, match_case: &'a MatchCase, diff --git a/crates/ruff_python_ast/src/stmt_if.rs b/crates/ruff_python_ast/src/stmt_if.rs new file mode 100644 index 0000000000..1ad617e1d8 --- /dev/null +++ b/crates/ruff_python_ast/src/stmt_if.rs @@ -0,0 +1,87 @@ +use crate::source_code::Locator; +use ruff_text_size::TextRange; +use rustpython_ast::{ElifElseClause, Expr, Ranged, Stmt, StmtIf}; +use rustpython_parser::{lexer, Mode, Tok}; +use std::iter; + +/// Return the `Range` of the first `Elif` or `Else` token in an `If` statement. +pub fn elif_else_range(clause: &ElifElseClause, locator: &Locator) -> Option { + let contents = &locator.contents()[clause.range]; + let token = lexer::lex_starts_at(contents, Mode::Module, clause.range.start()) + .flatten() + .next()?; + if matches!(token.0, Tok::Elif | Tok::Else) { + Some(token.1) + } else { + None + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum BranchKind { + If, + Elif, +} + +pub struct IfElifBranch<'a> { + pub kind: BranchKind, + pub test: &'a Expr, + pub body: &'a [Stmt], + pub range: TextRange, +} + +pub fn if_elif_branches(stmt_if: &StmtIf) -> impl Iterator { + iter::once(IfElifBranch { + kind: BranchKind::If, + test: stmt_if.test.as_ref(), + body: stmt_if.body.as_slice(), + range: TextRange::new(stmt_if.start(), stmt_if.body.last().unwrap().end()), + }) + .chain(stmt_if.elif_else_clauses.iter().filter_map(|clause| { + Some(IfElifBranch { + kind: BranchKind::Elif, + test: clause.test.as_ref()?, + body: clause.body.as_slice(), + range: clause.range, + }) + })) +} + +#[cfg(test)] +mod test { + use crate::source_code::Locator; + use crate::stmt_if::elif_else_range; + use anyhow::Result; + use ruff_text_size::TextSize; + use rustpython_ast::Stmt; + use rustpython_parser::Parse; + + #[test] + fn extract_elif_else_range() -> Result<()> { + let contents = "if a: + ... +elif b: + ... +"; + let stmt = Stmt::parse(contents, "")?; + let stmt = Stmt::as_if_stmt(&stmt).unwrap(); + let locator = Locator::new(contents); + let range = elif_else_range(&stmt.elif_else_clauses[0], &locator).unwrap(); + assert_eq!(range.start(), TextSize::from(14)); + assert_eq!(range.end(), TextSize::from(18)); + + let contents = "if a: + ... +else: + ... +"; + let stmt = Stmt::parse(contents, "")?; + let stmt = Stmt::as_if_stmt(&stmt).unwrap(); + let locator = Locator::new(contents); + let range = elif_else_range(&stmt.elif_else_clauses[0], &locator).unwrap(); + assert_eq!(range.start(), TextSize::from(14)); + assert_eq!(range.end(), TextSize::from(18)); + + Ok(()) + } +} diff --git a/crates/ruff_python_ast/src/token_kind.rs b/crates/ruff_python_ast/src/token_kind.rs index 7b460eba20..89b88c7fcc 100644 --- a/crates/ruff_python_ast/src/token_kind.rs +++ b/crates/ruff_python_ast/src/token_kind.rs @@ -431,6 +431,8 @@ impl TokenKind { Tok::StartModule => TokenKind::StartModule, Tok::StartInteractive => TokenKind::StartInteractive, Tok::StartExpression => TokenKind::StartExpression, + Tok::MagicCommand { .. } => todo!(), + Tok::Type => todo!(), } } } diff --git a/crates/ruff_python_ast/src/visitor.rs b/crates/ruff_python_ast/src/visitor.rs index 9d90036cf8..bf2f87ac92 100644 --- a/crates/ruff_python_ast/src/visitor.rs +++ b/crates/ruff_python_ast/src/visitor.rs @@ -2,6 +2,7 @@ pub mod preorder; +use rustpython_ast::ElifElseClause; use rustpython_parser::ast::{ self, Alias, Arg, Arguments, BoolOp, CmpOp, Comprehension, Decorator, ExceptHandler, Expr, ExprContext, Keyword, MatchCase, Operator, Pattern, Stmt, UnaryOp, WithItem, @@ -75,6 +76,9 @@ pub trait Visitor<'a> { fn visit_body(&mut self, body: &'a [Stmt]) { walk_body(self, body); } + fn visit_elif_else_clause(&mut self, elif_else_clause: &'a ElifElseClause) { + walk_elif_else_clause(self, elif_else_clause); + } } pub fn walk_body<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, body: &'a [Stmt]) { @@ -83,6 +87,16 @@ pub fn walk_body<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, body: &'a [Stmt]) } } +pub fn walk_elif_else_clause<'a, V: Visitor<'a> + ?Sized>( + visitor: &mut V, + elif_else_clause: &'a ElifElseClause, +) { + if let Some(test) = &elif_else_clause.test { + visitor.visit_expr(test); + } + visitor.visit_body(&elif_else_clause.body); +} + pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { match stmt { Stmt::FunctionDef(ast::StmtFunctionDef { @@ -216,12 +230,17 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _range, }) => { visitor.visit_expr(test); visitor.visit_body(body); - visitor.visit_body(orelse); + for clause in elif_else_clauses { + if let Some(test) = &clause.test { + visitor.visit_expr(test); + } + walk_elif_else_clause(visitor, clause); + } } Stmt::With(ast::StmtWith { items, body, .. }) => { for with_item in items { @@ -315,6 +334,7 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { range: _range, }) => visitor.visit_expr(value), Stmt::Pass(_) | Stmt::Break(_) | Stmt::Continue(_) => {} + Stmt::TypeAlias(_) => todo!(), } } diff --git a/crates/ruff_python_ast/src/visitor/preorder.rs b/crates/ruff_python_ast/src/visitor/preorder.rs index 5e83d1e782..ae9209d21f 100644 --- a/crates/ruff_python_ast/src/visitor/preorder.rs +++ b/crates/ruff_python_ast/src/visitor/preorder.rs @@ -1,4 +1,4 @@ -use rustpython_ast::{ArgWithDefault, Mod, TypeIgnore}; +use rustpython_ast::{ArgWithDefault, ElifElseClause, Mod, TypeIgnore}; use rustpython_parser::ast::{ self, Alias, Arg, Arguments, BoolOp, CmpOp, Comprehension, Constant, Decorator, ExceptHandler, Expr, Keyword, MatchCase, Operator, Pattern, Stmt, UnaryOp, WithItem, @@ -95,6 +95,10 @@ pub trait PreorderVisitor<'a> { fn visit_type_ignore(&mut self, type_ignore: &'a TypeIgnore) { walk_type_ignore(self, type_ignore); } + + fn visit_elif_else_clause(&mut self, elif_else_clause: &'a ElifElseClause) { + walk_elif_else_clause(self, elif_else_clause); + } } pub fn walk_module<'a, V>(visitor: &mut V, module: &'a Mod) @@ -286,12 +290,14 @@ where Stmt::If(ast::StmtIf { test, body, - orelse, + elif_else_clauses, range: _range, }) => { visitor.visit_expr(test); visitor.visit_body(body); - visitor.visit_body(orelse); + for clause in elif_else_clauses { + visitor.visit_elif_else_clause(clause); + } } Stmt::With(ast::StmtWith { @@ -394,6 +400,7 @@ where | Stmt::Continue(_) | Stmt::Global(_) | Stmt::Nonlocal(_) => {} + Stmt::TypeAlias(_) => todo!(), } } @@ -703,6 +710,16 @@ where } } +pub fn walk_elif_else_clause<'a, V>(visitor: &mut V, elif_else_clause: &'a ElifElseClause) +where + V: PreorderVisitor<'a> + ?Sized, +{ + if let Some(test) = &elif_else_clause.test { + visitor.visit_expr(test); + } + visitor.visit_body(&elif_else_clause.body); +} + pub fn walk_except_handler<'a, V>(visitor: &mut V, except_handler: &'a ExceptHandler) where V: PreorderVisitor<'a> + ?Sized, diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py index e737ebbab1..e332e221df 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py @@ -1,16 +1,20 @@ -if x == y: # trailing if condition - pass # trailing `pass` comment - # Root `if` trailing comment +# 1 leading if comment +if x == y: # 2 trailing if condition + # 3 leading pass + pass # 4 end-of-line trailing `pass` comment + # 5 Root `if` trailing comment -# Leading elif comment -elif x < y: # trailing elif condition - pass - # `elif` trailing comment +# 6 Leading elif comment +elif x < y: # 7 trailing elif condition + # 8 leading pass + pass # 9 end-of-line trailing `pass` comment + # 10 `elif` trailing comment -# Leading else comment -else: # trailing else condition - pass - # `else` trailing comment +# 11 Leading else comment +else: # 12 trailing else condition + # 13 leading pass + pass # 14 end-of-line trailing `pass` comment + # 15 `else` trailing comment if x == y: @@ -71,3 +75,14 @@ else: # Comment if False: pass pass + + +# Regression test for `last_child_in_body` special casing of `StmtIf` +# https://github.com/python/cpython/blob/aecf6aca515a203a823a87c711f15cbb82097c8b/Lib/test/test_pty.py#L260-L275 +def f(): + if True: + pass + else: + pass + + # comment diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 6effa9e3f5..e4d7ec9d82 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -292,12 +292,26 @@ fn handle_in_between_bodies_own_line_comment<'a>( // if x == y: // pass // # I'm a leading comment of the `elif` statement. - // elif: + // elif True: // print("nooop") // ``` - if following.is_stmt_if() || following.is_except_handler() { - // The `elif` or except handlers have their own body to which we can attach the leading comment + if following.is_except_handler() { + // The except handlers have their own body to which we can attach the leading comment CommentPlacement::leading(following, comment) + } else if let AnyNodeRef::StmtIf(stmt_if) = comment.enclosing_node() { + if let Some(clause) = stmt_if + .elif_else_clauses + .iter() + .find(|clause| are_same_optional(following, clause.test.as_ref())) + { + CommentPlacement::leading(clause.into(), comment) + } else { + // Since we know we're between bodies and we know that the following node is + // not the condition of any `elif`, we know the next node must be the `else` + let else_clause = stmt_if.elif_else_clauses.last().unwrap(); + debug_assert!(else_clause.test.is_none()); + CommentPlacement::leading(else_clause.into(), comment) + } } else { // There are no bodies for the "else" branch and other bodies that are represented as a `Vec`. // This means, there's no good place to attach the comments to. @@ -356,42 +370,42 @@ fn handle_in_between_bodies_end_of_line_comment<'a>( } if locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) { - // The `elif` or except handlers have their own body to which we can attach the trailing comment + // The except handlers have their own body to which we can attach the trailing comment // ```python - // if test: - // a - // elif c: # comment - // b + // try: + // f() # comment + // except RuntimeError: + // raise // ``` if following.is_except_handler() { return CommentPlacement::trailing(following, comment); - } else if following.is_stmt_if() { - // We have to exclude for following if statements that are not elif by checking the - // indentation - // ```python - // if True: - // pass - // else: # Comment - // if False: - // pass - // pass - // ``` - let base_if_indent = - whitespace::indentation_at_offset(locator, following.range().start()); - let maybe_elif_indent = whitespace::indentation_at_offset( - locator, - comment.enclosing_node().range().start(), - ); - if base_if_indent == maybe_elif_indent { - return CommentPlacement::trailing(following, comment); + } + + // Handle the `else` of an `if`. It is special because we don't have a test but unlike other + // `else` (e.g. for `while`), we have a dedicated node. + // ```python + // if x == y: + // pass + // elif x < y: + // pass + // else: # 12 trailing else condition + // pass + // ``` + if let AnyNodeRef::StmtIf(stmt_if) = comment.enclosing_node() { + if let Some(else_clause) = stmt_if.elif_else_clauses.last() { + if else_clause.test.is_none() + && following.ptr_eq(else_clause.body.first().unwrap().into()) + { + return CommentPlacement::dangling(else_clause.into(), comment); + } } } - // There are no bodies for the "else" branch and other bodies that are represented as a `Vec`. - // This means, there's no good place to attach the comments to. - // Make this a dangling comments and manually format the comment in - // in the enclosing node's formatting logic. For `try`, it's the formatters responsibility - // to correctly identify the comments for the `finally` and `orelse` block by looking - // at the comment's range. + + // There are no bodies for the "else" branch (only `Vec`) expect for StmtIf, so + // we make this a dangling comments of the node containing the alternate branch and + // manually format the comment in that node's formatting logic. For `try`, it's the + // formatters responsibility to correctly identify the comments for the `finally` and + // `orelse` block by looking at the comment's range. // // ```python // while x == y: @@ -425,6 +439,64 @@ fn handle_in_between_bodies_end_of_line_comment<'a>( } } +/// Without the `StmtIf` special, this function would just be the following: +/// ```ignore +/// if let Some(preceding_node) = comment.preceding_node() { +/// Some((preceding_node, last_child_in_body(preceding_node)?)) +/// } else { +/// None +/// } +/// ``` +/// We handle two special cases here: +/// ```python +/// if True: +/// pass +/// # Comment between if and elif/else clause, needs to be manually attached to the `StmtIf` +/// else: +/// pass +/// # Comment after the `StmtIf`, needs to be manually attached to the ElifElseClause +/// ``` +/// The problem is that `StmtIf` spans the whole range (there is no "inner if" node), so the first +/// comment doesn't see it as preceding node, and the second comment takes the entire `StmtIf` when +/// it should only take the `ElifElseClause` +fn find_preceding_and_handle_stmt_if_special_cases<'a>( + comment: &DecoratedComment<'a>, +) -> Option<(AnyNodeRef<'a>, AnyNodeRef<'a>)> { + if let (stmt_if @ AnyNodeRef::StmtIf(stmt_if_inner), Some(AnyNodeRef::ElifElseClause(..))) = + (comment.enclosing_node(), comment.following_node()) + { + if let Some(preceding_node @ AnyNodeRef::ElifElseClause(..)) = comment.preceding_node() { + // We're already after and elif or else, defaults work + Some((preceding_node, last_child_in_body(preceding_node)?)) + } else { + // Special case 1: The comment is between if body and an elif/else clause. We have + // to handle this separately since StmtIf spans the entire range, so it's not the + // preceding node + Some(( + stmt_if, + AnyNodeRef::from(stmt_if_inner.body.last().unwrap()), + )) + } + } else if let Some(preceding_node @ AnyNodeRef::StmtIf(stmt_if_inner)) = + comment.preceding_node() + { + if let Some(clause) = stmt_if_inner.elif_else_clauses.last() { + // Special case 2: We're after an if statement and need to narrow the preceding + // down to the elif/else clause + Some((clause.into(), last_child_in_body(clause.into())?)) + } else { + // After an if without any elif/else, defaults work + Some((preceding_node, last_child_in_body(preceding_node)?)) + } + } else if let Some(preceding_node) = comment.preceding_node() { + // The normal case + Some((preceding_node, last_child_in_body(preceding_node)?)) + } else { + // Only do something if the preceding node has a body (has indented statements). + None + } +} + /// Handles trailing comments at the end of a body block (or any other block that is indented). /// ```python /// def test(): @@ -442,12 +514,9 @@ fn handle_trailing_body_comment<'a>( return CommentPlacement::Default(comment); } - // Only do something if the preceding node has a body (has indented statements). - let Some(preceding_node) = comment.preceding_node() else { - return CommentPlacement::Default(comment); - }; - - let Some(last_child) = last_child_in_body(preceding_node) else { + let Some((preceding_node, last_child)) = + find_preceding_and_handle_stmt_if_special_cases(&comment) + else { return CommentPlacement::Default(comment); }; @@ -566,6 +635,22 @@ fn handle_trailing_end_of_line_body_comment<'a>( return CommentPlacement::Default(comment); }; + // Handle the StmtIf special case + // ```python + // if True: + // pass + // elif True: + // pass # 14 end-of-line trailing `pass` comment, set preceding to the ElifElseClause + // ``` + let preceding = if let AnyNodeRef::StmtIf(stmt_if) = preceding { + stmt_if + .elif_else_clauses + .last() + .map_or(preceding, AnyNodeRef::from) + } else { + preceding + }; + // Recursively get the last child of statements with a body. let last_children = std::iter::successors(last_child_in_body(preceding), |parent| { last_child_in_body(*parent) @@ -600,20 +685,40 @@ fn handle_trailing_end_of_line_condition_comment<'a>( return CommentPlacement::Default(comment); } + // We handle trailing else comments separately because we the preceding node is None for their + // case + // ```python + // if True: + // pass + // else: # 12 trailing else condition + // pass + // ``` + if let AnyNodeRef::ElifElseClause(ast::ElifElseClause { + body, test: None, .. + }) = comment.enclosing_node() + { + if comment.start() < body.first().unwrap().start() { + return CommentPlacement::dangling(comment.enclosing_node(), comment); + } + } + // Must be between the condition expression and the first body element let (Some(preceding), Some(following)) = (comment.preceding_node(), comment.following_node()) else { return CommentPlacement::Default(comment); }; - let expression_before_colon = match comment.enclosing_node() { + let enclosing_node = comment.enclosing_node(); + let expression_before_colon = match enclosing_node { + AnyNodeRef::ElifElseClause(ast::ElifElseClause { + test: Some(expr), .. + }) => Some(AnyNodeRef::from(expr)), AnyNodeRef::StmtIf(ast::StmtIf { test: expr, .. }) | AnyNodeRef::StmtWhile(ast::StmtWhile { test: expr, .. }) | AnyNodeRef::StmtFor(ast::StmtFor { iter: expr, .. }) | AnyNodeRef::StmtAsyncFor(ast::StmtAsyncFor { iter: expr, .. }) => { Some(AnyNodeRef::from(expr.as_ref())) } - AnyNodeRef::StmtWith(ast::StmtWith { items, .. }) | AnyNodeRef::StmtAsyncWith(ast::StmtAsyncWith { items, .. }) => { items.last().map(AnyNodeRef::from) @@ -656,7 +761,7 @@ fn handle_trailing_end_of_line_condition_comment<'a>( // while a: # comment // ... // ``` - return CommentPlacement::dangling(comment.enclosing_node(), comment); + return CommentPlacement::dangling(enclosing_node, comment); } // Comment comes before the colon @@ -1439,10 +1544,15 @@ fn last_child_in_body(node: AnyNodeRef) -> Option { | AnyNodeRef::MatchCase(ast::MatchCase { body, .. }) | AnyNodeRef::ExceptHandlerExceptHandler(ast::ExceptHandlerExceptHandler { body, .. - }) => body, + }) + | AnyNodeRef::ElifElseClause(ast::ElifElseClause { body, .. }) => body, + AnyNodeRef::StmtIf(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => elif_else_clauses.last().map_or(body, |clause| &clause.body), - AnyNodeRef::StmtIf(ast::StmtIf { body, orelse, .. }) - | AnyNodeRef::StmtFor(ast::StmtFor { body, orelse, .. }) + AnyNodeRef::StmtFor(ast::StmtFor { body, orelse, .. }) | AnyNodeRef::StmtAsyncFor(ast::StmtAsyncFor { body, orelse, .. }) | AnyNodeRef::StmtWhile(ast::StmtWhile { body, orelse, .. }) => { if orelse.is_empty() { @@ -1453,7 +1563,7 @@ fn last_child_in_body(node: AnyNodeRef) -> Option { } AnyNodeRef::StmtMatch(ast::StmtMatch { cases, .. }) => { - return cases.last().map(AnyNodeRef::from) + return cases.last().map(AnyNodeRef::from); } AnyNodeRef::StmtTry(ast::StmtTry { @@ -1498,8 +1608,26 @@ fn is_first_statement_in_enclosing_alternate_body( enclosing: AnyNodeRef, ) -> bool { match enclosing { - AnyNodeRef::StmtIf(ast::StmtIf { orelse, .. }) - | AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. }) + AnyNodeRef::StmtIf(ast::StmtIf { + elif_else_clauses, .. + }) => { + for clause in elif_else_clauses { + if let Some(test) = &clause.test { + // `elif`, the following node is the test + if following.ptr_eq(test.into()) { + return true; + } + } else { + // `else`, there is no test and the following node is the first entry in the + // body + if following.ptr_eq(clause.body.first().unwrap().into()) { + return true; + } + } + } + false + } + AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. }) | AnyNodeRef::StmtAsyncFor(ast::StmtAsyncFor { orelse, .. }) | AnyNodeRef::StmtWhile(ast::StmtWhile { orelse, .. }) => { are_same_optional(following, orelse.first()) diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__base_test.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__base_test.snap index ef24b1dbef..3ace969501 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__base_test.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__base_test.snap @@ -34,8 +34,8 @@ expression: comments.debug(test_case.source_code) "trailing": [], }, Node { - kind: StmtIf, - range: 144..212, + kind: ElifElseClause, + range: 144..177, source: `elif x < y:⏎`, }: { "leading": [ diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_else_comments.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_else_comments.snap index af56bdb4b9..99d380cc70 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_else_comments.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_else_comments.snap @@ -24,8 +24,8 @@ expression: comments.debug(test_case.source_code) ], }, Node { - kind: StmtIf, - range: 104..192, + kind: ElifElseClause, + range: 104..124, source: `elif x < y:⏎`, }: { "leading": [ @@ -35,13 +35,7 @@ expression: comments.debug(test_case.source_code) formatted: false, }, ], - "dangling": [ - SourceComment { - text: "# Leading else comment", - position: OwnLine, - formatted: false, - }, - ], + "dangling": [], "trailing": [], }, Node { @@ -59,6 +53,21 @@ expression: comments.debug(test_case.source_code) }, ], }, + Node { + kind: ElifElseClause, + range: 178..192, + source: `else:⏎`, + }: { + "leading": [ + SourceComment { + text: "# Leading else comment", + position: OwnLine, + formatted: false, + }, + ], + "dangling": [], + "trailing": [], + }, Node { kind: StmtPass, range: 188..192, diff --git a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_if_else_comments.snap b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_if_else_comments.snap index 1aa79f4f4e..1cd1f74ac2 100644 --- a/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_if_else_comments.snap +++ b/crates/ruff_python_formatter/src/comments/snapshots/ruff_python_formatter__comments__tests__if_elif_if_else_comments.snap @@ -3,21 +3,6 @@ source: crates/ruff_python_formatter/src/comments/mod.rs expression: comments.debug(test_case.source_code) --- { - Node { - kind: StmtIf, - range: 21..128, - source: `elif x < y:⏎`, - }: { - "leading": [], - "dangling": [ - SourceComment { - text: "# Leading else comment", - position: OwnLine, - formatted: false, - }, - ], - "trailing": [], - }, Node { kind: StmtIf, range: 37..60, @@ -33,4 +18,19 @@ expression: comments.debug(test_case.source_code) }, ], }, + Node { + kind: ElifElseClause, + range: 114..128, + source: `else:⏎`, + }: { + "leading": [ + SourceComment { + text: "# Leading else comment", + position: OwnLine, + formatted: false, + }, + ], + "dangling": [], + "trailing": [], + }, } diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index 947dc4b31b..28218bb985 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/crates/ruff_python_formatter/src/comments/visitor.rs @@ -2,8 +2,8 @@ use std::iter::Peekable; use ruff_text_size::{TextRange, TextSize}; use rustpython_parser::ast::{ - Alias, Arg, ArgWithDefault, Arguments, Comprehension, Decorator, ExceptHandler, Expr, Keyword, - MatchCase, Mod, Pattern, Ranged, Stmt, WithItem, + Alias, Arg, ArgWithDefault, Arguments, Comprehension, Decorator, ElifElseClause, ExceptHandler, + Expr, Keyword, MatchCase, Mod, Pattern, Ranged, Stmt, WithItem, }; use ruff_formatter::{SourceCode, SourceCodeSlice}; @@ -284,6 +284,13 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast> { } self.finish_node(pattern); } + + fn visit_elif_else_clause(&mut self, elif_else_clause: &'ast ElifElseClause) { + if self.start_node(elif_else_clause).is_traverse() { + walk_elif_else_clause(self, elif_else_clause); + } + self.finish_node(elif_else_clause); + } } fn text_position(comment_range: TextRange, source_code: SourceCode) -> CommentLinePosition { diff --git a/crates/ruff_python_formatter/src/generated.rs b/crates/ruff_python_formatter/src/generated.rs index c9bb761e30..e83c2746fb 100644 --- a/crates/ruff_python_formatter/src/generated.rs +++ b/crates/ruff_python_formatter/src/generated.rs @@ -617,6 +617,46 @@ impl<'ast> IntoFormat> for ast::StmtIf { } } +impl FormatRule> + for crate::statement::stmt_if::FormatElifElseClause +{ + #[inline] + fn fmt( + &self, + node: &ast::ElifElseClause, + f: &mut Formatter>, + ) -> FormatResult<()> { + FormatNodeRule::::fmt(self, node, f) + } +} +impl<'ast> AsFormat> for ast::ElifElseClause { + type Format<'a> = FormatRefWithRule< + 'a, + ast::ElifElseClause, + crate::statement::stmt_if::FormatElifElseClause, + PyFormatContext<'ast>, + >; + fn format(&self) -> Self::Format<'_> { + FormatRefWithRule::new( + self, + crate::statement::stmt_if::FormatElifElseClause::default(), + ) + } +} +impl<'ast> IntoFormat> for ast::ElifElseClause { + type Format = FormatOwnedWithRule< + ast::ElifElseClause, + crate::statement::stmt_if::FormatElifElseClause, + PyFormatContext<'ast>, + >; + fn into_format(self) -> Self::Format { + FormatOwnedWithRule::new( + self, + crate::statement::stmt_if::FormatElifElseClause::default(), + ) + } +} + impl FormatRule> for crate::statement::stmt_with::FormatStmtWith { diff --git a/crates/ruff_python_formatter/src/statement/mod.rs b/crates/ruff_python_formatter/src/statement/mod.rs index 4d5804a847..d0e0130da9 100644 --- a/crates/ruff_python_formatter/src/statement/mod.rs +++ b/crates/ruff_python_formatter/src/statement/mod.rs @@ -64,6 +64,7 @@ impl FormatRule> for FormatStmt { Stmt::Pass(x) => x.format().fmt(f), Stmt::Break(x) => x.format().fmt(f), Stmt::Continue(x) => x.format().fmt(f), + Stmt::TypeAlias(_) => todo!(), } } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs index 4f14e9df45..86c7688284 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs @@ -18,6 +18,7 @@ impl FormatNodeRule for FormatStmtClassDef { bases, keywords, body, + type_params: _, decorator_list, } = item; diff --git a/crates/ruff_python_formatter/src/statement/stmt_if.rs b/crates/ruff_python_formatter/src/statement/stmt_if.rs index ab249f228e..4e1391cbe6 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_if.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_if.rs @@ -1,91 +1,43 @@ -use crate::comments::{leading_alternate_branch_comments, trailing_comments, SourceComment}; +use crate::comments::{leading_alternate_branch_comments, trailing_comments}; use crate::expression::maybe_parenthesize_expression; use crate::expression::parentheses::Parenthesize; use crate::prelude::*; use crate::FormatNodeRule; -use ruff_formatter::{write, FormatError}; -use rustpython_parser::ast::{Ranged, Stmt, StmtIf, Suite}; +use ruff_formatter::write; +use ruff_python_ast::node::AnyNodeRef; +use rustpython_parser::ast::{ElifElseClause, StmtIf}; #[derive(Default)] pub struct FormatStmtIf; impl FormatNodeRule for FormatStmtIf { fn fmt_fields(&self, item: &StmtIf, f: &mut PyFormatter) -> FormatResult<()> { + let StmtIf { + range: _, + test, + body, + elif_else_clauses, + } = item; + let comments = f.context().comments().clone(); + let trailing_colon_comment = comments.dangling_comments(item); - let mut current = IfOrElIf::If(item); - let mut else_comments: &[SourceComment]; - let mut last_node_of_previous_body = None; + write!( + f, + [ + text("if"), + space(), + maybe_parenthesize_expression(test, item, Parenthesize::IfBreaks), + text(":"), + trailing_comments(trailing_colon_comment), + block_indent(&body.format()) + ] + )?; - loop { - let current_statement = current.statement(); - let StmtIf { - test, body, orelse, .. - } = current_statement; - - let first_statement = body.first().ok_or(FormatError::SyntaxError)?; - let trailing = comments.dangling_comments(current_statement); - - let trailing_if_comments_end = trailing - .partition_point(|comment| comment.slice().start() < first_statement.start()); - - let (if_trailing_comments, trailing_alternate_comments) = - trailing.split_at(trailing_if_comments_end); - - if current.is_elif() { - let elif_leading = comments.leading_comments(current_statement); - // Manually format the leading comments because the formatting bypasses `NodeRule::fmt` - write!( - f, - [ - leading_alternate_branch_comments(elif_leading, last_node_of_previous_body), - source_position(current_statement.start()) - ] - )?; - } - - write!( - f, - [ - text(current.keyword()), - space(), - maybe_parenthesize_expression(test, current_statement, Parenthesize::IfBreaks), - text(":"), - trailing_comments(if_trailing_comments), - block_indent(&body.format()) - ] - )?; - - // RustPython models `elif` by setting the body to a single `if` statement. The `orelse` - // of the most inner `if` statement then becomes the `else` of the whole `if` chain. - // That's why it's necessary to take the comments here from the most inner `elif`. - else_comments = trailing_alternate_comments; - last_node_of_previous_body = body.last(); - - if let Some(elif) = else_if(orelse) { - current = elif; - } else { - break; - } - } - - let orelse = ¤t.statement().orelse; - - if !orelse.is_empty() { - // Leading comments are always own line comments - let leading_else_comments_end = - else_comments.partition_point(|comment| comment.line_position().is_own_line()); - let (else_leading, else_trailing) = else_comments.split_at(leading_else_comments_end); - - write!( - f, - [ - leading_alternate_branch_comments(else_leading, last_node_of_previous_body), - text("else:"), - trailing_comments(else_trailing), - block_indent(&orelse.format()) - ] - )?; + let mut last_node = body.last().unwrap().into(); + for clause in elif_else_clauses { + format_elif_else_clause(clause, f, Some(last_node))?; + last_node = clause.body.last().unwrap().into(); } Ok(()) @@ -97,35 +49,56 @@ impl FormatNodeRule for FormatStmtIf { } } -fn else_if(or_else: &Suite) -> Option { - if let [Stmt::If(if_stmt)] = or_else.as_slice() { - Some(IfOrElIf::ElIf(if_stmt)) +/// Note that this implementation misses the leading newlines before the leading comments because +/// it does not have access to the last node of the previous branch. The `StmtIf` therefore doesn't +/// call this but `format_elif_else_clause` directly. +#[derive(Default)] +pub struct FormatElifElseClause; + +impl FormatNodeRule for FormatElifElseClause { + fn fmt_fields(&self, item: &ElifElseClause, f: &mut PyFormatter) -> FormatResult<()> { + format_elif_else_clause(item, f, None) + } +} + +/// Extracted so we can implement `FormatElifElseClause` but also pass in `last_node` from +/// `FormatStmtIf` +fn format_elif_else_clause( + item: &ElifElseClause, + f: &mut PyFormatter, + last_node: Option, +) -> FormatResult<()> { + let ElifElseClause { + range: _, + test, + body, + } = item; + + let comments = f.context().comments().clone(); + let trailing_colon_comment = comments.dangling_comments(item); + let leading_comments = comments.leading_comments(item); + + leading_alternate_branch_comments(leading_comments, last_node).fmt(f)?; + + if let Some(test) = test { + write!( + f, + [ + text("elif"), + space(), + maybe_parenthesize_expression(test, item, Parenthesize::IfBreaks), + ] + )?; } else { - None - } -} - -enum IfOrElIf<'a> { - If(&'a StmtIf), - ElIf(&'a StmtIf), -} - -impl<'a> IfOrElIf<'a> { - const fn statement(&self) -> &'a StmtIf { - match self { - IfOrElIf::If(statement) => statement, - IfOrElIf::ElIf(statement) => statement, - } - } - - const fn keyword(&self) -> &'static str { - match self { - IfOrElIf::If(_) => "if", - IfOrElIf::ElIf(_) => "elif", - } - } - - const fn is_elif(&self) -> bool { - matches!(self, IfOrElIf::ElIf(_)) + text("else").fmt(f)?; } + + write!( + f, + [ + text(":"), + trailing_comments(trailing_colon_comment), + block_indent(&body.format()) + ] + ) } diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap index 341956733f..31226f41f7 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap @@ -353,7 +353,7 @@ else: if True: def f2(): pass - # a + # a else: pass diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap index 9bbe6803f2..a695caa4e1 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__if.py.snap @@ -4,19 +4,23 @@ input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/ --- ## Input ```py -if x == y: # trailing if condition - pass # trailing `pass` comment - # Root `if` trailing comment +# 1 leading if comment +if x == y: # 2 trailing if condition + # 3 leading pass + pass # 4 end-of-line trailing `pass` comment + # 5 Root `if` trailing comment -# Leading elif comment -elif x < y: # trailing elif condition - pass - # `elif` trailing comment +# 6 Leading elif comment +elif x < y: # 7 trailing elif condition + # 8 leading pass + pass # 9 end-of-line trailing `pass` comment + # 10 `elif` trailing comment -# Leading else comment -else: # trailing else condition - pass - # `else` trailing comment +# 11 Leading else comment +else: # 12 trailing else condition + # 13 leading pass + pass # 14 end-of-line trailing `pass` comment + # 15 `else` trailing comment if x == y: @@ -77,23 +81,38 @@ else: # Comment if False: pass pass + + +# Regression test for `last_child_in_body` special casing of `StmtIf` +# https://github.com/python/cpython/blob/aecf6aca515a203a823a87c711f15cbb82097c8b/Lib/test/test_pty.py#L260-L275 +def f(): + if True: + pass + else: + pass + + # comment ``` ## Output ```py -if x == y: # trailing if condition - pass # trailing `pass` comment - # Root `if` trailing comment +# 1 leading if comment +if x == y: # 2 trailing if condition + # 3 leading pass + pass # 4 end-of-line trailing `pass` comment + # 5 Root `if` trailing comment -# Leading elif comment -elif x < y: # trailing elif condition - pass - # `elif` trailing comment +# 6 Leading elif comment +elif x < y: # 7 trailing elif condition + # 8 leading pass + pass # 9 end-of-line trailing `pass` comment + # 10 `elif` trailing comment -# Leading else comment -else: # trailing else condition - pass - # `else` trailing comment +# 11 Leading else comment +else: # 12 trailing else condition + # 13 leading pass + pass # 14 end-of-line trailing `pass` comment + # 15 `else` trailing comment if x == y: @@ -153,6 +172,17 @@ else: # Comment if False: pass pass + + +# Regression test for `last_child_in_body` special casing of `StmtIf` +# https://github.com/python/cpython/blob/aecf6aca515a203a823a87c711f15cbb82097c8b/Lib/test/test_pty.py#L260-L275 +def f(): + if True: + pass + else: + pass + + # comment ``` diff --git a/crates/ruff_python_semantic/src/analyze/branch_detection.rs b/crates/ruff_python_semantic/src/analyze/branch_detection.rs index 62ffb8e116..5314f2dce5 100644 --- a/crates/ruff_python_semantic/src/analyze/branch_detection.rs +++ b/crates/ruff_python_semantic/src/analyze/branch_detection.rs @@ -1,4 +1,5 @@ use std::cmp::Ordering; +use std::iter; use rustpython_parser::ast::{self, ExceptHandler, Stmt}; @@ -42,7 +43,17 @@ fn common_ancestor( /// Return the alternative branches for a given node. fn alternatives(stmt: &Stmt) -> Vec> { match stmt { - Stmt::If(ast::StmtIf { body, .. }) => vec![body.iter().collect()], + Stmt::If(ast::StmtIf { + body, + elif_else_clauses, + .. + }) => iter::once(body.iter().collect()) + .chain( + elif_else_clauses + .iter() + .map(|clause| clause.body.iter().collect()), + ) + .collect(), Stmt::Try(ast::StmtTry { body, handlers, diff --git a/crates/ruff_wasm/tests/api.rs b/crates/ruff_wasm/tests/api.rs index fd2a70c4ff..35c72dfc10 100644 --- a/crates/ruff_wasm/tests/api.rs +++ b/crates/ruff_wasm/tests/api.rs @@ -29,11 +29,11 @@ fn empty_config() { message: "If test is a tuple, which is always `True`".to_string(), location: SourceLocation { row: OneIndexed::from_zero_indexed(0), - column: OneIndexed::from_zero_indexed(0) + column: OneIndexed::from_zero_indexed(3) }, end_location: SourceLocation { - row: OneIndexed::from_zero_indexed(1), - column: OneIndexed::from_zero_indexed(8) + row: OneIndexed::from_zero_indexed(0), + column: OneIndexed::from_zero_indexed(9) }, fix: None, }] diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 27f3f16930..7e5c73aff0 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -24,8 +24,8 @@ ruff_python_ast = { path = "../crates/ruff_python_ast" } ruff_python_formatter = { path = "../crates/ruff_python_formatter" } similar = { version = "2.2.1" } -# Current tag: v0.0.7 -rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" , default-features = false, features = ["full-lexer", "num-bigint"] } +# Current tag: v0.0.9 +rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" , default-features = false, features = ["full-lexer", "num-bigint"] } # Prevent this from interfering with workspaces [workspace]