Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Feb 9, 2026

Thanks @jaens

Summary by CodeRabbit

  • Refactor
    • Switched the internal synchronization approach for source path handling. This preserves existing behavior while reducing internal overhead and simplifying access semantics; no user-facing functionality changes.

@youknowone youknowone marked this pull request as ready for review February 9, 2026 12:06
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

Replaced a mutex-based field with an atomic pointer: PyCode.source_path now uses AtomicPtr<PyStrInterned>. Constructors and accessors were updated to store/load raw pointers with Ordering::Relaxed, relying on interned strings never being deallocated.

Changes

Cohort / File(s) Summary
PyCode source_path change
crates/vm/src/builtins/code.rs
Replaced PyMutex<&'static PyStrInterned> with AtomicPtr<PyStrInterned>; updated constructor, source_path() accessor, and set_source_path() to use atomic load/store (Ordering::Relaxed). Adjusted imports (added AtomicPtr, Ordering) and removed mutex usage for this field.
Manifest
Cargo.toml
Minor changes to manifest lines (+14/-7) included in this diff (no code-level semantics altered here).

Sequence Diagram(s)

(omitted — changes are localized and do not introduce multi-component control flow requiring a sequence diagram)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 I swapped the lock for atomic cheer,
A pointer hops, no crowding near.
Relaxed it dances, light and spry,
Interned strings watch from on high. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing a synchronization primitive (PyMutex) with AtomicPtr for the PyCode.source_path field, which is the core modification in the changeset.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
crates/vm/src/builtins/code.rs (1)

349-359: Ordering::Relaxed is sufficient here, and the unsafe is sound.

Since the pointed-to PyStrInterned is immutable and 'static (interned strings are never deallocated), there's no dependent data requiring acquire/release synchronization. The only invariant is atomicity of the pointer swap, which Relaxed provides.

One minor observation: there's no doc-comment on set_source_path explaining why this is safe to call concurrently (i.e., the 'static + immutable invariant). Consider adding a brief // SAFETY: comment matching the one on source_path().

Suggested safety comment
     pub fn set_source_path(&self, new: &'static PyStrInterned) {
+        // SAFETY: interned strings are never deallocated, so the pointer remains valid
         self.source_path.store(
             new as *const PyStrInterned as *mut PyStrInterned,
             Ordering::Relaxed,
         );
     }

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@crates/vm/src/builtins/code.rs`:
- Line 22: The file contains an unused import of PyMutex (the line "use
rustpython_common::lock::PyMutex;") which should be removed; locate the use
statement referencing PyMutex in this module (e.g., the import line in code.rs)
and delete it so there are no unused imports or compile warnings/errors related
to PyMutex.

Add `source_path: AtomicPtr<PyStrInterned>` field to `PyCode`
for interior mutability, replacing the UB-inducing
`#[allow(invalid_reference_casting)]` + `write_volatile` pattern
in `update_code_filenames`. Use atomic load/store instead of a
mutex since the operation is a simple pointer swap on a 'static
reference. Update all read sites to use `source_path()` accessor.
@youknowone youknowone merged commit f594b0a into RustPython:main Feb 10, 2026
12 of 13 checks passed
@youknowone youknowone deleted the source-path branch February 10, 2026 16:28
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.

1 participant