astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
28.67k stars 928 forks source link

RSE102 error despite parentheses being necessary #5416

Closed sultur closed 12 months ago

sultur commented 1 year ago

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 https://github.com/astral-sh/ruff/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 12 months ago

This is being worked on in #5536.

sultur commented 12 months ago

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

lostcontrol commented 10 months ago

Still occurs to me in 0.0.285. Is that expected?

charliermarsh commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

charliermarsh commented 10 months ago

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

joostmeulenbeld commented 4 months ago

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 commented 4 months ago

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

charliermarsh commented 4 months ago

Fixed here: https://github.com/astral-sh/ruff/pull/10206