Allow parentheses for function calls for RSE102
#5444
Conversation
RSE102
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
| @@ -52,6 +52,18 @@ pub(crate) fn unnecessary_paren_on_raise_exception(checker: &mut Checker, expr: | |||
| range: _, | |||
| }) = expr | |||
| { | |||
| let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { | |||
| return; | |||
| }; | |||
There was a problem hiding this comment.
I think we need to allow arbitrary expressions here -- this would false-negative on anything accessed through an attribute:
raise module.Error()There was a problem hiding this comment.
What would be the best way to go about this, style and performance wise? Pattern matching against all possible expressions or locate the call by range and get the id from the string representation of that?
There was a problem hiding this comment.
I'd probably do like:
if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() {
if let Some(binding_id) = checker.semantic().lookup(id) {
let binding = checker.semantic().binding(binding_id);
if binding.kind.is_function_definition() {
return;
}
}
}There was a problem hiding this comment.
Ah check. I see what you mean now. That snippet complains about:
error[E0596]: cannot borrow data in a `&` reference as mutable
--> crates/ruff/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs:56:39
|
56 | if let Some(binding_id) = checker.semantic().lookup(id) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
But I'll fix that and push changes. Thanks!
There was a problem hiding this comment.
Hmm that's in fn lookup(&mut self, symbol: &str) by the looks of it. And it's implementation can't be taken over cause global() is private. I'll keep the structure but use the previous logic for now. Let me know if there's a better fix for the above
| if binding.kind.is_function_definition() { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not opposed to adding this, since it seems like an improvement over what we're doing now, but it's worth noting that it will miss a lot of cases, including the case from the linked issue, where they had a method within a class:
class CustomError(Exception):
def __init__(self, msg: str):
self.msg = msg
@staticmethod
def timeout():
return CustomError("Operation timed out!")
raise CustomError.timeout()There was a problem hiding this comment.
I might have misunderstood, but wasn't the point of the issue that we wanted to not flag the above snippet? Or do we want to flag it as a violation but just not autofix it? If the latter, do we have precedent for conditional fixing? Or would it be better to just keep flagging broadly but make the autofix suggested or manual?
There was a problem hiding this comment.
That's right -- we don't want to flag the above. But, I think the fix you have here would work for the wrong reason, which is that it's now avoiding flagging attribute accesses (a.b()).
There was a problem hiding this comment.
Clear, thanks for clarifying. Been trying to implement logic, which should only flag class method calls and not other attributes with the following:
match func.as_ref() {
// Exclude function calls
Expr::Name(ast::ExprName { id, .. }) => {
let scope = checker.semantic().scope();
if id_is_function_definition(checker, scope, id) {
return;
}
}
// Exclude Class method calls
Expr::Attribute(ast::ExprAttribute { attr, value, .. }) => {
if let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() {
if let Some((scope_id, _)) = checker.semantic().nonlocal(id) {
let scope = &checker.semantic().scopes[scope_id];
if id_is_function_definition(checker, scope, attr.as_str()) {
return;
}
}
}
}
_ => (),
};
...
fn id_is_function_definition(checker: &Checker, scope: &Scope, id: &str) -> bool {
if let Some(binding_id) = scope.get(id) {
let binding = checker.semantic().binding(binding_id);
return binding.kind.is_function_definition();
}
false
}But that's still flagging the scenario where:
class CustomError(Exception):
def __init__(self, msg: str):
self.msg = msg
@staticmethod
def timeout():
return CustomError("Operation timed out!")
raise CustomError.timeout()I'm guessing my estimation of how the scopes work is off. Any input on the most efficient/clear way to check whether the Expr::Attribute relates to a method call or a different attribute?
No worries at all. If I'd known about the semantic model limits and the Attribute vs Method niggle beforehand, I probably would not have committed myself as quick anyway. Lots learned and happy to see an elegant fix for the issue through your PR. |
Summary
Edit RSE102 to allow empty parentheses if the call is a function that raises an error (and not e.g. a Class instantiation) so that we allow scenarios like:
and we don't break the code during autofix into:
Test Plan
Added an extra case to the existing fixture
Issue link:
Closes: #5416