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

Invalid autofix for RSE102 #8228

Closed
Geo5 opened this issue Oct 25, 2023 · 4 comments · Fixed by #8231
Closed

Invalid autofix for RSE102 #8228

Geo5 opened this issue Oct 25, 2023 · 4 comments · Fixed by #8231
Assignees
Labels
bug Something isn't working

Comments

@Geo5
Copy link

Geo5 commented Oct 25, 2023

In the following simple example test_ruff.py:

from typing import NoReturn


class SomeObject:
    def get_error(self) -> ValueError:
        return ValueError("some error")


def testfun() -> NoReturn:
    obj = SomeObject()

    raise obj.get_error()


testfun()

When run:

$ python test_ruff.py 
Traceback (most recent call last):
  File ..., line 15, in <module>
    testfun()
  File ..., line 12, in testfun
    raise obj.get_error()
ValueError: some error

ruff reports the error RSE102 and suggests fixing it, producing:

$ ruff check --isolated --select RSE102 --fix --diff test_ruff.py
--- test_ruff.py
+++ test_ruff.py
@@ -9,7 +9,7 @@
 def testfun() -> NoReturn:
     obj = SomeObject()
 
-    raise obj.get_error()
+    raise obj.get_error
 
 
 testfun()

Would fix 1 error.

Which changes the behaviour when run from the original code:

$ python test_ruff.py 
Traceback (most recent call last):
  File ..., line 15, in <module>
    testfun()
  File ..., line 12, in testfun
    raise obj.get_error
TypeError: exceptions must derive from BaseException
@charliermarsh
Copy link
Member

Ahh yeah, I believe this is similar to #5416 -- we just can't detect this reliably in all cases.

@charliermarsh
Copy link
Member

We should mark this as an unsafe edit, so that users have to opt in to apply it.

@Geo5
Copy link
Author

Geo5 commented Oct 25, 2023

Ahh yeah, I believe this is similar to #5416 -- we just can't detect this reliably in all cases.

I agree.
For some reason i must have misused the Github search, so i did not find this issue before - sorry!

@charliermarsh
Copy link
Member

No worries! Thanks for reporting, since we did have at least one change to make here.

charliermarsh added a commit that referenced this issue Oct 26, 2023
## Summary

This rule is now unsafe if we can't verify that the `obj` in `raise
obj()` is a class or builtin. (If we verify that it's a function, we
don't raise at all, as before.)

See the documentation change for motivation behind the unsafe edit.

Closes #8228.
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
Development

Successfully merging a pull request may close this issue.

2 participants