-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
base: main
Are you sure you want to change the base?
Conversation
- 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.
There was a problem hiding this 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.
Doc/library/functions.rst
Outdated
specify the globals and locals respectively. If provided, *globals* must be | ||
a dictionary. If provided, *locals* can be any mapping object. |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There's something I don't quite feel is right in the current doc about
This is unclear, because elsewhere (in Language Reference > Data Model, it says that the two are only "approximately" analogous:
Indeed this is a "key difference": a 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 ... 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 So, what if we just... remove that sentence?
|
Also, the footnote on
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 |
Also I'd like to be more affirmative in the Note box on "modifying the default local namespace returned by |
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.
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. |
This PR is stale because it has been open for 30 days with no activity. |
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depends on scope information determined at compile-time. Interaction | |
depends on scope information determined at compile time. Interaction |
This unfortunately has a couple of merge conflicts now. Would you mind fixing those? |
@congma Would you still be interested in fixing the merge conflicts? I can open a separate one to get the conflicts resolved if not. |
See #100003 for conflict resolution of the |
There was a problem hiding this 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.
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 And if you don't make the requested changes, you will be put in the comfy chair! |
This does have merge conflicts, so a new PR (#100003) was opened to resolve part of the conflict. I'll open another PR on |
The following commit authors need to sign the Contributor License Agreement: |
execution's limitation with respect to namespaces.
argument accepts.
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().
__builtins__
in the text: clarify that inthe 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.
https://bugs.python.org/issue43605