-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
super() and property inheritance behavior #59170
Comments
super() objects allow access to inherited properties fget() but not fset() or fdel(), resulting in unexpected behavior. Today on pydev thread 'Property inheritance in Python' GvR said "I >>> class BaseProp(object):
... @property
... def p(self):
... return self._p
... @p.setter
... def p(self, value):
... self._p = value
>>> class DerivedProp(BaseProp):
... @property
... def p(self):
... return super(DerivedProp, self).p * 2
... @p.setter
... def p(self, value):
... super(DerivedProp, self).p = value / 2
>>> d = DerivedProp()
>>> d._p = 21
>>> d.p
42
>>> d.p = 50
Traceback (most recent call last):
...
AttributeError: 'super' object has no attribute 'p' |
>>> class DerivedProp(BaseProp):
... @property
... def p(self):
... return super(DerivedProp, self).p * 2
... @p.setter
... def p(self, value):
... BaseProp.p.__set__(self,value / 2) |
I'm attaching a patch implementing super.__setattr__ (and __delattr__). The implementation in the patch only works, if super can find a data descriptor in the MRO, otherwise it throws an AttributeError. As it can be seen in the tests, in some cases this may result in counter-intuitive behaviour. But I wasn't able to find another behaviour, that is consistent with both super.__getattr__ and normal __setattr__ semantics. |
I stumbled across this omission as well in 2010 and brought this up on python-dev: http://mail.python.org/pipermail/python-dev/2010-April/099672.html There were no replies, but perhaps my post adds a bit of information and also there is another patch linked from there. I attached my patch from 2010 for reference. |
I'm -0 for proposed changes, these changes reduce code readability from my perspective. |
I'm not a python dev, but would you say super(self.__class__, self.__class__).x.fset(self, value) is more readable than super().x = value |
I would say @x.deleter
def x(self):
del super().x confuses me a bit. |
Just as a note, there is a distinct possibility that a "property" in a superclass could be some other kind of descriptor object that's not a property. To handle that case, the solution of super(self.__class__, self.__class__).x.fset(self, value) would actually have to be rewritten as super(self.__class__, self.__class__).x.__set__(self, value) That said, I agree it would be nice to have a simplified means of accomplishing this. |
+1 to this feature, this will dramatically simplify property setting code. |
For those who want to use this right away, I've added a python implementation of the patch, which passes the unit tests. There's a slight difference in usage, where instead of using super() directly, super_prop(super()) needs to be used, so we can still use super without arguments. |
I had to add a workaround to ssl.SSLContext and would appreciate a better solution. |
There's a patch attached to this bug. Why is its stage "needs patch"? |
@daniel.urban I'm attempting to move this patch along, but since the contributing process has changed in the years since your patch, you'll need to sign the CLA. Are you interested in picking this back up at all? I haven't been given any indication of how to proceed if I'm doing this on your behalf, but hopefully the core team will enlighten us. |
@simonzack Your superprop.py doesn't work for multiple inheritance, because you're using __thisclass__.__mro__ in each step instead of the initial object mro |
@habnabit I believe I've already signed some contributor form some years ago. If there is a new one, I can sign that one too. |
Fixed superprop.py workaround, now works with multiple inheritance and follows mro adequately. Renamed to duper.py as inspired by Torsten. Uploading here and also to https://gist.github.com/willrazen/bef3fcb26a83dffb6692e5e10d3e67ac |
@daniel.urban would you kindly resubmit your patch as a PR to the cpython repo? I've learned out-of-band from someone else that putting patches on bpo is considered obsolete. you can use the PR I've submitted (#26194) and reset the author. I'd be happy to do it myself (giving you a branch that's all set up, so all you need to do is click the 'new PR' button) if you tell me what to set the author to. |
-0 from me as well. I don't think this is common or something that should be encouraged. As Andrew points out, "del super().x" doesn't have an obvious meaning and it could be regarded as a code smell. The OP's first example would be an unpleasant API to debug -- it exhibits tight coupling between the parent and child class, it has Liskov issues, and it has implicit forwarding and indirection through descriptors. The tight coupling is especially problematic because Python's super() isn't guaranteed to call the parent class; rather, it can call a sibling class as determined by the MRO which cannot be known at the time the class is written. Another thought is that super() is intentionally not a completely transparent proxy. While an explicit call super().__getitem__(k) works, we've denied support for super()[k]. To me, "super().x = 10" and "del super().x" fall in the same category. Looking at the OP's Fortunately, it doesn't seem to be a common need to use super() in a property setter or deleter to bypass the current class and call setter or deleter in a parent class property. Arguably, this kind of tight coupling isn't good design. The OP's first example would be an unpleasant API to debug. FWIW, |
super().x = y
anddel super().x
#26194super().x = y
anddel super().x
(updated) #29950Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: