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

[mypyc] Support attributes that override properties #14377

Merged
merged 25 commits into from
Jan 10, 2023

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Jan 2, 2023

Code like this is now supported by mypyc:

class B:
    @property
    def x(self) -> int:
        return 0

class C(B):
    x: int = 0 # Attribute overrides a property

The implementation generates implicit getter/setter methods for attributes as needed and puts them in the vtable. I had to change both the irbuild "prepare" pass (where we generate declarations), irbuild main pass (where we generate implicit accessor IRs), and codegen (where implicit properties aren't visible externally to CPython).

Also fix a minor mypy bug related to overriding properties and multiple inheritance that I encountered.

This doesn't handle glue methods yet.

@@ -223,8 +236,68 @@ def prepare_class_def(
# Supports copy.copy and pickle (including subclasses)
ir._serializable = True

# We sort the table for determinism here on Python 3.5
for name, node in sorted(info.names.items()):
# Check for subclassing from builtin types
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to restructure this to do things in a different order. Most of the logic hasn't changed. I also extracted some functionality to separate methods since the function was already quite big.

@github-actions

This comment has been minimized.

@tmke8
Copy link
Contributor

tmke8 commented Jan 9, 2023

Is the eventual goal to allow this for mypyc? (Which throws an exception in CPython)

class B:
    @property
    def x(self) -> int:
        return 0

class C(B):
    x: int
    def __init__(self) -> None:
        self.x = 4

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jan 9, 2023

Generally the goal is to reject things that don't work in CPython. Updated the example to something that actually works in CPython:

class B:
    @property
    def x(self) -> int:
        return 0

class C(B):
    x: int = 0 # Attribute overrides a property

Added #14413 to track the mypy false negative when there is no initializer.

I'll also need to remove cases that aren't supported by CPython from the tests in added in this PR.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 9, 2023

I always thought that the idiomatic way to override a read-only property with a writable attribute was supposed to be this, but mypy unfortunately emits a false positive for this snippet:

class A:
    _x: int
    @property
    def x(self) -> int:
        return self._x

class B(A):
    @A.x.setter  # mypy complains with 'error: "Callable[[A], int]" has no attribute "setter"  [attr-defined]'
    def x(self, val: int) -> None:
        self._x = val

b = B()
b.x = 42
b.x

(This false positive is already tracked in #5936, and #1465 (comment), and, until I closed it a few minutes ago, #4644.)

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jan 10, 2023

I always thought that the idiomatic way to override a read-only property with a writable attribute was supposed to be this ...

This idiom is almost nonexistent in the codebases I work on, though mypy not supporting it may be the reason why developers use something different. Still, it would be nice to support it. I focused on the particular use case addressed in this PR since I have a follow-up PR that needs this.

@JukkaL JukkaL merged commit 2475643 into master Jan 10, 2023
@JukkaL JukkaL deleted the mypyc-property-inheritance branch January 10, 2023 13:18
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.

4 participants