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

Fix bug in b030 #364

Merged
merged 3 commits into from
Mar 9, 2023
Merged

Fix bug in b030 #364

merged 3 commits into from
Mar 9, 2023

Conversation

aacunningham
Copy link
Contributor

I was working in ruff implementing B030 when I noticed that the test case for the warning didn't look right. It may be that another rule catches this issue, but it made sense to me that B030 would catch it as well.

try:
    raise ValueError()
except (ValueError, (RuntimeError, (KeyError, TypeError))):
    pass

This code causes an exception, TypeError: catching classes that do not inherit from BaseException is not allowed, because the inner tuple isn't expanded out automatically. It passes, however, if those tuples are starred:

try:
    raise ValueError()
except (ValueError, *(RuntimeError, *(KeyError, TypeError))):
    pass

The wording for B030 makes it sound like this is the exact error it's meant to catch, so I updated the _flatten_excepthandler method to be more selective when flattening tuples and arrays. In the updated code, the (RuntimeError, ...) tuple is returned fully, which then gets it placed in the bad_handlers list.

Let me know if there's anything I missed, any improvements I can make, or if I should have started this convo in an issue. I had already gotten this logic working in Rust, so I tried to port it into python and plug it in to see if I could get the same result. Work was already done, so I figured a PR was a decent way to present it.

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I actually started writing this in order to allow nested tuples, because I somehow convinced myself that it worked, but you're right that it actually doesn't work. Not sure what I was thinking.

@aacunningham
Copy link
Contributor Author

Honestly if it wasn't for the star example on line 23, I wouldn't have even thought twice about it. But then I saw that and then looked back up and asked "wait are we sure?".

Ran it in the interpreter 10ish times double checking it, lololol.

All in all, feels like an easy thing to miss (and tbh, I fear for the codebase that's writing out their except lines like this)

@aacunningham
Copy link
Contributor Author

aacunningham commented Mar 8, 2023

Also, it was getting late here when I wrote this up, but just a quick thanks for the project in the first place. Really appreciate y'all's work, and it's been a nice hands on introduction to the ast module (the one my coworkers auto-import from instead of typing ;) )

@cooperlees cooperlees merged commit 37bd2a0 into PyCQA:main Mar 9, 2023
@aacunningham
Copy link
Contributor Author

Thanks for the approvals/merge, y'all!

@aacunningham aacunningham deleted the fix-bug-in-b030 branch March 14, 2023 17:01
@cooperlees
Copy link
Collaborator

Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants