bpo-41295: Allow overriding __setattr__ on multiple inheritance by david-caro · Pull Request #21473 · python/cpython

david-caro · 2020-07-14T16:29:21Z

The change introduced in bpo-39960, broke being able to override the
__setattr__ function unless it's done by the next class in the MRO.

For example:

class Meta(type):
    def __setattr__(cls, key, value):
        type.__setattr__(cls, key, value)

class OtherClass:
    pass

class DefaultMeta(OtherClass, Meta):
    pass

obj = DefaultMeta('A', (object,), {})
obj.test = True
print(obj.test)

Would break as it would first check if OtherClass has a custom
__setattr__ and if not would raise error right away.

This change goes through the whole MRO to find out if there's any class
that overrides it before throwing error.

https://bugs.python.org/issue41295

david-caro · 2020-07-14T16:49:14Z

I would appreciate some guidance on where to put tests for this :)

tiran

Could you please add your regression as test case, too?

bedevere-bot · 2020-07-14T17:03:39Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

david-caro · 2020-07-14T17:24:18Z

Could you please add your regression as test case, too?

I added the test to test_desrc.py, I'm not sure if that's where it should go, let me know, thanks for the reviews! :)

david-caro · 2020-07-14T17:30:59Z

I have made the requested changes; please review again

bedevere-bot · 2020-07-14T17:31:03Z

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

bedevere-bot · 2020-07-14T17:35:58Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

david-caro · 2020-07-14T17:39:32Z

Awesome, thanks a lot for the quick review :)

I have made the requested changes; please review again

bedevere-bot · 2020-07-14T17:39:35Z

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

tiran · 2020-07-14T18:14:11Z

Thanks! I'm leaving the final review to @scoder .

scoder · 2020-07-14T21:24:46Z

I'll have to take a deeper look at this, just a quick comment: should_break is not a good name because it does not explain what it means if it's true of false. I would like to see a better explanation in the variable name and preferably also in comments of why this algorithm is correct. It's not clear to me from the first look.

david-caro · 2020-07-15T14:30:15Z

I started looking a bit more closely into this (I'm kinda new to the codebase :) ), fixed a bit the variable name, and added comments there.
The comments are assumptions though, I'm not 100% sure that that's the case, I've tested it with a bunch of different python codes to try to verify those assumptions, but the lack of proof is not proof by itself ;)
Please let me know what I got wrong, and/or what I should change to improve it.
Thanks!

For reference, I'm trying to do what's said in the email thread: https://mail.python.org/pipermail/python-dev/2003-April/034605.html

gvanrossum

I don't believe this is right yet. Here's another test case (which should raise TypeError on obj.test = False):

class A(type):
    def __setattr__(cls, key, value):
        object.__setattr__(cls, key, value)

class B:  # **UPDATE**: I mistakenly has B(A) here
    pass

class C(B, A):
    pass

obj = C('D', (object,), {})
try:
    obj.test = False
except Exception as err:
    print("Good, it raised", repr(err))
else:
    assert False, "Didn't raise!!"

gvanrossum · 2020-07-16T19:37:56Z

I would move the test after test_carloverre (see line 4296).

gvanrossum · 2020-07-16T19:41:13Z

bedevere-bot · 2020-07-16T19:48:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

gvanrossum · 2020-07-16T20:10:45Z

Sorry, I had a typo in my example; it should be class B: instead of class B(A):. I've fixed this above and added a comment drawing attention to my mistake. But my point stands: I think that example (so modified) should raise.

The way I understand the example, just because B.__setattr__ is the same as object.__setattr__ doesn't mean that (UPDATE: clarified) you can call it directly -- in C's MRO, type.__setattr__ overrides object.__setattr__.

The change introduced in bpo-39960, broke being able to override the __setattr__ function unless it's done by the next class in the MRO. For example: ``` class Meta(type): def __setattr__(cls, key, value): type.__setattr__(cls, key, value) class OtherClass: pass class DefaultMeta(OtherClass, Meta): pass obj = DefaultMeta('A', (object,), {}) obj.test = True print(obj.test) ``` Would break as it would first check if `OtherClass` has a custom `__setattr__` and if not would raise error right away. This change goes through the whole MRO, until it finds `type` or `object`, and if only if no class owns the method being called it will throw an error. Signed-off-by: David Caro <david@dcaro.es>

david-caro · 2020-07-17T18:27:39Z

Sorry, I had a typo in my example; it should be class B: instead of class B(A):. I've fixed this above and added a comment drawing attention to my mistake. But my point stands: I think that example (so modified) should raise.

The way I understand the example, just because B.__setattr__ is the same as object.__setattr__ doesn't mean that (UPDATE: clarified) you can call it directly -- in C's MRO, type.__setattr__ overrides object.__setattr__.

I've gotten the tests to pass if I look explicitly for PyType_Type and PyBaseObject_Type, but the some tests fail, I think because I should be checking in a more generic way (not only for type and object), I'll try to look into it this weekend, but if you know from the top of your head check for those to I'll really appreciate the (extra) tip. Thanks!

gvanrossum · 2020-07-17T22:59:46Z

PS. Please don't use git push -f. Just make a new local commit and push that. We'll squash commits when we land, but during code review the squashing is quite unhelpful -- reviewers need to be able to quickly see what changed since their last review.

scoder · 2020-07-18T06:47:02Z

        PyTypeObject *base = (PyTypeObject*) PyTuple_GET_ITEM(mro, i);
-        if (base->tp_setattro == func) {
-            /* 'func' is the earliest non-Python implementation in the MRO. */
+        if (base == &PyType_Type || base == &PyBaseObject_Type) {


This is definitely not right. Special-casing types here (any type, really) will not help the general case.

scoder · 2020-07-18T07:48:56Z

Let's see if the algorithm in #21528 works in all cases.

gvanrossum · 2020-07-18T21:16:23Z

@david-caro I'm sorry, but the fix by @scoder in GH-21528 seems to be more accurate, so I'm closing this PR. Thanks for your efforts -- they helped us to find the right fix. Better luck with your next PR!

the-knights-who-say-ni added the CLA signed label Jul 14, 2020

bedevere-bot added the awaiting review label Jul 14, 2020

kam193 reviewed Jul 14, 2020

View reviewed changes

Comment thread Objects/typeobject.c Outdated

kam193 reviewed Jul 14, 2020

View reviewed changes

Comment thread Objects/typeobject.c Outdated

david-caro force-pushed the fix-issue-41295 branch from 79a0723 to e0bc87d Compare July 14, 2020 16:56

tiran requested changes Jul 14, 2020

View reviewed changes

bedevere-bot removed the awaiting review label Jul 14, 2020

bedevere-bot added the awaiting changes label Jul 14, 2020

david-caro force-pushed the fix-issue-41295 branch from e0bc87d to 204f00a Compare July 14, 2020 17:23

david-caro requested a review from tiran July 14, 2020 17:29

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 14, 2020

tiran requested changes Jul 14, 2020

View reviewed changes

Comment thread Lib/test/test_descr.py Outdated

Comment thread Lib/test/test_descr.py Outdated

bedevere-bot added awaiting changes and removed awaiting change review labels Jul 14, 2020

david-caro force-pushed the fix-issue-41295 branch from 204f00a to 3feca47 Compare July 14, 2020 17:39

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 14, 2020

bedevere-bot requested a review from tiran July 14, 2020 17:39

tiran assigned scoder Jul 14, 2020

tiran requested a review from scoder July 14, 2020 18:13

david-caro force-pushed the fix-issue-41295 branch from 3feca47 to d6bb0e1 Compare July 15, 2020 14:28

gvanrossum requested changes Jul 16, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jul 16, 2020

gvanrossum added needs backport to 3.8 labels Jul 16, 2020

r4v5 mentioned this pull request Jul 17, 2020

Issue getting dev server running lucyparsons/OpenOversight#754

Closed

david-caro force-pushed the fix-issue-41295 branch from d6bb0e1 to ecc3b8a Compare July 17, 2020 18:26

scoder reviewed Jul 18, 2020

View reviewed changes

gvanrossum closed this Jul 18, 2020

david-caro deleted the fix-issue-41295 branch July 27, 2020 15:12

Uh oh!

Uh oh!

Conversation

david-caro commented Jul 14, 2020 • edited by gvanrossum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-caro commented Jul 14, 2020

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 14, 2020

Uh oh!

david-caro commented Jul 14, 2020

Uh oh!

david-caro commented Jul 14, 2020

Uh oh!

bedevere-bot commented Jul 14, 2020

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jul 14, 2020

Uh oh!

david-caro commented Jul 14, 2020

Uh oh!

bedevere-bot commented Jul 14, 2020

Uh oh!

tiran commented Jul 14, 2020

Uh oh!

scoder commented Jul 14, 2020

Uh oh!

david-caro commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 16, 2020

Uh oh!

gvanrossum commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-caro commented Jul 17, 2020

Uh oh!

gvanrossum commented Jul 17, 2020

Uh oh!

scoder Jul 18, 2020

Choose a reason for hiding this comment

Uh oh!

scoder commented Jul 18, 2020

Uh oh!

gvanrossum commented Jul 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

david-caro commented Jul 14, 2020 •

edited by gvanrossum

Loading

david-caro commented Jul 15, 2020 •

edited

Loading

gvanrossum left a comment •

edited

Loading

gvanrossum commented Jul 16, 2020 •

edited

Loading