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

Added checks for invalid usage of continue/break/return in except* block #18132

Merged
merged 8 commits into from
Nov 21, 2024

Conversation

coldwolverine
Copy link
Contributor

Fixes #18123

This PR addresses an issue where mypy incorrectly allows break/continue/return statements in the except* block. (see https://peps.python.org/pep-0654/#forbidden-combinations)

This comment has been minimized.

Copy link
Collaborator

@brianschubert brianschubert left a comment

Choose a reason for hiding this comment

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

Nice! This looks good overall.

A few comments for you to consider, plus one issue that needs to be fixed in the tests:

mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
test-data/unit/check-except-star.test Outdated Show resolved Hide resolved
test-data/unit/check-except-star.test Outdated Show resolved Hide resolved

This comment has been minimized.

@coldwolverine
Copy link
Contributor Author

Thanks for the comments! I've pushed a commit with all the changes.

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor comments.

mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
Comment on lines 227 to 229
from typing import Sequence
class range(Sequence[int]):
def __init__(self, __x: int, __y: int = ..., __z: int = ...) -> None: pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be replaced by a dummy list?

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

hauntsaninja pushed a commit that referenced this pull request Nov 14, 2024
The vast majority of mypy's code follows PEP 8 naming conventions.
However, we currently don't enforce this in the linter config. I noticed
this in a recent PR which included a `camelCase` name:
#18132 (comment).

Ruff has some rules to enforce PEP 8 naming style
([pep8-naming](https://docs.astral.sh/ruff/rules/#pep8-naming-n)). I
think it would be a good idea to enable some of these to help
contributors catch naming discrepancies before PR review.

We have a few notable exceptions to PEP 8 naming (e.g. functions named
to match ast node names), but these are easily accounted for with some
config file ignores and handful of `# noqa`'s.
@coldwolverine
Copy link
Contributor Author

Is this PR ready to merge?

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the updates!

@JukkaL JukkaL merged commit 499adae into python:master Nov 21, 2024
19 checks passed
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.

mypy doesn't flag continue/break/return in except*
3 participants