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

Add Final for stdlib constants #12318

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

max-muoto
Copy link
Contributor

@max-muoto max-muoto commented Jul 11, 2024

Use the following script: https://gist.github.com/max-muoto/3bdfd495d4ee48dba99c16513e855983 to add Final to all module-level standard library constants. (Used libcst as opposed to the native ast module, since it was the easiest way to preserve comments/formatting details across all modules)

Technically this script makes MY_CONSTANT: Literal[1] into MY_CONSTANT: Final[Literal[1]], but I ran the Ruff rule redundant-final-literal to fix cases of this.

Otherwise:

MY_CONSTANT = 1
MY_CONSTANT_2: int = 1

# Becomes:
MY_CONSTANT: Final = 1
MY_CONSTANT_2: Final[int] = 1

With TypeVar, ParamSpec, TypeVarTuple, and NewType excluded.

Additionally, ths this excludes the wintypes.pyi module (see commit here), adding final leads Pyright to raise a Variable not allowed in type expression warning. I'll dig a bit deeper post this going out, and possibly put up an issue on Pyright's end, since this initially seems like a bug to me.

The other commits on this PR are mostly fixing things like _TypeVar that my script didn't account, and removing an extra import of Final in typing.pyi and typing_extensions.pyi.

This comment has been minimized.

@max-muoto max-muoto marked this pull request as ready for review July 11, 2024 04:56
@max-muoto
Copy link
Contributor Author

max-muoto commented Jul 11, 2024

I think this uncovered a PyType bug, it choked on this (I think believing it was associated with the RegexFlag enum). Reverted the change for this one line: 76720ec

Edit: Same issue here.

@JelleZijlstra
Copy link
Member

We probably can't merge this with all the mysterious new error: _T? not callable. I'm not sure what's causing that, maybe the new imports messed up mypy's handling of the builtin import cycle.

This comment has been minimized.

@max-muoto
Copy link
Contributor Author

max-muoto commented Jul 11, 2024

We probably can't merge this with all the mysterious new error: _T? not callable. I'm not sure what's causing that, maybe the new imports messed up mypy's handling of the builtin import cycle.

That's from an older run, it's not occurring anymore if you look at the latest mypy primer. (I accidentally had some Final = _TypeVar cases that I had to fix manually, my script only accounted for typing.TypeVar and TypeVar)

This comment has been minimized.

@max-muoto
Copy link
Contributor Author

max-muoto commented Jul 11, 2024

Okay, consolidated the commits fixing PyType related stuff: abb0a27

I there's two issues here (the enum one mentioned above) and another with callables, I'll report both, and once they're fixed this commit can be unwinded.

This comment has been minimized.

Copy link
Contributor Author

@max-muoto max-muoto left a comment

Choose a reason for hiding this comment

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

For whatever reason one of the Literal[X] -> CONSTANT: Final = X fixed this issue: c2be29c

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/ext/autodoc/importer.py: note: In function "import_object":
+ sphinx/ext/autodoc/importer.py:183:25: error: Cannot assign to final name "TYPE_CHECKING"  [misc]
+ sphinx/ext/autodoc/importer.py:189:25: error: Cannot assign to final name "TYPE_CHECKING"  [misc]

urllib3 (https://github.com/urllib3/urllib3)
+ src/urllib3/util/ssl_.py:136: error: Cannot assign to final name "OP_NO_COMPRESSION"  [misc]
+ src/urllib3/util/ssl_.py:136: note: Error code "misc" not covered by "type: ignore" comment
+ src/urllib3/util/ssl_.py:137: error: Cannot assign to final name "OP_NO_TICKET"  [misc]
+ src/urllib3/util/ssl_.py:137: note: Error code "misc" not covered by "type: ignore" comment
+ src/urllib3/util/ssl_.py:140: error: Cannot assign to final name "PROTOCOL_TLS"  [misc]
+ src/urllib3/util/ssl_.py:140: note: Error code "misc" not covered by "type: ignore" comment
+ src/urllib3/util/ssl_.py:141: error: Cannot assign to final name "PROTOCOL_TLS_CLIENT"  [misc]
+ src/urllib3/util/ssl_.py:141: note: Error code "misc" not covered by "type: ignore" comment

mkdocs (https://github.com/mkdocs/mkdocs)
+ mkdocs/tests/__init__.py:4: error: Cannot assign to final name "_MAX_LENGTH"  [misc]

setuptools (https://github.com/pypa/setuptools)
+ setuptools/tests/contexts.py:74: error: Cannot assign to final name "ENABLE_USER_SITE"  [misc]

@AlexWaygood
Copy link
Member

It might be good to split this into two PRs: a first PR that makes changes like this, which are arguably no-brainers; we can merge this first PR quickly and easily:

- FOO: Literal[42]
+ FOO: Final = 42

And a second PR that makes changes like this, which will probably require more careful review from us:

- BAR = 42
+ BAR: Final = 42
- BAZ: int
+ BAZ: Final[int]

@max-muoto
Copy link
Contributor Author

It might be good to split this into two PRs: a first PR that makes changes like this, which are arguably no-brainers; we can merge this first PR quickly and easily:

- FOO: Literal[42]
+ FOO: Final = 42

And a second PR that makes changes like this, which will probably require more careful review from us:

- BAR = 42
+ BAR: Final = 42
- BAZ: int
+ BAZ: Final[int]

Sounds good - let me split out that first change.

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.

None yet

3 participants