Rework indexing and iteration in once crate - #219
Merged
Conversation
📝 WalkthroughWalkthroughRefactors iterator and indexing APIs across multiple sparse data structures: replaces enum-based iterators with concrete iterator types and value-only iterators, adds mutable iteration and Index/IndexMut implementations, updates public exports and docs (write-once → insert-once), and adapts call sites and benchmarks to the new iterator shapes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/algebra/src/dense_bigraded_algebra/mod.rs`:
- Around line 356-358: The use of Vec::drain_filter on
data.indecomposable_decompositions is unstable and won't compile on stable Rust;
replace the drain_filter call with a stable alternative such as Vec::extract_if
(Rust 1.80+) or manually retain/filter the vector: iterate over
data.indecomposable_decompositions and remove entries where
invalidated_bidegrees.iter().any(|&(x,y)| mono.contains_bidegree(x,y)) holds,
e.g. by using extract_if or by partitioning/retaining to preserve the intended
behavior of removing entries that satisfy mono.contains_bidegree for any
invalidated_bidegrees; adjust surrounding logic that relied on the drained items
accordingly.
In `@ext/crates/once/src/grove/mod.rs`:
- Around line 735-745: TwoEndedGrove::iter() yields storage order instead of
numeric index order because self.neg is iterated in increasing stored magnitude
then negated, producing -1,-2,-3... after non_neg; fix by iterating the negative
side in reverse (or iterate indices and produce negated indices in descending
order) so the chained sequence is ...1,0,-1,-2... (degree-sorted). Update the
iterator implementation that builds non_negs and neg (and similarly values()) to
reverse self.neg before mapping or to drive iteration from actual indices rather
than storage order; ensure you still map magnitudes to negative i32 via -(idx as
i32) but on the reversed sequence so ordering is numeric.
- Around line 374-380: The current iter_mut recreates a temporary &mut Grove on
each closure call (unsafe { (*ptr).get_mut(i) }) which can overlap with
previously yielded &mut T and is unsound; replace this by implementing a
dedicated mutable iterator type (e.g., GroveMutIter) that captures a single raw
pointer to the elements and current index/len and advances by returning &mut T
references derived directly from element pointers (no reborrowing of &mut Self),
then update Grove::iter_mut to return that iterator and change
TwoEndedGrove::iter_mut and values_mut to use/delegate to the new GroveMutIter;
alternatively, if you prefer not to return an Iterator, convert iter_mut to a
callback-based for_each_mut API that walks element pointers and invokes the
closure for each &mut T without reborrowing the whole Grove.
In `@ext/crates/once/src/multiindexed/kdtrie.rs`:
- Around line 262-275: Add a small unit test that directly exercises the
slice-based Index and IndexMut impls on KdTrie (the impls for Index<&[i32]> and
IndexMut<&[i32]> which call get/get_mut). The test should construct a KdTrie<V>
with at least one entry, assert read access via &trie[&slice] returns the
expected value, mutate via &mut trie[&slice] to change the value and assert the
change, and include a negative-case or unwrap panic expectation if desired;
place it alongside existing KdTrie tests so the slice-based path stays covered.
In `@ext/crates/once/src/multiindexed/mod.rs`:
- Around line 303-308: Update the module-level documentation to remove or revise
the sentence that claims values "cannot be changed directly" and instead
document the supported in-place mutation APIs: mention that values can be
mutated through MultiIndexed::get_mut, MultiIndexed::iter_mut and by indexing
via the IndexMut<[i32; K]> implementation (IndexMut::index_mut), and state any
contract or safety considerations for mutating entries (e.g., validity of
indices, borrowing rules). Ensure the docs reference the exact symbols get_mut,
iter_mut, and IndexMut (index_mut) so readers know the new mutation
capabilities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db942b13-d97f-42ed-a140-98f1341afe2e
📒 Files selected for processing (15)
ext/crates/algebra/src/dense_bigraded_algebra/mod.rsext/crates/algebra/src/module/free_module.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/grove/mod.rsext/crates/once/src/lib.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/node.rsext/crates/once/src/once.rsext/crates/sseq/src/bigraded.rsext/examples/ext_m_n.rsext/examples/steenrod.rsext/src/secondary.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ext/crates/once/src/grove/mod.rs (1)
374-380:⚠️ Potential issue | 🔴 Critical
Grove::iter_mut()is still unsound.Line 376 and Line 379 recreate
&mut Grove<T>fromptron everynext(). If a caller keeps one yielded&mut Talive and advances again, that temporary reborrow overlaps the earlier element borrow and violates Rust’s aliasing rules.TwoEndedGrove::iter_mut()andvalues_mut()inherit the same problem.🔧 Safer shape
- pub fn iter_mut(&mut self) -> impl Iterator<Item = (usize, &mut T)> { - let ptr = self as *mut Self; - (0..self.len()).filter_map(move |i| { - unsafe { (*ptr).get_mut(i).map(|value| (i, value)) } - }) - } + pub fn iter_mut(&mut self) -> IterMut<'_, T> { + IterMut::new(self) + }Implement
IterMut::next()over raw element pointers / occupied slots directly so it never callsget_mut()or otherwise reborrows&mut Grove<T>after yielding an element.The new
*_iter_mut_no_aliasingtests are good Miri repros, but they should still fail under Miri until this iterator stops materializing&mut Selfon each step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ext/crates/once/src/grove/mod.rs` around lines 374 - 380, Grove::iter_mut currently reconstructs &mut Grove<T> via a raw ptr on every iteration (using get_mut), which reborrows &mut self and can overlap previously yielded &mut T; change iter_mut to traverse occupied slots with raw element pointers instead of calling get_mut each step: implement IterMut::next to compute the element's raw pointer (from the internal block/offset or occupied slot metadata), advance indices/cursors using only pointer arithmetic, and return &mut T constructed from that raw pointer (ensuring the iterator holds the single &mut self borrow for its lifetime and does not reborrow it). Apply the same approach to TwoEndedGrove::iter_mut and values_mut, update IterMut internals to track raw pointers/indices, and re-run the *_iter_mut_no_aliasing tests/Miri to verify the aliasing issue is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/once/src/multiindexed/iter.rs`:
- Around line 266-286: Implement std::iter::FusedIterator for both Iter and
IterMut and add a size_hint implementation that delegates to the wrapped
KdIterator to enable downstream optimizations: update the Iterator impl blocks
for Iter<'a, V, C> and IterMut<'a, V, C> to include a fn size_hint(&self) ->
(usize, Option<usize>) that simply returns self.0.size_hint(), and add separate
impls `impl<'a, V, C: Coordinates> FusedIterator for Iter<'a, V, C> {}` and
`impl<'a, V, C: Coordinates> FusedIterator for IterMut<'a, V, C> {}` so the
types (Iter, IterMut), which wrap KdIterator and NodePtrMut, advertise they are
fused; if there is a more precise remaining-count field available in KdIterator
prefer returning that in size_hint.
---
Duplicate comments:
In `@ext/crates/once/src/grove/mod.rs`:
- Around line 374-380: Grove::iter_mut currently reconstructs &mut Grove<T> via
a raw ptr on every iteration (using get_mut), which reborrows &mut self and can
overlap previously yielded &mut T; change iter_mut to traverse occupied slots
with raw element pointers instead of calling get_mut each step: implement
IterMut::next to compute the element's raw pointer (from the internal
block/offset or occupied slot metadata), advance indices/cursors using only
pointer arithmetic, and return &mut T constructed from that raw pointer
(ensuring the iterator holds the single &mut self borrow for its lifetime and
does not reborrow it). Apply the same approach to TwoEndedGrove::iter_mut and
values_mut, update IterMut internals to track raw pointers/indices, and re-run
the *_iter_mut_no_aliasing tests/Miri to verify the aliasing issue is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df042ad7-127d-4bfe-af89-9253d5e30518
📒 Files selected for processing (4)
ext/crates/once/src/grove/mod.rsext/crates/once/src/lib.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crates/once/src/once.rs (1)
1215-1223:⚠️ Potential issue | 🟡 MinorAssert the pair count here.
This loop only validates the pairs that are produced. If
v.iter()accidentally stopped after two items, the test would still pass, so add an explicit length assertion before the loop.💡 Suggested test tightening
let expected_indices = [-3, -2, -1]; let expected_values = [10, 20, 30]; let actual_pairs: Vec<_> = v.iter().collect(); + assert_eq!(actual_pairs.len(), expected_indices.len()); for (i, (idx, val)) in actual_pairs.iter().enumerate() { assert_eq!(*idx, expected_indices[i]); assert_eq!(**val, expected_values[i]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ext/crates/once/src/once.rs` around lines 1215 - 1223, The test iterates actual_pairs collected from v.iter() but never asserts the count, so a truncated iterator could still make the test pass; add an explicit length assertion (compare actual_pairs.len() to expected_indices.len() or expected_values.len()) before the for-loop to ensure the number of produced pairs matches expectations (use the existing variables actual_pairs, expected_indices, expected_values and assert equality of lengths).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/once/src/once.rs`:
- Around line 754-799: The new OnceBiVecIter produced by OnceBiVec::iter (and
IntoIterator for &OnceBiVec) only implements Iterator, losing
DoubleEndedIterator support; implement DoubleEndedIterator for OnceBiVecIter by
adding back-traversal bookkeeping (e.g. store a second index like end_pos or
compute back index from self.pos + remaining_len - 1) and implement next_back()
to return (back_idx, value) using the inner OnceVecIter's back() (or add
front/back methods to OnceVecIter if needed), ensuring size_hint/len/count
remain correct so rev() works as before.
---
Outside diff comments:
In `@ext/crates/once/src/once.rs`:
- Around line 1215-1223: The test iterates actual_pairs collected from v.iter()
but never asserts the count, so a truncated iterator could still make the test
pass; add an explicit length assertion (compare actual_pairs.len() to
expected_indices.len() or expected_values.len()) before the for-loop to ensure
the number of produced pairs matches expectations (use the existing variables
actual_pairs, expected_indices, expected_values and assert equality of lengths).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a33bba8e-071b-4287-ac8f-cc6e8df53cb6
📒 Files selected for processing (3)
ext/crates/once/src/grove/mod.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/once.rs
Rename `iter_enum` to `iter` and `iter` to `values` on `OnceBiVec`, matching the convention that `iter()` yields key-value pairs for non-vector containers. Also rename `maybe_par_iter_enum` to `maybe_par_iter`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
JoeyBF
force-pushed
the
once_rework_access
branch
from
March 20, 2026 08:31
9a72555 to
55af68f
Compare
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ext/crates/once/src/multiindexed/mod.rs (1)
44-47:⚠️ Potential issue | 🟡 MinorMention
iter_mutin the mutation contract.This note still reads as if in-place mutation only happens through
get_mutandIndexMut, butiter_mutis now part of the public API too. Please list it here so the docs match the exposed surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ext/crates/once/src/multiindexed/mod.rs` around lines 44 - 47, Update the documentation paragraph describing in-place mutation to mention iter_mut in addition to get_mut and IndexMut: change the sentence that lists mutation entry points (currently mentioning [`get_mut`](MultiIndexed::get_mut) and [`IndexMut`]) to also include [`iter_mut`](MultiIndexed::iter_mut) so the docs reflect the public API and mutation contract accurately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/once/src/multiindexed/mod.rs`:
- Around line 49-50: The constructor for MultiIndexed currently documents that K
must be nonzero but doesn't enforce it; update MultiIndexed::new to validate the
dimensions/K parameter and return an Err or panic on K == 0 (whichever the
crate's API uses) so the invariant is enforced at construction; locate the
MultiIndexed::new function and add a guard that checks dimensions (or K) > 0 and
fails early with a clear message referencing MultiIndexed::new and the
dimensions/K argument.
---
Duplicate comments:
In `@ext/crates/once/src/multiindexed/mod.rs`:
- Around line 44-47: Update the documentation paragraph describing in-place
mutation to mention iter_mut in addition to get_mut and IndexMut: change the
sentence that lists mutation entry points (currently mentioning
[`get_mut`](MultiIndexed::get_mut) and [`IndexMut`]) to also include
[`iter_mut`](MultiIndexed::iter_mut) so the docs reflect the public API and
mutation contract accurately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c0fa193-212b-4819-8d08-2e147c7c93a5
📒 Files selected for processing (14)
ext/crates/algebra/src/module/free_module.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/grove/mod.rsext/crates/once/src/lib.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/node.rsext/crates/once/src/once.rsext/crates/sseq/src/bigraded.rsext/examples/ext_m_n.rsext/examples/steenrod.rsext/src/secondary.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/algebra/src/module/free_module.rs`:
- Around line 297-301: iter_gens (and likewise iter_gen_offsets) currently does
take((degree - self.min_degree + 1) as usize) which underflows when degree <
self.min_degree - 1; compute a non-negative count first (e.g. let count = if
degree < self.min_degree { 0 } else { (degree - self.min_degree + 1) as usize })
and use take(count) instead of casting the raw subtraction, ensuring you guard
the upper bound before converting to usize so no underflow occurs and the
iterator yields none when degree is below min_degree.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03f174a9-2ab7-449b-823a-9f63bbc3ff41
📒 Files selected for processing (14)
ext/crates/algebra/src/module/free_module.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/grove/mod.rsext/crates/once/src/lib.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/node.rsext/crates/once/src/once.rsext/crates/sseq/src/bigraded.rsext/examples/ext_m_n.rsext/examples/steenrod.rsext/src/secondary.rs
github-actions Bot
added a commit
that referenced
this pull request
Mar 21, 2026
* Add `Index` implementations * Rename iteration methods * Add `IntoIterator` impls * Add `IntoIterator` impls for other types in `once` * Rename `OnceBiVec` iteration methods Rename `iter_enum` to `iter` and `iter` to `values` on `OnceBiVec`, matching the convention that `iter()` yields key-value pairs for non-vector containers. Also rename `maybe_par_iter_enum` to `maybe_par_iter`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove unnessary `for_each_mut` * Adjust docs * Consolidate iteration tests * Add miri tests * Add marker traits and override default methods * Make iterator double-ended
github-actions Bot
added a commit
to JoeyBF/sseq
that referenced
this pull request
Mar 21, 2026
* Add `Index` implementations * Rename iteration methods * Add `IntoIterator` impls * Add `IntoIterator` impls for other types in `once` * Rename `OnceBiVec` iteration methods Rename `iter_enum` to `iter` and `iter` to `values` on `OnceBiVec`, matching the convention that `iter()` yields key-value pairs for non-vector containers. Also rename `maybe_par_iter_enum` to `maybe_par_iter`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove unnessary `for_each_mut` * Adjust docs * Consolidate iteration tests * Add miri tests * Add marker traits and override default methods * Make iterator double-ended
This was referenced Mar 25, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
API Changes
Docs