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

Don't reset custom __setattr__ in slotted classes #681

Merged
merged 11 commits into from Sep 5, 2020

Conversation

hynek
Copy link
Member

@hynek hynek commented Sep 1, 2020

The detection of whether or not we should be resetting __setattr__ in slotted classes is currently wrong.

Fixes #680

@hynek hynek force-pushed the slots-setattr-regression branch 2 times, most recently from d0a840a to 9e8cd23 Compare September 1, 2020 12:04
Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on this! Took me a while to grok how this works.

src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
tests/test_setattr.py Outdated Show resolved Hide resolved
# XXX: a non-attrs class and subclass the resulting class with an attrs
# XXX: class. See `test_slotted_confused for details. For now that's
# XXX: OK with us.
for base_cls in self._cls.__bases__:
Copy link
Member

Choose a reason for hiding this comment

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

I assume you had a good reason for this, but why __bases__ and not __mro__[1:-1] here? It seems that that would fix the test_slotted_confused issue, and changing from __bases__ to __mro__[1:-1] doesn't cause any other tests to fail.

I think it would be good to either add a test that demonstrates the behavior you're trying to preserve by using __bases__ or switch to __mro__.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because __attrs_own_setattr__ is a computed attribute.

Take this for instance:

@attr.define
class A:
    x: int = attr.field(on_setattr=frozen)

@attr.define
class B(A):
    x: int = attr.field(on_setattr=NO_OP)

A has __attrs_own_setattr__ = True but B doesn't. If I walk the whole MRO, it would turn up True.

IIUC, my only option would be to walk the MRO and apply the full resolution logic myself – whose avoidance's the whole reason behind __attrs_own_setattr__?

Copy link
Member

Choose a reason for hiding this comment

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

A has __attrs_own_setattr__ = True but B doesn't. If I walk the whole MRO, it would turn up True.

From my test, it seems like B does have __attrs_own_setattr__ == True, is this not intentional?

I think I've come up with a test that covers the situation you're trying to guard against, though:

    def test_custom_setattr_subclass(self):
        @attr.define
        class A(object):
            x = attr.field(on_setattr=setters.frozen)

        @attr.define
        class B(A):
            x = attr.field(on_setattr=setters.NO_OP)
            def __setattr__(self, key, value):
                raise SystemError

        @attr.define
        class C(B):
            pass

        with pytest.raises(SystemError):
            C(1).x = 2

Probably worth adding that as a regression test so that no one shows up with a PR fixing the xfail-ing test by switching from __bases__ to __mro__[1:-1] or something of that nature.

Copy link
Member Author

Choose a reason for hiding this comment

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

A has __attrs_own_setattr__ = True but B doesn't. If I walk the whole MRO, it would turn up True.

From my test, it seems like B does have __attrs_own_setattr__ == True, is this not intentional?

No it's not, since B doesn't have an own setattr. 🙈

I think I've come up with a test that covers the situation you're trying to guard against, though:

    def test_custom_setattr_subclass(self):
        @attr.define
        class A(object):
            x = attr.field(on_setattr=setters.frozen)

        @attr.define
        class B(A):
            x = attr.field(on_setattr=setters.NO_OP)
            def __setattr__(self, key, value):
                raise SystemError

        @attr.define
        class C(B):
            pass

        with pytest.raises(SystemError):
            C(1).x = 2

I'm pretty sure we do have such a test; I just wonder why anything works at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does f144cfa make any sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

That looks good, but (other than the strict XPASS), the test suite still succeeds when you use for base_cls in self._cls.__mro__[1:-1]:.

The only test that looks roughly like the one I suggested is this one, but the key to my test is that the "root" base class has an attrs-generated setattr and the intermediate class has a user-generated __setattr__. It's interesting, even with the __bases__ approach, it seems that there's some problem when some of these are slots classes and others aren't:

    @pytest.mark.parametrize("a_slots", [True, False])
    @pytest.mark.parametrize("b_slots", [True, False])
    @pytest.mark.parametrize("c_slots", [True, False])
    def test_setattr_inherited_do_not_reset_intermediate(self, a_slots, b_slots, c_slots):
        @attr.define(slots=a_slots)
        class A(object):
            x = attr.ib(on_setattr=setters.frozen)

        @attr.define(slots=b_slots)
        class B(A):
            x = attr.ib(on_setattr=setters.NO_OP)
            def __setattr__(self, key, value):
                raise SystemError

        @attr.define(slots=c_slots)
        class C(B):
            pass

        with pytest.raises(SystemError):
            C(1).x = 3

With the __mro__ approach, all 8 of these tests fail, but with the __bases__ approach, we still get failures whenever C is not a slots class. I assume that this is not intentional, and that __setattr__ inheritance should not be reset in this way? I think maybe this "check the base classes for a __setattr__" logic may need to be extended to non-slots classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Argl, yeah so the problem is that we treat "user wrote a __setattr__" and "we wrote a __setattr__" the same way which is obviously wrong. We only ought to reset the latter one. This requires some deeper work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Paul! PAUL! I think I've got it! Please check out c5b690f

Now the tests also fail if you use __mro__[1:-1] AFAICT.

(I've also noticed that we've always used __bases__ in the call to type() so no need to be defensive with getattr() unless someone complains)

pganssle
pganssle previously approved these changes Sep 2, 2020
Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Putting the LGTM on this one even though there are un-addressed comments because I think that my remaining comments are "nice to haves" ­— I think the "meat" of this PR looks good.

If you do make changes and want me to take another look, let me know.

src/attr/_make.py Outdated Show resolved Hide resolved
Fixes #680

Signed-off-by: Hynek Schlawack <hs@ox.cx>
Signed-off-by: Hynek Schlawack <hs@ox.cx>
Signed-off-by: Hynek Schlawack <hs@ox.cx>
Signed-off-by: Hynek Schlawack <hs@ox.cx>
hynek and others added 2 commits September 3, 2020 08:19
Signed-off-by: Hynek Schlawack <hs@ox.cx>
Co-authored-by: Paul Ganssle <paul@ganssle.io>
@hynek hynek dismissed pganssle’s stale review September 4, 2020 04:44

New discoveries :(

Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

Nice, I think this should cover it (or at least covers the test scenarios I came up with; this stuff is complicated enough that I doubt I'll ever be confident that it will "survive contact with the enemy").

Definitely the subject for another PR (since the scope of this one has already significantly expanded), but I wonder if it would make sense to add some hypothesis tests that generate inheritance hierarchies and assert some properties of the subclasses. I imagine doing so might be a bit complicated, but there do seem to be some useful properties we could assert here.

tests/test_setattr.py Outdated Show resolved Hide resolved
Co-authored-by: Paul Ganssle <paul@ganssle.io>
@hynek hynek merged commit 4875590 into master Sep 5, 2020
@hynek hynek deleted the slots-setattr-regression branch September 5, 2020 07:46
@hynek
Copy link
Member Author

hynek commented Sep 5, 2020

Thanks Paul!

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.

Using slots class overrides custom __setattr__ in 20.1.0
2 participants