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

Simple way to make individual attributes immutable? #133

Open
njsmith opened this issue Jan 11, 2017 · 24 comments

Comments

Projects
None yet
6 participants
@njsmith
Copy link

commented Jan 11, 2017

I'm a bad functional programmer and want to mutate parts of my classes. But it'd be nice if there were still a way to mark the parts of my class that are immutable as being immutable in a few key strokes. I usually won't bother if I have to work for it (I'm also lazy), but might catch some bugs. It seems this fits into attrs' general style. attr.ib(readonly=True)? Could be implemented as either a __setattr__ hook like frozen, or with a property.

@hynek hynek added the Feature label Jan 11, 2017

@hynek

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Well, hm. It would be frozen=True to mirror the main option but it really sounds like a lot of complexity/rewriting…

@offbyone

This comment has been minimized.

Copy link

commented Jan 12, 2017

This turns out to be super useful when retrofitting existing codebases to use attrs.

@njsmith

This comment has been minimized.

Copy link
Author

commented Jan 12, 2017

I suggested readonly instead of frozen because I thought frozen might suggest that the object stored there was immutable? Either could work really. And I can't comment on the implementation complexity, I just thought I'd throw it out there as an idle wish :-)

@hynek

This comment has been minimized.

Copy link
Member

commented Feb 4, 2017

Quoth @Lukasa: I’d merge a (good) PR. ;)

@offbyone

This comment has been minimized.

Copy link

commented Feb 4, 2017

Could you provide some pointers as to where one would start, and what a good PR might look like?

@hynek

This comment has been minimized.

Copy link
Member

commented Feb 4, 2017

Maybe @Tinche can be more helpful but I'd guess that we could adapt _frozen_setattrs or something?

The reason for my handwavevey delegation is mostly b/c I dunno. :)

@Tinche

This comment has been minimized.

Copy link
Member

commented Feb 4, 2017

I'm not really sure how you'd go about implementing this.

One way: for a field x, implement a shadow field _x and create a property without a setter. The equivalent of

class A:
    def __init__(self, x):
        self._x = x
    @property
    def x(self):
        return self._x

This is how it's normally done when using manual classes.

Another way would be to install a custom __setattr__ (and __delattr__) that basically looks like this:

class A:
    def __setattr__(self, name, value):
        if name in __frozen_attr_names__:
            raise FrozenAttributeError('{} is frozen'.format(name))
        else:
            object.__setattr__(self, name, value)

This will impose a penalty on every assignment and every __init__ (since you'd have to use object.__setattr__ in __init__ to set the initial value).

I think the first approach is a little better.

Inheritance needs to be taken into account as well.

Maybe there are more ingenious approaches, so comments welcome :)

@offbyone

This comment has been minimized.

Copy link

commented Feb 4, 2017

I was approaching it from the descriptor angle, to be honest. I'm not interested in blocking serious workaround, just making readonly properties relatively easy:

class Attribute:
    def __set__(self, obj, value):
        raise FrozenAttributeError()

Would that not be a direction to start in?

@manishtomar

This comment has been minimized.

Copy link

commented Feb 5, 2017

How about using having a frozen instance and use evolve for mutable attributes? Too much overhead?

@Tinche

This comment has been minimized.

Copy link
Member

commented Feb 5, 2017

class FrozenAttribute:
    def __set__(self, obj, value):
        raise AttributeError


class A:
    x = FrozenAttribute()

    def __init__(self, x):
        self.__dict__['x'] = x

>>> a = A(1)
>>> a.x
1
>>> a.x = 2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "tt.py", line 9, in __set__
    raise AttributeError
AttributeError

Something like this, you mean? It's an interesting approach, but it won't work with slot classes. Maybe it could be made to support slot classes with a little more investigation though.

@Tinche

This comment has been minimized.

Copy link
Member

commented Feb 5, 2017

Gah, if I ever meet @gvanrossum at a conference, I will buy him a drink and then continue to complain for 10 minutes about how immutability sucks in Python.

@Tinche

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

Alright, I've been poking at frozen attributes for slot classes for a few hours now.

Good news: I actually understand Python descriptors now! (I think.) 🤓
Bad news: I don't think we can have frozen attributes for slot classes without speed penalties. 😡
Mediocre news: With a bit of C code we could have perfectly immutable zero-overhead slot classes and attributes. 🤔

Let me elaborate.

Slot classes store their instance contents in a hidden array. As far as I can tell, this space is basically inaccessible directly from Python. This is different from normal classes, which use instance.__dict__, and we can poke at this from Python with no issues.

When a slot class is defined, this space is allocated somewhere and a descriptor is made for each attribute. This descriptor is the only way to actually access this space. The descriptors are then placed into the class __dict__ (descriptors are always placed there).

These descriptors are of type member_descriptor, and are defined in descobject.c. The member_descriptor class/struct is very limited though. I tried subclassing the type - they can't be subclassed. I can wrap them with a different descriptor, hiding the __set__ and __delete__ and proxying __get__, but I can't use their __get__ directly, so there's always the overhead of wrapping so attribute access becomes ~4x slower, which isn't really acceptable to me.

I don't really know how to write C extensions, nor am I sure attrs would allow C extensions. However writing a descriptor in C that duplicates __get__ from member_descriptor and throws exceptions in other methods would get us overhead-free frozen slot classes and attributes. Any takers? 👀

It's also possible I've made a mistake somewhere, this stuff isn't exactly the simplest.

@hynek

This comment has been minimized.

Copy link
Member

commented Feb 11, 2017

I think this is out of question for the reason alone that it will explode in our faces on anything that isn’t CPython, correct?

@Tinche

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

I have a pure Python proof of concept, but it slows down attribute access by a lot. Presumably PyPy would take care of the speed bump. Maybe I'll do some benchmarks.

@Tinche

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

Hm, there's a performance penalty on PyPy too. 0.10 ns +- 0.00 ns vs 2.46 ns +- 0.07 ns. The numbers are so small I'm not even sure the measurements are good.

@hynek

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

Whatever y’all do: it must not affect the performance of other at all.

I’m not gonna stop people driving around with handbrakes on but don’t take any hostages. :)

@glyph

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

It seems pretty clear to me that this isn't going to affect the performance of other cases. We can use our favorite tool to prevent this - subclassing! :)

FrozenAttribute could be a subclass of Attribute which implements the descriptor protocol to reject duplicate writes, and to read from a private storage attribute. The main difference between the __slots__ and non-__slots__ case is that the descriptor that you delegate to in the non-__slots__ case would be a synthetic thing that just writes to __dict__, the attribute that you write to in the __slots__ case in the storage decorator.

This would be a performance hit for the attributes that use it, of course, but, that's fine; the purpose of this is mostly just to prevent errors anyway, and I bet this is exactly the sort of no-op that PyPy will happily JIT away. And for the attributes that don't use it, nothing would change.

@glyph

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2017

Separate thought based on investigating this implementation - _CountingAttr is private, Attribute is public. Yet, Attribute has an undocumented, but public, from_counting_attr method. It would be easier to implement this, as well as just more cleanly factored, to have an as_attribute method on _CountingAttr instead - one which could select its type between FrozenAttribute and Attribute.

@hynek

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

Ugh, from_counting_attr is not prefixed by accident. I think I didn’t consider making Attribute public in the beginning and so it snook in. Doesn’t matter, that one is private and if this doesn’t go anywhere, I’ll prefix it.

Is the descriptor approach possible before we stop shaving off Attribute definitions? FWIW, I could live with shaving them off if someone uses FrozenAttributes.

P.S. I don’t believe in blackmailing but I’ve taken a screenshot of your suggestion to use subclassing just in case. ;)

@Tinche

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

Yes, we'll need descriptors for this. I don't see why we'd use Attributes for this though.

Is the descriptor approach possible before we stop shaving off Attribute definitions?

What does this mean?

@hynek

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

I meant “start”.

@Tinche

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

Ah. Yes it could be done, by making Attributes descriptors. When does the depreciation period end exactly? If it's soon it's not really worth it.

@hynek

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

2017-08-30

I’d say this can wait but I also don’t care so dunno if that counts. :D

@Tinche

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

I've started picking away at this over at #172.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.