T325 weakref with slots by altendky · Pull Request #420 · python-attrs/attrs

altendky · 2018-08-01T16:12:28Z

altendky · 2018-08-02T01:27:56Z

Hmm, I should probably add a test to confirm that no extra fields are added. Kind of a given I think, but seeing as the workaround involved creating one and I don't think we want that, it seems worth a couple lines of test code I show and verify that desire.

I should also probably change my added tests to use hypothesis... :|

hynek · 2018-08-08T06:05:07Z

Hmmm, IIUC, you’re only setting __weakref__ if it's not set already, right? ISTM like we could set the option to True by default because I simply can’t come up with a realistic scenario that might break here?

It’s even questionable whether it should be an option at all, but if it is, it might even be renamed to slots_weakref or something since it has a very narrow use case.

Am I missing something @Tinche @njsmith?

njsmith · 2018-08-08T07:54:50Z

I'd be fine with switching on the weakref=True behavior unconditionally. I only really suggested making it a switch because @Tinche was against wasting 8 bytes of memory per instance when a slotted type is never actually weakref'ed, so I was hoping making it an option would sneak it past his objections :-).

altendky · 2018-08-08T19:49:33Z

@hynek, yes, the intent was to only add "__weakref__" if it wasn't already in cd... a thing that was just created that explicitly avoids including "__weakref__". So that check is pointless. But I am missing a check for "__weakref__" already being in the super class. Adding now. Presently type() raises an exception.

TypeError: __weakref__ slot disallowed: either we already got one, or __itemsize__ != 0

Same thing happens with an attribute named __weakref__.

altendky · 2018-08-08T20:30:24Z

If the option is kept and renamed, perhaps weakref_slot? That seems to fit a bit better in my head. It is a thing (like init or hash or slots) which we either create or don't.

Would we want an exception in case of slots=False and weakref=True? Though this would make more sense if it were weakref_slot. If not renamed, it could be argued that slots=False, weakref=True is actually valid since you can weakref without slots and that the invalid case is actually slots=False, weakref=False since we don't disable weakrefability. So I think yes, rename to be slot-specific and raise an exception if set to True without slots.

hynek

I think to me it’s more important that we do what’s right/expected so I’m gonna say: make it an option but make it True by default. People like Tin who care about 8 bytes can always set it to False.
Yes, it should be renamed. weakref_slot sounds fine.
Yes, exception would be good.

changelog.d/420.change.rst

@@ -0,0 +1 @@
+Add weakref parameter to attr.s() allowing for weak referenceable slots classes.


src/attr/_make.py

               ``object.__setattr__(self, "attribute_name", value)``.

        ..  _slots: https://docs.python.org/3/reference/datamodel.html#slots
+    :param bool weakref: Make instancess weak-referenceable.  This has no


altendky · 2018-08-12T03:24:58Z

Once we switch to weakref_slot=True as the name and default then an exception becomes less obvious to me. It seems the case to raise an exception then would be a regular non-slotted class without specifying weakref_slot=False because, well, a slot was 'requested' (by default) and we aren't going to be making it. Since we will be defaulting to True, weakref or weakrefable makes more sense to me and we raise an exception in any case where the requested state is not achieved (though that leads to exceptions in PyPy which wouldn't occur in CPython). I dunno, I guess I'm not totally sold on the exception (that I suggested).

I'm coding towards weakref_slot=True but won't add the exception yet.

altendky · 2018-08-12T03:49:53Z

Changing the default broke some tests so I stashed that and went ahead and added parametrization to them for weakref_slots True/False to show the issue first. But, it's bedtime so I'll have to try to fix that later. Then back to making the default True.

njsmith · 2018-08-12T03:53:05Z

It does make sense to me that weakref=True would mean "this has to support weakrefs", and weakref=False would mean "I don't care if this supports weakrefs". I don't think anyone is writing code that relies on getting an exception when attempting to take a weakref, and it would be very annoying if you had to go through and remove all your weakref=False kwargs in order to port to PyPy.

hynek · 2018-08-19T05:23:16Z

FWIF, I can live with not having an exception.

I’d like to point out tho, that it’s specifically weakref_slot=True not weakref=True for the reason you mentioned.

weakref=False sounds like “explodes on weakref” and the name makes it clear, that it’s a slots thing.

Otherwise I think everything we settled before is still true? True by default so Tin can save his precious bytes, …?

altendky · 2018-08-19T14:35:12Z

I couldn't reason to a sensible exception with the name and default weakref_slot=True. Presumably we wouldn't want the default to result in an exception in any case. And for the value False, we wouldn't want the default of a not-slotted class to raise an exception, I mean it doesn't have a weakref slot as requested... So I'm just not sure what to implement for an exception.

Anyways, I'm not waiting on anyone but myself to get back to this and figure out the failures.

hynek · 2018-08-20T04:24:35Z

Nah it’s fine, as I said, you can drop the exception. Default True, the docs explain that it’s a NOP on dict classes.

altendky · 2018-08-20T20:47:28Z

Do we have outstanding actions on this at this point? Should any of the tests I added use Hypothesis? Or others that haven't been added but should be? weakref_slot has already been added so that other things using the Hypothesis strategies should get variations against weakref_slot.

hynek

I think it’s mostly fine now, it just needs a bunch of updates where is still refers to weakref instead of weakref_slot.

Also please add an example to tests/typing_example.py to ensure the stubs work (we’ve almost missed the wrong name!).

changelog.d/420.change.rst

@@ -0,0 +1 @@
+Add weakref parameter to attr.s() allowing for weak referenceable slotted classes.


docs/glossary.rst


-      - As always with slotted classes, you must specify a ``__weakref__`` slot if you wish for the class to be weak-referenceable.
-        Here's how it looks using ``attrs``:
+      - Slotted classes are weak-referenceable by default.  This can be disabled in CPython by passing ``weakref_slot=False`` to ``@attr.s`` [#pypyweakref]_.


src/attr/__init__.pyi

    init: bool = ...,
    slots: bool = ...,
    frozen: bool = ...,
+    weakref: bool = ...,


src/attr/__init__.pyi

    init: bool = ...,
    slots: bool = ...,
    frozen: bool = ...,
+    weakref: bool = ...,


src/attr/__init__.pyi

    init: bool = ...,
    slots: bool = ...,
    frozen: bool = ...,
+    weakref: bool = ...,


src/attr/_make.py

               ``object.__setattr__(self, "attribute_name", value)``.

        ..  _slots: https://docs.python.org/3/reference/datamodel.html#slots
+    :param bool weakref: Make instances weak-referenceable.  This has no


tests/strategies.py

-    If `slots=True` is passed in, only slots classes will be generated, and
-    if `slots=False` is passed in, no slot classes will be generated. The same
-    applies to `frozen`.
+    By default, all combinations of slots, frozen, and weakref classes will be


tests/strategies.py

+    By default, all combinations of slots, frozen, and weakref classes will be
+    generated. If `slots=True` is passed in, only slots classes will be
+    generated, and if `slots=False` is passed in, no slot classes will be
+    generated. The same applies to `frozen` and `weakref`.


altendky · 2018-08-21T11:08:11Z

@hynek, sorry... i thought i had done all that a few days ago.

hynek · 2018-08-25T07:01:10Z

Thanks!

altendky added 3 commits August 1, 2018 11:51

Add weakref parameter to attr.s()

168275a

python-attrs#325

Add weakref to simple_classes() strategy

70cbec8

Rename changelog file with actual PR (420.change.rst)

1113a58

altendky and others added 4 commits August 2, 2018 12:24

Add test_weakref_does_not_add_a_field()

03e8513

Add docstrings to new weakref tests

7a3b8ea

Merge branch 'master' into t325-weakref_with_slots

32e6fa1

Add missing blank line

bf43f85

altendky and others added 2 commits August 8, 2018 16:26

Correct weakref handling of __weakref__ attributes or an inherrited slot

4cec94f

Merge branch 'master' into t325-weakref_with_slots

21daee9

hynek requested changes Aug 11, 2018

View reviewed changes

altendky added 3 commits August 11, 2018 23:48

Correct typos and terms

5c2d82f

Rename to weakref_slot

fedbf6c

Parametrize more tests against weakref_slot True/False to expose trouble

e46604f

altendky added 4 commits August 20, 2018 16:04

Check if __weakref__ has been inherited via a __dict__

73c9ab3

Correct copy/pasted comment

9176851

Change weakref_slot default to True

6f00899

Merge branch 'master' into t325-weakref_with_slots

51cae57

hynek requested changes Aug 21, 2018

View reviewed changes

hynek added this to the 18.2 milestone Aug 21, 2018

Tidy and correct weakref -> weakref_slot

75a90ca

hynek approved these changes Aug 25, 2018

View reviewed changes

Merge branch 'master' into t325-weakref_with_slots

b8efa70

hynek merged commit 7fe111c into python-attrs:master Aug 25, 2018

hynek mentioned this pull request Sep 1, 2018

First class __weakref__ support #325

Closed

ghost mentioned this pull request Sep 2, 2018

attrs versions available: 18.2.0 Harmon758/Harmonbot#316

Closed

altendky deleted the t325-weakref_with_slots branch September 28, 2020 17:05

		@@ -0,0 +1 @@
		Add weakref parameter to attr.s() allowing for weak referenceable slots classes. No newline at end of file

Uh oh!

Conversation

altendky commented Aug 1, 2018

Uh oh!

altendky commented Aug 2, 2018

Uh oh!

hynek commented Aug 8, 2018

Uh oh!

njsmith commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

altendky commented Aug 8, 2018

Uh oh!

altendky commented Aug 8, 2018

Uh oh!

hynek left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

altendky commented Aug 12, 2018

Uh oh!

altendky commented Aug 12, 2018

Uh oh!

njsmith commented Aug 12, 2018

Uh oh!

hynek commented Aug 19, 2018

Uh oh!

altendky commented Aug 19, 2018

Uh oh!

hynek commented Aug 20, 2018

Uh oh!

altendky commented Aug 20, 2018

Uh oh!

hynek left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

altendky commented Aug 21, 2018

Uh oh!

hynek commented Aug 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

njsmith commented Aug 8, 2018 •

edited

Loading