Skip to content

bpo-43605: Improve the documentation to exec() and eval() #25039

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

congma
Copy link

@congma congma commented Mar 27, 2021

  • Add links to the relevant section in Language Reference about dynamic
    execution's limitation with respect to namespaces.
  • For eval(), move some explanatory text into a "Note" box.
  • Make the first paragraph of eval's doc consistent about what the first
    argument accepts.
  • For exec(), remove the text about the "globals" optional argument
    having to be a dict but not an instance of a subclass of dict. This is
    no longer true -- the code calls PyDict_Check(), not
    PyDict_CheckExact().
  • Put quotes around the __builtins__ in the text: clarify that in
    the context it means a string key in the dict passed to eval/exec as
    the globals dict. Otherwise, since the identifier builtins refers
    to a module, it can be confusing and misleading.
  • Reordering some paragraphs so that overall the structure is improved.
  • Re-wrap some long lines in RST source.

https://bugs.python.org/issue43605

- Add links to the relevant section in Language Reference about dynamic
  execution's limitation with respect to namespaces.
- For eval(), move some explanatory text into a "Note" box.
- Make the first paragraph of eval's doc consistent about what the first
  argument accepts.
- For exec(), remove the text about the "globals" optional argument
  having to be a dict but not an instance of a subclass of dict. This is
  no longer true -- the code calls PyDict_Check(), not
  PyDict_CheckExact().
- Put quotes around the ``__builtins__`` in the text: clarify that in
  the context it means a string key in the dict passed to eval/exec as
  the globals dict. Otherwise, since the identifier __builtins__ refers
  to a module, it can be confusing and misleading.
- Reordering some paragraphs so that overall the structure is improved.
- Re-wrap some long lines in RST source.
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR to improve Python docs! I think this looks good overall, though I have a few minor comments below.

Additionally, I wonder if this would've been better if it were split into multiple smaller PRs? I found this slightly long to review.

Comment on lines 501 to 502
specify the globals and locals respectively. If provided, *globals* must be
a dictionary. If provided, *locals* can be any mapping object.
Copy link
Member

@Fidget-Spinner Fidget-Spinner Mar 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think locals can be any mapping object. Eg. collections.Counter() is a mapping object (isinstance(collections.Counter('a'), collections.abc.Mapping) is True), but it cannot be used for eval:

>>> Counter("abcd")
Counter({'a': 1, 'b': 1, 'c': 1, 'd': 1})
>>> eval("print(a)", {}, Counter("abcd"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
TypeError: 'int' object is not callable
>>> eval("print(a)", {}, {'a':1})
1

Surprisingly, git blame says this line has been around since this paragraph's inception 14 years ago.

Suggested change
specify the globals and locals respectively. If provided, *globals* must be
a dictionary. If provided, *locals* can be any mapping object.
specify the globals and locals respectively. If provided, *globals*
and *locals* must be dictionaries.

Copy link
Author

@congma congma Mar 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What?? In the source for eval/exec in Python/bltinmodule.c, the locals are checked by PyMapping_Check() while globals checked by PyDict_Check(). I'm not sure why the exception was about "'int' object is not callable" -- it seems the name a did get resolved in the locals through the Counter object, but somehow there was an action to the effect of 1(). This might be a bug! Right now I don't have enough spoons to track it down, but I should be verifying it and maybe submitting a bug report soon. Edit: working as intended, but in a tricky way: see my reply below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works as intended:

>>> ct = Counter("abcd")
>>> eval("a", {}, ct)
1

But not this:

>>> eval("print(a)", {}, ct)

Expected return value of eval is None (return value of print function), but the cryptic exception is raised.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this:

>>> eval("print(1)", {}, ct)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
TypeError: 'int' object is not callable

Copy link
Author

@congma congma Mar 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! The locals have the highest priority in name resolution. The Counter instance (ct in the above example) behaves as if any key is in it (with initial value 0). When the machinery underlying eval does its work, it first looks up the name "print" in the locals given to it. Unfortunately, as a result of how Counter behaves, it gets what it wishes for: the count for the key "print" in the counter ct, which is the int instance 0. Then 0 gets called as the function. The evaluation boils down to 0(1) which raises TypeError.

Copy link
Member

@Fidget-Spinner Fidget-Spinner Mar 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you for investigating this! I learnt a lot :).

I forgot that Counter sort of behaves like a defaultdict and returns 0 for keys it doesn't have.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, me too. This is the weirdest thing I've seen in Python in a while.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think this might need a news item (sometimes significant docs changes require them). Lets see what the core dev says when they review this.

@congma
Copy link
Author

congma commented Mar 28, 2021

There's something I don't quite feel is right in the current doc about exec with both globals and locals arguments supplied:

If exec gets two separate objects as globals and locals, the code will be executed as if it were embedded in a class definition.

This is unclear, because elsewhere (in Language Reference > Data Model, it says that the two are only "approximately" analogous:

The key difference from a normal call to exec() is that lexical scoping allows the class body (including any methods) to reference names from the current and outer scopes when the class definition occurs inside a function.

Indeed this is a "key difference": a class block inside a def block can "dereference" a cell variable bound in the enclosing def block. I think the bytecode emitted is called LOAD_CLASSDEREF. This is not available to exec() because the information about which variable is a cell variable has been fixed before the dynamic exec().

This works:

>>> def f():
...     fcvar = 0
...     def g():
...         class C:
...             a = fcvar
...     g()
>>> f()

This doesn't:

>>> def h():
...     hcvar = 0
...     def henc():
...         exec("a = hcvar", globals(), locals())
...     henc()
>>> h()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in h
  File "<stdin>", line 4, in henc
  File "<string>", line 1, in <module>
NameError: name 'hcvar' is not defined

The exec() call in function henc cannot just "magically" access hcvar as if the statement in the string were compiled with the rest of the program. The compiler doesn't know what the characters "hcvar" mean in the string literal passed to exec(). Unless, of course, if we accommodate rest of the program to it:

...     def henc():
...         hcvar
...         exec("a = hcvar", globals(), locals())

But this works for a totally different reason -- we've already dereferenced the cell variable in the henc block. There are still further differences, such as not being able to say nonlocal hcvar in the string as if the statement were embedded in the program.

So, what if we just... remove that sentence?

If exec gets two separate objects as globals and locals, the code will be executed as if it were embedded in a class definition.

@congma
Copy link
Author

congma commented Mar 28, 2021

Also, the footnote on exec() expecting LF newline only.

Note that the parser only accepts the Unix-style end of line convention. If you are reading the code from a file, make sure to use newline conversion mode to convert Windows or Mac-style newlines.

Is this still true? (Edit: the footnote was introduced in 2009. It's no longer the same parser anymore...)

>>> exec("print('foo')\r\nprint('bar')\r\n")
foo
bar

@congma
Copy link
Author

congma commented Mar 28, 2021

Also I'd like to be more affirmative in the Note box on "modifying the default local namespace returned by locals()", but it seems so far the live modification of the default local namespace is indeed not a well-defined thing. (I wouldn't use the phrase "undefined behaviour", but my understanding is that it involves too much implementation detail). If PEP 558 becomes standard, perhaps we can say something more definitive about modifying the locals.

The documentations for eval/exec are further modified for consistency
and accuracy.

- For both functions, the documentation mostly follows the following
  structure: description of arguments, default values for optional
  arguments, special treatment of the "__builtins__" key, and notices
  about limitations or other noteworthy material. Previously, the
  description about the special "__builtins__" key is intermingled with
  the description about default values for missing arguments. Now, these
  are in separate paragraphs. Also, it is stressed that the special key
  is inserted in-place.
- In the notice about limitations to dynamic eval/exec w.r.t. scopes,
  also mention the assignment expression succinctly.
- Remove the description that exec() with both globals and locals behave
  "as if" the source string is embedded in a class block. Again, a class
  block inside a function can access the non-locals, but this is not
  available to exec().
- Instead of mentioning passing the return values of globals() and
  locals() around, we now say that the programmer can construct their
  own namespaces based on their return values. The reason is that
  currently the semantics of locals() is not very well defined, and in
  the light of PEP 558 it may change in the future.  Explicitly
  constructing new namespaces is more predictable than using the "raw"
  return value of locals(). In the future, this part of the docs can be
  further updated to keep up with the new definition of locals().
- Omit the footnote to exec() about the parser only accepting Unix-style
  LF newlines. This piece of information is outdated.
- Other small fixes for grammar.
@congma
Copy link
Author

congma commented Mar 29, 2021

I've updated the PR with a new commit, in the hope that clarity/accuracy is further improved.

If the review is in favour of the proposed revision, I'll add a NEWS entry.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 29, 2021

The return value is the result of
the evaluated expression. Syntax errors are reported as exceptions. Example:
This function supports the dynamic evaluation of Python expressions. The
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This function supports the dynamic evaluation of Python expressions. The
This function dynamically evaluates Python expressions. The

The return value is the result of
the evaluated expression. Syntax errors are reported as exceptions. Example:
This function supports the dynamic evaluation of Python expressions. The
first argument can be a string or a code object. The optional arguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe link this to the documentation for code objects.

*globals* must be a dictionary. If provided, *locals* can be any
:term:`mapping` object.

In the most common case, the *expression* argument is a string, and it is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In the most common case, the *expression* argument is a string, and it is
When the *expression* argument is a string, it is

Tighten the wording

given, the local namespace defaults to *globals*.

Before evaluation, the special key ``"__builtins__"`` is searched for in the
global namespace that is explicitly or impcitly specified for :func:`eval`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
global namespace that is explicitly or impcitly specified for :func:`eval`.
global namespace that is explicitly or implicitly specified.

:ref:`comprehensions <comprehensions>`, and :term:`generator expressions
<generator expression>` which create an inner scope of their own. Since
Python 3.8, the action of an assignment expression (see :pep:`572`) also
depends on scope information determined at compile-time. Interaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
depends on scope information determined at compile-time. Interaction
depends on scope information determined at compile time. Interaction

@JelleZijlstra
Copy link
Member

This unfortunately has a couple of merge conflicts now. Would you mind fixing those?

@JelleZijlstra JelleZijlstra removed their assignment May 3, 2022
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 6, 2022
@slateny
Copy link
Contributor

slateny commented Oct 9, 2022

@congma Would you still be interested in fixing the merge conflicts? I can open a separate one to get the conflicts resolved if not.

@slateny
Copy link
Contributor

slateny commented Dec 5, 2022

See #100003 for conflict resolution of the eval section

Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has merge conflicts now.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

@slateny
Copy link
Contributor

slateny commented Dec 7, 2022

This has merge conflicts now.

This does have merge conflicts, so a new PR (#100003) was opened to resolve part of the conflict. I'll open another PR on exec once eval's good to go since there's some duplication in there.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

The following commit authors need to sign the Contributor License Agreement:

CLA signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes docs Documentation in the Doc dir
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

8 participants