Skip to content

Commit

Permalink
add __builtins__ to globals in py.run() if they're missing
Browse files Browse the repository at this point in the history
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
  • Loading branch information
GoldsteinE committed Aug 11, 2023
1 parent 28b8623 commit 0be94a5
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
1 change: 1 addition & 0 deletions newsfragments/3378.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add `__builtins__` to globals in `py.run()` and `py.eval()` if they're missing.
42 changes: 41 additions & 1 deletion src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,9 @@ impl<'py> Python<'py> {
/// If `globals` is `None`, it defaults to Python module `__main__`.
/// If `locals` is `None`, it defaults to the value of `globals`.
///
/// If `globals` doesn't contain `__builtins__`, default `__builtins__`
/// will be added automatically.
///
/// # Examples
///
/// ```
Expand All @@ -568,6 +571,9 @@ impl<'py> Python<'py> {
/// If `globals` is `None`, it defaults to Python module `__main__`.
/// If `locals` is `None`, it defaults to the value of `globals`.
///
/// If `globals` doesn't contain `__builtins__`, default `__builtins__`
/// will be added automatically.
///
/// # Examples
/// ```
/// use pyo3::{
Expand Down Expand Up @@ -632,6 +638,29 @@ impl<'py> Python<'py> {
.unwrap_or_else(|| ffi::PyModule_GetDict(mptr));
let locals = locals.map(AsPyPointer::as_ptr).unwrap_or(globals);

// If `globals` don't provide `__builtins__`, most of the code will fail if Python
// version is <3.10. That's probably not what user intended, so insert `__builtins__`
// for them.
//
// See also:
// - https://github.com/python/cpython/pull/24564 (the same fix in CPython 3.10)
// - https://github.com/PyO3/pyo3/issues/3370
let builtins_s = crate::intern!(self, "__builtins__").as_ptr();
let has_builtins = ffi::PyDict_Contains(globals, builtins_s);
if has_builtins == -1 {
return Err(PyErr::fetch(self));
}
if has_builtins == 0 {
// Inherit current builtins.
let builtins = ffi::PyEval_GetBuiltins();

// `PyDict_SetItem` doesn't take ownership of `builtins`, but `PyEval_GetBuiltins`
// seems to return a borrowed reference, so no leak here.
if ffi::PyDict_SetItem(globals, builtins_s, builtins) == -1 {
return Err(PyErr::fetch(self));
}
}

let code_obj = ffi::Py_CompileString(code.as_ptr(), "<string>\0".as_ptr() as _, start);
if code_obj.is_null() {
return Err(PyErr::fetch(self));
Expand Down Expand Up @@ -1031,7 +1060,7 @@ impl<'unbound> Python<'unbound> {
#[cfg(test)]
mod tests {
use super::*;
use crate::types::{IntoPyDict, PyList};
use crate::types::{IntoPyDict, PyDict, PyList};
use crate::Py;
use std::sync::Arc;

Expand Down Expand Up @@ -1166,4 +1195,15 @@ mod tests {
assert!(v.eq(py.Ellipsis()).unwrap());
});
}

#[test]
fn test_py_run_inserts_globals() {
Python::with_gil(|py| {
let namespace = PyDict::new(py);
py.run("class Foo: pass", Some(namespace), Some(namespace))
.unwrap();
assert!(namespace.get_item("Foo").is_some());
assert!(namespace.get_item("__builtins__").is_some());
})
}
}

0 comments on commit 0be94a5

Please sign in to comment.