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.

Sign up for GitHub

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

Jump to bottom

Enums with annotations and no values are fine to be subclassed #11579

Merged
merged 5 commits into from Dec 21, 2021
Merged

Enums with annotations and no values are fine to be subclassed #11579

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7989a2c
Enums with annotations and no values are fine to be subclasses
sobolevn Nov 19, 2021
901ca52
Address review
sobolevn Nov 19, 2021
e4d4112
Adds `.pyi` test for uninitialized values
sobolevn Nov 20, 2021
04afff5
Now using `Var.has_explicit_value` boolean flag instead of AST traversal
sobolevn Dec 18, 2021
af90ec4
Ensure that cache works
sobolevn Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions mypy/nodes.py
View file
Open in desktop
Expand Up @@ -870,6 +870,7 @@ class Var(SymbolNode):
'is_suppressed_import',
'explicit_self_type',
'from_module_getattr',
'has_explicit_value',
)

def __init__(self, name: str, type: 'Optional[mypy.types.Type]' = None) -> None:
Expand Down Expand Up @@ -914,6 +915,9 @@ def __init__(self, name: str, type: 'Optional[mypy.types.Type]' = None) -> None:
self.explicit_self_type = False
# If True, this is an implicit Var created due to module-level __getattr__.
self.from_module_getattr = False
# Var can be created with an explicit value `a = 1` or without one `a: int`,
# we need a way to tell which one is which.
self.has_explicit_value = False
sobolevn marked this conversation as resolved.
Show resolved Hide resolved

@property
def name(self) -> str:
Expand Down
70 changes: 55 additions & 15 deletions mypy/semanal.py
View file
Open in desktop
Expand Up @@ -1552,17 +1552,12 @@ def configure_base_classes(self,
elif isinstance(base, Instance):
if base.type.is_newtype:
self.fail('Cannot subclass "NewType"', defn)
if (
base.type.is_enum
and base.type.fullname not in ENUM_BASES
and base.type.names
and any(not isinstance(n.node, (FuncBase, Decorator))
for n in base.type.names.values())
):
if self.enum_has_final_values(base):
# This means that are trying to subclass a non-default
# Enum class, with defined members. This is not possible.
# In runtime, it will raise. We need to mark this type as final.
# However, methods can be defined on a type: only values can't.
# We also don't count values with annotations only.
base.type.is_final = True
base_types.append(base)
elif isinstance(base, AnyType):
Expand Down Expand Up @@ -1601,6 +1596,25 @@ def configure_base_classes(self,
return
self.calculate_class_mro(defn, self.object_type)

def enum_has_final_values(self, base: Instance) -> bool:
if (
base.type.is_enum
and base.type.fullname not in ENUM_BASES
and base.type.names
and base.type.defn
sobolevn marked this conversation as resolved.
Show resolved Hide resolved
):
for sym in base.type.names.values():
if isinstance(sym.node, (FuncBase, Decorator)):
continue # A method
if not isinstance(sym.node, Var):
return True # Can be a class
if self.is_stub_file or sym.node.has_explicit_value:
# Corner case: assignments like `x: int` are fine in `.py` files.
# But, not is `.pyi` files, because we don't know
# if there's aactually a value or not.
return True
return False

def configure_tuple_base_class(self,
defn: ClassDef,
base: TupleType,
Expand Down Expand Up @@ -2040,7 +2054,7 @@ def visit_import_all(self, i: ImportAll) -> None:

def visit_assignment_expr(self, s: AssignmentExpr) -> None:
s.value.accept(self)
self.analyze_lvalue(s.target, escape_comprehensions=True)
self.analyze_lvalue(s.target, escape_comprehensions=True, has_explicit_value=True)

def visit_assignment_stmt(self, s: AssignmentStmt) -> None:
self.statement = s
Expand Down Expand Up @@ -2333,10 +2347,20 @@ def analyze_lvalues(self, s: AssignmentStmt) -> None:
assert isinstance(s.unanalyzed_type, UnboundType)
if not s.unanalyzed_type.args:
explicit = False

if s.rvalue:
if isinstance(s.rvalue, TempNode):
has_explicit_value = not s.rvalue.no_rhs
else:
has_explicit_value = True
else:
has_explicit_value = False

for lval in s.lvalues:
self.analyze_lvalue(lval,
explicit_type=explicit,
is_final=s.is_final_def)
is_final=s.is_final_def,
has_explicit_value=has_explicit_value)

def apply_dynamic_class_hook(self, s: AssignmentStmt) -> None:
if not isinstance(s.rvalue, CallExpr):
Expand Down Expand Up @@ -2776,7 +2800,8 @@ def analyze_lvalue(self,
nested: bool = False,
explicit_type: bool = False,
is_final: bool = False,
escape_comprehensions: bool = False) -> None:
escape_comprehensions: bool = False,
has_explicit_value: bool = False) -> None:
"""Analyze an lvalue or assignment target.

Args:
Expand All @@ -2790,7 +2815,11 @@ def analyze_lvalue(self,
if escape_comprehensions:
assert isinstance(lval, NameExpr), "assignment expression target must be NameExpr"
if isinstance(lval, NameExpr):
self.analyze_name_lvalue(lval, explicit_type, is_final, escape_comprehensions)
self.analyze_name_lvalue(
lval, explicit_type, is_final,
escape_comprehensions,
has_explicit_value=has_explicit_value,
)
elif isinstance(lval, MemberExpr):
self.analyze_member_lvalue(lval, explicit_type, is_final)
if explicit_type and not self.is_self_member_ref(lval):
Expand All @@ -2814,7 +2843,8 @@ def analyze_name_lvalue(self,
lvalue: NameExpr,
explicit_type: bool,
is_final: bool,
escape_comprehensions: bool) -> None:
escape_comprehensions: bool,
has_explicit_value: bool) -> None:
"""Analyze an lvalue that targets a name expression.

Arguments are similar to "analyze_lvalue".
Expand Down Expand Up @@ -2844,7 +2874,7 @@ def analyze_name_lvalue(self,

if (not existing or isinstance(existing.node, PlaceholderNode)) and not outer:
# Define new variable.
var = self.make_name_lvalue_var(lvalue, kind, not explicit_type)
var = self.make_name_lvalue_var(lvalue, kind, not explicit_type, has_explicit_value)
added = self.add_symbol(name, var, lvalue, escape_comprehensions=escape_comprehensions)
# Only bind expression if we successfully added name to symbol table.
if added:
Expand Down Expand Up @@ -2895,7 +2925,9 @@ def is_alias_for_final_name(self, name: str) -> bool:
existing = self.globals.get(orig_name)
return existing is not None and is_final_node(existing.node)

def make_name_lvalue_var(self, lvalue: NameExpr, kind: int, inferred: bool) -> Var:
def make_name_lvalue_var(
self, lvalue: NameExpr, kind: int, inferred: bool, has_explicit_value: bool,
) -> Var:
"""Return a Var node for an lvalue that is a name expression."""
v = Var(lvalue.name)
v.set_line(lvalue)
Expand All @@ -2910,6 +2942,7 @@ def make_name_lvalue_var(self, lvalue: NameExpr, kind: int, inferred: bool) -> V
# fullanme should never stay None
v._fullname = lvalue.name
v.is_ready = False # Type not inferred yet
v.has_explicit_value = has_explicit_value
return v

def make_name_lvalue_point_to_existing_def(
Expand Down Expand Up @@ -2953,7 +2986,14 @@ def analyze_tuple_or_list_lvalue(self, lval: TupleExpr,
if len(star_exprs) == 1:
star_exprs[0].valid = True
for i in items:
self.analyze_lvalue(i, nested=True, explicit_type=explicit_type)
self.analyze_lvalue(
lval=i,
nested=True,
explicit_type=explicit_type,
# Lists and tuples always have explicit values defined:
# `a, b, c = value`
has_explicit_value=True,
)

def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool, is_final: bool) -> None:
"""Analyze lvalue that is a member expression.
Expand Down
39 changes: 38 additions & 1 deletion test-data/unit/check-enum.test
View file
Open in desktop
Expand Up @@ -1678,7 +1678,6 @@ class A(Enum):
class B(A): pass # E: Cannot inherit from final class "A"
[builtins fixtures/bool.pyi]


[case testEnumFinalSpecialProps]
# https://github.com/python/mypy/issues/11699
from enum import Enum, IntEnum
Expand All @@ -1702,3 +1701,41 @@ class EI(IntEnum):
E._order_ = 'a' # E: Cannot assign to final attribute "_order_"
EI.value = 2 # E: Cannot assign to final attribute "value"
[builtins fixtures/bool.pyi]

[case testEnumNotFinalWithMethodsAndUninitializedValues]
# https://github.com/python/mypy/issues/11578
from enum import Enum
from typing import Final

class A(Enum):
x: int
def method(self) -> int: pass
class B(A):
x = 1 # E: Cannot override writable attribute "x" with a final one
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be fixed. I am working on it right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

But, on the other hand. Looks like it is the same for regular classes:

from typing import Final

class A:
    x: int
    y: int

class B(A):
    x: Final = 1
    y: Final = 2
# out/ex.py:8: error: Cannot override writable attribute "x" with a final one
# out/ex.py:9: error: Cannot override writable attribute "y" with a final one

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like PEP-591 prohibits Final on a subclass, so it looks like it's a logic error that Mypy does not allow subclassing like you did ^^^.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though this is pretty clear why this is the case (and makes sense)

mypy/mypy/checker.py

Lines 2505 to 2525 in d7c4e69

def check_if_final_var_override_writable(self,
name: str,
base_node:
Optional[Node],
ctx: Context) -> None:
"""Check that a final variable doesn't override writeable attribute.
This is done to prevent situations like this:
class C:
attr = 1
class D(C):
attr: Final = 2
x: C = D()
x.attr = 3 # Oops!
"""
writable = True
if base_node:
writable = self.is_writable_attribute(base_node)
if writable:
self.msg.final_cant_override_writable(name, ctx)
(it's just not covered by PEP-591. It may be possible to check if the base class just has a type like you did in the Enum code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, let's make this a subject for a next PR. This would require some extra work. And might have unwanted consequences.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. This is intended. I don't remember why we didn't mention it in the PEP (it may be we indeed just wanted to be terse).

Copy link
Contributor

Choose a reason for hiding this comment

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

It should still be allowed if the variable is not initialized yet, right? Based on the PEP it looks like it would be expected to be initialized in the __init__, but it sounds like there should be a special case where the class is basically completely abstract.

Copy link
Member

Choose a reason for hiding this comment

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

No, it shouldn't. Consider this

class A:
    x: int

class B(A):
    x: Final = 1

# somewhere in modules that depend on the above, and therefore are being processed later
def set_x(a: A) -> None:
    a.x = 42

set_x(B())  # ???

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense in general but I wonder if it makes sense to special case enums here. We may need more details from the use case in #11578.

Copy link
Contributor

Choose a reason for hiding this comment

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

The example given here #11578 (comment) is basically how the code is used. The only thing that the base class does is define a __new__ that stores the actual values that all the subclasses provide (additional constants like defined in this example https://docs.python.org/3/library/enum.html#planet).

The interpreter doesn't complain about subclassing an enum, and as I realize now that's because there are no values defined on the base class. I do agree enums probably need a special case because the class that does define enum values is the concrete class that is also final whereas the example above the interpreter does not effectively define final like it would for enums.


class A1(Enum):
x: int = 1
class B1(A1): # E: Cannot inherit from final class "A1"
pass

class A2(Enum):
x = 2
class B2(A2): # E: Cannot inherit from final class "A2"
pass
[builtins fixtures/bool.pyi]

[case testEnumNotFinalWithMethodsAndUninitializedValuesStub]
import lib

[file lib.pyi]
from enum import Enum
class A(Enum):
x: int
class B(A): # E: Cannot inherit from final class "A"
x = 1 # E: Cannot override writable attribute "x" with a final one

class C(Enum):
x = 1
class D(C): # E: Cannot inherit from final class "C"
x: int # E: Cannot assign to final name "x"
[builtins fixtures/bool.pyi]