Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/refcount.rs`:
- Around line 4-6: The layout comment mentions a "[1 bit: weaked]" flag that has
no corresponding implementation; either remove that token from the comment to
match the current code or add the missing bit definition and accessors. To fix,
decide whether to actually reserve the bit: if unused, update the comment to
drop "weaked" and adjust bit position explanations (remove any reference to
WEAKED/State::weaked); if intended, add a WEAKED constant (e.g., WEAKED_MASK)
and implement State::weaked(&self) -> bool plus setter/clearer logic where State
bits are manipulated (matching how destructed/leaked are handled), and ensure
weak_count/strong_count masks/shift constants are unchanged. Use existing
symbols like State, destructed, leaked, weak_count, strong_count, and follow the
pattern for other flag constants so the comment and code remain consistent.
- Around line 144-153: The dec() method currently does an unconditional
self.state.fetch_sub(COUNT, Ordering::SeqCst) then checks
State::from_raw(...).leaked(), which allows underflow to clear the LEAKED bit;
fix by detecting leaked() after the fetch and immediately restoring the
subtraction (call self.state.fetch_add(COUNT, Ordering::SeqCst)) before
returning false so the LEAKED flag and count aren't corrupted; update dec() to
perform the undo restore when old.leaked() is true and then return false
(optionally note you could replace with a CAS loop to avoid the transient
window).
- Around line 179-193: The deferred-drop thread-local and APIs must be compiled
only when the standard library is available: wrap the thread_local! block
(IN_DEFERRED_CONTEXT and DEFERRED_QUEUE), the DeferredDropGuard struct, and the
functions with_deferred_drops, try_defer_drop, and flush_deferred_drops with
#[cfg(feature = "std")] so they are excluded from no_std builds; ensure the
attribute is applied to each item (or to a surrounding mod) referencing those
symbols so the crate compiles without std just like refcount_overflow() does.
- Around line 99-110: Update the doc comment and safety semantics for pub fn
inc(&self) in refcount.rs: clarify that inc() assumes the caller already holds a
valid strong reference (e.g., used in to_owned()) and is not a resurrection path
— recommend safe_inc() for resurrecting from weak refs; replace the vague
comment "// The previous fetch_add created a permission to run decrement again"
with a brief protocol description (double fetch_add is an optimization to avoid
CAS when caller already had a strong ref and may race with a destructor that is
already in-flight), and make overflow handling consistent with inc_by() by
adding the same strong-count overflow check (use State::strong()/STRONG logic
and call refcount_overflow() on overflow) or, if you intentionally omit overflow
detection, document why it is safe to omit it; reference the symbols inc,
safe_inc, inc_by, State, COUNT, STRONG, destructed, and refcount_overflow in the
change.
In `@crates/vm/src/gc_state.rs`:
- Around line 439-444: The unconditional reset of generations[0].count loses
increments from concurrent track_object calls after gen_locks are dropped;
change the logic in the collection exit paths (where
generations[0].count.store(0, ...) is called) to either (A) switch maybe_collect
to use alloc_count as the authoritative allocation counter and reset alloc_count
there instead of touching generations[0].count, ensuring track_object increments
alloc_count, or (B) recompute gen0's true count after promotions by reading
generation_objects[0] (via its read lock) and store its length into
generations[0].count before releasing locks; update all early-return branches
(the places with generations[0].count.store(0, ...)) to use the chosen approach
so concurrent increments are not lost.
- Around line 734-758: promote_survivors currently removes an object from its
source generation and then silently skips adding it to next_gen if acquiring the
write lock for self.generation_objects[next_gen] fails, orphaning the object;
change the target-generation lock acquisition to propagate a poisoned-lock panic
instead of silently failing by replacing the conditional try-lock for next_gen
with an unconditional write() followed by .expect(...) (e.g. let mut next_set =
self.generation_objects[next_gen].write().expect("poisoned
generation_objects[next_gen] lock");) and then perform next_set.insert(ptr) and
the fetch_add on self.generations[next_gen].count, so that a poisoned lock
triggers a panic and the invariant that every tracked object is in exactly one
generation is preserved.
In `@crates/vm/src/vm/mod.rs`:
- Line 880: The doc comment above the block that clears the `sys` and `builtins`
dicts is mislabeled as "Phase 6" but `finalize_modules` actually treats that
block as Phase 7; update the comment to read "Phase 7" (or match the existing
phase numbering used in `finalize_modules`) so the phase labels are consistent
with the earlier Phase 6 GC collection and the surrounding phase comments.
---
Outside diff comments:
In `@crates/vm/src/vm/interpreter.rs`:
- Around line 386-395: Update the "Finalization steps" doc comment so it matches
the actual shutdown sequence in this file: enumerate the real order (flush
stdout/stderr via flush_std, handle exit exception -> exit code, call
threading._shutdown to wait for threads, set the finalizing flag (suppresses
unraisable exceptions), run atexit exit functions, run GC collect_force (where
the doc previously claimed atexit ran elsewhere), call finalize_modules for
module cleanup, and note any final flush/GC behavior; remove the duplicate
threading._shutdown step and correct the step labels to reference the actual
calls (threading._shutdown, atexit, collect_force, finalize_modules, flush_std)
so the numbered list matches the code flow.
In `@crates/vm/src/vm/mod.rs`:
- Around line 1122-1149: The defer cleanup (scopeguard::defer!) must be created
before running the trace/execution so panics still trigger cleanup; move the
scopeguard::defer! block to just after setting up the
frame/owner/exception/recursion state and before calling
self.trace_event(TraceEvent::Call, None) and f(frame). Ensure TraceEvent::Return
is still emitted on successful result (keep the existing trace_event call after
f(frame)), and preserve the same cleanup actions inside the guard
(frame.owner.store(...), self.exceptions.borrow_mut().stack.pop(),
crate::vm::thread::set_current_frame(old_frame), self.frames.borrow_mut().pop(),
cfg thread pop_thread_frame(), and self.recursion_depth.update(|d| d - 1)) so
any panic during trace_event or f(frame) will run the guard.
---
Nitpick comments:
In `@crates/common/src/refcount.rs`:
- Around line 43-46: The code does unnecessary arithmetic because COUNT is 1;
simplify by removing the redundant / COUNT in strong() and * COUNT in
add_strong(): in the strong() method use ((self.inner & STRONG) as u32) (or
equivalent masking) to extract the strong count directly, and in add_strong()
add the increment directly to the strong bits instead of multiplying by COUNT;
update any related bit-manipulation comments to reflect the simplification and
keep the STRONG and COUNT constants as needed (or remove COUNT if unused).
- Around line 250-252: The #[inline] attribute on flush_deferred_drops is
misleading because the function always allocates a new Vec when draining,
preventing meaningful inlining; remove the #[inline] on the flush_deferred_drops
function or restructure it so the cheap fast-path (e.g., an early is_empty check
on the deferred queue) is performed in a small #[inline] helper that returns
early while the heavier Vec-allocating work remains in a non-inlined function;
locate flush_deferred_drops and either drop the attribute or split out the
empty-queue check into an inline helper to preserve the intended optimization.
In `@crates/vm/src/gc_state.rs`:
- Around line 464-480: The code currently calls unsafe
obj.gc_get_referent_ptrs() twice per object; change to compute and store each
object's adjacency once and reuse it: during the initial loop over collecting
(the block that iterates over collecting, reads obj.gc_get_referent_ptrs(), and
updates gc_refs) build a HashMap<GcObjectPtr, Vec<GcObjectPtr>> (or similar)
keyed by gc_ptr and populate it from obj.gc_get_referent_ptrs(); then in the
later reachability traversal (the code that walks reachable objects) read
referents from that cache instead of calling gc_get_referent_ptrs() again.
Update lookups to use the cache where you currently call gc_get_referent_ptrs(),
and ensure you still respect the same alive/strong_count checks and membership
tests against collecting and gc_refs.
- Around line 598-614: Consolidate the separate read and write lock around
finalized_objects into a single write-lock per object: in the loop over
unreachable_refs (using GcObjectPtr), acquire self.finalized_objects.write()
once, check if the pointer is already present, and if not insert it; then drop
the write lock and call obj_ref.try_call_finalizer() only when you performed the
insert. This removes the TOCTOU window with untrack_object while preserving the
behavior that the object is marked finalized before __del__ runs
(is_finalized()/mark_finalized semantics).