make function __closure__ writable

[YurySelivanov]

BPO	14369
Nosy	@rhettinger, @ncoghlan, @pitrou, @benjaminp, @asvetlov, @meadori, @ericsnowcurrently, @akvadrako
Files	writable_closure.patch writable_closure_02.patch writable_closure_03.patch writable_closure_04.patch writable_closure_with_checking.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2012-03-19.17:51:16.433>
labels = ['interpreter-core', 'type-feature', '3.7']
title = 'make function __closure__ writable'
updated_at = <Date 2022-03-28.16:35:30.895>
user = 'https://bugs.python.org/YurySelivanov'

bugs.python.org fields:

activity = <Date 2022-03-28.16:35:30.895>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2012-03-19.17:51:16.433>
creator = 'Yury.Selivanov'
dependencies = []
files = ['24946', '24947', '24962', '24975', '25363']
hgrepos = []
issue_num = 14369
keywords = ['patch']
message_count = 14.0
messages = ['156350', '156357', '156404', '156412', '156452', '159117', '159182', '159235', '159238', '159287', '159519', '304056', '304106', '304199']
nosy_count = 10.0
nosy_names = ['rhettinger', 'ncoghlan', 'pitrou', 'benjamin.peterson', 'asvetlov', 'meador.inge', 'Yury.Selivanov', 'sbt', 'eric.snow', 'akvadrako']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14369'
versions = ['Python 3.7']

Linked PRs

• gh-58577: make function __closure__ writable #104946

[YurySelivanov]

__code__ and __closure__ are designed to work together. There is no point in allowing to completely substitute the __code__ object, but protecting the __closure__.

[YurySelivanov]

Updated patch as per Andrew's code review. Thank you.

[asvetlov]

Please update the doc also. I think changing from 'Read-only' to 'Writable' in Doc/reference/datamodel.rst is enough.

[YurySelivanov]

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.

[YurySelivanov]

Updated patch per Benjamin's review. See writable_closure_04.patch.

[ncoghlan]

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/

[sbt]

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.

[sbt]

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

[YurySelivanov]

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.

[sbt]

Version of patch which checks invariants in the setter and adds tests.

[asvetlov]

sbt, looks good for me.

[akvadrako]

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.

[ncoghlan]

This still seems like a reasonable enhancement, but would presumably need updates to apply against the 3.7 development branch.

[ncoghlan]

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)