From be3307e9a62a4fe0fd65f6c3fcb0c86b5f0d99d6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 26 Oct 2023 08:33:54 -0700 Subject: [PATCH] Make `unnecessary-paren-on-raise-exception` an unsafe edit (#8231) ## Summary This rule is now unsafe if we can't verify that the `obj` in `raise obj()` is a class or builtin. (If we verify that it's a function, we don't raise at all, as before.) See the documentation change for motivation behind the unsafe edit. Closes https://github.com/astral-sh/ruff/issues/8228. --- .../test/fixtures/flake8_raise/RSE102.py | 3 + .../unnecessary_paren_on_raise_exception.rs | 59 ++++++++++++++----- ...ry-paren-on-raise-exception_RSE102.py.snap | 21 +++++++ 3 files changed, 67 insertions(+), 16 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_raise/RSE102.py b/crates/ruff_linter/resources/test/fixtures/flake8_raise/RSE102.py index bdf30e0a51cd5..0a750b97cb190 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_raise/RSE102.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_raise/RSE102.py @@ -79,3 +79,6 @@ def error(): raise IndexError() from ZeroDivisionError raise IndexError(); + +# RSE102 +raise Foo() diff --git a/crates/ruff_linter/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs b/crates/ruff_linter/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs index d82c00decadd6..ad8cebf7bad6e 100644 --- a/crates/ruff_linter/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs +++ b/crates/ruff_linter/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr}; +use ruff_python_semantic::BindingKind; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -15,6 +16,13 @@ use crate::checkers::ast::Checker; /// /// Removing the parentheses makes the code more concise. /// +/// ## Known problems +/// Parentheses can only be omitted if the exception is a class, as opposed to +/// a function call. This rule isn't always capable of distinguishing between +/// the two. For example, if you define a method `get_exception` that itself +/// returns an exception object, this rule will falsy mark the parentheses +/// in `raise get_exception()` as unnecessary. +/// /// ## Example /// ```python /// raise TypeError() @@ -54,25 +62,32 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr: if arguments.is_empty() { // `raise func()` still requires parentheses; only `raise Class()` does not. - if checker - .semantic() - .lookup_attribute(func) - .is_some_and(|id| checker.semantic().binding(id).kind.is_function_definition()) - { - return; - } + let exception_type = if let Some(id) = checker.semantic().lookup_attribute(func) { + match checker.semantic().binding(id).kind { + BindingKind::FunctionDefinition(_) => return, + BindingKind::ClassDefinition(_) => Some(ExceptionType::Class), + BindingKind::Builtin => Some(ExceptionType::Builtin), + _ => None, + } + } else { + None + }; // `ctypes.WinError()` is a function, not a class. It's part of the standard library, so // we might as well get it right. - if checker - .semantic() - .resolve_call_path(func) - .is_some_and(|call_path| matches!(call_path.as_slice(), ["ctypes", "WinError"])) + if exception_type + .as_ref() + .is_some_and(ExceptionType::is_builtin) + && checker + .semantic() + .resolve_call_path(func) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["ctypes", "WinError"])) { return; } let mut diagnostic = Diagnostic::new(UnnecessaryParenOnRaiseException, arguments.range()); + // If the arguments are immediately followed by a `from`, insert whitespace to avoid // a syntax error, as in: // ```python @@ -85,13 +100,25 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr: .next() .is_some_and(char::is_alphanumeric) { - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - " ".to_string(), - arguments.range(), - ))); + diagnostic.set_fix(if exception_type.is_some() { + Fix::safe_edit(Edit::range_replacement(" ".to_string(), arguments.range())) + } else { + Fix::unsafe_edit(Edit::range_replacement(" ".to_string(), arguments.range())) + }); } else { - diagnostic.set_fix(Fix::safe_edit(Edit::range_deletion(arguments.range()))); + diagnostic.set_fix(if exception_type.is_some() { + Fix::safe_edit(Edit::range_deletion(arguments.range())) + } else { + Fix::unsafe_edit(Edit::range_deletion(arguments.range())) + }); } + checker.diagnostics.push(diagnostic); } } + +#[derive(Debug, is_macro::Is)] +enum ExceptionType { + Class, + Builtin, +} diff --git a/crates/ruff_linter/src/rules/flake8_raise/snapshots/ruff_linter__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap b/crates/ruff_linter/src/rules/flake8_raise/snapshots/ruff_linter__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap index 4a87268b618c9..7c839f78592e4 100644 --- a/crates/ruff_linter/src/rules/flake8_raise/snapshots/ruff_linter__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap +++ b/crates/ruff_linter/src/rules/flake8_raise/snapshots/ruff_linter__rules__flake8_raise__tests__unnecessary-paren-on-raise-exception_RSE102.py.snap @@ -238,6 +238,7 @@ RSE102.py:79:17: RSE102 [*] Unnecessary parentheses on raised exception 79 |+raise IndexError from ZeroDivisionError 80 80 | 81 81 | raise IndexError(); +82 82 | RSE102.py:81:17: RSE102 [*] Unnecessary parentheses on raised exception | @@ -245,6 +246,8 @@ RSE102.py:81:17: RSE102 [*] Unnecessary parentheses on raised exception 80 | 81 | raise IndexError(); | ^^ RSE102 +82 | +83 | # RSE102 | = help: Remove unnecessary parentheses @@ -254,5 +257,23 @@ RSE102.py:81:17: RSE102 [*] Unnecessary parentheses on raised exception 80 80 | 81 |-raise IndexError(); 81 |+raise IndexError; +82 82 | +83 83 | # RSE102 +84 84 | raise Foo() + +RSE102.py:84:10: RSE102 [*] Unnecessary parentheses on raised exception + | +83 | # RSE102 +84 | raise Foo() + | ^^ RSE102 + | + = help: Remove unnecessary parentheses + +ℹ Suggested fix +81 81 | raise IndexError(); +82 82 | +83 83 | # RSE102 +84 |-raise Foo() + 84 |+raise Foo