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

Allow covariance for read-only attributes #8350

Merged
merged 7 commits into from
Feb 7, 2020
Merged

Allow covariance for read-only attributes #8350

merged 7 commits into from
Feb 7, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie TH3CHARLie commented Jan 31, 2020

resolves #5836

For now, I've not added any tests yet. I've tested against code samples in #5836 and #3260 and the patch is working but adding either of them seems to require modifying a lot in test fixtures

The new helper function is_writable_attribute is actually borrowed from check_no_writable. I'd also like to refactor the latter function's name and implement it using the new helper in this PR, since I think its name is kind of misleading considering the comments

mypy/mypy/checker.py

Lines 2391 to 2402 in ea3c65c

def check_no_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!
"""

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 3, 2020

but adding either of them seems to require modifying a lot in test fixtures

What's missing in text fixtures? Maybe you can simplify the examples to not rely on so many features.

@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented Feb 3, 2020

but adding either of them seems to require modifying a lot in test fixtures

What's missing in text fixtures? Maybe you can simplify the examples to not rely on so many features.

Good point. I've simplified the case from #5836 and added it. I've also done some refactor work to reuse the new helper added and modify the function's name to make it less misleading. But it is not the main focus of the PR and I'd like to know your thoughts on it. @JukkaL

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

OK this seems like a reasonable improvement. I think this doesn't affect all attributes. Regular attributes we just always allow covariant overriding of, even when it is totally wrong.

@msullivan msullivan merged commit 3bd6e47 into python:master Feb 7, 2020
@TH3CHARLie TH3CHARLie deleted the fix-5836 branch February 8, 2020 08:56
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.

Decorated method type-incompatibility with superclass
3 participants