-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ty] Add invalid-enum-member-annotation lint rule
#23648
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
Changes from all commits
21c223b
164141b
7d902f3
f9dbb60
2b55e21
a0d77a6
aa80329
63bd568
3723b91
efd2d3f
af51815
c9efb49
381a8b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,8 +78,7 @@ class Answer(Enum): | |
|
|
||
| non_member_1: int | ||
|
|
||
| # TODO: this could be considered an error: | ||
| non_member_1: str = "some value" | ||
| non_member_1: str = "some value" # error: [invalid-enum-member-annotation] | ||
|
|
||
| # revealed: tuple[Literal["YES"], Literal["NO"]] | ||
| reveal_type(enum_members(Answer)) | ||
|
|
@@ -100,6 +99,109 @@ class Answer(Enum): | |
| reveal_type(enum_members(Answer)) | ||
| ``` | ||
|
|
||
| ### Annotated enum members | ||
|
|
||
| The [typing spec] states that enum members should not have explicit type annotations. Type checkers | ||
| should report an error for annotated enum members because the annotation is misleading — the actual | ||
| type of an enum member is the enum class itself, not the annotated type. | ||
|
|
||
| ```toml | ||
| [environment] | ||
| python-version = "3.11" | ||
| ``` | ||
|
|
||
| ```py | ||
| from enum import Enum, IntEnum, StrEnum, member | ||
| from typing import Callable, Final | ||
|
|
||
| class Pet(Enum): | ||
| CAT = 1 | ||
| DOG: int = 2 # error: [invalid-enum-member-annotation] "Type annotation on enum member `DOG` is not allowed" | ||
| BIRD: str = "bird" # error: [invalid-enum-member-annotation] | ||
| ``` | ||
|
|
||
| Bare `Final` annotations are allowed (they don't specify a type): | ||
|
|
||
| ```py | ||
| class Pet2(Enum): | ||
| CAT: Final = 1 # OK | ||
| DOG: Final = 2 # OK | ||
| ``` | ||
|
|
||
| But `Final` with a type argument is not allowed: | ||
|
|
||
| ```py | ||
| class Pet3(Enum): | ||
| CAT: Final[int] = 1 # error: [invalid-enum-member-annotation] | ||
| DOG: Final[str] = "woof" # error: [invalid-enum-member-annotation] | ||
| ``` | ||
|
|
||
| `enum.member` used as value wrapper is the standard way to declare members explicitly: | ||
|
|
||
| ```py | ||
| class Pet4(Enum): | ||
| CAT = member(1) # OK | ||
| ``` | ||
|
|
||
| Dunder and private names are not enum members, so they don't trigger the diagnostic: | ||
|
|
||
| ```py | ||
| class Pet5(Enum): | ||
| CAT = 1 | ||
| __private: int = 2 # OK: dunder/private names are never members | ||
| __module__: str = "my_module" # OK | ||
| ``` | ||
|
|
||
| Pure declarations (annotations without values) are non-members and are fine: | ||
|
|
||
| ```py | ||
| class Pet6(Enum): | ||
| CAT = 1 | ||
| species: str # OK: no value, so this is a non-member declaration | ||
| ``` | ||
|
|
||
| Callable values are never enum members at runtime, so annotating them is fine: | ||
|
|
||
| ```py | ||
| def identity(x: int) -> int: | ||
| return x | ||
|
|
||
| class Pet7(Enum): | ||
| CAT = 1 | ||
| declared_callable: Callable[[int], int] = identity # OK: callables are never members | ||
| ``` | ||
|
|
||
| The check also works for subclasses of `Enum`: | ||
|
|
||
| ```py | ||
| class Status(IntEnum): | ||
| OK: int = 200 # error: [invalid-enum-member-annotation] | ||
| NOT_FOUND = 404 # OK | ||
|
|
||
| class Color(StrEnum): | ||
| RED: str = "red" # error: [invalid-enum-member-annotation] | ||
| GREEN = "green" # OK | ||
| ``` | ||
|
|
||
| Special sunder names like `_value_` and `_ignore_` are not flagged: | ||
|
|
||
| ```py | ||
| class Pet8(Enum): | ||
| _value_: int = 0 # OK: `_value_` is a special enum name | ||
| _ignore_: str = "TEMP" # OK: `_ignore_` is a special enum name | ||
| CAT = 1 | ||
| ``` | ||
|
|
||
| Names listed in `_ignore_` are not members, so annotating them is fine: | ||
|
|
||
| ```py | ||
| class Pet9(Enum): | ||
| _ignore_ = "A B" | ||
| A: int = 42 # OK: `A` is listed in `_ignore_` | ||
| B: str = "hello" # OK: `B` is listed in `_ignore_` | ||
| C: int = 3 # error: [invalid-enum-member-annotation] | ||
|
Comment on lines
+199
to
+202
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😵💫 |
||
| ``` | ||
|
|
||
| ### Declared `_value_` annotation | ||
|
|
||
| If a `_value_` annotation is defined on an `Enum` class, all enum member values must be compatible | ||
|
|
@@ -814,7 +916,7 @@ class Answer(Enum): | |
|
|
||
| def is_yes(self) -> bool: | ||
| return self == Answer.YES | ||
| constant: int = 1 | ||
| constant: int = 1 # error: [invalid-enum-member-annotation] | ||
|
|
||
| reveal_type(Answer.YES.is_yes()) # revealed: bool | ||
| reveal_type(Answer.YES.constant) # revealed: int | ||
|
|
@@ -1353,3 +1455,4 @@ class MyEnum[T](MyEnumBase): | |
| - Documentation: <https://docs.python.org/3/library/enum.html> | ||
|
|
||
| [class-private names]: https://docs.python.org/3/reference/lexical_analysis.html#reserved-classes-of-identifiers | ||
| [typing spec]: https://typing.python.org/en/latest/spec/enums.html#enum-members | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,18 +74,18 @@ use crate::types::diagnostic::{ | |
| DATACLASS_FIELD_ORDER, DUPLICATE_BASE, DUPLICATE_KW_ONLY, FINAL_ON_NON_METHOD, | ||
| FINAL_WITHOUT_VALUE, INCONSISTENT_MRO, INEFFECTIVE_FINAL, INVALID_ARGUMENT_TYPE, | ||
| INVALID_ASSIGNMENT, INVALID_ATTRIBUTE_ACCESS, INVALID_BASE, INVALID_DATACLASS, | ||
| INVALID_DECLARATION, INVALID_GENERIC_CLASS, INVALID_GENERIC_ENUM, INVALID_KEY, | ||
| INVALID_LEGACY_POSITIONAL_PARAMETER, INVALID_LEGACY_TYPE_VARIABLE, INVALID_METACLASS, | ||
| INVALID_NAMED_TUPLE, INVALID_NEWTYPE, INVALID_OVERLOAD, INVALID_PARAMETER_DEFAULT, | ||
| INVALID_PARAMSPEC, INVALID_PROTOCOL, INVALID_TYPE_ALIAS_TYPE, INVALID_TYPE_ARGUMENTS, | ||
| INVALID_TYPE_FORM, INVALID_TYPE_GUARD_CALL, INVALID_TYPE_GUARD_DEFINITION, | ||
| INVALID_TYPE_VARIABLE_BOUND, INVALID_TYPE_VARIABLE_CONSTRAINTS, INVALID_TYPE_VARIABLE_DEFAULT, | ||
| INVALID_TYPED_DICT_HEADER, INVALID_TYPED_DICT_STATEMENT, IncompatibleBases, MISSING_ARGUMENT, | ||
| NO_MATCHING_OVERLOAD, PARAMETER_ALREADY_ASSIGNED, POSSIBLY_MISSING_ATTRIBUTE, | ||
| POSSIBLY_MISSING_IMPLICIT_CALL, POSSIBLY_MISSING_IMPORT, SUBCLASS_OF_FINAL_CLASS, | ||
| TOO_MANY_POSITIONAL_ARGUMENTS, TypedDictDeleteErrorKind, UNDEFINED_REVEAL, UNKNOWN_ARGUMENT, | ||
| UNRESOLVED_ATTRIBUTE, UNRESOLVED_GLOBAL, UNRESOLVED_IMPORT, UNRESOLVED_REFERENCE, | ||
| UNSUPPORTED_DYNAMIC_BASE, UNSUPPORTED_OPERATOR, USELESS_OVERLOAD_BODY, | ||
| INVALID_DECLARATION, INVALID_ENUM_MEMBER_ANNOTATION, INVALID_GENERIC_CLASS, | ||
| INVALID_GENERIC_ENUM, INVALID_KEY, INVALID_LEGACY_POSITIONAL_PARAMETER, | ||
| INVALID_LEGACY_TYPE_VARIABLE, INVALID_METACLASS, INVALID_NAMED_TUPLE, INVALID_NEWTYPE, | ||
| INVALID_OVERLOAD, INVALID_PARAMETER_DEFAULT, INVALID_PARAMSPEC, INVALID_PROTOCOL, | ||
| INVALID_TYPE_ALIAS_TYPE, INVALID_TYPE_ARGUMENTS, INVALID_TYPE_FORM, INVALID_TYPE_GUARD_CALL, | ||
| INVALID_TYPE_GUARD_DEFINITION, INVALID_TYPE_VARIABLE_BOUND, INVALID_TYPE_VARIABLE_CONSTRAINTS, | ||
| INVALID_TYPE_VARIABLE_DEFAULT, INVALID_TYPED_DICT_HEADER, INVALID_TYPED_DICT_STATEMENT, | ||
| IncompatibleBases, MISSING_ARGUMENT, NO_MATCHING_OVERLOAD, PARAMETER_ALREADY_ASSIGNED, | ||
| POSSIBLY_MISSING_ATTRIBUTE, POSSIBLY_MISSING_IMPLICIT_CALL, POSSIBLY_MISSING_IMPORT, | ||
| SUBCLASS_OF_FINAL_CLASS, TOO_MANY_POSITIONAL_ARGUMENTS, TypedDictDeleteErrorKind, | ||
| UNDEFINED_REVEAL, UNKNOWN_ARGUMENT, UNRESOLVED_ATTRIBUTE, UNRESOLVED_GLOBAL, UNRESOLVED_IMPORT, | ||
| UNRESOLVED_REFERENCE, UNSUPPORTED_DYNAMIC_BASE, UNSUPPORTED_OPERATOR, USELESS_OVERLOAD_BODY, | ||
| hint_if_stdlib_attribute_exists_on_other_versions, | ||
| hint_if_stdlib_submodule_exists_on_other_versions, report_attempted_protocol_instantiation, | ||
| report_bad_dunder_set_call, report_bad_frozen_dataclass_inheritance, | ||
|
|
@@ -107,7 +107,7 @@ use crate::types::diagnostic::{ | |
| report_shadowed_type_variable, report_unsupported_augmented_assignment, | ||
| report_unsupported_base, report_unsupported_comparison, | ||
| }; | ||
| use crate::types::enums::is_enum_class_by_inheritance; | ||
| use crate::types::enums::{enum_ignored_names, is_enum_class_by_inheritance}; | ||
| use crate::types::function::{ | ||
| FunctionBodyKind, FunctionDecorators, FunctionLiteral, FunctionType, KnownFunction, | ||
| OverloadLiteral, function_body_kind, is_implicit_classmethod, | ||
|
|
@@ -9635,6 +9635,43 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { | |
| &DeclaredAndInferredType::AreTheSame(TypeAndQualifiers::declared(inferred_ty)), | ||
| ); | ||
| } else { | ||
| // Check for annotated enum members. The typing spec states that enum | ||
| // members should not have explicit type annotations. | ||
| if let Some(name_expr) = target.as_name_expr() | ||
| && !name_expr.id.starts_with("__") | ||
| && !matches!(name_expr.id.as_str(), "_ignore_" | "_value_" | "_name_") | ||
| // Not bare Final (bare Final is allowed on enum members) | ||
| && !(declared.qualifiers.contains(TypeQualifiers::FINAL) | ||
| && matches!(declared.inner_type(), Type::Dynamic(DynamicType::Unknown))) | ||
| // Value type would be an enum member at runtime (exclude callables, | ||
| // which are never members) | ||
| && !inferred_ty.is_subtype_of( | ||
| self.db(), | ||
| Type::Callable(CallableType::unknown(self.db())) | ||
| .top_materialization(self.db()), | ||
| ) | ||
|
Comment on lines
+9640
to
+9652
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this all feels a little duplicative of the logic we have elsewhere for deciding whether an assignment in an enum class is an enum member to begin with, but I think it's okay
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's just different enough in form that I agree that we shouldn't bend over backwards to eliminate the copy/pasta. My only thought is that maybe this logic should live as a top-level helper method in if !allowed_enum_member_annotation(name_expr, inferred_ty, current_scope)But I'm also fine leaving as-is |
||
| { | ||
| let current_scope_id = self.scope().file_scope_id(self.db()); | ||
| let current_scope = self.index.scope(current_scope_id); | ||
| if current_scope.kind() == ScopeKind::Class | ||
| && let Some(class) = | ||
| nearest_enclosing_class(self.db(), self.index, self.scope()) | ||
| && is_enum_class_by_inheritance(self.db(), class) | ||
| && !enum_ignored_names(self.db(), self.scope()).contains(&name_expr.id) | ||
| && let Some(builder) = self | ||
| .context | ||
| .report_lint(&INVALID_ENUM_MEMBER_ANNOTATION, annotation) | ||
| { | ||
| let mut diag = builder.into_diagnostic(format_args!( | ||
| "Type annotation on enum member `{}` is not allowed", | ||
| &name_expr.id | ||
| )); | ||
| diag.info( | ||
| "See: https://typing.python.org/en/latest/spec/enums.html#enum-members", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| self.add_declaration_with_binding( | ||
| target.into(), | ||
| definition, | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would suggest either checking both error messages, or neither. (Probably preferably neither, unless you switch to using a snapshot test for that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I disagree -- I think it's useful to assert the error message once, and a snapshot test feels overkill for that unless you have detailed secondary annotations etc. But asserting it more than once makes it tedious to update the mdtests in the future if you ever change the error message.