Sage: Ticket #14524: Decorator for methods requiring mutability or immutability
https://trac.sagemath.org/ticket/14524
<p>
Some methods (most notably <code>__hash__</code>) require that an object is immutable. Other methods (such as <code>add_vertex</code> of a graph) require that an object is mutable.
</p>
<p>
I suggest to introduce a method decorator, such that (im)mutability is automatically verified before calling the method. Note that the patch just creates the decorator, but does not use it yet. In a second step, I plan to use it on graphs.
</p>
<p>
The decorator tests immutability by the value of an attribute called <code>_is_immutable</code>, which is used by <code>sage.structure.mutability.Mutability</code> anyway (but I change this attribute into a public attribute). Unfortunately, <code>sage.graphs.generic_graph.GenericGraph.__hash__</code> uses another name, namely <code>_immutable</code>. But I think this can be changed in the second step.
</p>
<p>
From the doctests:
</p>
<pre class="wiki"> sage: from sage.structure.mutability import require_mutable, require_immutable
sage: class A:
... def __init__(self, val):
... self._m = val
... @require_mutable
... def change(self, new_val):
... 'change self'
... self._m = new_val
... @require_immutable
... def __hash__(self):
... 'implement hash'
... return hash(self._m)
sage: a = A(5)
sage: a.change(6)
sage: hash(a)
Traceback (most recent call last):
...
AssertionError: <type 'instance'> instance is mutable, <function __hash__ at ...> must not be called
sage: a._is_immutable = True
sage: hash(a)
6
sage: a.change(7) # indirect doctest
Traceback (most recent call last):
...
AssertionError: <type 'instance'> instance is immutable, <function change at ...> must not be called
sage: from sage.misc.sageinspect import sage_getdoc
sage: print sage_getdoc(a.change)
change self
</pre><p>
As one can see, by default an object is supposed to be mutable. This may be important during initialisation of the object. So, one would first initialise it and then declare that it is (from now on) immutable.
</p>
<p>
<span class="underline">Questions</span>
</p>
<ul><li>Is <code>AssertionError</code> the right thing to raise here? Usually, when hashing does not work, a <code>TypeError</code> is raised.
</li><li>One could be more intrusive. For example, when calling a method decorated by <code>require_immutable</code> on a mutable object, one could instead <em>make</em> it immutable (by assignment to <code>_is_immutable</code>. In that way, after the first call to <code>hash</code>, the object can not be accidentally changed (but of course, if the user wants so, <code>_is_immutable</code> can be deleted). Good idea, or not pythonic?
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/14524
Trac 1.1.6SimonKingFri, 03 May 2013 13:55:53 GMTstatus changed
https://trac.sagemath.org/ticket/14524#comment:1
https://trac.sagemath.org/ticket/14524#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketvbraunFri, 03 May 2013 20:44:40 GMT
https://trac.sagemath.org/ticket/14524#comment:2
https://trac.sagemath.org/ticket/14524#comment:2
<p>
+1 for TypeError, or perhaps subclass <code>MutabilityError(TypeError)</code> so you can just try/except on the mutability.
</p>
<p>
If you also want to change the mutability in the decorator then you should provide additional ones like <code>@force_immutable</code>. Though this might be overengineering things.
</p>
TicketSimonKingMon, 06 May 2013 07:17:54 GMTattachment set
https://trac.sagemath.org/ticket/14524
https://trac.sagemath.org/ticket/14524
<ul>
<li><strong>attachment</strong>
set to <em>trac14524-immutability_decorators.patch</em>
</li>
</ul>
TicketSimonKingMon, 06 May 2013 07:19:36 GMT
https://trac.sagemath.org/ticket/14524#comment:3
https://trac.sagemath.org/ticket/14524#comment:3
<p>
I have changed it into <code>ValueError</code>, because this is what is raised in <code>Mutability._require_mutable</code> as well.
</p>
<p>
I agree that it would be over-engineering to change mutability when hash is called.
</p>
TicketSimonKingMon, 06 May 2013 07:22:01 GMT
https://trac.sagemath.org/ticket/14524#comment:4
https://trac.sagemath.org/ticket/14524#comment:4
<p>
By the way, here is some data on the overhead:
</p>
<pre class="wiki">sage: from sage.structure.mutability import require_mutable
sage: class A:
....: def bla(self):
....: return 1
....: @require_mutable
....: def blabla(self):
....: return 1
....:
sage: a = A()
sage: %timeit x=a.bla()
1000000 loops, best of 3: 404 ns per loop
sage: %timeit x=a.blabla()
1000000 loops, best of 3: 1.78 us per loop
sage: a._is_immutable = False
sage: %timeit x=a.blabla()
1000000 loops, best of 3: 711 ns per loop
</pre>
TicketSimonKingMon, 06 May 2013 07:22:23 GMTtype changed
https://trac.sagemath.org/ticket/14524#comment:5
https://trac.sagemath.org/ticket/14524#comment:5
<ul>
<li><strong>type</strong>
changed from <em>PLEASE CHANGE</em> to <em>enhancement</em>
</li>
</ul>
TicketvbraunSun, 02 Jun 2013 20:24:34 GMTstatus, milestone changed; reviewer set
https://trac.sagemath.org/ticket/14524#comment:6
https://trac.sagemath.org/ticket/14524#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Volker Braun</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-5.10</em> to <em>sage-5.11</em>
</li>
</ul>
<p>
Looks good to me.
</p>
TicketjdemeyerThu, 06 Jun 2013 12:33:13 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/14524#comment:7
https://trac.sagemath.org/ticket/14524#comment:7
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.11.beta0</em>
</li>
</ul>
Ticket