crates/vm/src/lib.rs (1)

95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Narrow the module visibility to pub(crate).

crates/vm/src/sorting.rs exports only pub(crate) fn timsort, and the sole consumer is crates/vm/src/builtins/list.rs. Declaring the module pub adds an empty public namespace to the crate API and makes an accidental future pub item externally visible.

♻️ Proposed change

-pub mod sorting;
+pub(crate) mod sorting;

🤖 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/vm/src/lib.rs` at line 95, Change the sorting module declaration in
the crate root from public to crate-visible by using `pub(crate) mod sorting`,
while leaving the existing `timsort` implementation and its internal consumer
unchanged.

crates/vm/src/sorting.rs (3)

652-697: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift

Consider moving elements instead of cloning them.

The T: Clone bound forces merge_lo and merge_hi to clone on every element movement. For PyObjectRef each move costs one reference-count increment plus one decrement on the overwritten slot, while CPython moves raw pointers. This is a plausible contributor to the remaining gap against CPython (0.78s vs 0.21s in the PR description).

A bitwise-move implementation (ptr::copy_nonoverlapping into a MaybeUninit buffer, with the buffer treated as logically uninitialized after each element is moved out) removes both the Clone bound and the refcount traffic. It also removes the duplicate-element hazard on the comparator error path, because moved-out slots are never live twice.

Treat this as a follow-up, not a merge blocker. The unsafe code needs careful drop and panic-safety handling.

🤖 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/vm/src/sorting.rs` around lines 652 - 697, The sorting implementation
currently requires T: Clone and clones elements during merge operations. As a
follow-up, update timsort and its merge helpers such as merge_lo and merge_hi to
move elements through a carefully managed MaybeUninit buffer using
non-overlapping pointer copies, removing the Clone bound while preserving
correct ownership, drop, comparator-error, and panic-safety behavior.

---

699-741: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add tests for stability, galloping merges, and comparator errors.

Current coverage misses the highest-risk paths:

• No stability test. Sort (key, index) pairs on key only and assert the indices stay ascending within each key group.
• pseudorandom_collection produces a permutation, because 7919 is prime and coprime with 500. The input has no duplicate keys, so runs of equal elements never occur. Add an input with many repeated keys and at least 64 elements to drive merge_lo and merge_hi galloping.
• one_thousand_elements is one fully descending run, so it returns before any merge. Add a multi-run input (for example alternating ascending and descending blocks).
• No test uses a non-() error type. Add a test where is_lt returns Err after N calls, and assert both that the error propagates and that the sorted slice is still a permutation of the input. That test covers the failure-recovery gap flagged in merge_lo and merge_hi.

Do you want me to generate these tests?

🤖 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/vm/src/sorting.rs` around lines 699 - 741, Expand the sorting tests
around the `sort` helper with a stability test for `(key, index)` pairs using
key-only comparison, repeated-key inputs of at least 64 elements that exercise
galloping merges, and a multi-run input such as alternating ascending and
descending blocks. Add a comparator-error test using a non-`() ` error after a
fixed number of calls, asserting the error propagates and the resulting slice
remains a permutation of the original input.

---

1-3: 📐 Maintainability & Code Quality | 🔵 Trivial

Leftover porting TODO references a symbol that does not exist here.

MERGESTATE_TEMP_SIZE is a CPython C-level constant and has no counterpart in this file. The note describes an intentional design decision (dynamic Vec buffer), not pending work. Consider converting it to a short explanatory // comment or removing it. A second TODO at Line 618 tracks CPython 3.12+ incremental minrun.

Do you want me to open a tracking issue for the incremental minrun improvement?

🤖 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/vm/src/sorting.rs` around lines 1 - 3, Remove the obsolete TODO above
MIN_GALLOP because MERGESTATE_TEMP_SIZE is not defined in this Rust
implementation. If the dynamic Vec buffer choice should remain documented,
replace the TODO with a brief explanatory comment; leave the separate
incremental-minrun TODO near the later sorting logic unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes