compiler-core: implement DerefMut and IndexMut for Constants - #8557
Conversation
Sorry, something went wrong.
`Constants<C>` exposed `Deref` and `Index<ConstIdx>` but neither mutable counterpart, so an owner of a `CodeObject` could read a constant but not replace one in place and had to rebuild the whole boxed slice. `[T]` already carries both halves of the `VarNum` pair in this file. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 29f946a4-cc14-4c6d-9d3e-ad8dc907ea14 📥 CommitsReviewing files that changed from the base of the PR and between 86d9407 and 4349d4a. 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 Walkthrough WalkthroughConstants<C> now supports mutable access to its underlying constant slice and individual constants through oparg::ConstIdx. ChangesMutable constant access
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to 4349d This localized API addition enables mutable constant access without evidence of a correctness or production-impact risk at the current head; no actionable merge-blocking risk remains after normal checks. Possibly related PRs
Suggested reviewers: shaharnaveh 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Constants<C> is a newtype over Box<[C]> that exposes Deref and
Index<oparg::ConstIdx> but neither mutable counterpart. An owner of a
CodeObject can therefore read a constant but cannot replace one in place —
the field is public, but Constants' own .0 is not, so the only way to
change a single entry is to map the whole slice and rebuild it with
FromIterator.
This adds the two missing halves. The file already carries both sides of the
same pair for VarNum (impl<T> Index<oparg::VarNum> for [T] and its
IndexMut), so this makes ConstIdx symmetric with it.
No behaviour change: both impls are new trait implementations over the
existing private field, and nothing in the tree currently mutates a
Constants value.
Motivation: an out-of-tree consumer of rustpython-compiler-core that owns
its CodeObjects rewrites nested code constants (the _imp._fix_co_filename
path) and today has to rebuild the constants array to do it, which also moves
every entry and invalidates pointers into it. RustPython itself does not need
this — update_code_filenames reaches its nested code objects as already
wrapped PyRef<PyCode> and mutates them through interior mutability — so this
is an API-completeness change rather than a fix for in-tree code.
— commented by Claude
Summary by CodeRabbit