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

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

Merged
merged 5 commits into from Dec 21, 2021

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Nov 19, 2021

I am using AST here, because Var does not seem to have information about having an explicit value.
I left a TODO item for it. Should we add this to Var?

Closes #11578

@sobolevn sobolevn changed the title Enums with annotations and no values are fine to be subclasses Enums with annotations and no values are fine to be subclassed Nov 19, 2021
@sobolevn
Copy link
Member Author

sobolevn commented Nov 19, 2021

CC @terencehonles

def method(self) -> int: pass

class B(A):
x = 1 # E: Cannot override writable attribute "x" with a final one
Copy link
Member Author

@sobolevn sobolevn Nov 19, 2021

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

@sobolevn sobolevn Nov 19, 2021

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

@terencehonles terencehonles Nov 19, 2021

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

@terencehonles terencehonles Nov 19, 2021

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

@sobolevn sobolevn Nov 19, 2021

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

@ilevkivskyi ilevkivskyi Nov 22, 2021

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

@terencehonles terencehonles Nov 22, 2021

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

@ilevkivskyi ilevkivskyi Nov 22, 2021

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

@JelleZijlstra JelleZijlstra Nov 22, 2021

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

@terencehonles terencehonles Nov 23, 2021

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.

Copy link
Contributor

@terencehonles terencehonles left a comment

This looks good to me, but this still doesn't fix the second two errors from #11578 (comment), right?

mypy/semanal.py Outdated
for node in base.type.defn.defs.body:
if isinstance(node, (FuncBase, Decorator)):
continue # A method
if isinstance(node, AssignmentStmt) and isinstance(node.rvalue, TempNode):
Copy link
Contributor

@terencehonles terencehonles Nov 19, 2021

Choose a reason for hiding this comment

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

should this also check node.rvalue.no_rhs like

if (isinstance(s.rvalue, TempNode) and s.rvalue.no_rhs and

Copy link
Member Author

@sobolevn sobolevn Nov 19, 2021

Choose a reason for hiding this comment

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

but this still doesn't fix the second two errors from #11578 (comment), right?

Yes, see this comment: #11579 (comment)

should this also check node.rvalue.no_rhs like

Done! Thanks.

@sobolevn
Copy link
Member Author

sobolevn commented Nov 20, 2021

I probably also have to check that values are only required for .py files and not .pyi files.

Because we can use code like

class Encoding(Enum):
    PEM: str
    DER: str
    OpenSSH: str
    Raw: str
    X962: str
    SMIME: str

https://github.com/python/typeshed/blob/ee487304d76c671aa353a15e34bc1102bffa2362/stubs/cryptography/cryptography/hazmat/primitives/serialization/__init__.pyi#L29-L41 in stubs.

mypy/semanal.py Outdated Show resolved Hide resolved
mypy/nodes.py Show resolved Hide resolved
mypy/semanal.py Show resolved Hide resolved
JukkaL
JukkaL approved these changes Dec 21, 2021
Copy link
Collaborator

@JukkaL JukkaL left a comment

I'm going to merge this now so that we can include it in the 0.930 release. It would be great to have an additional test cases in a follow-up PR that mirrors the case in #11702 (assignment to an attribute via self without a type annotation in class body, and __new__).

@JukkaL JukkaL merged commit 6de254e into python:master Dec 21, 2021
14 checks passed
@sobolevn
Copy link
Member Author

sobolevn commented Dec 21, 2021

Thanks! I will add new test cases shortly 👍

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.

5 participants