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

builtin discovery #37

Merged
merged 2 commits into from
Oct 17, 2019
Merged

builtin discovery #37

merged 2 commits into from
Oct 17, 2019

Conversation

llllllllll
Copy link
Collaborator

Fixes two issues in the way the builtins get found for a frame:

  1. The builtins are only inherited from the parent if f_back.f_globals is f_globals.
  2. If we can't inherit the builtins and the name __builtins__ is undefined in the f_globals, then the builtins are a new dictionary with a value of {'None': None}.

References to the CPython source are provided in the two commit messages.

Joe Jevnik added 2 commits October 15, 2019 13:31
According the CPython, the parent's builtins are only selected if the
`f_globals` of the parent are exactly the same as the `f_globals` of the
function to be executed. If not, the builtins are read from the `__builtins__`
of the current function's `f_globals`, not the `f_locals`.

https://github.com/python/cpython/blob/3cd21aa6a1467723ccc85e6411a6cbe7fa81ef76/Objects/frameobject.c#L624-L654
When a frame's globals differ from it's parents, or a frame is the top of the
execution stack, but there is no `__builtins__`, then the builtins should just
be {'None': None}.

https://github.com/python/cpython/blob/3cd21aa6a1467723ccc85e6411a6cbe7fa81ef76/Objects/frameobject.c#L636-L643
def f(NameError=NameError, AssertionError=AssertionError):
# capture NameError and AssertionError early because
# we are deleting the builtins
None
Copy link

@gerrymanoim gerrymanoim Oct 16, 2019

Choose a reason for hiding this comment

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

Is the None here just a test that the builtins are now {'None': None}

Choose a reason for hiding this comment

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

That's None of your business.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should add a comment here. The default builtins, when not inherited nor provided in the globals, is just the mapping {'None': None}. Having this name lookup here was supposed to test that the name None resolves to None. I believe this test is insufficient in Python 3 where None will compile into a LOAD_CONST instead of a LOAD_GLOBAL or LOAD_NAME. I am not totally sure how to test that None is actually in the builtins given that by definition of this test, there is no __builtins__ object in scope.

@gerrymanoim
Copy link

One small understanding question, but otherwise LGTM.

@llllllllll llllllllll merged commit 62f9b1a into master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants