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

Fix frozen subclass and callable issues in attrs plugin #4755

Merged
merged 2 commits into from Mar 19, 2018

Conversation

Projects
None yet
2 participants
@euresti
Contributor

euresti commented Mar 18, 2018

Fixes #4655 and #4688

euresti added some commits Mar 17, 2018

Make attributes in attr classes be instance variables.
This is technically only a problem when trying to assign callables into attrs.
@ilevkivskyi

Thanks, looks good! Just one comment.

frozen_base.a = 17 # E: Property "a" defined in "FrozenBase" is read-only
a = FrozenNonFrozen(1, 2)
a.a = 17 # E: Property "a" defined in "FrozenNonFrozen" is read-only

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 19, 2018

Collaborator

This error message is a bit weird because a is defined in NonFrozenBase, not in FrozenNonFrozen. My understanding is that this is attrs semantics to "redefine" all parent class attributes as read-only if the class is frozen.

If you think this error message can be somehow easily improved, then could you please do this today? (I am going to cut the release branch today evening or tomorrow morning.) Otherwise, the error message can be fixed in a separate PR (please create an issue in this case).

This comment has been minimized.

@euresti

euresti Mar 19, 2018

Contributor

The attrs semantics actually add a __setattr__ that raises to the frozen class. But mypy doesn't seem to handle that.

As for the message I think it's an existing problem:

class C:
    @property
    def foo(self) -> int:
        return 17
class D(C):
    pass
d = D()
d.foo = 18  # error: Property "foo" defined in "D" is read-only

PS. Another way to make frozen better would be to implement #4572

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Mar 19, 2018

Collaborator

OK, I opened a separate issue for error message.

As for __setattr__, I think we shouldn't wait, this PR already correctly fixes the crash, so I am going to merge it now.

@ilevkivskyi ilevkivskyi merged commit e505c32 into python:master Mar 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment