Reduce number of string clones when compiling python source code by ShaharNaveh · Pull Request #8103 · RustPython/RustPython

ShaharNaveh

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: a68f514a-aed0-494f-a450-37233ea846e6

📥 Commits

Reviewing files that changed from the base of the PR and between 0df603a and 215003a.

⛔ Files ignored due to path filters (1)

Lib/test/test_thread.py is excluded by !Lib/**

📒 Files selected for processing (1)

src/lib.rs

✅ Files skipped from review due to trivial changes (1)

src/lib.rs

📝 Walkthrough

Walkthrough

VirtualMachine::compile, compile_with_opts, run_string, run_code_string, and import_file change their source_path/file_path parameter from owned String to &str. PyGetSet::new and three Context getset helpers change name from impl Into<String> to &str. All call sites across the codebase are updated to remove .to_owned() and .clone(). context.rs also receives minor idiomatic Rust refactors unrelated to the signature changes.

Changes

source_path & name: String → &str refactor

Layer / File(s)	Summary
Core compile/run API signature changes `crates/vm/src/vm/compile.rs`, `crates/vm/src/vm/python_run.rs`, `crates/vm/src/import.rs`	`VirtualMachine::compile`, `compile_with_opts`, `run_string`, `run_code_string` change `source_path` from `String` to `&str`; `import_file` changes `file_path` likewise; internal forwarding calls updated accordingly.
PyGetSet::new and Context getset helper signature changes `crates/vm/src/builtins/getset.rs`, `crates/vm/src/vm/context.rs`	`PyGetSet::new` accepts `name: &str` with internal `into()` conversion; `Context::new_readonly_getset`, `new_static_getset`, and `new_getset` change from `impl Into<String>` to `&str`, removing the internal conversion.
VM-internal call-site updates `crates/vm/src/eval.rs`, `crates/vm/src/stdlib/builtins.rs`, `crates/vm/src/stdlib/sys.rs`, `crates/vm/src/vm/interpreter.rs`, `crates/vm/src/vm/mod.rs`	All VM-internal call sites remove `.to_owned()` or `.clone()` when passing `source_path` or `name` to the updated APIs.
External/entry-point call-site updates `crates/capi/src/ceval.rs`, `crates/wasm/src/vm_class.rs`, `crates/stdlib/src/_opcode.rs`, `src/shell.rs`, `src/lib.rs`, `examples/`, `benches/`	C API, WASM, stdlib tests, shell, main library, examples, and benchmarks remove `.to_owned()` / `.clone()` at their respective `vm.compile` / `vm.run_string` call sites.
context.rs idiomatic cleanups `crates/vm/src/vm/context.rs`	`latin1_char_cache` uses `u8::MIN..=u8::MAX`; `latin1_singleton_index` and `new_str` condensed to expression form; `interned_or_new_str` switches to `if let`; `new_exception_type` uses `unwrap_or_else`.
Cosmetic changes `crates/vm/src/vm/thread.rs`	Import reorder and two blank line insertions; no logic changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

RustPython/RustPython#7740: Both PRs modify src/lib.rs's run_file implementation, with the main change passing path by reference instead of to_owned().
RustPython/RustPython#8050: Both PRs modify crates/vm/src/builtins/getset.rs, adding #[must_use] to PyGetSet::new which this PR updates to accept name: &str.

Suggested reviewers

youknowone

Poem

🐇 No more to_owned(), no needless heap fuss,
A &str slice passed without any muss!
The compiler now borrows what once was owned,
Across every call site, the pattern is honed.
Fewer allocations, the rabbit hops free—
Just borrow the string and let memory be! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.79% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: reducing string clones during Python source code compilation, which aligns with all modified files throughout the PR.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}