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

RSE102 error despite parentheses being necessary #5416

Closed
sultur opened this issue Jun 28, 2023 · 12 comments · Fixed by #5536
Closed

RSE102 error despite parentheses being necessary #5416

sultur opened this issue Jun 28, 2023 · 12 comments · Fixed by #5536
Labels
bug Something isn't working

Comments

@sultur
Copy link

sultur commented Jun 28, 2023

Hello, thank you for ruff, it's fantastic!
I encountered this small bug regarding rule RSE102.

Ruff version: ruff 0.0.274
Ruff settings: All rules turned on (ignoring a few specific ones, not relevant here).

Minimal code snippet:

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

raise return_error()

Commands to reproduce:

$ ruff bug.py
test.py:4:19: RSE102 [*] Unnecessary parentheses on raised exception
Found 1 error.
[*] 1 potentially fixable with the --fix option.
$ ruff bug.py --fix
Found 1 error (1 fixed, 0 remaining).

Resulting file after autofix:

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

raise return_error

Issue:
The line raise return_error() is diagnosed as violating RSE102 (Unecessary parentheses on raised exception), despite the parentheses being necessary.
This gets autofixed as raise return_error, which raises a TypeError (as the function isn't an exception) instead of a ValueError.


Solution idea (I took a quick glance at the changes in #2596, but I'm not well versed enough in Rust to fully understand them.):
When autofixing, instead of removing any empty parentheses at the end of raise ... lines, only attempt to remove them when they come directly after the name of a built-in exception (e.g. ValueError, TypeError, (asyncio.)?CancelledError, etc.).

So lines like

raise NotImplementedError()
raise ValueError()

can be autofixed into

raise NotImplementedError
raise ValueError

but lines like

raise SomeCustomError()
# or
raise my_error_function()

would require manual fixing.

Maybe you have some more clever solution to this than matching a long list of built-in exceptions, but I hope this helps though!

@charliermarsh charliermarsh added the bug Something isn't working label Jun 28, 2023
@charliermarsh
Copy link
Member

Thank you for the kind words :)

So, I think the issue here is that a function call (rather than a class instantiation) is being passed to the raise statement, which is not invalid per se, but does strike me as non-idiomatic (FWIW). It's totally fine to call raise MyCustomError on custom errors, they just have to be classes, per the Python docs:

If an exception class is passed, it will be implicitly instantiated by calling its constructor with no arguments.

In other words, what we'd like to do here is guard against triggering RSE102 on raise foo() where foo is a function, not a class.

@sultur
Copy link
Author

sultur commented Jun 29, 2023

I agree, the raising from a function example is a bit odd :).

I encountered this in a situation closer to the following example (perhaps a bit less odd? Still raising from a function call though):

class CustomError(Exception):
    def __init__(self, msg: str):
        self.msg = msg

    @staticmethod
    def timeout():
        return CustomError("Operation timed out!")

raise CustomError.timeout()

These kinds of instantiations of exceptions of course don't trigger RSE102 when they are called with arguments.

But if you have a way of distinguishing function calls from class instantiations with no arguments, then that would of course be the perfect solution!

@charliermarsh
Copy link
Member

charliermarsh commented Jul 5, 2023

This is being worked on in #5536.

charliermarsh added a commit that referenced this issue Jul 5, 2023
## Summary

This PR enables us to resolve attribute accesses within files, at least
for static and class methods. For example, we can now detect that this
is a function access (and avoid a false-positive):

```python
class Class:
    @staticmethod
    def error():
        return ValueError("Something")


# OK
raise Class.error()
```

Closes #5487.

Closes #5416.
@sultur
Copy link
Author

sultur commented Jul 6, 2023

Great work! Thank you for the quick response and fix. Looking forward to this in upcoming releases :)

@lostcontrol
Copy link

lostcontrol commented Aug 18, 2023

Still occurs to me in 0.0.285. Is that expected?

@charliermarsh
Copy link
Member

I think the current version only works for definitions and raises that occur in the same file. Fixing this for multi-file references will be part of a larger project. Can you post a repro for your use-case?

@lostcontrol
Copy link

I think the current version only works for definitions and raises that occur in the same file. Fixing this for multi-file references will be part of a larger project. Can you post a repro for your use-case?

Indeed, that's the problem.

custom.py

class CustomError(Exception):
    def __init__(self, msg: str):
        self.msg = msg

    @staticmethod
    def timeout():
        return CustomError("Operation timed out!")

bug.py

from backend.ruff_bug.custom import CustomError

raise CustomError().timeout()

Auto-fix will remove the method call:

bug.py

from ruff_bug.custom import CustomError

raise CustomError().timeout

@tdulcet
Copy link

tdulcet commented Aug 21, 2023

Raising ctypes.WinError() also triggers this. There is a code snippet here for example.

@charliermarsh
Copy link
Member

Wow, that's a function? That's a rough API. Anyway, we can special-case it.

@joostmeulenbeld
Copy link

An example from the python standard library: concurrent.futures.Future.exception() returns an exception raised by the called function, or None if no exception was raised.

from concurrent.futures import ThreadPoolExecutor

with ThreadPoolExecutor() as executor:
    future = executor.submit(float, "a")
    if future.exception():
        raise future.exception()  # RSE102 Unnecessary parentheses on raised exception

@Rizhiy
Copy link

Rizhiy commented Mar 2, 2024

@charliermarsh future.exception() example given above is part of the standard library and should not be linted.
Can we get a fix, please?

@charliermarsh
Copy link
Member

Fixed here: #10206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants