Skip to content

Commit 2110d32

Browse files
authored
[pylint] Narrow diagnostic range and exclude cases without exception handlers (PLW0717) (#25440)
Summary -- This PR fixes #25438 by narrowing the diagnostic range from the entire `try` statement to only the `try` keyword. This was one of the two alternatives mentioned on #25438, and I thought this looked a bit nicer than only narrowing the range to the `try` body. I think this is also consistent with the range for `too-many-statements` (`PLR0915`), which marks the function name: https://play.ruff.rs/dafe23a1-2e89-48a5-b4d7-b0825b737152 I'm happy to reconsider, though. Another possible alternative along these lines is marking the first statement that exceeds the limit, or the last statement, or something like that. This PR also fixes #25390 by limiting the rule to `try` statements with `except` handlers. Either an `except` or a `finally` clause is required, so this avoids emitting diagnostics for context-manager-like cases, such as the one reported in the issue: ```py session = ... try: print() print() print() print() print() print() finally: session.close() ``` where the `try` is just ensuring that some cleanup is performed and can't actually trigger the bad behavior described in the rule docs of mistakenly catching the wrong exception. Test Plan -- New mdtests based on the issues
1 parent f9ba228 commit 2110d32

4 files changed

Lines changed: 67 additions & 19 deletions

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
# `too-many-statements-in-try-clause` (`PLW0717`)
2+
3+
```toml
4+
[lint]
5+
preview = true
6+
select = ["PLW0717"]
7+
```
8+
9+
## Diagnostic range
10+
11+
Avoid marking the entire `try` statement:
12+
13+
```py
14+
# snapshot: too-many-statements-in-try-clause
15+
try:
16+
call1()
17+
call2()
18+
call3()
19+
call4()
20+
call5()
21+
call6()
22+
except:
23+
...
24+
```
25+
26+
```snapshot
27+
error[PLW0717]: Try clause contains too many statements (6 > 5)
28+
--> src/mdtest_snippet.py:2:1
29+
|
30+
2 | try:
31+
| ^^^
32+
|
33+
```
34+
35+
## Context managers
36+
37+
Avoid emitting a diagnostic when the `try` statement is being used like a context manager, i.e. it
38+
catches no exceptions and just ensures that some kind of cleanup is run in a `finally` clause:
39+
40+
```py
41+
try:
42+
call1()
43+
call2()
44+
call3()
45+
call4()
46+
call5()
47+
call6()
48+
finally:
49+
cleanup()
50+
```

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1341,8 +1341,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
13411341
if checker.is_rule_enabled(Rule::TooManyStatementsInTryClause) {
13421342
pylint::rules::too_many_try_statements(
13431343
checker,
1344-
stmt,
1345-
body,
1344+
try_stmt,
13461345
checker.settings().pylint.max_statements_in_try,
13471346
);
13481347
}

crates/ruff_linter/src/rules/pylint/rules/too_many_try_statements.rs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use ruff_macros::{ViolationMetadata, derive_message_formats};
2-
use ruff_python_ast::Stmt;
3-
use ruff_python_ast::identifier::Identifier;
2+
use ruff_python_ast::StmtTry;
3+
use ruff_text_size::{Ranged, TextLen, TextRange};
44

55
use crate::Violation;
66

@@ -89,18 +89,23 @@ impl Violation for TooManyStatementsInTryClause {
8989
/// W0717
9090
pub(crate) fn too_many_try_statements(
9191
checker: &Checker,
92-
stmt: &Stmt,
93-
body: &[Stmt],
92+
try_stmt: &StmtTry,
9493
max_statements: usize,
9594
) {
96-
let statements = num_statements(body);
95+
// Ignore cases with only `finally` clauses and no `except` handlers. These can't accidentally
96+
// catch the wrong exception and are instead being used more like context managers.
97+
if try_stmt.handlers.is_empty() {
98+
return;
99+
}
100+
101+
let statements = num_statements(&try_stmt.body);
97102
if statements > max_statements {
98103
checker.report_diagnostic(
99104
TooManyStatementsInTryClause {
100105
statements,
101106
max_statements,
102107
},
103-
stmt.identifier(),
108+
TextRange::at(try_stmt.start(), "try".text_len()),
104109
);
105110
}
106111
}

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0717_too_many_try_statements.py.snap

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,10 @@ source: crates/ruff_linter/src/rules/pylint/mod.rs
44
PLW0717 Try clause contains too many statements (6 > 5)
55
--> too_many_try_statements.py:63:1
66
|
7-
61 | print()
7+
61 | print()
88
62 |
9-
63 | / try: # Too many try statements (6/5)
10-
64 | | print()
11-
65 | | print()
12-
66 | | print()
13-
67 | | print()
14-
68 | | print()
15-
69 | | print()
16-
70 | | except Exception:
17-
71 | | raise
18-
| |_________^
9+
63 | try: # Too many try statements (6/5)
10+
| ^^^
11+
64 | print()
12+
65 | print()
1913
|

0 commit comments

Comments
 (0)