Skip to content
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.

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

Make unnecessary-paren-on-raise-exception an unsafe edit #8231

Merged
merged 1 commit into from Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -79,3 +79,6 @@ def error():
raise IndexError() from ZeroDivisionError

raise IndexError();

# RSE102
raise Foo()
@@ -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;
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// returns an exception object, this rule will falsy mark the parentheses
/// returns an exception object, this rule will incorrectly mark the parentheses

Copy link
Member

Choose a reason for hiding this comment

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

I'm also a little confused as I thought function definitions were excluded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved in #8256, thank you!

/// in `raise get_exception()` as unnecessary.
///
/// ## Example
/// ```python
/// raise TypeError()
Expand Down Expand Up @@ -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
Expand All @@ -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,
}
Expand Up @@ -238,13 +238,16 @@ 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
|
79 | raise IndexError() from ZeroDivisionError
80 |
81 | raise IndexError();
| ^^ RSE102
82 |
83 | # RSE102
|
= help: Remove unnecessary parentheses

Expand All @@ -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