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.
This commit is contained in:
Charlie Marsh 2023-10-26 08:33:54 -07:00 committed by GitHub
parent 317d3dd612
commit be3307e9a6
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 67 additions and 16 deletions

View file

@ -79,3 +79,6 @@ from ZeroDivisionError
raise IndexError() from ZeroDivisionError raise IndexError() from ZeroDivisionError
raise IndexError(); raise IndexError();
# RSE102
raise Foo()

View file

@ -1,6 +1,7 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr}; use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::BindingKind;
use ruff_text_size::Ranged; use ruff_text_size::Ranged;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -15,6 +16,13 @@ use crate::checkers::ast::Checker;
/// ///
/// Removing the parentheses makes the code more concise. /// 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 /// ## Example
/// ```python /// ```python
/// raise TypeError() /// raise TypeError()
@ -54,17 +62,23 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr:
if arguments.is_empty() { if arguments.is_empty() {
// `raise func()` still requires parentheses; only `raise Class()` does not. // `raise func()` still requires parentheses; only `raise Class()` does not.
if checker let exception_type = if let Some(id) = checker.semantic().lookup_attribute(func) {
.semantic() match checker.semantic().binding(id).kind {
.lookup_attribute(func) BindingKind::FunctionDefinition(_) => return,
.is_some_and(|id| checker.semantic().binding(id).kind.is_function_definition()) BindingKind::ClassDefinition(_) => Some(ExceptionType::Class),
{ BindingKind::Builtin => Some(ExceptionType::Builtin),
return; _ => None,
} }
} else {
None
};
// `ctypes.WinError()` is a function, not a class. It's part of the standard library, so // `ctypes.WinError()` is a function, not a class. It's part of the standard library, so
// we might as well get it right. // we might as well get it right.
if checker if exception_type
.as_ref()
.is_some_and(ExceptionType::is_builtin)
&& checker
.semantic() .semantic()
.resolve_call_path(func) .resolve_call_path(func)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["ctypes", "WinError"])) .is_some_and(|call_path| matches!(call_path.as_slice(), ["ctypes", "WinError"]))
@ -73,6 +87,7 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr:
} }
let mut diagnostic = Diagnostic::new(UnnecessaryParenOnRaiseException, arguments.range()); let mut diagnostic = Diagnostic::new(UnnecessaryParenOnRaiseException, arguments.range());
// If the arguments are immediately followed by a `from`, insert whitespace to avoid // If the arguments are immediately followed by a `from`, insert whitespace to avoid
// a syntax error, as in: // a syntax error, as in:
// ```python // ```python
@ -85,13 +100,25 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr:
.next() .next()
.is_some_and(char::is_alphanumeric) .is_some_and(char::is_alphanumeric)
{ {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( diagnostic.set_fix(if exception_type.is_some() {
" ".to_string(), Fix::safe_edit(Edit::range_replacement(" ".to_string(), arguments.range()))
arguments.range(),
)));
} else { } else {
diagnostic.set_fix(Fix::safe_edit(Edit::range_deletion(arguments.range()))); Fix::unsafe_edit(Edit::range_replacement(" ".to_string(), arguments.range()))
});
} else {
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); checker.diagnostics.push(diagnostic);
} }
} }
#[derive(Debug, is_macro::Is)]
enum ExceptionType {
Class,
Builtin,
}

View file

@ -238,6 +238,7 @@ RSE102.py:79:17: RSE102 [*] Unnecessary parentheses on raised exception
79 |+raise IndexError from ZeroDivisionError 79 |+raise IndexError from ZeroDivisionError
80 80 | 80 80 |
81 81 | raise IndexError(); 81 81 | raise IndexError();
82 82 |
RSE102.py:81:17: RSE102 [*] Unnecessary parentheses on raised exception 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 | 80 |
81 | raise IndexError(); 81 | raise IndexError();
| ^^ RSE102 | ^^ RSE102
82 |
83 | # RSE102
| |
= help: Remove unnecessary parentheses = help: Remove unnecessary parentheses
@ -254,5 +257,23 @@ RSE102.py:81:17: RSE102 [*] Unnecessary parentheses on raised exception
80 80 | 80 80 |
81 |-raise IndexError(); 81 |-raise IndexError();
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