classification
Title: make __closure__ writable
Type: enhancement Stage: patch review
Components: Interpreter Core Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Yury.Selivanov, akvadrako, asvetlov, benjamin.peterson, eric.snow, meador.inge, ncoghlan, pitrou, rhettinger, sbt
Priority: normal Keywords: patch

Created on 2012-03-19 17:51 by Yury.Selivanov, last changed 2017-10-12 02:46 by ncoghlan.

Files
File name Uploaded Description Edit
writable_closure.patch Yury.Selivanov, 2012-03-19 19:34 review
writable_closure_02.patch Yury.Selivanov, 2012-03-19 19:50 review
writable_closure_03.patch Yury.Selivanov, 2012-03-20 14:35 review
writable_closure_04.patch Yury.Selivanov, 2012-03-20 20:03 review
writable_closure_with_checking.patch sbt, 2012-04-25 14:33 review
Messages (14)
msg156350 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2012-03-19 17:51
__code__ and __closure__ are designed to work together.  There is no point in allowing to completely substitute the __code__ object, but protecting the __closure__.
msg156357 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2012-03-19 19:50
Updated patch as per Andrew's code review.  Thank you.
msg156404 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-03-20 13:37
Please update the doc also. I think changing from 'Read-only' to 'Writable' in Doc/reference/datamodel.rst is enough.
msg156412 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2012-03-20 14:35
> Please update the doc also. I think changing from 'Read-only' to 'Writable' in Doc/reference/datamodel.rst is enough.

Updated in writable_closure_03.patch.  Thanks.
msg156452 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2012-03-20 20:03
Updated patch per Benjamin's review. See writable_closure_04.patch.
msg159117 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-04-24 04:54
Another use case for a writeable __closure__ attribute is to make it possible to manually break reference cycles:
http://blog.ccpgames.com/kristjan/2012/04/23/reference-cycles-with-closures/
msg159182 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-24 18:19
Shouldn't test___closure__() also test what happens when the closure is replaced with None, or a tuple which is too long or too short or contains non-cell objects?

All of these things seem to be checked when you create a new function using types.FunctionType:

>>> h = types.FunctionType(g.__code__, g.__globals__, "h", g.__defaults__, None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: arg 5 (closure) must be tuple
>>> h = types.FunctionType(g.__code__, g.__globals__, "h", g.__defaults__, ())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: g requires closure of length 2, not 0
>>> h = types.FunctionType(g.__code__, g.__globals__, "h", g.__defaults__, (1,2))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: arg 5 (closure) expected cell, found int

I think the setter should make similar checks.  Maybe there is C code which assumes "broken" closures never happen.
msg159235 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-24 23:37
The patch causes crashes.  If I define

  def cell(o):
    def f(): o
    return f.__closure__[0]

  def f():
    a = 1
    b = 2
    def g():
      return a + b
    return g

  g = f()

then I find

  g.__closure__ = None; g()                          -> crash
  g.__closure__ = (cell(3),); g()                    -> crash
  g.__closure__ = (1, 2); g()                        -> SystemError *
  g.__closure__ = (cell(3), cell(4), cell(5)); g()   -> returns 7

* SystemError: ..\Objects\cellobject.c:24: bad argument to internal function
msg159238 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2012-04-24 23:52
> The patch causes crashes.

Yes, that's known.  

First, we need to check, that we can only write tuple of cell objects or None in __closure__ (that's easy to add).  Secondly, perhaps, we can check __closure__ correctness each time we start evaluating a code object.  The latter would offer us better stability, but may also introduce some slowdowns -- need to find some time to implement and benchmark this.
msg159287 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-04-25 14:33
Version of patch which checks invariants in the setter and adds tests.
msg159519 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-04-28 10:56
sbt, looks good for me.
msg304056 - (view) Author: Devin Bayer (akvadrako) * Date: 2017-10-10 15:50
Any updates on this? I'm trying to implement hot module reloading using code from IPython, which tries to modify __closure__ in place. It fails silently and doesn't work, but indeed would be nice to have.
msg304106 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-10-11 05:33
This still seems like a reasonable enhancement, but would presumably need updates to apply against the 3.7 development branch.
msg304199 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-10-12 02:46
Thinking about the interaction between this idea and https://bugs.python.org/issue30744 made me realise that there's a subtlety here that would probably need to be spelled out more clearly in the docs for __closure__ than it is for __code__: any changes made to a function object (whether it's a synchronous function, a generator, or a coroutine) will only affect *future* function invocations, as execution frames capture references to both the code object and all the closure cells when they're created.

(Thought prompted by asking myself "What would happen to existing generator-iterators if you rebound the closure on the generator function?". The answer is "Nothing", but I figure if I had to think about it, that answer likely isn't going to be obvious to folks that are less familiar with how the eval loop works in practice)
History
Date User Action Args
2017-10-12 02:46:41ncoghlansetmessages: + msg304199
2017-10-11 06:26:40rhettingersetnosy: + rhettinger
2017-10-11 05:33:11ncoghlansetmessages: + msg304106
versions: + Python 3.7, - Python 3.3
2017-10-10 15:50:04akvadrakosetmessages: + msg304056
2017-10-10 15:48:16akvadrakosetnosy: + akvadrako
2012-12-29 03:40:25meador.ingesetnosy: + meador.inge
2012-11-13 03:42:01eric.snowsetnosy: + eric.snow
2012-04-28 10:56:50asvetlovsetmessages: + msg159519
2012-04-25 14:33:37sbtsetfiles: + writable_closure_with_checking.patch

messages: + msg159287
2012-04-24 23:52:18Yury.Selivanovsetmessages: + msg159238
2012-04-24 23:37:56sbtsetmessages: + msg159235
2012-04-24 18:19:50sbtsetnosy: + sbt
messages: + msg159182
2012-04-24 04:54:27ncoghlansetmessages: + msg159117
2012-03-21 02:05:53Yury.Selivanovsetnosy: + ncoghlan
2012-03-20 20:03:04Yury.Selivanovsetfiles: + writable_closure_04.patch

messages: + msg156452
2012-03-20 19:59:29asvetlovsetnosy: + benjamin.peterson
2012-03-20 19:58:34asvetlovsetstage: patch review
2012-03-20 14:35:27Yury.Selivanovsetfiles: + writable_closure_03.patch

messages: + msg156412
2012-03-20 13:37:27asvetlovsetmessages: + msg156404
2012-03-19 19:50:58Yury.Selivanovsetfiles: + writable_closure_02.patch

messages: + msg156357
2012-03-19 19:34:06Yury.Selivanovsetfiles: + writable_closure.patch
2012-03-19 19:33:53Yury.Selivanovsetfiles: - writable_closure.patch
2012-03-19 17:51:16Yury.Selivanovcreate