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

Created on 2012-03-19 17:51 by Yury.Selivanov, last changed 2012-12-29 03:40 by meador.inge.

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 (11)
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.
History
Date User Action Args
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