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

T325 weakref with slots #420

Merged
merged 18 commits into from Aug 25, 2018
Merged

Conversation

@altendky
Copy link
Contributor

@altendky altendky commented Aug 1, 2018

@altendky
Copy link
Contributor Author

@altendky altendky commented Aug 2, 2018

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... :|

altendky added 4 commits Aug 2, 2018
@hynek
Copy link
Member

@hynek hynek commented Aug 8, 2018

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
Copy link

@njsmith njsmith commented Aug 8, 2018

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
Copy link
Contributor Author

@altendky altendky commented Aug 8, 2018

@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 added 2 commits Aug 8, 2018
@altendky
Copy link
Contributor Author

@altendky altendky commented Aug 8, 2018

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.

Copy link
Member

@hynek hynek left a comment

  1. 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.
  2. Yes, it should be renamed. weakref_slot sounds fine.
  3. Yes, exception would be good.
@@ -0,0 +1 @@
Add weakref parameter to attr.s() allowing for weak referenceable slots classes.

This comment has been minimized.

@hynek

hynek Aug 11, 2018
Member

We call them “slotted classes”: http://www.attrs.org/en/stable/glossary.html as you do later in the diff. :)

This comment has been minimized.

@altendky

altendky Aug 12, 2018
Author Contributor

FYI, a search actually shows several hits for slots class. But yes, I'm correcting this.

@@ -719,6 +730,8 @@ def attrs(
``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

This comment has been minimized.

@hynek

hynek Aug 11, 2018
Member

“instancess”

@altendky
Copy link
Contributor Author

@altendky altendky commented Aug 12, 2018

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
Copy link
Contributor Author

@altendky altendky commented Aug 12, 2018

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
Copy link

@njsmith njsmith commented Aug 12, 2018

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
Copy link
Member

@hynek hynek commented Aug 19, 2018

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
Copy link
Contributor Author

@altendky altendky commented Aug 19, 2018

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
Copy link
Member

@hynek hynek commented Aug 20, 2018

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
Copy link
Contributor Author

@altendky altendky commented Aug 20, 2018

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.

Copy link
Member

@hynek hynek left a comment

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!).

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

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

This needs an update of the name and attr.s() should be in double backticks.

@@ -43,21 +43,11 @@ Glossary
Those methods are created for frozen slotted classes because they won't pickle otherwise.
`Think twice <https://www.youtube.com/watch?v=7KnfGDajDQw>`_ before using :mod:`pickle` though.

- 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]_.

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

We use semantic newlines please.

@@ -164,6 +164,7 @@ def attrs(
init: bool = ...,
slots: bool = ...,
frozen: bool = ...,
weakref: bool = ...,

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

needs update

@@ -180,6 +181,7 @@ def attrs(
init: bool = ...,
slots: bool = ...,
frozen: bool = ...,
weakref: bool = ...,

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

needs update

@@ -207,6 +209,7 @@ def make_class(
init: bool = ...,
slots: bool = ...,
frozen: bool = ...,
weakref: bool = ...,

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

needs update

@@ -759,6 +785,8 @@ def attrs(
``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

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

needs update

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

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

needs update

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`.

This comment has been minimized.

@hynek

hynek Aug 21, 2018
Member

needs update

@hynek hynek added this to the 18.2 milestone Aug 21, 2018
@altendky
Copy link
Contributor Author

@altendky altendky commented Aug 21, 2018

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

@hynek
hynek approved these changes Aug 25, 2018
@hynek hynek merged commit 7fe111c into python-attrs:master Aug 25, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek
Copy link
Member

@hynek hynek commented Aug 25, 2018

Thanks!

@altendky altendky deleted the altendky:t325-weakref_with_slots branch Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.