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

add __builtins__ to globals in py.run() if they're missing #3378

Merged
merged 1 commit into from Aug 11, 2023

Conversation

GoldsteinE
Copy link
Contributor

@GoldsteinE GoldsteinE commented Aug 11, 2023

Python code doesn't like to run without __builtins__, so adding them if missing seems to be a good idea. Also that's what CPython >3.10 does.

See python/cpython#24564
Resolves #3370

@GoldsteinE GoldsteinE force-pushed the implicit-builtins branch 3 times, most recently from f3e1a67 to 1ebcb0a Compare August 11, 2023 10:19
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Many thanks, implementation looks great. My only thought: after evaluation should we attempt to remove __builtins__ from the globals dictionary if we added it?

Comment on lines +651 to +658
// `PyDict_SetItem` doesn't take ownership of `builtins`, but `PyEval_GetBuiltins`
// seems to return a borrowed reference, so no leak here.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@GoldsteinE
Copy link
Contributor Author

Many thanks, implementation looks great. My only thought: after evaluation should we attempt to remove builtins from the globals dictionary if we added it?

I think that’s more confusing than the current option (especially given possibility of code like __builtins__ = new_value). CPython uses some private APIs to specify __builtins__ for the new frame explicitly instead of adding them to globals, so we can’t easily mimic its behaviour here (in particular, we’d need _PyFunction_FromConstructor: I don’t see a way to manually specify __builtins__ otherwise).

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Agreed, in which case I think let's just document that globals will be modified in this way (on both Python::run and Python::eval) and call it a day.

newsfragments/3378.changed.md Outdated Show resolved Hide resolved
Python code doesn't like to run without `__builtins__`, so adding them
if missing seems to be a good idea. Also that's what CPython >3.10 does.

See python/cpython#24564
Resolves PyO3#3370
@GoldsteinE
Copy link
Contributor Author

Fixed the docs.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks again!

@davidhewitt davidhewitt added this pull request to the merge queue Aug 11, 2023
Merged via the queue into PyO3:main with commit 8031794 Aug 11, 2023
32 checks passed
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.

Python 3.{8,9}: NameError: __build_class__ not found when globals are provided to py.run()
2 participants