Skip to content

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

Merged
MichaReiser merged 8 commits intoastral-sh:mainfrom
smokyabdulrahman:fix/various-rules-refer-to-except-star-as-except-14791
Dec 8, 2024
Merged

Improve error messages for except* (B025, B029, B030, B904) #14791#14815
MichaReiser merged 8 commits intoastral-sh:mainfrom
smokyabdulrahman:fix/various-rules-refer-to-except-star-as-except-14791

Conversation

@smokyabdulrahman
Copy link
Contributor

@smokyabdulrahman smokyabdulrahman commented Dec 6, 2024 •
edited
Loading

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

charliermarsh commented Dec 6, 2024

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

smokyabdulrahman commented Dec 6, 2024

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

smokyabdulrahman commented Dec 6, 2024

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

charliermarsh commented Dec 6, 2024

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2024 •
edited
Loading

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
@smokyabdulrahman smokyabdulrahman mentioned this pull request Dec 6, 2024
Closed
@dylwil3 dylwil3 added the diagnostics Related to reporting of diagnostics. label Dec 6, 2024
MichaReiser
MichaReiser reviewed Dec 7, 2024
View reviewed changes
MichaReiser
MichaReiser approved these changes Dec 8, 2024
View reviewed changes
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
smokyabdulrahman
smokyabdulrahman commented Dec 8, 2024
View reviewed changes
crates/ruff_linter/src/rules/flake8_bugbear/rules/duplicate_exceptions.rs
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

@smokyabdulrahman smokyabdulrahman Dec 8, 2024

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
@BrewTestBot BrewTestBot mentioned this pull request Dec 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

No one assigned

Labels

diagnostics Related to reporting of diagnostics.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Various rules refer to except* as except

4 participants

@smokyabdulrahman @charliermarsh @MichaReiser @dylwil3