Add eval function to c-api#7927
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new public ChangesPython C-API Code Evaluation and Compilation
Sequence Diagram(s)sequenceDiagram
participant C as C_client
participant CevalCompile as capi::ceval::Py_CompileString
participant VM as rustpython_vm::VM
C->>CevalCompile: pass `code`, `filename`, `start` (C strings)
CevalCompile->>VM: convert strings, call compile(mode)
VM-->>CevalCompile: Compiled PyCode or CompileError
CevalCompile-->>C: return PyObject* or raise SyntaxError/SystemError
sequenceDiagram
participant C as C_client
participant CevalEval as capi::ceval::PyEval_EvalCode
participant VM as rustpython_vm::VM
participant Builtins as VM::builtins
C->>CevalEval: pass `co`, `globals`, `locals`
CevalEval->>VM: prepare Scope(globals, locals, Builtins)
CevalEval->>VM: VM.run_code_object(PyCode, Scope)
VM-->>CevalEval: result PyObject or exception
CevalEval-->>C: return PyObject* or exception
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/capi/src/ceval.rs (1)
64-84: 🏗️ Heavy liftThis test module doesn't cover the new FFI surface.
#[cfg(false)]keeps it permanently disabled, and the assertions call PyO3'seval/runAPIs rather thanPy_CompileStringorPyEval_EvalCode. Please replace this with real coverage for the exported C-API entrypoints or drop the dead module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/capi/src/ceval.rs` around lines 64 - 84, The disabled test module (mod tests) uses #[cfg(false)] and exercises PyO3's Python::eval/run rather than the crate's exported C-API; either remove this dead module or replace it with tests that call the real FFI surface (e.g., tests that invoke the exported functions which wrap Py_CompileString and PyEval_EvalCode) — update or add test functions (replace test_code_eval and test_code_run_exception) to call the crate's C-API entrypoints, assert correct return values and that exceptions from the FFI path produce PyException instances, and remove #[cfg(false)] so the tests run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/ceval.rs`:
- Around line 59-61: PyEval_GetBuiltins currently returns the interpreter-wide
vm.builtins unconditionally; change it to check vm.current_frame() and, if
Some(frame), return that frame's InterpreterFrame.builtins raw pointer,
otherwise fall back to vm.builtins. Update the implementation inside with_vm to
call vm.current_frame(), access frame.builtins (via as_object().as_raw() or
equivalent), and only use vm.builtins when no current frame is present so the
function honors per-frame __builtins__ overrides.
- Around line 11-22: The FFI functions (notably Py_CompileString and other C
entrypoints that call CStr::from_ptr or dereference raw pointers like &*co and
&*globals) must validate incoming pointers before dereferencing to avoid UB:
check that raw pointers (code, filename, co, globals) are non-null (use
NonNull::new or explicit null checks) and return an appropriate Python exception
via the VM when null or invalid UTF-8 occurs; then safely call CStr::from_ptr
and only dereference pointers after wrapping them in NonNull and converting to
references. Ensure you mirror the existing locals pattern (NonNull::new(locals))
for code, filename, co, globals and produce the VM error return path instead of
dereferencing null.
---
Nitpick comments:
In `@crates/capi/src/ceval.rs`:
- Around line 64-84: The disabled test module (mod tests) uses #[cfg(false)] and
exercises PyO3's Python::eval/run rather than the crate's exported C-API; either
remove this dead module or replace it with tests that call the real FFI surface
(e.g., tests that invoke the exported functions which wrap Py_CompileString and
PyEval_EvalCode) — update or add test functions (replace test_code_eval and
test_code_run_exception) to call the crate's C-API entrypoints, assert correct
return values and that exceptions from the FFI path produce PyException
instances, and remove #[cfg(false)] so the tests run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 34637ca5-adfb-4270-8cf4-97d2937d543d
📒 Files selected for processing (3)
crates/capi/Cargo.tomlcrates/capi/src/ceval.rscrates/capi/src/lib.rs
2ec3a03 to
9057729
Compare
| .map_err(|_| vm.new_system_error("Py_CompileString called with non UTF-8 filename"))?; | ||
|
|
||
| let mode = match start { | ||
| 256 => Mode::Single, |
There was a problem hiding this comment.
what's these numbers? do they have proper name or symbol?
There was a problem hiding this comment.
I've added constants for these values.
Summary by CodeRabbit