Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOCS: Suggest always calling exec with a globals argument and no locals argument #119235

Merged
merged 2 commits into from
May 20, 2024

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented May 20, 2024

Many users think they want a locals argument for various reasons but they do not understand that it makes code be treated as a class definition. They do not want their code treated as a class definition and get surprised. The reason not to pass locals specifically is that the following code raises a NameError:

exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})

The reason not to leave out globals is as follows:

def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)

📚 Documentation preview 📚: https://cpython-previews--119235.org.readthedocs.build/

…ls argument

Many users think they want a locals argument for various reasons but they do not
understand that it makes code be treated as a class definition. They do not want
their code treated as a class definition and get surprised. The reason not
to pass locals specifically is that the following code raises a `NameError`:

```py
exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})

```
The reason not to leave out globals is as follows:
```py
def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)
```
@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels May 20, 2024
@gpshead gpshead added skip issue needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels May 20, 2024
@gpshead gpshead self-assigned this May 20, 2024
same overall meaning.
@gpshead gpshead enabled auto-merge (squash) May 20, 2024 17:32
@gpshead gpshead merged commit 7e1a130 into python:main May 20, 2024
25 checks passed
@miss-islington-app
Copy link

Thanks @hoodmane for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 20, 2024
…ls argument (pythonGH-119235)

Many users think they want a locals argument for various reasons but they do not
understand that it makes code be treated as a class definition. They do not want
their code treated as a class definition and get surprised. The reason not
to pass locals specifically is that the following code raises a `NameError`:

```py
exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})
```

The reason not to leave out globals is as follows:

```py
def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)
```
(cherry picked from commit 7e1a130)

Co-authored-by: Hood Chatham <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 20, 2024
…ls argument (pythonGH-119235)

Many users think they want a locals argument for various reasons but they do not
understand that it makes code be treated as a class definition. They do not want
their code treated as a class definition and get surprised. The reason not
to pass locals specifically is that the following code raises a `NameError`:

```py
exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})
```

The reason not to leave out globals is as follows:

```py
def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)
```
(cherry picked from commit 7e1a130)

Co-authored-by: Hood Chatham <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

GH-119239 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 20, 2024
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

GH-119240 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label May 20, 2024
gpshead pushed a commit that referenced this pull request May 20, 2024
…no locals argument (GH-119235) (#119240)

DOCS: Suggest always calling exec with a globals argument and no locals argument (GH-119235)

Many users think they want a locals argument for various reasons but they do not
understand that it makes code be treated as a class definition. They do not want
their code treated as a class definition and get surprised. The reason not
to pass locals specifically is that the following code raises a `NameError`:

```py
exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})
```

The reason not to leave out globals is as follows:

```py
def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)
```
(cherry picked from commit 7e1a130)

Co-authored-by: Hood Chatham <[email protected]>
@hoodmane hoodmane deleted the docs-exec-locals branch May 20, 2024 18:15
gpshead pushed a commit that referenced this pull request May 20, 2024
…no locals argument (GH-119235) (#119239)

DOCS: Suggest always calling exec with a globals argument and no locals argument (GH-119235)

Many users think they want a locals argument for various reasons but they do not
understand that it makes code be treated as a class definition. They do not want
their code treated as a class definition and get surprised. The reason not
to pass locals specifically is that the following code raises a `NameError`:

```py
exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})
```

The reason not to leave out globals is as follows:

```py
def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)
```
(cherry picked from commit 7e1a130)

Co-authored-by: Hood Chatham <[email protected]>
@ncoghlan
Copy link
Contributor

ncoghlan commented May 21, 2024

Just a heads up that I included a rewording of this note in the PEP 667 docs PR at #119201 (see https://github.com/python/cpython/pull/119201/files#r1607516295 ) after hitting a conflict between this change and that one.

Users pass a separate locals to exec all the time in order to capture writes, and it only breaks if the code being executed defines and calls functions that attempt to access top-level variables (or includes a class definition that needs to access the class by name). The updated wording both specifies what breaks, and also specifies how to use collections.ChainMap to capture writes while still running the code with module semantics:

   .. note::

      When ``exec`` gets two separate objects as *globals* and *locals*, the
      code will be executed as if it were embedded in a class definition. This
      means functions and classes defined in the executed code will not be
      able to access variables assigned at the top level (as the "top level"
      variables are treated as class variables in a class definition).
      Passing a :class:`collections.ChainMap` instance as *globals* allows name
      lookups to be chained across multiple mappings without triggering this
      behaviour. Values assigned to top level names in the executed code can be
      retrieved by passing an empty dictionary as the first entry in the chain.

That PR will only get backported as far as 3.13 (since the rest of the PR is specific to a 3.13 change). I'm not sure it's worth worrying about changing the note in 3.12, though.

@hoodmane
Copy link
Contributor Author

Thanks @ncoghlan! I wanted to keep to as few words as possible here so that the PR wouldn't get bogged down in discussion. I am always concerned that each added word makes it more likely that the PR leads to an endless discussion, I run out of energy, and it goes stale.

@hoodmane
Copy link
Contributor Author

Passing a :class:collections.ChainMap instance as globals

Interesting!

@ncoghlan
Copy link
Contributor

I actually need to revisit that updated note, as I was reminded today that globals has to be a real dict, so passing ChainMap doesn't work:

>>> from collections import ChainMap
>>> ns = ChainMap({}, globals())
>>> exec("print(__name__)", globals())
__main__
>>> exec("print(__name__)", ns)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: exec() globals must be a dict, not ChainMap

(and there are unfortunately genuine performance reasons for that restriction, so relaxing it would be easier said than done)

ncoghlan added a commit to ncoghlan/cpython that referenced this pull request May 22, 2024
When updating the new exec note added in pythongh-119235 as part of the
PEP 667 general docs PR, I suggested a workaround that isn't valid.

Since it's far from the first time I've considered that workaround,
and the fact it doesn't work has surprised me every time, amend the
new note to explicitly state that dict merging is the only option.
@ncoghlan
Copy link
Contributor

PR fixing the error: #119378 (and tagged @gpshead for a review)

ncoghlan added a commit that referenced this pull request May 22, 2024
When updating the new exec note added in gh-119235 as part of the
PEP 667 general docs PR, I suggested a workaround that isn't valid.

The first half of the note is still reasonable, so just omit the invalid text.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 22, 2024
When updating the new exec note added in pythongh-119235 as part of the
PEP 667 general docs PR, I suggested a workaround that isn't valid.

The first half of the note is still reasonable, so just omit the invalid text.
(cherry picked from commit 31d61a7)

Co-authored-by: Alyssa Coghlan <[email protected]>
ncoghlan added a commit that referenced this pull request May 22, 2024
When updating the new exec note added in gh-119235 as part of the
PEP 667 general docs PR, I suggested a workaround that isn't valid.

The first half of the note is still reasonable, so just omit the invalid text.

(cherry picked from commit 31d61a7)

Co-authored-by: Alyssa Coghlan <[email protected]>
@hoodmane
Copy link
Contributor Author

I think what people may want when they pass locals is for globals to work like a custom overlay over builtins and locals to work like globals? I wonder if that is possible.

@gpshead
Copy link
Member

gpshead commented May 22, 2024

Thanks Alyssa, I wasn't totally happy with the wording in this PR but felt it started to shift us towards the right thing to express. I like your wording better. On of my key issues is that long existing "the code will be executed as if it were embedded in a class definition" wording (from before this PR) is the kind of thing that most docs readers are not likely to understand as it's "weird" but most people need never understand it. Your new text expands upon that in an accessible descriptive manner.

@CNSeniorious000
Copy link

CNSeniorious000 commented Jun 3, 2024

for globals to work like a custom overlay over builtins and locals to work like globals? I wonder if that is possible.

I think the best practise to implement layered context is to use ChainMap.

Let's say you have 2 maps, a Mapping global_namespace (acts as the readonly fallback layer, like __builtins__) and a MutableMapping local_namespace (acts as the globals). And build a map namespace = ChainMap(local_namespace, global_namespace) and getitem will first try local_namespace than try global_namespace, while setitem will never modify local_namespace.

But! eval's globals can only be a dict, not a ChainMap, so as to pyodide.runPython. So you have to use a mixin hack like this:

from collections import ChainMap

class ChainMap(ChainMap, dict):
    pass

Reproduction:

>>> from collections import ChainMap

>>> g, l = {"a": 2}, {}

>>> source = """
... def f():
...     return g() + 1
...
... def g():
...     return a
...
... a = f()
... """

>>> exec(source, ChainMap(l, g))
Traceback (most recent call last):
  File "<console>", line 1, in <module>
TypeError: globals must be a real dict; try eval(expr, {}, mapping)

>>> class ChainMap(ChainMap, dict):
...     pass
...
>>> exec(source, ChainMap(l, g))

>>> g
{'a': 2}

>>> l
{'f': <function f at 0xa04368>, 'g': <function g at 0x12654d0>, 'a': 3}

I think its a common use case to avoid pollution on the global namespace when using exec and eval, so maybe CPython can make ChainMap to inherit from dict so that we don't need to mixin anymore?

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…ls argument (pythonGH-119235)

Many users think they want a locals argument for various reasons but they do not
understand that it makes code be treated as a class definition. They do not want
their code treated as a class definition and get surprised. The reason not
to pass locals specifically is that the following code raises a `NameError`:

```py
exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
""", {}, {})
```

The reason not to leave out globals is as follows:

```py
def t():
    exec("""
def f():
    print("hi")

f()

def g():
    f()
g()
    """)
```
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
When updating the new exec note added in pythongh-119235 as part of the
PEP 667 general docs PR, I suggested a workaround that isn't valid.

The first half of the note is still reasonable, so just omit the invalid text.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants