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

Introduce Y052: Disallow assignments to constant values in global or class namespaces where the assignments don't have type annotations #339

Merged
merged 7 commits into from
Jan 28, 2023

Conversation

AlexWaygood
Copy link
Collaborator

@AlexWaygood AlexWaygood commented Jan 23, 2023

Following #326, we now allow constructs such as these in a stub file:

x = 1
y = "foo"
z = b"bar"

It's good that we now allow variables to have default values. However, the above constructs are problematic in a stub, as we're leaving it up to the type checker to make inferences about the types of these variables, which can have unpredictable consequences. For example, the x variable could be inferred to be of type int, Final[int], Literal[1], or Final[Literal[1]]. In a class namespace, the problem is even worse -- the eggs variable in the following snippet could reasonably be inferred as being of type str, ClassVar[str], Final[str], Literal["EGGS"], ClassVar[Literal["EGGS"]], or Final[Literal["EGGS"]]:

class Whatever:
    eggs = "EGGS"

This PR introduces Y052, enforcing type annotations for assignments to simple constants.

@@ -83,7 +83,8 @@ currently emitted:
| Y048 | Function bodies should contain exactly one statement. (Note that if a function body includes a docstring, the docstring counts as a "statement".)
| Y049 | A private `TypedDict` should be used at least once in the file in which it is defined.
| Y050 | Prefer `typing_extensions.Never` over `typing.NoReturn` for argument annotations. This is a purely stylistic choice in the name of readability.
| Y051 | Y051 detect redundant unions between `Literal` types and builtin supertypes. For example, `Literal[5]` is redundant in the union `int \| Literal[5]`, and `Literal[True]` is redundant in the union `Literal[True] \| bool`.
| Y051 | Y051 detects redundant unions between `Literal` types and builtin supertypes. For example, `Literal[5]` is redundant in the union `int \| Literal[5]`, and `Literal[True]` is redundant in the union `Literal[True] \| bool`.
Copy link
Collaborator Author

@AlexWaygood AlexWaygood Jan 23, 2023

Choose a reason for hiding this comment

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

The only change in this line is a small grammar fix: "detect" -> "detects"

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as draft January 23, 2023 17:08
@AlexWaygood AlexWaygood marked this pull request as ready for review January 23, 2023 17:10
@AlexWaygood
Copy link
Collaborator Author

The tensorflow hit is two enums here: https://github.com/python/typeshed/blob/307aadc632bd7e80f427ad22ff036ebc53a5df2c/stubs/tensorflow/tensorflow/__init__.pyi#L79-L89

Strictly speaking, these are false positives, since the type of enum members needs to be special-cased by type checkers anyway, and the semantics of enums are well established. However, I also don't think it would do a huge amount of harm to add explicit type annotations:

 class VariableSynchronization(Enum):
-    AUTO = 0
-    NONE = 1
-    ON_WRITE = 2
-    ON_READ = 3
+    AUTO: int = 0
+    NONE: int = 1
+    ON_WRITE: int = 2
+    ON_READ: int = 3

 class VariableAggregation(Enum):
-    AUTO = 0
-    NONE = 1
-    ON_WRITE = 2
-    ON_READ = 3
+    AUTO: int = 0
+    NONE: int = 1
+    ON_WRITE: int = 2
+    ON_READ: int = 3

The netaddr hit is here: https://github.com/python/typeshed/blob/307aadc632bd7e80f427ad22ff036ebc53a5df2c/stubs/netaddr/netaddr/strategy/ipv6.pyi#L11

It does this:

from netaddr.fbsocket import AF_INET6

family: Final = AF_INET6

AF_INET6 is defined as Literal[2] in netaddr.fbsocket. It would arguably be more explicit to annotate the family variable above as Literal[2] as well, so this is arguably a true positive.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Collaborator Author

I changed my mind about how acceptable the netaddr hit was; added some special-casing for Final to fix it.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Collaborator

This looks good but the enum change you suggest would cause a regression: https://mypy-play.net/?mypy=latest&python=3.10&gist=b2ec11ec1691fec01fd1054d51335357

With enum members defined as x = 3, mypy infers that .x.value is Literal[3]; with x: int = 3 it doesn't.

@JelleZijlstra
Copy link
Collaborator

Though x: Literal[3] = 3 does work.

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link

This change has no effect on typeshed. 🤖🎉

@AlexWaygood
Copy link
Collaborator Author

Okay, I added some hacky special-casing for enums so that they're excluded from the new check :(

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.

Thanks! At some point we may have to do some consensus-gathering to get aligned on how to annotate enums in stubs.

@JelleZijlstra JelleZijlstra merged commit 516317d into main Jan 28, 2023
@JelleZijlstra JelleZijlstra deleted the defaults-without-annotations branch January 28, 2023 14:09
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.

2 participants