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

Improve error messages for except* (B025, B029, B030, B904) #14791 #14815

Conversation

smokyabdulrahman
Copy link
Contributor

@smokyabdulrahman smokyabdulrahman commented Dec 6, 2024

Improves error message for except* (Rules: B025, B029, B030, B904)

Example python snippet:

try:
    a = 1
except* ValueError:
    a = 2
except* ValueError:
    a = 2

try:
    pass
except* ():
    pass

try:
    pass
except* 1:  # error
    pass

try:
    raise ValueError
except* ValueError:
    raise UserWarning

Error messages
Before:

$ ruff check --select=B foo.py
foo.py:6:9: B025 try-except block with duplicate exception `ValueError`
foo.py:11:1: B029 Using `except ():` with an empty tuple does not catch anything; add exceptions to handle
foo.py:16:9: B030 `except` handlers should only be exception classes or tuples of exception classes
foo.py:22:5: B904 Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
Found 4 errors.

After:

$ ruff check --select=B foo.py
foo.py:6:9: B025 try-except* block with duplicate exception `ValueError`
foo.py:11:1: B029 Using `except* ():` with an empty tuple does not catch anything; add exceptions to handle
foo.py:16:9: B030 `except*` handlers should only be exception classes or tuples of exception classes
foo.py:22:5: B904 Within an `except*` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling
Found 4 errors.

Closes #14791

@charliermarsh
Copy link
Member

If you're able, can you run cargo test -p ruff_linter --lib and then cargo insta accept to update the checked-in snapshots? You'll need to run cargo install cargo-insta first.

@smokyabdulrahman
Copy link
Contributor Author

If you're able, can you run cargo test -p ruff_linter --lib and then cargo insta accept to update the checked-in snapshots? You'll need to run cargo install cargo-insta first.

Yes, sure thing. I've ran the tests locally before committing. However, I didn't know how to accept my test modifications.

Thank you 👌 I will be doing that with in the next commit.

@smokyabdulrahman
Copy link
Contributor Author

BTW @charliermarsh

Since I am going to fix a couple more rules also. (B025, B029, B030, B904)

Does it make sense to do the following change in the statement parser:

            ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler {
                type_,
                name,
                body: except_body,
                range: self.node_range(start),
                in_trystar: block_kind.is_star(), <---- `try` block kind
            }),

so that in_trystar is accessible in all the rules handlers.
or is that an overkill?

@charliermarsh
Copy link
Member

@smokyabdulrahman -- I'd prefer not to change the parser / AST, if possible.

Copy link
Contributor

github-actions bot commented Dec 6, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@smokyabdulrahman smokyabdulrahman force-pushed the fix/various-rules-refer-to-except-star-as-except-14791 branch from b075f68 to e57470a Compare December 6, 2024 19:58
@smokyabdulrahman smokyabdulrahman marked this pull request as ready for review December 6, 2024 20:40
@smokyabdulrahman smokyabdulrahman changed the title WIP: Improve error messages for except* #14791 Improve error messages for except* (B025, B029, B030, B904) #14791 Dec 6, 2024
@dylwil3 dylwil3 added the diagnostics Related to reporting of diagnostics. label Dec 6, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichaReiser MichaReiser enabled auto-merge (squash) December 8, 2024 17:33
Comment on lines +215 to +219
let is_star = checker
.semantic()
.current_statement()
.as_try_stmt()
.is_some_and(|try_stmt| try_stmt.is_star);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for learning purposes. @MichaReiser
Why was this moved inside the diagnostic-branch?

I thought calculating it outside the loop once would be better.

@MichaReiser MichaReiser merged commit 8540209 into astral-sh:main Dec 8, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostics Related to reporting of diagnostics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Various rules refer to except* as except
4 participants