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.

Sign up for GitHub

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

Allow parentheses for function calls for RSE102 #5444

Closed
wants to merge 1 commit into from
Closed

Allow parentheses for function calls for RSE102 #5444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_raise/RSE102.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,9 @@
raise AssertionError

raise AttributeError("test message")


def return_error():
return ValueError("Something")

raise return_error()
12 changes: 12 additions & 0 deletions crates/ruff/src/rules/flake8_raise/rules/unnecessary_paren_on_raise_exception.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to allow arbitrary expressions here -- this would false-negative on anything accessed through an attribute:

raise module.Error()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@charliermarsh charliermarsh Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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;
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


let scope = checker.semantic().scope();
if let Some(binding_id) = scope.get(id) {
let binding = checker.semantic().binding(binding_id);
if binding.kind.is_function_definition() {
return;
}
}
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 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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blocked by #5487


if args.is_empty() && keywords.is_empty() {
let range = match_parens(func.end(), checker.locator)
.expect("Expected call to include parentheses");
Expand Down