Skip to content

Commit 5967e13

Browse files
konstinMichaReiser
andcommitted
Refactor StmtIf: Formatter and Linter (#5459)
## Summary Previously, `StmtIf` was defined recursively as ```rust pub struct StmtIf { pub range: TextRange, pub test: Box<Expr>, pub body: Vec<Stmt>, pub orelse: Vec<Stmt>, } ``` Every `elif` was represented as an `orelse` with a single `StmtIf`. This means that this representation couldn't differentiate between ```python if cond1: x = 1 else: if cond2: x = 2 ``` and ```python if cond1: x = 1 elif cond2: x = 2 ``` It also makes many checks harder than they need to be because we have to recurse just to iterate over an entire if-elif-else and because we're lacking nodes and ranges on the `elif` and `else` branches. We change the representation to a flat ```rust pub struct StmtIf { pub range: TextRange, pub test: Box<Expr>, pub body: Vec<Stmt>, pub elif_else_clauses: Vec<ElifElseClause>, } pub struct ElifElseClause { pub range: TextRange, pub test: Option<Expr>, pub body: Vec<Stmt>, } ``` where `test: Some(_)` represents an `elif` and `test: None` an else. This representation is different tradeoff, e.g. we need to allocate the `Vec<ElifElseClause>`, the `elif`s are now different than the `if`s (which matters in rules where want to check both `if`s and `elif`s) and the type system doesn't guarantee that the `test: None` else is actually last. We're also now a bit more inconsistent since all other `else`, those from `for`, `while` and `try`, still don't have nodes. With the new representation some things became easier, e.g. finding the `elif` token (we can use the start of the `ElifElseClause`) and formatting comments for if-elif-else (no more dangling comments splitting, we only have to insert the dangling comment after the colon manually and set `leading_alternate_branch_comments`, everything else is taken of by having nodes for each branch and the usual placement.rs fixups). ## Merge Plan This PR requires coordination between the parser repo and the main ruff repo. I've split the ruff part, into two stacked PRs which have to be merged together (only the second one fixes all tests), the first for the formatter to be reviewed by @MichaReiser and the second for the linter to be reviewed by @charliermarsh. * MH: Review and merge astral-sh/RustPython-Parser#20 * MH: Review and merge or move later in stack astral-sh/RustPython-Parser#21 * MH: Review and approve astral-sh/RustPython-Parser#22 * MH: Review and approve formatter PR #5459 * CM: Review and approve linter PR #5460 * Merge linter PR in formatter PR, fix ecosystem checks (ecosystem checks can't run on the formatter PR and won't run on the linter PR, so we need to merge them first) * Merge astral-sh/RustPython-Parser#22 * Create tag in the parser, update linter+formatter PR * Merge linter+formatter PR #5459 --------- Co-authored-by: Micha Reiser <micha@reiser.io>
1 parent d5d0a91 commit 5967e13

File tree

82 files changed

+2323
-1999
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

82 files changed

+2323
-1999
lines changed

Cargo.lock

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,12 @@ libcst = { git = "https://github.com/Instagram/LibCST.git", rev = "3cacca1a1029f
5454

5555
# Please tag the RustPython version every time you update its revision here and in fuzz/Cargo.toml
5656
# Tagging the version ensures that older ruff versions continue to build from source even when we rebase our RustPython fork.
57-
# Current tag: v0.0.7
58-
ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" }
59-
rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" , default-features = false, features = ["num-bigint"]}
60-
rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a", default-features = false, features = ["num-bigint"] }
61-
rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a", default-features = false }
62-
rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "b996b21ffca562ecb2086f632a6a0b05c245c24a" , default-features = false, features = ["full-lexer", "num-bigint"] }
57+
# Current tag: v0.0.9
58+
ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" }
59+
rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" , default-features = false, features = ["num-bigint"]}
60+
rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c", default-features = false, features = ["num-bigint"] }
61+
rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c", default-features = false }
62+
rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "db04fd415774032e1e2ceb03bcbf5305e0d22c8c" , default-features = false, features = ["full-lexer", "num-bigint"] }
6363

6464
[profile.release]
6565
lto = "fat"

crates/ruff/resources/test/fixtures/flake8_bugbear/B007.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,3 +97,10 @@ def f():
9797
# variable name).
9898
for line_ in range(self.header_lines):
9999
fp.readline()
100+
101+
# Regression test: visitor didn't walk the elif test
102+
for key, value in current_crawler_tags.items():
103+
if key:
104+
pass
105+
elif wanted_tag_value != value:
106+
pass

crates/ruff/resources/test/fixtures/flake8_simplify/SIM102.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,14 @@
100100
):
101101
print("Bad module!")
102102

103+
# SIM102
104+
# Regression test for https://github.com/apache/airflow/blob/145b16caaa43f0c42bffd97344df916c602cddde/airflow/configuration.py#L1161
105+
if a:
106+
if b:
107+
if c:
108+
print("if")
109+
elif d:
110+
print("elif")
103111

104112
# OK
105113
if a:

crates/ruff/resources/test/fixtures/flake8_simplify/SIM108.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
else:
2424
b = 2
2525

26-
# OK (false negative)
26+
# SIM108
2727
if True:
2828
pass
2929
else:

crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,3 +94,10 @@ def foo(p):
9494
errors = 1
9595
else:
9696
errors = 1
97+
98+
if a:
99+
# Ignore branches with diverging comments because it means we're repeating
100+
# the bodies because we have different reasons for each branch
101+
x = 1
102+
elif c:
103+
x = 1

crates/ruff/resources/test/fixtures/flake8_simplify/SIM116.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,15 @@
8484
return "D"
8585
elif func_name == "move":
8686
return "MV"
87+
88+
# OK
89+
def no_return_in_else(platform):
90+
if platform == "linux":
91+
return "auditwheel repair -w {dest_dir} {wheel}"
92+
elif platform == "macos":
93+
return "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel}"
94+
elif platform == "windows":
95+
return ""
96+
else:
97+
msg = f"Unknown platform: {platform!r}"
98+
raise ValueError(msg)

crates/ruff/resources/test/fixtures/flake8_simplify/SIM401.py

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,15 @@
3838
else:
3939
vars[idx] = "defaultß9💣2ℝ6789ß9💣2ℝ6789ß9💣2ℝ6789ß9💣2ℝ6789ß9💣2ℝ6789"
4040

41+
# SIM401
42+
if foo():
43+
pass
44+
else:
45+
if key in a_dict:
46+
vars[idx] = a_dict[key]
47+
else:
48+
vars[idx] = "default"
49+
4150
###
4251
# Negative cases
4352
###
@@ -105,12 +114,3 @@
105114
vars[idx] = a_dict[key]
106115
else:
107116
vars[idx] = "default"
108-
109-
# OK (false negative for nested else)
110-
if foo():
111-
pass
112-
else:
113-
if key in a_dict:
114-
vars[idx] = a_dict[key]
115-
else:
116-
vars[idx] = "default"

crates/ruff/resources/test/fixtures/pyflakes/F634.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
if (1, 2):
22
pass
33

4+
if (3, 4):
5+
pass
6+
elif foo:
7+
pass
8+
49
for _ in range(5):
510
if True:
611
pass
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# Regression test for branch detection from
2+
# https://github.com/pypa/build/blob/5800521541e5e749d4429617420d1ef8cdb40b46/src/build/_importlib.py
3+
import sys
4+
5+
if sys.version_info < (3, 8):
6+
import importlib_metadata as metadata
7+
elif sys.version_info < (3, 9, 10) or (3, 10, 0) <= sys.version_info < (3, 10, 2):
8+
try:
9+
import importlib_metadata as metadata
10+
except ModuleNotFoundError:
11+
from importlib import metadata
12+
else:
13+
from importlib import metadata
14+
15+
__all__ = ["metadata"]

crates/ruff/resources/test/fixtures/pylint/collapsible_else_if.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,17 @@ def not_ok1():
4747
pass
4848
else:
4949
pass
50+
51+
52+
# Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737
53+
def not_ok2():
54+
if True:
55+
print(1)
56+
elif True:
57+
print(2)
58+
else:
59+
if True:
60+
print(3)
61+
else:
62+
print(4)
63+

crates/ruff/resources/test/fixtures/pyupgrade/UP036_4.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,28 +7,28 @@
77

88
if True:
99
if foo:
10-
pass
10+
print()
1111
elif sys.version_info < (3, 3):
1212
cmd = [sys.executable, "-m", "test.regrtest"]
1313

1414
if True:
1515
if foo:
16-
pass
16+
print()
1717
elif sys.version_info < (3, 3):
1818
cmd = [sys.executable, "-m", "test.regrtest"]
1919
elif foo:
2020
cmd = [sys.executable, "-m", "test", "-j0"]
2121

2222
if foo:
23-
pass
23+
print()
2424
elif sys.version_info < (3, 3):
2525
cmd = [sys.executable, "-m", "test.regrtest"]
2626

2727
if sys.version_info < (3, 3):
2828
cmd = [sys.executable, "-m", "test.regrtest"]
2929

3030
if foo:
31-
pass
31+
print()
3232
elif sys.version_info < (3, 3):
3333
cmd = [sys.executable, "-m", "test.regrtest"]
3434
else:

crates/ruff/resources/test/fixtures/tryceratops/TRY004.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,15 @@ def incorrect_multi_conditional(arg1, arg2):
230230
raise Exception("...") # should be typeerror
231231

232232

233+
def multiple_is_instance_checks(some_arg):
234+
if isinstance(some_arg, str):
235+
pass
236+
elif isinstance(some_arg, int):
237+
pass
238+
else:
239+
raise Exception("...") # should be typeerror
240+
241+
233242
class MyCustomTypeValidation(Exception):
234243
pass
235244

@@ -296,6 +305,17 @@ def multiple_ifs(some_args):
296305
pass
297306

298307

308+
def else_body(obj):
309+
if isinstance(obj, datetime.timedelta):
310+
return "TimeDelta"
311+
elif isinstance(obj, relativedelta.relativedelta):
312+
return "RelativeDelta"
313+
elif isinstance(obj, CronExpression):
314+
return "CronExpression"
315+
else:
316+
raise Exception(f"Unknown object type: {obj.__class__.__name__}")
317+
318+
299319
def early_return():
300320
if isinstance(this, some_type):
301321
if x in this:

crates/ruff/src/autofix/edits.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,12 +190,24 @@ fn is_lone_child(child: &Stmt, parent: &Stmt) -> bool {
190190
}
191191
Stmt::For(ast::StmtFor { body, orelse, .. })
192192
| Stmt::AsyncFor(ast::StmtAsyncFor { body, orelse, .. })
193-
| Stmt::While(ast::StmtWhile { body, orelse, .. })
194-
| Stmt::If(ast::StmtIf { body, orelse, .. }) => {
193+
| Stmt::While(ast::StmtWhile { body, orelse, .. }) => {
195194
if is_only(body, child) || is_only(orelse, child) {
196195
return true;
197196
}
198197
}
198+
Stmt::If(ast::StmtIf {
199+
body,
200+
elif_else_clauses,
201+
..
202+
}) => {
203+
if is_only(body, child)
204+
|| elif_else_clauses
205+
.iter()
206+
.any(|ast::ElifElseClause { body, .. }| is_only(body, child))
207+
{
208+
return true;
209+
}
210+
}
199211
Stmt::Try(ast::StmtTry {
200212
body,
201213
handlers,

0 commit comments

Comments
 (0)