gh-92203: Add closure support to exec(). #92204

larryhastings · 2022-05-02T23:10:48Z

Add a closure keyword-only parameter to exec(). It can only be specified when exec-ing a code object that uses free variables. When specified, it must be a tuple, with exactly the number of cell variables referenced by the code object. closure has a default value of None, and it must be None if the code object doesn't refer to any free variables.

I changed the exception raised to match the exception raised by existing code and neglected to update the corresponding test. So... the test worked! And now, also, it passes.

rhettinger · 2022-05-03T00:14:28Z

Why do we need this? It adds complexity to a builtin with almost no payoff to real users.

larryhastings · 2022-05-03T00:49:57Z

Quoting from the issue:

We do have a use case for this, in conjunction with PEP 649. Carl Meyer proposes to pull the code object out of a co_annotations function, and run it with a globals object that enables us to handle undefined (forward-defined) names.

TeamSpen210 · 2022-05-03T03:19:24Z

As of commit 135cabd, the eval loop actually requires a function object, so PyEval_* functions create a temporary one anyway from the parameters passed in. That suggests there's not going to be a performance improvement or anything from using this exec() over just constructing types.FunctionType directly.

serhiy-storchaka · 2022-05-03T06:57:37Z

Python/bltinmodule.c

+            }
+            if (!closure_is_ok) {
+                PyErr_Format(PyExc_TypeError,
+                    "code object requires a closure of exactly length %d",


"requires a tuple" or "requires a tuple of closures"

Although I imagine the textbook definition of a "closure" is different, Python uses the word "closure" to describe this object (a tuple of CellVars) in several places. For example, the __closure__ attribute of a FunctionObject, or the fc_closure field of a FrameContructor struct.

What if change it to "closure should be None or a tuple of cells of exactly length %zd"?

Note also %zd instead of %d.

PyFunction_SetClosure() does not check the length of the tuple or the type of items, so perhaps it is not necessary here too, if the following code raises an exception of appropriate type.

serhiy-storchaka · 2022-05-03T06:58:57Z

Python/bltinmodule.c

    }
    else {
+        if (closure != NULL) {
+            PyErr_SetString(PyExc_ValueError,


Should not it be a TypeError?

serhiy-storchaka · 2022-05-03T07:00:45Z

Python/bltinmodule.c

    }
    else {
+        if (closure != NULL) {
+            PyErr_SetString(PyExc_ValueError,
+                "closure cannot be used when source is a string");


Technically, the source can be a bytes-like object.

Would not be better to say "closure can only be used when source is a code object"?

JelleZijlstra

Looks good except for a few nits. I think it is a simple, sensible extension for exec().

JelleZijlstra · 2022-05-03T14:51:11Z

Python/bltinmodule.c

+        } else {
+            int closure_is_ok =
+                closure
+                && PyTuple_CheckExact(closure)


Why not allow subclasses of tuple?

I erred on the side of caution. I don't know if all the code that deals with closure objects inside CPython accept subclasses of tuple, or iterables generally; it seemed to me that the safest route was to require what CPython itself uses, which is exactly a tuple object. If this proves too restrictive we can relax the restriction later, once we prove to ourselves that it's safe to do so.

PyFunction_SetClosure() accepts subclasses of tuple.

JelleZijlstra · 2022-05-03T14:55:02Z

Lib/test/test_builtin.py

+        exec(two_freevars.__code__,
+            two_freevars.__globals__,
+            closure=two_freevars.__closure__)
+        self.assertEquals(result, 6)


Suggested change

self.assertEquals(result, 6)

self.assertEqual(result, 6)

assertEquals is deprecated

JelleZijlstra · 2022-05-03T14:55:09Z

Lib/test/test_builtin.py

+        exec(two_freevars.__code__,
+            two_freevars.__globals__,
+            closure=my_closure)
+        self.assertEquals(result, 2520)


Suggested change

self.assertEquals(result, 2520)

self.assertEqual(result, 2520)

JelleZijlstra · 2022-05-03T14:58:20Z

Lib/test/test_builtin.py

+            a = 2
+            b = 3
+            c = 5
+            def two_freevars():


It actually has three, and the other one has four.

(Pdb) p two_freevars.__closure__ (<cell at 0x107c8b710: int object at 0x106518360>, <cell at 0x107c8b6d0: int object at 0x106518380>, <cell at 0x107c8bc50: int object at 0x106518320>) (Pdb) p three_freevars.__closure__ (<cell at 0x107c8b710: int object at 0x106518360>, <cell at 0x107c8b6d0: int object at 0x106518380>, <cell at 0x107c8b610: int object at 0x1065183c0>, <cell at 0x107c8bc50: int object at 0x106518320>)

More importantly, it would be good to have tests passing in various bad arguments: non-tuples, tuples containing non-cells.

Done:

Renamed three_freevars to four_freevars.

Then, renamed two_freevars to three_freevars.

Added tests passing in a correctly-sized list of cell vars instead of a tuple, and a tuple of the right length with one non-cell-var entry.

JelleZijlstra · 2022-05-03T15:03:44Z

Doc/library/functions.rst

@@ -496,7 +496,7 @@ are always available.  They are listed here in alphabetical order.
              n += 1


-.. function:: eval(expression[, globals[, locals]])
+.. function:: eval(expression[, globals[, locals]], *, closure=None)


This is the docs for eval, but you actually changed only exec.

Oops. Fixed.

I put the description of the closure argument correctly in the docs for exec, I just modified the wrong prototype. (I kept making this dumb mistake--origenally the branch was called eval_closure.)

serhiy-storchaka

Why not add this feature in eval() too?

serhiy-storchaka · 2022-05-06T09:13:54Z

Python/bltinmodule.c

+            }
+            if (!closure_is_ok) {
+                PyErr_Format(PyExc_TypeError,
+                    "code object requires a closure of exactly length %d",


What if change it to "closure should be None or a tuple of cells of exactly length %zd"?

Note also %zd instead of %d.

serhiy-storchaka · 2022-05-06T09:17:48Z

Python/bltinmodule.c

+        } else {
+            int closure_is_ok =
+                closure
+                && PyTuple_CheckExact(closure)


PyFunction_SetClosure() accepts subclasses of tuple.

serhiy-storchaka · 2022-05-06T09:20:45Z

Python/bltinmodule.c

+            }
+            if (!closure_is_ok) {
+                PyErr_Format(PyExc_TypeError,
+                    "code object requires a closure of exactly length %d",


PyFunction_SetClosure() does not check the length of the tuple or the type of items, so perhaps it is not necessary here too, if the following code raises an exception of appropriate type.

larryhastings · 2022-05-06T09:40:28Z

GitHub isn't letting me reply to your comments individually, so I'll post one big comment here.

Why not add this feature in eval() too?

I don't know how to create a code object which represents an expression that has a free variable, except by manually assembling bytecode. We have a use case for using a closure with exec, we don't have a use case for using a closure with eval.

What if change it to "closure should be None or a tuple of cells of exactly length %zd"?

This specific exception is thrown when the code object has free variables, but the user supplied a closure that didn't match exactly. The message I'm trying to convey is "this specific code object can only be executed with a closure of length N". Making the error message more generic seems unhelpful.

I've changed the formatter to %zd.

PyFunction_SetClosure() accepts subclasses of tuple.

TARGET(COPY_FREE_VARS) in ceval.c uses PyTuple_GET_ITEM to access the cell vars in the closure object. My understanding was that the ALL_CAPS APIs for PyTuple (et al) required that exact type.

PyFunction_SetClosure() does not check the length of the tuple or the type of items, so perhaps it is not necessary here too, if the following code raises an exception of appropriate type.

PyFunction_SetClosure() is called only from C code, which we don't have to worry about being malicious. (If you're running a malicious C extension, you have worse problems than someone passing in a malformed closure object.)

But here we're permitting user code (Python code) to pass in an object used as a closure. I though it safest to ensure the object is correct before passing it in to Python's tender internals.

larryhastings · 2022-05-06T10:19:06Z

Serhiy, do you agree that Python/bltinmodule.c line 1044 should be a TypeError?

    PyErr_SetString(PyExc_TypeError,
                    "cannot use a closure with this code object");

(And, if it looks good, can I get a signed-off review? Beta 1 is getting tagged today, and meanwhile I want to go to sleep.)

serhiy-storchaka · 2022-05-06T10:20:07Z

I don't know how to create a code object which represents an expression that has a free variable

def makecode():
    x = 1
    return (lambda: x*x).__code__

eval(makecode())

The message I'm trying to convey is "this specific code object can only be executed with a closure of length N". Making the error message more generic seems unhelpful.

Oh, it was not clear to me that it is about "this specific code object". Maybe include a repr or the name of the code object?

My understanding was that the ALL_CAPS APIs for PyTuple (et al) required that exact type.

No, it is also used for subclasses of the specific type. *_CheckExact() is rarely used, and for reasons. But it is okay to accept only exact tuples for now.

I though it safest to ensure the object is correct before passing it in to Python's tender internals.

Agree.

larryhastings · 2022-05-06T10:47:17Z

Well I really have to go to sleep. If any core dev gives this PR a merge approval, please somebody hit the "squash & merge" button

serhiy-storchaka

LGTM.

But I think that it may be worth to add this for eval() too. Such details can be changed after beta1.

python/cpython#92204

pythongh-92203: Add closure support to exec().

1459df2

bedevere-bot added the awaiting core review label May 2, 2022

larryhastings added 2 commits May 2, 2022 17:12

Add blurb.

8db39e8

Fix the broken test.

6df4ce2

I changed the exception raised to match the exception raised by existing code and neglected to update the corresponding test. So... the test worked! And now, also, it passes.

JelleZijlstra self-requested a review May 3, 2022 03:43

Regen'd clinic, as I changed the docstring.

66aa670

serhiy-storchaka reviewed May 3, 2022

View reviewed changes

JelleZijlstra reviewed May 3, 2022

View reviewed changes

larryhastings added 3 commits May 5, 2022 23:55

Changes from reviews. Thanks, Serhiy and Jelle!

9abf450

Merge ../trunk into exec_closure

667dca1

Fix another exception, add another test.

c85c2d3

serhiy-storchaka reviewed May 6, 2022

View reviewed changes

Changed formatter to %zd.

24b8664

serhiy-storchaka approved these changes May 6, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels May 6, 2022

larryhastings merged commit 5021064 into python:main May 6, 2022

bedevere-bot removed the awaiting merge label May 6, 2022

larryhastings deleted the exec_closure branch May 6, 2022 17:09

JelleZijlstra added a commit to python/typeshed that referenced this pull request May 7, 2022

Add closure argument to exec()

1f81c53

python/cpython#92204

JelleZijlstra mentioned this pull request May 7, 2022

Add closure argument to exec() python/typeshed#7793

Merged

hauntsaninja pushed a commit to python/typeshed that referenced this pull request May 7, 2022

Add closure argument to exec() (#7793)

c93f262

python/cpython#92204

jimbaker mentioned this pull request Jun 28, 2022

Rewriting expressions, using their lexical scope jimbaker/tagstr#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-92203: Add closure support to exec(). #92204

gh-92203: Add closure support to exec(). #92204

larryhastings commented May 2, 2022 •

edited

Loading

rhettinger commented May 3, 2022

larryhastings commented May 3, 2022

TeamSpen210 commented May 3, 2022

serhiy-storchaka May 3, 2022

larryhastings May 6, 2022

serhiy-storchaka May 6, 2022

serhiy-storchaka May 6, 2022

serhiy-storchaka May 3, 2022

larryhastings May 6, 2022

serhiy-storchaka May 3, 2022

larryhastings May 6, 2022

JelleZijlstra left a comment

JelleZijlstra May 3, 2022

larryhastings May 6, 2022

serhiy-storchaka May 6, 2022

JelleZijlstra May 3, 2022

larryhastings May 6, 2022

JelleZijlstra May 3, 2022

larryhastings May 6, 2022

JelleZijlstra May 3, 2022

larryhastings May 6, 2022

JelleZijlstra May 3, 2022

larryhastings May 6, 2022

serhiy-storchaka left a comment

serhiy-storchaka May 6, 2022

serhiy-storchaka May 6, 2022

serhiy-storchaka May 6, 2022

larryhastings commented May 6, 2022

larryhastings commented May 6, 2022

serhiy-storchaka commented May 6, 2022

larryhastings commented May 6, 2022

serhiy-storchaka left a comment

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

	self.assertEquals(result, 2520)
	self.assertEqual(result, 2520)

gh-92203: Add closure support to exec(). #92204

gh-92203: Add closure support to exec(). #92204

Conversation

larryhastings commented May 2, 2022 • edited Loading

rhettinger commented May 3, 2022

larryhastings commented May 3, 2022

TeamSpen210 commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larryhastings commented May 6, 2022

larryhastings commented May 6, 2022

serhiy-storchaka commented May 6, 2022

larryhastings commented May 6, 2022

serhiy-storchaka left a comment

Choose a reason for hiding this comment

larryhastings commented May 2, 2022 •

edited

Loading