Skip to content

[flake8-simplify] Preserve original behavior for except () and bare except (SIM105) #18213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ntBre merged 3 commits into from
Jun 23, 2025
Merged

[flake8-simplify] Preserve original behavior for except () and bare except (SIM105) #18213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71f563a
fix SIM105
VascoSch92 May 20, 2025
f5f117d
import BaseException
VascoSch92 May 21, 2025
5792784
new test case and fix
VascoSch92 May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM105_0.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,16 @@ def write_models(directory, Models):
pass; \
\
#

# Regression tests for: https://github.com/astral-sh/ruff/issues/18209
try:
1 / 0
except ():
pass


BaseException = ValueError
try:
int("a")
except BaseException:
pass
45 changes: 34 additions & 11 deletions crates/ruff_linter/src/rules/flake8_simplify/rules/suppressible_exception.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,11 @@ pub(crate) fn suppressible_exception(
return;
}

let [ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, range, .. })] =
handlers
let [
ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler {
body, range, type_, ..
}),
] = handlers
else {
return;
};
Expand All @@ -115,7 +118,13 @@ pub(crate) fn suppressible_exception(
};

let exception = if handler_names.is_empty() {
"Exception".to_string()
if type_.is_none() {
Copy link
Contributor

@ntBre ntBre Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could collapse this check like the one below to

if handler_names.is_empty() && type_.is_none() {

because join on an empty Vec should also give the empty string. Maybe that's getting too clever though.

Copy link
Contributor Author

@VascoSch92 VascoSch92 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we lose one else or?

the complete snippet is

 let exception = if handler_names.is_empty() {
        if type_.is_none() {
            // case where there are no handler names provided at all
            "BaseException".to_string()
        } else {
            // case where handler names is an empty tuple
            String::new()
        }
    } else {
        handler_names.join(", ")
    };

Copy link
Contributor

@ntBre ntBre Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, I was suggesting this:

 let exception = if handler_names.is_empty() && type_.is_none() {
         "BaseException".to_string()
    } else {
        handler_names.join(", ")
    };

That does drop the middle else, but String::new() and handler_names.join(", ") are equivalent when handler_names is empty, so it's safe to combine the two else branches. However, I think this is evidence that your current code is more clear 😄

// case where there are no handler names provided at all
"BaseException".to_string()
Copy link

@dscorbett dscorbett May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won’t work if BaseException is shadowed. Here is an example of how to safely insert a builtin symbol:

ruff/crates/ruff_linter/src/rules/flake8_bugbear/rules/unreliable_callable_check.rs

Lines 78 to 82 in b35bf8a

let (import_edit, binding) = checker.importer().get_or_import_builtin_symbol(
"callable",
expr.start(),
checker.semantic(),
)?;

} else {
// case where handler names is an empty tuple
String::new()
}
} else {
handler_names.join(", ")
};
Expand All @@ -136,15 +145,29 @@ pub(crate) fn suppressible_exception(
stmt.start(),
checker.semantic(),
)?;
let replace_try = Edit::range_replacement(
format!("with {binding}({exception})"),
let mut rest: Vec<Edit> = Vec::new();
let content: String;
if exception == "BaseException" && handler_names.is_empty() {
let (import_exception, binding_exception) =
checker.importer().get_or_import_symbol(
&ImportRequest::import("builtins", &exception),
stmt.start(),
checker.semantic(),
)?;
content = format!("with {binding}({binding_exception})");
rest.push(import_exception);
} else {
content = format!("with {binding}({exception})");
}
rest.push(Edit::range_deletion(
checker.locator().full_lines_range(*range),
));
rest.push(Edit::range_replacement(
content,
TextRange::at(stmt.start(), "try".text_len()),
);
let remove_handler = Edit::range_deletion(checker.locator().full_lines_range(*range));
Ok(Fix::unsafe_edits(
import_edit,
[replace_try, remove_handler],
))
));

Ok(Fix::unsafe_edits(import_edit, rest))
});
}
checker.report_diagnostic(diagnostic);
Expand Down
83 changes: 70 additions & 13 deletions ...e8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM105_SIM105_0.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ SIM105_0.py:19:1: SIM105 [*] Use `contextlib.suppress(ValueError, OSError)` inst
24 23 | # SIM105
25 24 | try:

SIM105_0.py:25:1: SIM105 [*] Use `contextlib.suppress(Exception)` instead of `try`-`except`-`pass`
SIM105_0.py:25:1: SIM105 [*] Use `contextlib.suppress(BaseException)` instead of `try`-`except`-`pass`
|
24 | # SIM105
25 | / try:
Expand All @@ -101,25 +101,26 @@ SIM105_0.py:25:1: SIM105 [*] Use `contextlib.suppress(Exception)` instead of `tr
29 |
30 | # SIM105
|
= help: Replace with `contextlib.suppress(Exception)`
= help: Replace with `contextlib.suppress(BaseException)`

ℹ Unsafe fix
1 |+import contextlib
1 2 | def foo():
2 3 | pass
3 4 |
2 |+import builtins
1 3 | def foo():
2 4 | pass
3 5 |
--------------------------------------------------------------------------------
22 23 | pass
23 24 |
24 25 | # SIM105
22 24 | pass
23 25 |
24 26 | # SIM105
25 |-try:
26 |+with contextlib.suppress(Exception):
26 27 | foo()
27 |+with contextlib.suppress(builtins.BaseException):
26 28 | foo()
27 |-except:
28 |- pass
29 28 |
30 29 | # SIM105
31 30 | try:
29 29 |
30 30 | # SIM105
31 31 | try:

SIM105_0.py:31:1: SIM105 [*] Use `contextlib.suppress(a.Error, b.Error)` instead of `try`-`except`-`pass`
|
Expand Down Expand Up @@ -285,3 +286,59 @@ SIM105_0.py:126:5: SIM105 [*] Use `contextlib.suppress(OSError)` instead of `try
127 |+ with contextlib.suppress(OSError): os.makedirs(model_dir);
129 128 | \
130 129 | #
131 130 |

SIM105_0.py:133:1: SIM105 [*] Use `contextlib.suppress()` instead of `try`-`except`-`pass`
|
132 | # Regression tests for: https://github.com/astral-sh/ruff/issues/18209
133 | / try:
134 | | 1 / 0
135 | | except ():
136 | | pass
| |________^ SIM105
|
= help: Replace with `contextlib.suppress()`

ℹ Unsafe fix
1 |+import contextlib
1 2 | def foo():
2 3 | pass
3 4 |
--------------------------------------------------------------------------------
130 131 | #
131 132 |
132 133 | # Regression tests for: https://github.com/astral-sh/ruff/issues/18209
133 |-try:
134 |+with contextlib.suppress():
134 135 | 1 / 0
135 |-except ():
136 |- pass
137 136 |
138 137 |
139 138 | BaseException = ValueError

SIM105_0.py:140:1: SIM105 [*] Use `contextlib.suppress(BaseException)` instead of `try`-`except`-`pass`
|
139 | BaseException = ValueError
140 | / try:
141 | | int("a")
142 | | except BaseException:
143 | | pass
| |________^ SIM105
|
= help: Replace with `contextlib.suppress(BaseException)`

ℹ Unsafe fix
1 |+import contextlib
1 2 | def foo():
2 3 | pass
3 4 |
--------------------------------------------------------------------------------
137 138 |
138 139 |
139 140 | BaseException = ValueError
140 |-try:
141 |+with contextlib.suppress(BaseException):
141 142 | int("a")
142 |-except BaseException:
143 |- pass
Loading