Do not raise SIM105 for non-exceptions (#5985)

Closes #5977 Added a test case from `refurb`
astral-sh · Jul 22, 2023 · ed7d2b8 · ed7d2b8
1 parent c7e4c58
commit ed7d2b8
Show file tree

Hide file tree

Showing 6 changed files with 170 additions and 107 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM105_0.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM105_0.py
@@ -14,6 +14,12 @@ def foo():
 except (ValueError, OSError):
     pass
 
+# SIM105
+try:
+    foo()
+except (ValueError, OSError) as e:
+    pass
+
 # SIM105
 try:
     foo()
@@ -94,3 +100,13 @@ def with_comment():
         foo()
     except (ValueError, OSError):
         pass  # Trailing comment.
+
+try:
+    print()
+except ("not", "an", "exception"):
+    pass
+
+try:
+    print()
+except "not an exception":
+    pass
diff --git a/crates/ruff/src/rules/flake8_simplify/rules/suppressible_exception.rs b/crates/ruff/src/rules/flake8_simplify/rules/suppressible_exception.rs
@@ -63,6 +63,20 @@ impl Violation for SuppressibleException {
     }
 }
 
+fn is_empty(body: &[Stmt]) -> bool {
+    match body {
+        [Stmt::Pass(_)] => true,
+        [Stmt::Expr(ast::StmtExpr { value, range: _ })] => matches!(
+            value.as_ref(),
+            Expr::Constant(ast::ExprConstant {
+                value: Constant::Ellipsis,
+                ..
+            })
+        ),
+        _ => false,
+    }
+}
+
 /// SIM105
 pub(crate) fn suppressible_exception(
     checker: &mut Checker,
@@ -83,61 +97,62 @@ pub(crate) fn suppressible_exception(
             | Stmt::ImportFrom(_)
             | Stmt::Expr(_)
             | Stmt::Pass(_)]
-    ) || handlers.len() != 1
-        || !orelse.is_empty()
+    ) || !orelse.is_empty()
         || !finalbody.is_empty()
     {
         return;
     }
-    let handler = &handlers[0];
-    let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler;
-    if body.len() == 1 {
-        let node = &body[0];
-        if node.is_pass_stmt()
-            || (matches!(
-            node,
-            Stmt::Expr(ast::StmtExpr { value, range: _ })
-            if matches!(**value, Expr::Constant(ast::ExprConstant { value: Constant::Ellipsis, .. }))
-            ))
-        {
-            let handler_names: Vec<String> = helpers::extract_handled_exceptions(handlers)
-                .into_iter()
-                .filter_map(compose_call_path)
-                .collect();
-            let exception = if handler_names.is_empty() {
-                "Exception".to_string()
-            } else {
-                handler_names.join(", ")
-            };
 
-            let mut diagnostic = Diagnostic::new(
-                SuppressibleException {
-                    exception: exception.clone(),
-                },
-                stmt.range(),
-            );
-            if checker.patch(diagnostic.kind.rule()) {
-                if !has_comments(stmt, checker.locator(), checker.indexer()) {
-                    diagnostic.try_set_fix(|| {
-                        let (import_edit, binding) = checker.importer().get_or_import_symbol(
-                            &ImportRequest::import("contextlib", "suppress"),
-                            stmt.start(),
-                            checker.semantic(),
-                        )?;
-                        let replace_try = Edit::range_replacement(
-                            format!("with {binding}({exception})"),
-                            TextRange::at(stmt.start(), "try".text_len()),
-                        );
-                        let handler_line_begin = checker.locator().line_start(handler.start());
-                        let remove_handler = Edit::deletion(handler_line_begin, handler.end());
-                        Ok(Fix::suggested_edits(
-                            import_edit,
-                            [replace_try, remove_handler],
-                        ))
-                    });
-                }
-            }
-            checker.diagnostics.push(diagnostic);
+    let [ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, range, .. })] =
+        handlers
+    else {
+        return;
+    };
+
+    if !is_empty(body) {
+        return;
+    }
+
+    let Some(handler_names) = helpers::extract_handled_exceptions(handlers)
+        .into_iter()
+        .map(compose_call_path)
+        .collect::<Option<Vec<String>>>()
+    else {
+        return;
+    };
+
+    let exception = if handler_names.is_empty() {
+        "Exception".to_string()
+    } else {
+        handler_names.join(", ")
+    };
+
+    let mut diagnostic = Diagnostic::new(
+        SuppressibleException {
+            exception: exception.clone(),
+        },
+        stmt.range(),
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        if !has_comments(stmt, checker.locator(), checker.indexer()) {
+            diagnostic.try_set_fix(|| {
+                let (import_edit, binding) = checker.importer().get_or_import_symbol(
+                    &ImportRequest::import("contextlib", "suppress"),
+                    stmt.start(),
+                    checker.semantic(),
+                )?;
+                let replace_try = Edit::range_replacement(
+                    format!("with {binding}({exception})"),
+                    TextRange::at(stmt.start(), "try".text_len()),
+                );
+                let handler_line_begin = checker.locator().line_start(range.start());
+                let remove_handler = Edit::deletion(handler_line_begin, range.end());
+                Ok(Fix::suggested_edits(
+                    import_edit,
+                    [replace_try, remove_handler],
+                ))
+            });
         }
     }
+    checker.diagnostics.push(diagnostic);
 }
diff --git a/...es/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM105_SIM105_0.py.snap b/...es/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM105_SIM105_0.py.snap
@@ -65,18 +65,18 @@ SIM105_0.py:12:1: SIM105 [*] Use `contextlib.suppress(ValueError, OSError)` inst
 18 18 | try:
 19 19 |     foo()
 
-SIM105_0.py:18:1: SIM105 [*] Use `contextlib.suppress(Exception)` instead of `try`-`except`-`pass`
+SIM105_0.py:18:1: SIM105 [*] Use `contextlib.suppress(ValueError, OSError)` instead of `try`-`except`-`pass`
    |
 17 |   # SIM105
 18 | / try:
 19 | |     foo()
-20 | | except:
+20 | | except (ValueError, OSError) as e:
 21 | |     pass
    | |________^ SIM105
 22 |   
 23 |   # SIM105
    |
-   = help: Replace with `contextlib.suppress(Exception)`
+   = help: Replace with `contextlib.suppress(ValueError, OSError)`
 
 ℹ Suggested fix
    1  |+import contextlib
@@ -88,28 +88,28 @@ SIM105_0.py:18:1: SIM105 [*] Use `contextlib.suppress(Exception)` instead of `tr
 16 17 | 
 17 18 | # SIM105
 18    |-try:
-   19 |+with contextlib.suppress(Exception):
+   19 |+with contextlib.suppress(ValueError, OSError):
 19 20 |     foo()
-20    |-except:
+20    |-except (ValueError, OSError) as e:
 21    |-    pass
 22 21 | 
    22 |+
 23 23 | # SIM105
 24 24 | try:
 25 25 |     foo()
 
-SIM105_0.py:24:1: SIM105 [*] Use `contextlib.suppress(a.Error, b.Error)` instead of `try`-`except`-`pass`
+SIM105_0.py:24:1: SIM105 [*] Use `contextlib.suppress(Exception)` instead of `try`-`except`-`pass`
    |
 23 |   # SIM105
 24 | / try:
 25 | |     foo()
-26 | | except (a.Error, b.Error):
+26 | | except:
 27 | |     pass
    | |________^ SIM105
 28 |   
-29 |   # OK
+29 |   # SIM105
    |
-   = help: Replace with `contextlib.suppress(a.Error, b.Error)`
+   = help: Replace with `contextlib.suppress(Exception)`
 
 ℹ Suggested fix
    1  |+import contextlib
@@ -121,60 +121,95 @@ SIM105_0.py:24:1: SIM105 [*] Use `contextlib.suppress(a.Error, b.Error)` instead
 22 23 | 
 23 24 | # SIM105
 24    |-try:
-   25 |+with contextlib.suppress(a.Error, b.Error):
+   25 |+with contextlib.suppress(Exception):
 25 26 |     foo()
-26    |-except (a.Error, b.Error):
+26    |-except:
 27    |-    pass
 28 27 | 
    28 |+
-29 29 | # OK
+29 29 | # SIM105
 30 30 | try:
 31 31 |     foo()
 
-SIM105_0.py:78:5: SIM105 [*] Use `contextlib.suppress(ValueError)` instead of `try`-`except`-`pass`
+SIM105_0.py:30:1: SIM105 [*] Use `contextlib.suppress(a.Error, b.Error)` instead of `try`-`except`-`pass`
    |
-76 |   def with_ellipsis():
-77 |       # OK
-78 |       try:
-   |  _____^
-79 | |         foo()
-80 | |     except ValueError:
-81 | |         ...
-   | |___________^ SIM105
+29 |   # SIM105
+30 | / try:
+31 | |     foo()
+32 | | except (a.Error, b.Error):
+33 | |     pass
+   | |________^ SIM105
+34 |   
+35 |   # OK
    |
-   = help: Replace with `contextlib.suppress(ValueError)`
+   = help: Replace with `contextlib.suppress(a.Error, b.Error)`
 
 ℹ Suggested fix
    1  |+import contextlib
 1  2  | def foo():
 2  3  |     pass
 3  4  | 
 --------------------------------------------------------------------------------
-75 76 | 
-76 77 | def with_ellipsis():
-77 78 |     # OK
-78    |-    try:
-   79 |+    with contextlib.suppress(ValueError):
-79 80 |         foo()
-80    |-    except ValueError:
-81    |-        ...
-82 81 | 
-83 82 | 
-   83 |+
-84 84 | def with_ellipsis_and_return():
-85 85 |     # OK
-86 86 |     try:
+27 28 |     pass
+28 29 | 
+29 30 | # SIM105
+30    |-try:
+   31 |+with contextlib.suppress(a.Error, b.Error):
+31 32 |     foo()
+32    |-except (a.Error, b.Error):
+33    |-    pass
+34 33 | 
+   34 |+
+35 35 | # OK
+36 36 | try:
+37 37 |     foo()
 
-SIM105_0.py:93:5: SIM105 Use `contextlib.suppress(ValueError, OSError)` instead of `try`-`except`-`pass`
+SIM105_0.py:84:5: SIM105 [*] Use `contextlib.suppress(ValueError)` instead of `try`-`except`-`pass`
    |
-92 |   def with_comment():
-93 |       try:
+82 |   def with_ellipsis():
+83 |       # OK
+84 |       try:
    |  _____^
-94 | |         foo()
-95 | |     except (ValueError, OSError):
-96 | |         pass  # Trailing comment.
-   | |____________^ SIM105
+85 | |         foo()
+86 | |     except ValueError:
+87 | |         ...
+   | |___________^ SIM105
    |
-   = help: Replace with `contextlib.suppress(ValueError, OSError)`
+   = help: Replace with `contextlib.suppress(ValueError)`
+
+ℹ Suggested fix
+   1  |+import contextlib
+1  2  | def foo():
+2  3  |     pass
+3  4  | 
+--------------------------------------------------------------------------------
+81 82 | 
+82 83 | def with_ellipsis():
+83 84 |     # OK
+84    |-    try:
+   85 |+    with contextlib.suppress(ValueError):
+85 86 |         foo()
+86    |-    except ValueError:
+87    |-        ...
+88 87 | 
+89 88 | 
+   89 |+
+90 90 | def with_ellipsis_and_return():
+91 91 |     # OK
+92 92 |     try:
+
+SIM105_0.py:99:5: SIM105 Use `contextlib.suppress(ValueError, OSError)` instead of `try`-`except`-`pass`
+    |
+ 98 |   def with_comment():
+ 99 |       try:
+    |  _____^
+100 | |         foo()
+101 | |     except (ValueError, OSError):
+102 | |         pass  # Trailing comment.
+    | |____________^ SIM105
+103 |   
+104 |   try:
+    |
+    = help: Replace with `contextlib.suppress(ValueError, OSError)`
 
 
diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_default.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_default.rs
@@ -92,10 +92,10 @@ pub(crate) fn invalid_envvar_default(
         })
     {
         // Find the `default` argument, if it exists.
-        let Some(expr) = args.get(1).or_else(|| {
-            find_keyword(keywords, "default")
-                .map(|keyword| &keyword.value)
-        }) else {
+        let Some(expr) = args
+            .get(1)
+            .or_else(|| find_keyword(keywords, "default").map(|keyword| &keyword.value))
+        else {
             return;
         };
 

diff --git a/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs b/crates/ruff/src/rules/pylint/rules/invalid_envvar_value.rs
@@ -89,10 +89,10 @@ pub(crate) fn invalid_envvar_value(
         })
     {
         // Find the `key` argument, if it exists.
-        let Some(expr) = args.get(0).or_else(|| {
-            find_keyword(keywords, "key")
-                .map(|keyword| &keyword.value)
-        }) else {
+        let Some(expr) = args
+            .get(0)
+            .or_else(|| find_keyword(keywords, "key").map(|keyword| &keyword.value))
+        else {
             return;
         };
 

diff --git a/crates/ruff/src/rules/pylint/rules/type_bivariance.rs b/crates/ruff/src/rules/pylint/rules/type_bivariance.rs
@@ -84,14 +84,11 @@ pub(crate) fn type_bivariance(checker: &mut Checker, value: &Expr) {
         return;
     };
 
-    let Some(covariant) = find_keyword(keywords, "covariant")
-        .map(|keyword| &keyword.value)
-    else {
+    let Some(covariant) = find_keyword(keywords, "covariant").map(|keyword| &keyword.value) else {
         return;
     };
 
-    let Some(contravariant) = find_keyword(keywords, "contravariant")
-        .map(|keyword| &keyword.value)
+    let Some(contravariant) = find_keyword(keywords, "contravariant").map(|keyword| &keyword.value)
     else {
         return;
     };