| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
- 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.
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.
Sorry, something went wrong.
| 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.
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})
1Surprisingly, 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
This works as intended:
>>> ct = Counter("abcd")
>>> eval("a", {}, ct)
1But not this:
>>> eval("print(a)", {}, ct)Expected return value of eval is None (return value of print function), but the cryptic exception is raised.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, me too. This is the weirdest thing I've seen in Python in a while.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
There's something I don't quite feel is right in the current doc about exec with both globals and locals arguments supplied:
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 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 definedThe 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?
|
Sorry, something went wrong.
|
Also, the footnote on exec() expecting LF newline only.
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
|
||
| 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.
| This function supports the dynamic evaluation of Python expressions. The | |
| This function dynamically evaluates Python expressions. The |
Sorry, something went wrong.
| 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.
Maybe link this to the documentation for code objects.
Sorry, something went wrong.
| *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.
| 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
Sorry, something went wrong.
| 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.
| global namespace that is explicitly or impcitly specified for :func:`eval`. | |
| global namespace that is explicitly or implicitly specified. |
Sorry, something went wrong.
| :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.
| depends on scope information determined at compile-time. Interaction | |
| depends on scope information determined at compile time. Interaction |
Sorry, something went wrong.
|
This unfortunately has a couple of merge conflicts now. Would you mind fixing those? |
Sorry, something went wrong.
|
@congma Would you still be interested in fixing the merge conflicts? I can open a separate one to get the conflicts resolved if not. |
Sorry, something went wrong.
|
See #100003 for conflict resolution of the eval section |
Sorry, something went wrong.
There was a problem hiding this comment.
This has merge conflicts now.
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
The following commit authors need to sign the Contributor License Agreement: |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
Hello! I'm sorry but I'm closing this PR as it is stale, has requested changes that have not been addressed in quite some time, an unsigned CLA, and the issue as been resolved. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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().
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.
https://bugs.python.org/issue43605